[PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events

["Ankur Arora" <ankur.a.arora@oracle.com>]

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>
---

Notes:
    Addresses the following comments from v8:
      (1) Fix commit message to mention that we collect cpu-selectors as well.
      (2,3,6) s/UnplugSelector/UnplugSelectors/ in CpuHotplug.c, QemuCpuhp.c
      (4) Fix comment above the declaration of the now renamed mToUnplugSelector.
      (5) Fix spacing around "||".
      (7) Fix QemuCpuCollectApicIds() comments to line up descriptions for
          ToUnplugSelectors and other params.
      (8) s/ExtendSel/ExtendSels/.
      (9) Add the (ExtendSels => ExtendIds) assert.
      (10) Fix the missing CurrentSelector++ bug.

 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                 |   1 +
 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h |   1 +
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c                |  29 +++++--
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                 | 101 +++++++++++++++-------
 4 files changed, 93 insertions(+), 39 deletions(-)

diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
index 8adaa0ad91f0..3e2c2192e1c0 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
   );
 
diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
index a34a6d3fae61..2ec7a107a64d 100644
--- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
+++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
@@ -34,6 +34,7 @@
 #define QEMU_CPUHP_STAT_ENABLED                BIT0
 #define QEMU_CPUHP_STAT_INSERT                 BIT1
 #define QEMU_CPUHP_STAT_REMOVE                 BIT2
+#define QEMU_CPUHP_STAT_FW_REMOVE              BIT4
 
 #define QEMU_CPUHP_RW_CMD_DATA               0x8
 
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index bf68fcd42914..ee1497b93140 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 8d4a6693c8d6..8434dd446b96 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;
-- 
2.9.3