OvmfPkg/CpuHotplugSmm: collect hot-unplug events

Process fw_remove events in QemuCpuhpCollectApicIds(), and collect APIC IDs
and QEMU CPU Selectors for CPUs being hot-unplugged.

In addition, we now ignore CPUs which only have remove set. These
CPUs haven't been processed by OSPM yet.

This is based on the QEMU hot-unplug protocol documented here:
  https://lore.kernel.org/qemu-devel/20201204170939.1815522-3-imammedo@redhat.com/

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
Message-Id: <20210312062656.2477515-3-ankur.a.arora@oracle.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

3 files changed, 92 insertions, 39 deletions

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index bf68fcd429..ee1497b931 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -45,13 +45,16 @@ STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
 // don't want to allocate SMRAM at OS runtime, and potentially fail (or

 // fragment the SMRAM map).

 //

-// These arrays provide room for ("possible CPU count" minus one) APIC IDs

-// each, as we don't expect every possible CPU to appear, or disappear, in a

-// single MMI. The numbers of used (populated) elements in the arrays are

+// The first array stores APIC IDs for hot-plug events, the second and the

+// third store APIC IDs and QEMU CPU Selectors (both indexed similarly) for

+// hot-unplug events. All of these provide room for "possible CPU count" minus

+// one elements as we don't expect every possible CPU to appear, or disappear,

+// in a single MMI. The numbers of used (populated) elements in the arrays are

 // determined on every MMI separately.

 //

 STATIC APIC_ID *mPluggedApicIds;

 STATIC APIC_ID *mToUnplugApicIds;

+STATIC UINT32  *mToUnplugSelectors;

 //

 // Address of the non-SMRAM reserved memory page that contains the Post-SMM Pen

 // for hot-added CPUs.

@@ -289,6 +292,7 @@ CpuHotplugMmi (
              mPluggedApicIds,

              &PluggedCount,

              mToUnplugApicIds,

+             mToUnplugSelectors,

              &ToUnplugCount

              );

   if (EFI_ERROR (Status)) {

@@ -333,7 +337,9 @@ CpuHotplugEntry (
   )

 {

   EFI_STATUS Status;

+  UINTN      Len;

   UINTN      Size;

+  UINTN      SizeSel;

 

   //

   // This module should only be included when SMM support is required.

@@ -387,8 +393,9 @@ CpuHotplugEntry (
   //

   // Allocate the data structures that depend on the possible CPU count.

   //

-  if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Size)) ||

-      RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Size, &Size))) {

+  if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Len)) ||

+      RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Len, &Size)) ||

+      RETURN_ERROR (SafeUintnMult (sizeof (UINT32), Len, &SizeSel))) {

     Status = EFI_ABORTED;

     DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_PLUG_DATA\n", __FUNCTION__));

     goto Fatal;

@@ -405,6 +412,12 @@ CpuHotplugEntry (
     DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status));

     goto ReleasePluggedApicIds;

   }

+  Status = gMmst->MmAllocatePool (EfiRuntimeServicesData, SizeSel,

+                    (VOID **)&mToUnplugSelectors);

+  if (EFI_ERROR (Status)) {

+    DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status));

+    goto ReleaseToUnplugApicIds;

+  }

 

   //

   // Allocate the Post-SMM Pen for hot-added CPUs.

@@ -412,7 +425,7 @@ CpuHotplugEntry (
   Status = SmbaseAllocatePostSmmPen (&mPostSmmPenAddress,

              SystemTable->BootServices);

   if (EFI_ERROR (Status)) {

-    goto ReleaseToUnplugApicIds;

+    goto ReleaseToUnplugSelectors;

   }

 

   //

@@ -472,6 +485,10 @@ ReleasePostSmmPen:
   SmbaseReleasePostSmmPen (mPostSmmPenAddress, SystemTable->BootServices);

   mPostSmmPenAddress = 0;

 

+ReleaseToUnplugSelectors:

+  gMmst->MmFreePool (mToUnplugSelectors);

+  mToUnplugSelectors = NULL;

+

 ReleaseToUnplugApicIds:

   gMmst->MmFreePool (mToUnplugApicIds);

   mToUnplugApicIds = NULL;

diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
index 8d4a6693c8..8434dd446b 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
@@ -145,27 +145,30 @@ QemuCpuhpWriteCommand (
 

   On error, the contents of the output parameters are undefined.

 

-  @param[in] MmCpuIo           The EFI_MM_CPU_IO_PROTOCOL instance for

-                               accessing IO Ports.

+  @param[in] MmCpuIo             The EFI_MM_CPU_IO_PROTOCOL instance for

+                                 accessing IO Ports.

 

-  @param[in] PossibleCpuCount  The number of possible CPUs in the system. Must

-                               be positive.

+  @param[in] PossibleCpuCount    The number of possible CPUs in the system. Must

+                                 be positive.

 

-  @param[in] ApicIdCount       The number of elements each one of the

-                               PluggedApicIds and ToUnplugApicIds arrays can

-                               accommodate. Must be positive.

+  @param[in] ApicIdCount         The number of elements each one of the

+                                 PluggedApicIds and ToUnplugApicIds arrays can

+                                 accommodate. Must be positive.

 

-  @param[out] PluggedApicIds   The APIC IDs of the CPUs that have been

-                               hot-plugged.

+  @param[out] PluggedApicIds     The APIC IDs of the CPUs that have been

+                                 hot-plugged.

 

-  @param[out] PluggedCount     The number of filled-in APIC IDs in

-                               PluggedApicIds.

+  @param[out] PluggedCount       The number of filled-in APIC IDs in

+                                 PluggedApicIds.

 

-  @param[out] ToUnplugApicIds  The APIC IDs of the CPUs that are about to be

-                               hot-unplugged.

+  @param[out] ToUnplugApicIds    The APIC IDs of the CPUs that are about to be

+                                 hot-unplugged.

 

-  @param[out] ToUnplugCount    The number of filled-in APIC IDs in

-                               ToUnplugApicIds.

+  @param[out] ToUnplugSelectors  The QEMU Selectors of the CPUs that are about

+                                 to be hot-unplugged.

+

+  @param[out] ToUnplugCount      The number of filled-in APIC IDs in

+                                 ToUnplugApicIds.

 

   @retval EFI_INVALID_PARAMETER  PossibleCpuCount is zero, or ApicIdCount is

                                  zero.

@@ -187,6 +190,7 @@ QemuCpuhpCollectApicIds (
   OUT APIC_ID                      *PluggedApicIds,

   OUT UINT32                       *PluggedCount,

   OUT APIC_ID                      *ToUnplugApicIds,

+  OUT UINT32                       *ToUnplugSelectors,

   OUT UINT32                       *ToUnplugCount

   )

 {

@@ -204,6 +208,7 @@ QemuCpuhpCollectApicIds (
     UINT32  PendingSelector;

     UINT8   CpuStatus;

     APIC_ID *ExtendIds;

+    UINT32  *ExtendSels;

     UINT32  *ExtendCount;

     APIC_ID NewApicId;

 

@@ -245,10 +250,10 @@ QemuCpuhpCollectApicIds (
     if ((CpuStatus & QEMU_CPUHP_STAT_INSERT) != 0) {

       //

       // The "insert" event guarantees the "enabled" status; plus it excludes

-      // the "remove" event.

+      // the "fw_remove" event.

       //

       if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0 ||

-          (CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {

+          (CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) {

         DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: "

           "inconsistent CPU status\n", __FUNCTION__, CurrentSelector,

           CpuStatus));

@@ -259,33 +264,63 @@ QemuCpuhpCollectApicIds (
         CurrentSelector));

 

       ExtendIds   = PluggedApicIds;

+      ExtendSels  = NULL;

       ExtendCount = PluggedCount;

-    } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {

-      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", __FUNCTION__,

-        CurrentSelector));

+    } else if ((CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) {

+      //

+      // "fw_remove" event guarantees "enabled".

+      //

+      if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0) {

+        DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: "

+          "inconsistent CPU status\n", __FUNCTION__, CurrentSelector,

+          CpuStatus));

+        return EFI_PROTOCOL_ERROR;

+      }

+

+      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: fw_remove\n",

+        __FUNCTION__, CurrentSelector));

 

       ExtendIds   = ToUnplugApicIds;

+      ExtendSels  = ToUnplugSelectors;

       ExtendCount = ToUnplugCount;

+    } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {

+      //

+      // Let the OSPM deal with the "remove" event.

+      //

+      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove (ignored)\n",

+        __FUNCTION__, CurrentSelector));

+

+      ExtendIds   = NULL;

+      ExtendSels  = NULL;

+      ExtendCount = NULL;

     } else {

       DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: no event\n",

         __FUNCTION__, CurrentSelector));

       break;

     }

 

-    //

-    // Save the APIC ID of the CPU with the pending event, to the corresponding

-    // APIC ID array.

-    //

-    if (*ExtendCount == ApicIdCount) {

-      DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__));

-      return EFI_BUFFER_TOO_SMALL;

-    }

-    QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);

-    NewApicId = QemuCpuhpReadCommandData (MmCpuIo);

-    DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,

-      NewApicId));

-    ExtendIds[(*ExtendCount)++] = NewApicId;

+    ASSERT ((ExtendIds == NULL) == (ExtendCount == NULL));

+    ASSERT ((ExtendSels == NULL) || (ExtendIds != NULL));

 

+    if (ExtendIds != NULL) {

+      //

+      // Save the APIC ID of the CPU with the pending event, to the

+      // corresponding APIC ID array.

+      // For unplug events, also save the CurrentSelector.

+      //

+      if (*ExtendCount == ApicIdCount) {

+        DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__));

+        return EFI_BUFFER_TOO_SMALL;

+      }

+      QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);

+      NewApicId = QemuCpuhpReadCommandData (MmCpuIo);

+      DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,

+        NewApicId));

+      if (ExtendSels != NULL) {

+        ExtendSels[(*ExtendCount)] = CurrentSelector;

+      }

+      ExtendIds[(*ExtendCount)++] = NewApicId;

+    }

     //

     // We've processed the CPU with (known) pending events, but we must never

     // clear events. Therefore we need to advance past this CPU manually;

diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
index 8adaa0ad91..3e2c2192e1 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
@@ -55,6 +55,7 @@ QemuCpuhpCollectApicIds (
   OUT APIC_ID                      *PluggedApicIds,

   OUT UINT32                       *PluggedCount,

   OUT APIC_ID                      *ToUnplugApicIds,

+  OUT UINT32                       *ToUnplugSelectors,

   OUT UINT32                       *ToUnplugCount

   );