[PATCH v5 0/9] support CPU hot-unplug

[PATCH v5 0/9] support CPU hot-unplug

[Ankur Arora]

Hi,

This series adds support for CPU hot-unplug with OVMF.

Please see this in conjunction with the QEMU secureboot hot-unplug v2
series posted here (now upstreamed):
  https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/

Patches 1 and 3,
  ("OvmfPkg/CpuHotplugSmm: refactor hotplug logic")
  ("OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper")
are either refactors or add support functions.

  OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
  OvmfPkg/CpuHotplugSmm: add CpuEject()
  OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection

Patch 2 and 9,
  ("OvmfPkg/CpuHotplugSmm: collect hot-unplug events")
  ("OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug")
handle the QEMU protocol logic for collection of CPU hot-unplug events
or the protocol negotiation.

Patch 4,
  ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()")
adds the MMI logic for CPU hot-unplug handling and informing
the PiSmmCpuDxeSmm of CPU removal.

Patches 5 and 6,
  ("OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA")
  ("OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state")
sets up state for doing the CPU ejection as part of hot-unplug.

Patches 7, and 8,
  ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()")
  ("OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection")
add the CPU ejection logic.

Testing (with QEMU 5.2.50):
 - Stable with randomized CPU plug/unplug (guest maxcpus=1,8,128)
 - Synthetic tests with simultaneous multi CPU hot-unplug
 - Negotiation with/without CPU hotplug enabled

Also at:
  github.com/terminus/edk2/ hot-unplug-v5

Changelog:
v5:
  - fixes ECC errors (all but one in "OvmfPkg/CpuHotplugSmm: add
    add Qemu Cpu Status helper").

v4:
  - Gets rid of unnecessary UefiCpuPkg changes
  URL: https://patchew.org/EDK2/20210118063457.358581-1-ankur.a.arora@oracle.com/

v3:
  - Use a saner PCD based interface to share state between PiSmmCpuDxeSmm
    and OvmfPkg/CpuHotplugSmm
  - Cleaner split of the hot-unplug code
  URL: https://patchew.org/EDK2/20210115074533.277448-1-ankur.a.arora@oracle.com/

v2:
  - Do the ejection via SmmCpuFeaturesRendezvousExit()
  URL: https://patchew.org/EDK2/20210107195515.106158-1-ankur.a.arora@oracle.com/

RFC:
  URL: https://patchew.org/EDK2/20201208053432.2690694-1-ankur.a.arora@oracle.com/


Please review.

Thanks
Ankur

Ankur Arora (9):
  OvmfPkg/CpuHotplugSmm: refactor hotplug logic
  OvmfPkg/CpuHotplugSmm: collect hot-unplug events
  OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
  OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
  OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA
  OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
  OvmfPkg/CpuHotplugSmm: add CpuEject()
  OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
  OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug

 OvmfPkg/OvmfPkg.dec                                |  10 +
 OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf            |   1 +
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   3 +
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                  |   6 +
 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h  |   2 +
 OvmfPkg/Include/Library/CpuHotEjectData.h          |  32 ++
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c                 | 450 ++++++++++++++++-----
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                  |  57 ++-
 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  68 ++++
 OvmfPkg/SmmControl2Dxe/SmiFeatures.c               |  25 +-
 10 files changed, 552 insertions(+), 102 deletions(-)
 create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h

-- 
2.9.3

[PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

[Ankur Arora]

Refactor CpuHotplugMmi() to pull out the CPU hotplug logic into
PlugCpus(). This is in preparation for supporting CPU hot-unplug.

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>
---
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 208 ++++++++++++++++++++++---------------
 1 file changed, 123 insertions(+), 85 deletions(-)

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index cfe698ed2b5e..a5052a501e5a 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -62,6 +62,124 @@ STATIC UINT32 mPostSmmPenAddress;
 //
 STATIC EFI_HANDLE mDispatchHandle;
 
+/**
+  CPU Hotplug handler function.
+
+  @param[in] PluggedApicIds      List of APIC IDs to be plugged.
+
+  @param[in] PluggedCount        Count of APIC IDs to be plugged.
+
+  @retval EFI_SUCCESS            Some of the requested APIC IDs were hot-plugged.
+
+  @retval EFI_INTERRUPT_PENDING  Fatal error while hot-plugging.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+PlugCpus(
+  IN APIC_ID                      *PluggedApicIds,
+  IN UINT32                       PluggedCount
+  )
+{
+  EFI_STATUS Status;
+  UINT32     PluggedIdx;
+  UINT32     NewSlot;
+
+  //
+  // Process hot-added CPUs.
+  //
+  // The Post-SMM Pen need not be reinstalled multiple times within a single
+  // root MMI handling. Even reinstalling once per root MMI is only prudence;
+  // in theory installing the pen in the driver's entry point function should
+  // suffice.
+  //
+  SmbaseReinstallPostSmmPen (mPostSmmPenAddress);
+
+  PluggedIdx = 0;
+  NewSlot = 0;
+  while (PluggedIdx < PluggedCount) {
+    APIC_ID NewApicId;
+    UINT32  CheckSlot;
+    UINTN   NewProcessorNumberByProtocol;
+
+    NewApicId = PluggedApicIds[PluggedIdx];
+
+    //
+    // Check if the supposedly hot-added CPU is already known to us.
+    //
+    for (CheckSlot = 0;
+         CheckSlot < mCpuHotPlugData->ArrayLength;
+         CheckSlot++) {
+      if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {
+        break;
+      }
+    }
+    if (CheckSlot < mCpuHotPlugData->ArrayLength) {
+      DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "
+        "before; ignoring it\n", __FUNCTION__, NewApicId));
+      PluggedIdx++;
+      continue;
+    }
+
+    //
+    // Find the first empty slot in CPU_HOT_PLUG_DATA.
+    //
+    while (NewSlot < mCpuHotPlugData->ArrayLength &&
+           mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) {
+      NewSlot++;
+    }
+    if (NewSlot == mCpuHotPlugData->ArrayLength) {
+      DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n",
+        __FUNCTION__, NewApicId));
+      goto Fatal;
+    }
+
+    //
+    // Store the APIC ID of the new processor to the slot.
+    //
+    mCpuHotPlugData->ApicId[NewSlot] = NewApicId;
+
+    //
+    // Relocate the SMBASE of the new CPU.
+    //
+    Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot],
+               mPostSmmPenAddress);
+    if (EFI_ERROR (Status)) {
+      goto RevokeNewSlot;
+    }
+
+    //
+    // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
+    //
+    Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId,
+                              &NewProcessorNumberByProtocol);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n",
+        __FUNCTION__, NewApicId, Status));
+      goto RevokeNewSlot;
+    }
+
+    DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, "
+      "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__,
+      NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot],
+      (UINT64)NewProcessorNumberByProtocol));
+
+    NewSlot++;
+    PluggedIdx++;
+  }
+
+  //
+  // We've handled this hotplug.
+  //
+  return EFI_SUCCESS;
+
+RevokeNewSlot:
+  mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
+
+Fatal:
+  return EFI_INTERRUPT_PENDING;
+}
 
 /**
   CPU Hotplug MMI handler function.
@@ -122,8 +240,6 @@ CpuHotplugMmi (
   UINT8      ApmControl;
   UINT32     PluggedCount;
   UINT32     ToUnplugCount;
-  UINT32     PluggedIdx;
-  UINT32     NewSlot;
 
   //
   // Assert that we are entering this function due to our root MMI handler
@@ -179,87 +295,12 @@ CpuHotplugMmi (
     goto Fatal;
   }
 
-  //
-  // Process hot-added CPUs.
-  //
-  // The Post-SMM Pen need not be reinstalled multiple times within a single
-  // root MMI handling. Even reinstalling once per root MMI is only prudence;
-  // in theory installing the pen in the driver's entry point function should
-  // suffice.
-  //
-  SmbaseReinstallPostSmmPen (mPostSmmPenAddress);
+  if (PluggedCount > 0) {
+    Status = PlugCpus(mPluggedApicIds, PluggedCount);
+  }
 
-  PluggedIdx = 0;
-  NewSlot = 0;
-  while (PluggedIdx < PluggedCount) {
-    APIC_ID NewApicId;
-    UINT32  CheckSlot;
-    UINTN   NewProcessorNumberByProtocol;
-
-    NewApicId = mPluggedApicIds[PluggedIdx];
-
-    //
-    // Check if the supposedly hot-added CPU is already known to us.
-    //
-    for (CheckSlot = 0;
-         CheckSlot < mCpuHotPlugData->ArrayLength;
-         CheckSlot++) {
-      if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {
-        break;
-      }
-    }
-    if (CheckSlot < mCpuHotPlugData->ArrayLength) {
-      DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "
-        "before; ignoring it\n", __FUNCTION__, NewApicId));
-      PluggedIdx++;
-      continue;
-    }
-
-    //
-    // Find the first empty slot in CPU_HOT_PLUG_DATA.
-    //
-    while (NewSlot < mCpuHotPlugData->ArrayLength &&
-           mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) {
-      NewSlot++;
-    }
-    if (NewSlot == mCpuHotPlugData->ArrayLength) {
-      DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n",
-        __FUNCTION__, NewApicId));
-      goto Fatal;
-    }
-
-    //
-    // Store the APIC ID of the new processor to the slot.
-    //
-    mCpuHotPlugData->ApicId[NewSlot] = NewApicId;
-
-    //
-    // Relocate the SMBASE of the new CPU.
-    //
-    Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot],
-               mPostSmmPenAddress);
-    if (EFI_ERROR (Status)) {
-      goto RevokeNewSlot;
-    }
-
-    //
-    // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
-    //
-    Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId,
-                              &NewProcessorNumberByProtocol);
-    if (EFI_ERROR (Status)) {
-      DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n",
-        __FUNCTION__, NewApicId, Status));
-      goto RevokeNewSlot;
-    }
-
-    DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, "
-      "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__,
-      NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot],
-      (UINT64)NewProcessorNumberByProtocol));
-
-    NewSlot++;
-    PluggedIdx++;
+  if (EFI_ERROR(Status)) {
+    goto Fatal;
   }
 
   //
@@ -267,9 +308,6 @@ CpuHotplugMmi (
   //
   return EFI_SUCCESS;
 
-RevokeNewSlot:
-  mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
-
 Fatal:
   ASSERT (FALSE);
   CpuDeadLoop ();
-- 
2.9.3

[PATCH v5 2/9] OvmfPkg/CpuHotplugSmm: collect hot-unplug events

[Ankur Arora]

Process fw_remove events in QemuCpuhpCollectApicIds() and collect
corresponding APIC IDs for CPUs that are 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/

Also define QEMU_CPUHP_STAT_EJECTED while we are at it.

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>
---
 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h |  2 ++
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                 | 35 +++++++++++++++++------
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
index a34a6d3fae61..692e3072598c 100644
--- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
+++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
@@ -34,6 +34,8 @@
 #define QEMU_CPUHP_STAT_ENABLED                BIT0
 #define QEMU_CPUHP_STAT_INSERT                 BIT1
 #define QEMU_CPUHP_STAT_REMOVE                 BIT2
+#define QEMU_CPUHP_STAT_EJECTED                BIT3
+#define QEMU_CPUHP_STAT_FW_REMOVE              BIT4
 
 #define QEMU_CPUHP_RW_CMD_DATA               0x8
 
diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
index 8d4a6693c8d6..2dd783ebf42e 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
@@ -205,7 +205,7 @@ QemuCpuhpCollectApicIds (
     UINT8   CpuStatus;
     APIC_ID *ExtendIds;
     UINT32  *ExtendCount;
-    APIC_ID NewApicId;
+    APIC_ID OpApicId;
 
     //
     // Write CurrentSelector (which is valid) to the CPU selector register.
@@ -245,10 +245,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));
@@ -260,12 +260,31 @@ QemuCpuhpCollectApicIds (
 
       ExtendIds   = PluggedApicIds;
       ExtendCount = PluggedCount;
-    } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
-      DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", __FUNCTION__,
+    } 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;
       ExtendCount = ToUnplugCount;
+    } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
+      //
+      // Let the OSPM deal with the "remove" event.
+      //
+      DEBUG ((DEBUG_INFO, "%a: CurrentSelector=%u: remove (ignored)\n", __FUNCTION__,
+        CurrentSelector));
+
+      CurrentSelector++;
+      continue;
     } else {
       DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: no event\n",
         __FUNCTION__, CurrentSelector));
@@ -281,10 +300,10 @@ QemuCpuhpCollectApicIds (
       return EFI_BUFFER_TOO_SMALL;
     }
     QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);
-    NewApicId = QemuCpuhpReadCommandData (MmCpuIo);
+    OpApicId = QemuCpuhpReadCommandData (MmCpuIo);
     DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,
-      NewApicId));
-    ExtendIds[(*ExtendCount)++] = NewApicId;
+      OpApicId, ExtendCount));
+    ExtendIds[(*ExtendCount)++] = OpApicId;
 
     //
     // We've processed the CPU with (known) pending events, but we must never
-- 
2.9.3

[PATCH v5 3/9] OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper

[Ankur Arora]

Add QemuCpuhpWriteCpuStatus() which will be used to update the QEMU
CPU status register. On error, it hangs in a similar fashion as
other helper functions.

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>
---
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h |  6 ++++++
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
index 8adaa0ad91f0..804809846890 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
@@ -30,6 +30,12 @@ QemuCpuhpReadCpuStatus (
   IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
   );
 
+VOID
+QemuCpuhpWriteCpuStatus (
+  IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
+  IN UINT8                        CpuStatus
+  );
+
 UINT32
 QemuCpuhpReadCommandData (
   IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
index 2dd783ebf42e..c35ae31d2db0 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
@@ -67,6 +67,28 @@ QemuCpuhpReadCpuStatus (
   return CpuStatus;
 }
 
+VOID
+QemuCpuhpWriteCpuStatus (
+  IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
+  IN UINT8                        CpuStatus
+  )
+{
+  EFI_STATUS Status;
+
+  Status = MmCpuIo->Io.Write(
+                         MmCpuIo,
+                         MM_IO_UINT8,
+                         ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_R_CPU_STAT,
+                         1,
+                         &CpuStatus
+                         );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+}
+
 UINT32
 QemuCpuhpReadCommandData (
   IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
-- 
2.9.3

[PATCH v5 4/9] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()

[Ankur Arora]

Introduce UnplugCpus() which maps each APIC ID being unplugged
onto the hardware ID of the processor and informs PiSmmCpuDxeSmm
of removal by calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor().

With this change we handle the first phase of unplug where we collect
the CPUs that need to be unplugged and mark them for removal in SMM
data structures.

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>
---
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 77 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index a5052a501e5a..d165d6ccea0d 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -182,6 +182,81 @@ Fatal:
 }
 
 /**
+  CPU Hot-unplug MMI handler function.
+
+  @param[in] UnplugApicIds       List of APIC IDs to be unplugged.
+
+  @param[in] ToUnplugCount       Count of APIC IDs to be unplugged.
+
+  @retval EFI_SUCCESS            Some of the requested APIC IDs will be hot-unplugged.
+
+  @retval EFI_INTERRUPT_PENDING  Fatal error while hot-plugging.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+UnplugCpus(
+  IN APIC_ID                      *UnplugApicIds,
+  IN UINT32                       ToUnplugCount
+  )
+{
+  EFI_STATUS Status;
+  UINT32     ToUnplugIdx;
+  UINTN      ProcessorNum;
+
+  ToUnplugIdx = 0;
+  while (ToUnplugIdx < ToUnplugCount) {
+    APIC_ID    RemoveApicId;
+
+    RemoveApicId = UnplugApicIds[ToUnplugIdx];
+
+    //
+    // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find
+    // the ProcessorNum for the APIC ID to be removed.
+    //
+    for (ProcessorNum = 0;
+         ProcessorNum < mCpuHotPlugData->ArrayLength;
+         ProcessorNum++) {
+      if (mCpuHotPlugData->ApicId[ProcessorNum] == RemoveApicId) {
+        break;
+      }
+    }
+
+    //
+    // Ignore the unplug if APIC ID not found
+    //
+    if (ProcessorNum == mCpuHotPlugData->ArrayLength) {
+      DEBUG ((DEBUG_INFO, "%a: did not find APIC ID " FMT_APIC_ID " to unplug\n",
+        __FUNCTION__, RemoveApicId));
+      ToUnplugIdx++;
+      continue;
+    }
+
+    //
+    // Mark ProcessorNum for removal from SMM data structures
+    //
+    Status = mMmCpuService->RemoveProcessor (mMmCpuService, ProcessorNum);
+
+    if (EFI_ERROR(Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: RemoveProcessor(" FMT_APIC_ID "): %r\n",
+        __FUNCTION__, RemoveApicId, Status));
+      goto Fatal;
+    }
+
+    ToUnplugIdx++;
+  }
+
+  //
+  // We've handled this unplug.
+  //
+  return EFI_SUCCESS;
+
+Fatal:
+  return EFI_INTERRUPT_PENDING;
+}
+
+/**
   CPU Hotplug MMI handler function.
 
   This is a root MMI handler.
@@ -297,6 +372,8 @@ CpuHotplugMmi (
 
   if (PluggedCount > 0) {
     Status = PlugCpus(mPluggedApicIds, PluggedCount);
+  } else if (ToUnplugCount > 0) {
+    Status = UnplugCpus(mToUnplugApicIds, ToUnplugCount);
   }
 
   if (EFI_ERROR(Status)) {
-- 
2.9.3

[PATCH v5 5/9] OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA

[Ankur Arora]

Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress, which
will be used to share CPU ejection state between OvmfPkg/CpuHotPlugSmm
and PiSmmCpuDxeSmm.

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>
---
 OvmfPkg/OvmfPkg.dec                       | 10 ++++++++++
 OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf   |  1 +
 OvmfPkg/Include/Library/CpuHotEjectData.h | 32 +++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)
 create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4348bb45c64a..e79ff28465e3 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -106,6 +106,10 @@ [LibraryClasses]
   #
   XenPlatformLib|Include/Library/XenPlatformLib.h
 
+  ##  @libraryclass  Share CPU hot-eject state
+  #
+  CpuHotEjectData|Include/Library/CpuHotEjectData.h
+
 [Guids]
   gUefiOvmfPkgTokenSpaceGuid            = {0x93bb96af, 0xb9f2, 0x4eb8, {0x94, 0x62, 0xe0, 0xba, 0x74, 0x56, 0x42, 0x36}}
   gEfiXenInfoGuid                       = {0xd3b46f3b, 0xd441, 0x1244, {0x9a, 0x12, 0x0, 0x12, 0x27, 0x3f, 0xc1, 0x4d}}
@@ -352,6 +356,12 @@ [PcdsDynamic, PcdsDynamicEx]
   #  This PCD is only accessed if PcdSmmSmramRequire is TRUE (see below).
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE|BOOLEAN|0x34
 
+  ## This PCD adds a communication channel between PiSmmCpuDxe and
+  #  CpuHotplugSmm.
+  #
+  #  Only accessed if PcdCpuHotPlugSupport is TRUE
+  gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46
+
 [PcdsFeatureFlag]
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
index 04322b0d7855..e08b572ef169 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
@@ -54,6 +54,7 @@ [Protocols]
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress                ## CONSUMES
+  gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress              ## CONSUMES
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase             ## CONSUMES
 
 [FeaturePcd]
diff --git a/OvmfPkg/Include/Library/CpuHotEjectData.h b/OvmfPkg/Include/Library/CpuHotEjectData.h
new file mode 100644
index 000000000000..839ddad6d8f8
--- /dev/null
+++ b/OvmfPkg/Include/Library/CpuHotEjectData.h
@@ -0,0 +1,32 @@
+/** @file
+  Definition for a structure sharing state for CPU hot-eject.
+
+  Copyright (C) 2021, Oracle Corporation.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef _CPU_HOT_EJECT_DATA_H_
+#define _CPU_HOT_EJECT_DATA_H_
+
+typedef
+VOID
+(EFIAPI *CPU_HOT_EJECT_FN)(
+  IN UINTN  ProcessorNum
+  );
+
+#define CPU_EJECT_INVALID               (MAX_UINT64)
+#define CPU_EJECT_WORKER                (MAX_UINT64-1)
+
+#define  CPU_HOT_EJECT_DATA_REVISION_1  0x00000001
+
+typedef struct {
+  UINT32           Revision;          // Used for version identification for this structure
+  UINT32           ArrayLength;       // Number of entries for the ApicIdMap array
+
+  UINT64           *ApicIdMap;        // Pointer to CpuIndex->ApicId map for pending ejects
+  CPU_HOT_EJECT_FN Handler;           // Handler to do the CPU ejection
+  UINT64           Reserved;
+} CPU_HOT_EJECT_DATA;
+
+#endif /* _CPU_HOT_EJECT_DATA_H_ */
-- 
2.9.3

[PATCH v5 6/9] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state

[Ankur Arora]

Init CPU_HOT_EJECT_DATA, which will be used to share CPU ejection state
between SmmCpuFeaturesLib (via PiSmmCpuDxeSmm) and CpuHotPlugSmm.
CpuHotplugSmm also sets up the CPU ejection mechanism via
CPU_HOT_EJECT_DATA->Handler.

Additionally, expose CPU_HOT_EJECT_DATA via PcdCpuHotEjectDataAddress.

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>
---
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  3 +
 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  | 68 ++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 97a10afb6e27..32c63722ee62 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -35,4 +35,7 @@ [LibraryClasses]
   UefiBootServicesTableLib
 
 [Pcd]
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
+  gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 7ef7ed98342e..5c9cdc6710e4 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -14,7 +14,9 @@
 #include <Library/PcdLib.h>
 #include <Library/SmmCpuFeaturesLib.h>
 #include <Library/SmmServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>                 // AllocatePool()
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/CpuHotEjectData.h>
 #include <PiSmm.h>
 #include <Register/Intel/SmramSaveStateMap.h>
 #include <Register/QemuSmramSaveStateMap.h>
@@ -171,6 +173,60 @@ SmmCpuFeaturesHookReturnFromSmm (
   return OriginalInstructionPointer;
 }
 
+GLOBAL_REMOVE_IF_UNREFERENCED
+CPU_HOT_EJECT_DATA *mCpuHotEjectData = NULL;
+
+/**
+  This function initializes CpuHotEjectData if PcdCpuHotPlugSupport is
+  enabled and if more than 1 CPU is configured.
+
+  Also sets up the corresponding PcdCpuHotEjectDataAddress.
+**/
+STATIC
+VOID
+EFIAPI
+SmmCpuFeaturesSmmInitHotEject(
+  VOID
+  )
+{
+  UINT32 mMaxNumberOfCpus;
+  EFI_STATUS Status;
+
+  if (!FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    return;
+  }
+
+  // PcdCpuHotPlugSupport => PcdCpuMaxLogicalProcessorNumber
+  mMaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+
+  // No spare CPUs to eject
+  if (mMaxNumberOfCpus == 1) {
+    return;
+  }
+
+  mCpuHotEjectData =
+    (CPU_HOT_EJECT_DATA *)AllocatePool (sizeof (*mCpuHotEjectData));
+  ASSERT (mCpuHotEjectData != NULL);
+
+  //
+  // Allocate buffer for pointers to array in CPU_HOT_EJECT_DATA.
+  //
+  mCpuHotEjectData->Revision = CPU_HOT_EJECT_DATA_REVISION_1;   // Revision
+  mCpuHotEjectData->ArrayLength = mMaxNumberOfCpus;             // Array Length of APIC ID
+  mCpuHotEjectData->ApicIdMap =                                 // CpuIndex -> APIC ID map
+    (UINT64 *)AllocatePool (sizeof (UINT64) * mCpuHotEjectData->ArrayLength);
+  mCpuHotEjectData->Handler = NULL;                             // Hot Eject handler
+  mCpuHotEjectData->Handler = 0;                                // Reserved
+
+  ASSERT (mCpuHotEjectData->ApicIdMap != NULL);
+
+  //
+  // Expose address of CPU Hot eject Data structure
+  //
+  Status = PcdSet64S (PcdCpuHotEjectDataAddress, (UINT64)(VOID *)mCpuHotEjectData);
+  ASSERT_EFI_ERROR (Status);
+}
+
 /**
   Hook point in normal execution mode that allows the one CPU that was elected
   as monarch during System Management Mode initialization to perform additional
@@ -188,6 +244,9 @@ SmmCpuFeaturesSmmRelocationComplete (
   UINTN      MapPagesBase;
   UINTN      MapPagesCount;
 
+
+  SmmCpuFeaturesSmmInitHotEject();
+
   if (!MemEncryptSevIsEnabled ()) {
     return;
   }
@@ -375,6 +434,15 @@ SmmCpuFeaturesRendezvousExit (
   IN UINTN  CpuIndex
   )
 {
+  //
+  // CPU Hot eject not enabled.
+  //
+  if (mCpuHotEjectData == NULL ||
+      mCpuHotEjectData->Handler == NULL) {
+    return;
+  }
+
+  mCpuHotEjectData->Handler(CpuIndex);
 }
 
 /**
-- 
2.9.3

[PATCH v5 7/9] OvmfPkg/CpuHotplugSmm: add CpuEject()

[Ankur Arora]

Add CpuEject(), which handles the CPU ejection, and provides
a holding area for said CPUs. It is called via
SmmCpuFeaturesRendezvousExit(), at the tail end of the SMI
handling.

Also UnplugCpus() now stashes APIC IDs of CPUs which need to
be ejected in CPU_HOT_EJECT_DATA.ApicIdMap. These are used by
CpuEject() to identify such CPUs.

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>
---
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 94 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index d165d6ccea0d..99e6845a12d9 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -14,6 +14,7 @@
 #include <Library/MmServicesTableLib.h>      // gMmst
 #include <Library/PcdLib.h>                  // PcdGetBool()
 #include <Library/SafeIntLib.h>              // SafeUintnSub()
+#include <Library/CpuHotEjectData.h>         // CPU_HOT_EJECT_DATA
 #include <Protocol/MmCpuIo.h>                // EFI_MM_CPU_IO_PROTOCOL
 #include <Protocol/SmmCpuService.h>          // EFI_SMM_CPU_SERVICE_PROTOCOL
 #include <Uefi/UefiBaseType.h>               // EFI_STATUS
@@ -32,11 +33,12 @@ STATIC EFI_MM_CPU_IO_PROTOCOL *mMmCpuIo;
 //
 STATIC EFI_SMM_CPU_SERVICE_PROTOCOL *mMmCpuService;
 //
-// This structure is a communication side-channel between the
+// These structures serve as communication side-channels between the
 // EFI_SMM_CPU_SERVICE_PROTOCOL consumer (i.e., this driver) and provider
 // (i.e., PiSmmCpuDxeSmm).
 //
 STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
+STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData;
 //
 // SMRAM arrays for fetching the APIC IDs of processors with pending events (of
 // known event types), for the time of just one MMI.
@@ -182,6 +184,43 @@ Fatal:
 }
 
 /**
+  CPU Hot-eject handler function.
+
+  If the executing CPU is not being ejected: NOP.
+  If the executing CPU is being ejected: wait in a CpuDeadLoop()
+  until ejected.
+
+  @param[in] ProcessorNum      Index of executing CPU.
+**/
+VOID
+EFIAPI
+CpuEject(
+  IN UINTN ProcessorNum
+  )
+{
+  //
+  // APIC ID is UINT32, but mCpuHotEjectData->ApicIdMap[] is UINT64
+  // so use UINT64 throughout.
+  //
+  UINT64 ApicId;
+
+  ApicId = mCpuHotEjectData->ApicIdMap[ProcessorNum];
+  if (ApicId == CPU_EJECT_INVALID) {
+    return;
+  }
+
+  //
+  // CPU(s) being unplugged get here from SmmCpuFeaturesSmiRendezvousExit()
+  // after having been cleared to exit the SMI by the monarch and thus have
+  // no SMM processing remaining.
+  //
+  // Given that we cannot allow them to escape to the guest, we pen them
+  // here until the SMM monarch tells the HW to unplug them.
+  //
+  CpuDeadLoop ();
+}
+
+/**
   CPU Hot-unplug MMI handler function.
 
   @param[in] UnplugApicIds       List of APIC IDs to be unplugged.
@@ -203,9 +242,11 @@ UnplugCpus(
 {
   EFI_STATUS Status;
   UINT32     ToUnplugIdx;
+  UINT32     EjectCount;
   UINTN      ProcessorNum;
 
   ToUnplugIdx = 0;
+  EjectCount = 0;
   while (ToUnplugIdx < ToUnplugCount) {
     APIC_ID    RemoveApicId;
 
@@ -242,11 +283,35 @@ UnplugCpus(
       DEBUG ((DEBUG_ERROR, "%a: RemoveProcessor(" FMT_APIC_ID "): %r\n",
         __FUNCTION__, RemoveApicId, Status));
       goto Fatal;
+    } else {
+      //
+      // Stash the APIC IDs so we can do the actual unplug later.
+      //
+      if (mCpuHotEjectData->ApicIdMap[ProcessorNum] != CPU_EJECT_INVALID) {
+        //
+        // Since ProcessorNum and APIC-ID map 1-1, so a valid
+        // mCpuHotEjectData->ApicIdMap[ProcessorNum] means something
+        // is horribly wrong.
+        //
+        DEBUG ((DEBUG_ERROR, "%a: ProcessorNum %u maps to %llx, cannot map to " FMT_APIC_ID "\n",
+          __FUNCTION__, ProcessorNum, mCpuHotEjectData->ApicIdMap[ProcessorNum], RemoveApicId));
+        goto Fatal;
+      }
+
+      mCpuHotEjectData->ApicIdMap[ProcessorNum] = (UINT64)RemoveApicId;
+      EjectCount++;
     }
 
     ToUnplugIdx++;
   }
 
+  if (EjectCount != 0) {
+    //
+    // We have processors to be ejected; install the handler.
+    //
+    mCpuHotEjectData->Handler = CpuEject;
+  }
+
   //
   // We've handled this unplug.
   //
@@ -445,7 +510,13 @@ CpuHotplugEntry (
   // Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm
   // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM.
   //
+  // Additionally, CPU Hot-unplug is available only if CPU Hotplug is, so
+  // the same DEPEX also guarantees that PcdCpuHotEjectDataAddress points
+  // to CPU_HOT_EJECT_DATA in SMRAM.
+  //
   mCpuHotPlugData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotPlugDataAddress);
+  mCpuHotEjectData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotEjectDataAddress);
+
   if (mCpuHotPlugData == NULL) {
     Status = EFI_NOT_FOUND;
     DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_PLUG_DATA: %r\n", __FUNCTION__, Status));
@@ -457,6 +528,9 @@ CpuHotplugEntry (
   if (mCpuHotPlugData->ArrayLength == 1) {
     return EFI_UNSUPPORTED;
   }
+  ASSERT (mCpuHotEjectData &&
+          (mCpuHotPlugData->ArrayLength == mCpuHotEjectData->ArrayLength));
+
   //
   // Allocate the data structures that depend on the possible CPU count.
   //
@@ -539,6 +613,24 @@ CpuHotplugEntry (
   //
   SmbaseInstallFirstSmiHandler ();
 
+  if (mCpuHotEjectData) {
+  UINT32     Idx;
+    //
+    // For CPU ejection we need to map ProcessorNum -> APIC_ID. By the time
+    // we do that, however, the Processor's APIC ID has already been removed
+    // from SMM data structures. So we will use mCpuHotEjectData->ApicIdMap
+    // to map from ProcessorNum -> APIC_ID.
+    //
+    for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
+      mCpuHotEjectData->ApicIdMap[Idx] = CPU_EJECT_INVALID;
+    }
+
+    //
+    // Wait to init the handler until an ejection is warranted
+    //
+    mCpuHotEjectData->Handler = NULL;
+  }
+
   return EFI_SUCCESS;
 
 ReleasePostSmmPen:
-- 
2.9.3

[PATCH v5 8/9] OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection

[Ankur Arora]

Designate a worker CPU (we use the one executing the root MMI
handler), which will do the actual ejection via QEMU in CpuEject().

CpuEject(), on the worker CPU, ejects each marked CPU by first
selecting its APIC ID and then sending the QEMU "eject" command.
QEMU in-turn signals the remote VCPU thread which context-switches
it out of the SMI.

CpuEject(), on the CPU being ejected, spins around in its holding
area until this final context-switch. This does mean that there is
some CPU state that would ordinarily be restored (in SmiRendezvous()
and in SmiEntry.nasm::CommonHandler), but will not be anymore.
This unrestored state includes FPU state, CET enable, stuffing of
RSB and the final RSM. Since the CPU state is destroyed by QEMU,
this should be okay.

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>
---
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 73 ++++++++++++++++++++++++++++++++++----
 1 file changed, 67 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 99e6845a12d9..d4e27d641dc5 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -186,10 +186,13 @@ Fatal:
 /**
   CPU Hot-eject handler function.
 
-  If the executing CPU is not being ejected: NOP.
+  If the executing CPU is neither a worker, nor being ejected: NOP.
   If the executing CPU is being ejected: wait in a CpuDeadLoop()
   until ejected.
 
+  If the executing CPU is a worker CPU: set QEMU Cpu status to eject
+  for CPUs being ejected.
+
   @param[in] ProcessorNum      Index of executing CPU.
 **/
 VOID
@@ -209,6 +212,56 @@ CpuEject(
     return;
   }
 
+  if (ApicId == CPU_EJECT_WORKER) {
+    UINT32 CpuIndex;
+
+    for (CpuIndex = 0; CpuIndex < mCpuHotEjectData->ArrayLength; CpuIndex++) {
+      UINT64 RemoveApicId;
+
+      RemoveApicId = mCpuHotEjectData->ApicIdMap[CpuIndex];
+
+      if ((RemoveApicId != CPU_EJECT_INVALID &&
+           RemoveApicId != CPU_EJECT_WORKER)) {
+        //
+        // This to-be-ejected-CPU has already received the BSP's SMI exit
+        // signal and, will execute SmmCpuFeaturesSmiRendezvousExit()
+        // followed by this callback or is already waiting in the
+        // CpuDeadLoop() below.
+        //
+        // Tell QEMU to context-switch it out.
+        //
+        QemuCpuhpWriteCpuSelector (mMmCpuIo, (APIC_ID) RemoveApicId);
+        QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
+
+        //
+        // Compiler barrier to ensure the next store isn't reordered
+        //
+        MemoryFence();
+
+        //
+        // Clear the eject status for CpuIndex to ensure that an invalid
+        // SMI later does not end up trying to eject it or a newly
+        // hotplugged CpuIndex does not go into the dead loop.
+        //
+        mCpuHotEjectData->ApicIdMap[CpuIndex] = CPU_EJECT_INVALID;
+
+        DEBUG ((DEBUG_INFO, "%a: Unplugged CPU %u -> " FMT_APIC_ID "\n",
+               __FUNCTION__, CpuIndex, RemoveApicId));
+      }
+    }
+
+    //
+    // Clear our own worker status.
+    //
+    mCpuHotEjectData->ApicIdMap[ProcessorNum] = CPU_EJECT_INVALID;
+
+    //
+    // We are done until the next hot-unplug; clear the handler.
+    //
+    mCpuHotEjectData->Handler = NULL;
+    return;
+  }
+
   //
   // CPU(s) being unplugged get here from SmmCpuFeaturesSmiRendezvousExit()
   // after having been cleared to exit the SMI by the monarch and thus have
@@ -306,6 +359,19 @@ UnplugCpus(
   }
 
   if (EjectCount != 0) {
+    UINTN  Worker;
+
+    Status = mMmCpuService->WhoAmI(mMmCpuService, &Worker);
+    ASSERT_EFI_ERROR(Status);
+    //
+    // UnplugCpus() is called via the root MMI handler and thus we are
+    // executing in the BSP context.
+    //
+    // Mark ourselves as the worker CPU.
+    //
+    ASSERT (mCpuHotEjectData->ApicIdMap[Worker] == CPU_EJECT_INVALID);
+    mCpuHotEjectData->ApicIdMap[Worker] = CPU_EJECT_WORKER;
+
     //
     // We have processors to be ejected; install the handler.
     //
@@ -429,11 +495,6 @@ CpuHotplugMmi (
   if (EFI_ERROR (Status)) {
     goto Fatal;
   }
-  if (ToUnplugCount > 0) {
-    DEBUG ((DEBUG_ERROR, "%a: hot-unplug is not supported yet\n",
-      __FUNCTION__));
-    goto Fatal;
-  }
 
   if (PluggedCount > 0) {
     Status = PlugCpus(mPluggedApicIds, PluggedCount);
-- 
2.9.3

[PATCH v5 9/9] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug

[Ankur Arora]

As part of the negotiation treat ICH9_LPC_SMI_F_CPU_HOT_UNPLUG as a
subfeature of feature flag ICH9_LPC_SMI_F_CPU_HOTPLUG, so enable it
only if the other is also being negotiated.

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>
---
 OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
index c9d875543205..af01104d69c6 100644
--- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
+++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
@@ -29,6 +29,13 @@
 //
 #define ICH9_LPC_SMI_F_CPU_HOTPLUG BIT1
 
+// The following bit value stands for "enable CPU hot unplug, and inject an SMI
+// with control value ICH9_APM_CNT_CPU_HOT_UNPLUG upon hot unplug", in the
+// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
+// Can only be negotiated alongside ICH9_LPC_SMI_F_CPU_HOTPLUG.
+//
+#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG BIT2
+
 //
 // Provides a scratch buffer (allocated in EfiReservedMemoryType type memory)
 // for the S3 boot script fragment to write to and read from.
@@ -112,7 +119,8 @@ NegotiateSmiFeatures (
   QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures);
 
   //
-  // We want broadcast SMI, SMI on CPU hotplug, and nothing else.
+  // We want broadcast SMI, SMI on CPU hotplug, on CPU hot-unplug
+  // and nothing else.
   //
   RequestedFeaturesMask = ICH9_LPC_SMI_F_BROADCAST;
   if (!MemEncryptSevIsEnabled ()) {
@@ -120,8 +128,18 @@ NegotiateSmiFeatures (
     // For now, we only support hotplug with SEV disabled.
     //
     RequestedFeaturesMask |= ICH9_LPC_SMI_F_CPU_HOTPLUG;
+    RequestedFeaturesMask |= ICH9_LPC_SMI_F_CPU_HOT_UNPLUG;
   }
   mSmiFeatures &= RequestedFeaturesMask;
+
+  if (!(mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOTPLUG) &&
+      (mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOT_UNPLUG)) {
+    DEBUG ((DEBUG_WARN, "%a CPU host-features %Lx, requested mask %Lx\n",
+      __FUNCTION__, mSmiFeatures, RequestedFeaturesMask));
+
+    mSmiFeatures &= ~ICH9_LPC_SMI_F_CPU_HOT_UNPLUG;
+  }
+
   QemuFwCfgSelectItem (mRequestedFeaturesItem);
   QemuFwCfgWriteBytes (sizeof mSmiFeatures, &mSmiFeatures);
 
@@ -162,8 +180,9 @@ NegotiateSmiFeatures (
   if ((mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOTPLUG) == 0) {
     DEBUG ((DEBUG_INFO, "%a: CPU hotplug not negotiated\n", __FUNCTION__));
   } else {
-    DEBUG ((DEBUG_INFO, "%a: CPU hotplug with SMI negotiated\n",
-      __FUNCTION__));
+    DEBUG ((DEBUG_INFO, "%a: CPU hotplug%s with SMI negotiated\n",
+      __FUNCTION__,
+      (mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOT_UNPLUG) ? ", unplug" : ""));
   }
 
   //
-- 
2.9.3

Re: [edk2-devel] [PATCH v5 0/9] support CPU hot-unplug

[Laszlo Ersek]

Hi Ankur,

On 01/26/21 07:44, Ankur Arora wrote:
> Hi,
> 
> This series adds support for CPU hot-unplug with OVMF.
> 
> Please see this in conjunction with the QEMU secureboot hot-unplug v2
> series posted here (now upstreamed):
>   https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
> 
> Patches 1 and 3,
>   ("OvmfPkg/CpuHotplugSmm: refactor hotplug logic")
>   ("OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper")
> are either refactors or add support functions.
> 
>   OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
>   OvmfPkg/CpuHotplugSmm: add CpuEject()
>   OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
> 
> Patch 2 and 9,
>   ("OvmfPkg/CpuHotplugSmm: collect hot-unplug events")
>   ("OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug")
> handle the QEMU protocol logic for collection of CPU hot-unplug events
> or the protocol negotiation.
> 
> Patch 4,
>   ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()")
> adds the MMI logic for CPU hot-unplug handling and informing
> the PiSmmCpuDxeSmm of CPU removal.
> 
> Patches 5 and 6,
>   ("OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA")
>   ("OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state")
> sets up state for doing the CPU ejection as part of hot-unplug.
> 
> Patches 7, and 8,
>   ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()")
>   ("OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection")
> add the CPU ejection logic.
> 
> Testing (with QEMU 5.2.50):
>  - Stable with randomized CPU plug/unplug (guest maxcpus=1,8,128)
>  - Synthetic tests with simultaneous multi CPU hot-unplug
>  - Negotiation with/without CPU hotplug enabled
> 
> Also at:
>   github.com/terminus/edk2/ hot-unplug-v5
> 
> Changelog:
> v5:
>   - fixes ECC errors (all but one in "OvmfPkg/CpuHotplugSmm: add
>     add Qemu Cpu Status helper").
> 
> v4:
>   - Gets rid of unnecessary UefiCpuPkg changes
>   URL: https://patchew.org/EDK2/20210118063457.358581-1-ankur.a.arora@oracle.com/
> 
> v3:
>   - Use a saner PCD based interface to share state between PiSmmCpuDxeSmm
>     and OvmfPkg/CpuHotplugSmm
>   - Cleaner split of the hot-unplug code
>   URL: https://patchew.org/EDK2/20210115074533.277448-1-ankur.a.arora@oracle.com/
> 
> v2:
>   - Do the ejection via SmmCpuFeaturesRendezvousExit()
>   URL: https://patchew.org/EDK2/20210107195515.106158-1-ankur.a.arora@oracle.com/
> 
> RFC:
>   URL: https://patchew.org/EDK2/20201208053432.2690694-1-ankur.a.arora@oracle.com/
> 
> 
> Please review.
> 
> Thanks
> Ankur
> 
> Ankur Arora (9):
>   OvmfPkg/CpuHotplugSmm: refactor hotplug logic
>   OvmfPkg/CpuHotplugSmm: collect hot-unplug events
>   OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
>   OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
>   OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA
>   OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
>   OvmfPkg/CpuHotplugSmm: add CpuEject()
>   OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
>   OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
> 
>  OvmfPkg/OvmfPkg.dec                                |  10 +
>  OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf            |   1 +
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   3 +
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                  |   6 +
>  OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h  |   2 +
>  OvmfPkg/Include/Library/CpuHotEjectData.h          |  32 ++
>  OvmfPkg/CpuHotplugSmm/CpuHotplug.c                 | 450 ++++++++++++++++-----
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                  |  57 ++-
>  .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  68 ++++
>  OvmfPkg/SmmControl2Dxe/SmiFeatures.c               |  25 +-
>  10 files changed, 552 insertions(+), 102 deletions(-)
>  create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h
> 

This series is still mal-formatted; for some reason it has one too many
CRs per line.

Did you try to set up the "base64" or the "8bit"
content-transfer-encoding? The emails reflected through the list are
still "quoted-printable".

Anyway, I managed to apply the patches with some trickery on a local branch.

Thanks
Laszlo

Re: [PATCH v5 0/9] support CPU hot-unplug

[Laszlo Ersek]

On 01/26/21 07:44, Ankur Arora wrote:
> Hi,
> 
> This series adds support for CPU hot-unplug with OVMF.
> 
> Please see this in conjunction with the QEMU secureboot hot-unplug v2
> series posted here (now upstreamed):
>   https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
> 
> Patches 1 and 3,
>   ("OvmfPkg/CpuHotplugSmm: refactor hotplug logic")
>   ("OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper")
> are either refactors or add support functions.
> 
>   OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
>   OvmfPkg/CpuHotplugSmm: add CpuEject()
>   OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
> 
> Patch 2 and 9,
>   ("OvmfPkg/CpuHotplugSmm: collect hot-unplug events")
>   ("OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug")
> handle the QEMU protocol logic for collection of CPU hot-unplug events
> or the protocol negotiation.
> 
> Patch 4,
>   ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()")
> adds the MMI logic for CPU hot-unplug handling and informing
> the PiSmmCpuDxeSmm of CPU removal.
> 
> Patches 5 and 6,
>   ("OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA")
>   ("OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state")
> sets up state for doing the CPU ejection as part of hot-unplug.
> 
> Patches 7, and 8,
>   ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()")
>   ("OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection")
> add the CPU ejection logic.
> 
> Testing (with QEMU 5.2.50):
>  - Stable with randomized CPU plug/unplug (guest maxcpus=1,8,128)
>  - Synthetic tests with simultaneous multi CPU hot-unplug
>  - Negotiation with/without CPU hotplug enabled
> 
> Also at:
>   github.com/terminus/edk2/ hot-unplug-v5
> 
> Changelog:
> v5:
>   - fixes ECC errors (all but one in "OvmfPkg/CpuHotplugSmm: add
>     add Qemu Cpu Status helper").
> 
> v4:
>   - Gets rid of unnecessary UefiCpuPkg changes
>   URL: https://patchew.org/EDK2/20210118063457.358581-1-ankur.a.arora@oracle.com/
> 
> v3:
>   - Use a saner PCD based interface to share state between PiSmmCpuDxeSmm
>     and OvmfPkg/CpuHotplugSmm
>   - Cleaner split of the hot-unplug code
>   URL: https://patchew.org/EDK2/20210115074533.277448-1-ankur.a.arora@oracle.com/
> 
> v2:
>   - Do the ejection via SmmCpuFeaturesRendezvousExit()
>   URL: https://patchew.org/EDK2/20210107195515.106158-1-ankur.a.arora@oracle.com/
> 
> RFC:
>   URL: https://patchew.org/EDK2/20201208053432.2690694-1-ankur.a.arora@oracle.com/
> 
> 
> Please review.
> 
> Thanks
> Ankur
> 
> Ankur Arora (9):
>   OvmfPkg/CpuHotplugSmm: refactor hotplug logic
>   OvmfPkg/CpuHotplugSmm: collect hot-unplug events
>   OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
>   OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
>   OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA
>   OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
>   OvmfPkg/CpuHotplugSmm: add CpuEject()
>   OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
>   OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
> 
>  OvmfPkg/OvmfPkg.dec                                |  10 +
>  OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf            |   1 +
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   3 +
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                  |   6 +
>  OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h  |   2 +
>  OvmfPkg/Include/Library/CpuHotEjectData.h          |  32 ++
>  OvmfPkg/CpuHotplugSmm/CpuHotplug.c                 | 450 ++++++++++++++++-----
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                  |  57 ++-
>  .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  68 ++++
>  OvmfPkg/SmmControl2Dxe/SmiFeatures.c               |  25 +-
>  10 files changed, 552 insertions(+), 102 deletions(-)
>  create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h
> 

Comparing the maximum line lengths for each file modified, before and
after the series:

- before:

    73 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
    78 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
    79 OvmfPkg/CpuHotplugSmm/CpuHotplug.c
    79 OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
    79 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
    79 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
    79 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
    79 OvmfPkg/SmmControl2Dxe/SmiFeatures.c
   120 OvmfPkg/OvmfPkg.dec
   120 total

- after:

    73 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
    78 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
    79 OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
    79 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
    79 OvmfPkg/SmmControl2Dxe/SmiFeatures.c
    85 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
    90 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
    91 OvmfPkg/Include/Library/CpuHotEjectData.h
    97 OvmfPkg/CpuHotplugSmm/CpuHotplug.c
   120 OvmfPkg/OvmfPkg.dec
   120 total

Please rework the patches that modify:

    85 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
    90 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
    91 OvmfPkg/Include/Library/CpuHotEjectData.h
    97 OvmfPkg/CpuHotplugSmm/CpuHotplug.c

so that their line lengths remain under 80 characters.

Thanks
Laszlo

Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

[Laszlo Ersek]

On 01/26/21 07:44, Ankur Arora wrote:
> Refactor CpuHotplugMmi() to pull out the CPU hotplug logic into
> PlugCpus(). This is in preparation for supporting CPU hot-unplug.
>
> 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>
> ---
>  OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 208 ++++++++++++++++++++++---------------
>  1 file changed, 123 insertions(+), 85 deletions(-)
>
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index cfe698ed2b5e..a5052a501e5a 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -62,6 +62,124 @@ STATIC UINT32 mPostSmmPenAddress;
>  //
>  STATIC EFI_HANDLE mDispatchHandle;
>
> +/**
> +  CPU Hotplug handler function.
> +
> +  @param[in] PluggedApicIds      List of APIC IDs to be plugged.
> +
> +  @param[in] PluggedCount        Count of APIC IDs to be plugged.

(1) These comments are not optimal.

The variable names say "Plugged", meaning that the CPUs have already
been plugged, from the QEMU perspective. The purpose of this function is
to go over those CPUs whose APIC IDs have been collected with events
pending, relocate the SMBASE on each, and then expose each such CPU to
EFI_SMM_CPU_SERVICE_PROTOCOL. For a given CPU, I think the comment on
the EFI_SMM_ADD_PROCESSOR prototype
[UefiCpuPkg/Include/Protocol/SmmCpuService.h] best expresses the goal:
"Notify that a new processor has been added to the system ... The SMM
CPU driver should add the processor to the SMM CPU list".

See also the comment on QemuCpuhpCollectApicIds():

"""
  Collect the APIC IDs of
  - the CPUs that have been hot-plugged,
  - the CPUs that are about to be hot-unplugged.
"""

This closely reflects which agent (firmware vs. QEMU) is driving each
particular operation / direction.

(1a) So please replace the leading comment with:

  Process those CPUs that have been hot-added, according to
  QemuCpuhpCollectApicIds().

  For each such CPU, relocate the SMBASE, and report the CPU to PiSmmCpuDxeSmm
  via EFI_SMM_CPU_SERVICE_PROTOCOL. If a supposedly hot-added CPU is already
  known, skip it silently.

(1b) Similarly, in the parameter comments, "to be plugged" is wrong. I
suggest copying the parameter descriptions from
QemuCpuhpCollectApicIds():

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


> +
> +  @retval EFI_SUCCESS            Some of the requested APIC IDs were hot-plugged.

(2) This is inexact; on successful return, each one of the collected
CPUs has been relocated and exposed to the SMM CPU driver. (Either in
this particular invocation, or in an earlier invocation, but on success,
there is no entry in PluggedApicIds that is *not known* to the SMM CPU
driver, or whose SMBASE is not relocated.)


> +
> +  @retval EFI_INTERRUPT_PENDING  Fatal error while hot-plugging.

(3) This error code is very uncommon, and it is mostly/only required
from the function -- CpuHotplugMmi() -- that is registered with
gMmst->MmiHandlerRegister(). The meaning of the error code is, "The MMI
source could not be quiesced", which is a situation that can be
identified at the level of CpuHotplugMmi(), but not at the level of
PlugCpus().

Please list the following return values instead:

  @retval EFI_OUT_OF_RESOURCES  Out of APIC ID space in "mCpuHotPlugData".

  @return                       Error codes propagated from SmbaseRelocate()
                                and mMmCpuService->AddProcessor().

(General remark, in addition: please note the difference between
"@retval" and "@return". The latter does not name a particular value;
the set of values is described in natural language instead.)


> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI

(4) There is no need to make this function EFIAPI.


> +PlugCpus(

(5) Space character missing between function name and opening
parenthesis.

Please check every function prototype and function call for this -- one
space char before the opening paren is required, except in the
definition of function-like macros (where the language standard requires
the "(" to be directly adjacent).


(6) According to the discussion above, this function should be called
ProcessHotAddedCpus().

... The best hint for this function name is actually the comment that
sits atop the stretch of code you are extracting, namely:

  //
  // Process hot-added CPUs.
  //


> +  IN APIC_ID                      *PluggedApicIds,
> +  IN UINT32                       PluggedCount
> +  )
> +{
> +  EFI_STATUS Status;
> +  UINT32     PluggedIdx;
> +  UINT32     NewSlot;
> +
> +  //
> +  // Process hot-added CPUs.

(7) This short introductory comment is no longer needed, as it should be
promoted to the name of the function.


> +  //
> +  // The Post-SMM Pen need not be reinstalled multiple times within a single
> +  // root MMI handling. Even reinstalling once per root MMI is only prudence;
> +  // in theory installing the pen in the driver's entry point function should
> +  // suffice.
> +  //
> +  SmbaseReinstallPostSmmPen (mPostSmmPenAddress);
> +
> +  PluggedIdx = 0;
> +  NewSlot = 0;
> +  while (PluggedIdx < PluggedCount) {
> +    APIC_ID NewApicId;
> +    UINT32  CheckSlot;
> +    UINTN   NewProcessorNumberByProtocol;
> +
> +    NewApicId = PluggedApicIds[PluggedIdx];
> +
> +    //
> +    // Check if the supposedly hot-added CPU is already known to us.
> +    //
> +    for (CheckSlot = 0;
> +         CheckSlot < mCpuHotPlugData->ArrayLength;
> +         CheckSlot++) {
> +      if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {
> +        break;
> +      }
> +    }
> +    if (CheckSlot < mCpuHotPlugData->ArrayLength) {
> +      DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "
> +        "before; ignoring it\n", __FUNCTION__, NewApicId));
> +      PluggedIdx++;
> +      continue;
> +    }
> +
> +    //
> +    // Find the first empty slot in CPU_HOT_PLUG_DATA.
> +    //
> +    while (NewSlot < mCpuHotPlugData->ArrayLength &&
> +           mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) {
> +      NewSlot++;
> +    }
> +    if (NewSlot == mCpuHotPlugData->ArrayLength) {
> +      DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n",
> +        __FUNCTION__, NewApicId));
> +      goto Fatal;

(8) Please replace the "goto" with "return EFI_OUT_OF_RESOURCES".


> +    }
> +
> +    //
> +    // Store the APIC ID of the new processor to the slot.
> +    //
> +    mCpuHotPlugData->ApicId[NewSlot] = NewApicId;
> +
> +    //
> +    // Relocate the SMBASE of the new CPU.
> +    //
> +    Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot],
> +               mPostSmmPenAddress);
> +    if (EFI_ERROR (Status)) {
> +      goto RevokeNewSlot;
> +    }
> +
> +    //
> +    // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
> +    //
> +    Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId,
> +                              &NewProcessorNumberByProtocol);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n",
> +        __FUNCTION__, NewApicId, Status));
> +      goto RevokeNewSlot;
> +    }
> +
> +    DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, "
> +      "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__,
> +      NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot],
> +      (UINT64)NewProcessorNumberByProtocol));
> +
> +    NewSlot++;
> +    PluggedIdx++;
> +  }
> +
> +  //
> +  // We've handled this hotplug.
> +  //

(9) I suggest: "We've processed this batch of hot-added CPUs.".


> +  return EFI_SUCCESS;
> +
> +RevokeNewSlot:
> +  mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
> +
> +Fatal:

(10) Please drop this label.


> +  return EFI_INTERRUPT_PENDING;

(11) This should be "return Status".


> +}
>
>  /**
>    CPU Hotplug MMI handler function.
> @@ -122,8 +240,6 @@ CpuHotplugMmi (
>    UINT8      ApmControl;
>    UINT32     PluggedCount;
>    UINT32     ToUnplugCount;
> -  UINT32     PluggedIdx;
> -  UINT32     NewSlot;
>
>    //
>    // Assert that we are entering this function due to our root MMI handler
> @@ -179,87 +295,12 @@ CpuHotplugMmi (
>      goto Fatal;
>    }
>
> -  //
> -  // Process hot-added CPUs.
> -  //
> -  // The Post-SMM Pen need not be reinstalled multiple times within a single
> -  // root MMI handling. Even reinstalling once per root MMI is only prudence;
> -  // in theory installing the pen in the driver's entry point function should
> -  // suffice.
> -  //
> -  SmbaseReinstallPostSmmPen (mPostSmmPenAddress);
> +  if (PluggedCount > 0) {
> +    Status = PlugCpus(mPluggedApicIds, PluggedCount);

(12) Space missing before "(".


> +  }
>
> -  PluggedIdx = 0;
> -  NewSlot = 0;
> -  while (PluggedIdx < PluggedCount) {
> -    APIC_ID NewApicId;
> -    UINT32  CheckSlot;
> -    UINTN   NewProcessorNumberByProtocol;
> -
> -    NewApicId = mPluggedApicIds[PluggedIdx];
> -
> -    //
> -    // Check if the supposedly hot-added CPU is already known to us.
> -    //
> -    for (CheckSlot = 0;
> -         CheckSlot < mCpuHotPlugData->ArrayLength;
> -         CheckSlot++) {
> -      if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {
> -        break;
> -      }
> -    }
> -    if (CheckSlot < mCpuHotPlugData->ArrayLength) {
> -      DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "
> -        "before; ignoring it\n", __FUNCTION__, NewApicId));
> -      PluggedIdx++;
> -      continue;
> -    }
> -
> -    //
> -    // Find the first empty slot in CPU_HOT_PLUG_DATA.
> -    //
> -    while (NewSlot < mCpuHotPlugData->ArrayLength &&
> -           mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) {
> -      NewSlot++;
> -    }
> -    if (NewSlot == mCpuHotPlugData->ArrayLength) {
> -      DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n",
> -        __FUNCTION__, NewApicId));
> -      goto Fatal;
> -    }
> -
> -    //
> -    // Store the APIC ID of the new processor to the slot.
> -    //
> -    mCpuHotPlugData->ApicId[NewSlot] = NewApicId;
> -
> -    //
> -    // Relocate the SMBASE of the new CPU.
> -    //
> -    Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot],
> -               mPostSmmPenAddress);
> -    if (EFI_ERROR (Status)) {
> -      goto RevokeNewSlot;
> -    }
> -
> -    //
> -    // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
> -    //
> -    Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId,
> -                              &NewProcessorNumberByProtocol);
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n",
> -        __FUNCTION__, NewApicId, Status));
> -      goto RevokeNewSlot;
> -    }
> -
> -    DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, "
> -      "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__,
> -      NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot],
> -      (UINT64)NewProcessorNumberByProtocol));
> -
> -    NewSlot++;
> -    PluggedIdx++;
> +  if (EFI_ERROR(Status)) {
> +    goto Fatal;
>    }

(13) Without having seen the rest of the patches, I think this error
check should be nested under the same (PluggedCount > 0) condition; in
other words, I think it only makes sense to check Status after we
actually call ProcessHotAddedCpus().


>
>    //
> @@ -267,9 +308,6 @@ CpuHotplugMmi (
>    //
>    return EFI_SUCCESS;
>
> -RevokeNewSlot:
> -  mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
> -
>  Fatal:
>    ASSERT (FALSE);
>    CpuDeadLoop ();
>

I'll continue the review later this week.

Thanks
Laszlo

Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

[Laszlo Ersek]

On 01/26/21 20:01, Laszlo Ersek wrote:

> (1b) Similarly, in the parameter comments, "to be plugged" is wrong. I
> suggest copying the parameter descriptions from
> QemuCpuhpCollectApicIds():
> 
>   @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.

(Of course, in this location, the parameters are [in], not [out].)

Thanks
Laszlo

Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

[Laszlo Ersek]

On 01/26/21 20:01, Laszlo Ersek wrote:
> On 01/26/21 07:44, Ankur Arora wrote:

>> +  if (EFI_ERROR(Status)) {
>> +    goto Fatal;
>>    }
> 
> (13) Without having seen the rest of the patches, I think this error
> check should be nested under the same (PluggedCount > 0) condition; in
> other words, I think it only makes sense to check Status after we
> actually call ProcessHotAddedCpus().

(14) Missing space after "EFI_ERROR".

(I'll not point out further instances of this issue; please review all
the patches with regard to it.)

Thanks
Laszlo

Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

[Ankur Arora]

On 2021-01-26 11:01 a.m., Laszlo Ersek wrote:
> On 01/26/21 07:44, Ankur Arora wrote:
>> Refactor CpuHotplugMmi() to pull out the CPU hotplug logic into
>> PlugCpus(). This is in preparation for supporting CPU hot-unplug.
>>
>> 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>
>> ---
>>   OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 208 ++++++++++++++++++++++---------------
>>   1 file changed, 123 insertions(+), 85 deletions(-)
>>
>> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
>> index cfe698ed2b5e..a5052a501e5a 100644
>> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
>> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
>> @@ -62,6 +62,124 @@ STATIC UINT32 mPostSmmPenAddress;
>>   //
>>   STATIC EFI_HANDLE mDispatchHandle;
>>
>> +/**
>> +  CPU Hotplug handler function.
>> +
>> +  @param[in] PluggedApicIds      List of APIC IDs to be plugged.
>> +
>> +  @param[in] PluggedCount        Count of APIC IDs to be plugged.
> 
> (1) These comments are not optimal.
> 
> The variable names say "Plugged", meaning that the CPUs have already
> been plugged, from the QEMU perspective. The purpose of this function is
> to go over those CPUs whose APIC IDs have been collected with events
> pending, relocate the SMBASE on each, and then expose each such CPU to
> EFI_SMM_CPU_SERVICE_PROTOCOL. For a given CPU, I think the comment on
> the EFI_SMM_ADD_PROCESSOR prototype
> [UefiCpuPkg/Include/Protocol/SmmCpuService.h] best expresses the goal:
> "Notify that a new processor has been added to the system ... The SMM
> CPU driver should add the processor to the SMM CPU list".
> 
> See also the comment on QemuCpuhpCollectApicIds():
> 
> """
>    Collect the APIC IDs of
>    - the CPUs that have been hot-plugged,
>    - the CPUs that are about to be hot-unplugged.
> """
> 
> This closely reflects which agent (firmware vs. QEMU) is driving each
> particular operation / direction.
> 
> (1a) So please replace the leading comment with:
> 
>    Process those CPUs that have been hot-added, according to
>    QemuCpuhpCollectApicIds().
> 
>    For each such CPU, relocate the SMBASE, and report the CPU to PiSmmCpuDxeSmm
>    via EFI_SMM_CPU_SERVICE_PROTOCOL. If a supposedly hot-added CPU is already
>    known, skip it silently.
> 
> (1b) Similarly, in the parameter comments, "to be plugged" is wrong. I
> suggest copying the parameter descriptions from
> QemuCpuhpCollectApicIds():
> 
>    @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.
> 
> 
>> +
>> +  @retval EFI_SUCCESS            Some of the requested APIC IDs were hot-plugged.
> 
> (2) This is inexact; on successful return, each one of the collected
> CPUs has been relocated and exposed to the SMM CPU driver. (Either in
> this particular invocation, or in an earlier invocation, but on success,
> there is no entry in PluggedApicIds that is *not known* to the SMM CPU
> driver, or whose SMBASE is not relocated.)
> 
> 
>> +
>> +  @retval EFI_INTERRUPT_PENDING  Fatal error while hot-plugging.
> 
> (3) This error code is very uncommon, and it is mostly/only required
> from the function -- CpuHotplugMmi() -- that is registered with
> gMmst->MmiHandlerRegister(). The meaning of the error code is, "The MMI
> source could not be quiesced", which is a situation that can be
> identified at the level of CpuHotplugMmi(), but not at the level of
> PlugCpus().
> 
> Please list the following return values instead:
> 
>    @retval EFI_OUT_OF_RESOURCES  Out of APIC ID space in "mCpuHotPlugData".
> 
>    @return                       Error codes propagated from SmbaseRelocate()
>                                  and mMmCpuService->AddProcessor().
> 
> (General remark, in addition: please note the difference between
> "@retval" and "@return". The latter does not name a particular value;
> the set of values is described in natural language instead.)
> 
> 
>> +
>> +**/
>> +STATIC
>> +EFI_STATUS
>> +EFIAPI
> 
> (4) There is no need to make this function EFIAPI.
> 
> 
>> +PlugCpus(
> 
> (5) Space character missing between function name and opening
> parenthesis.
> 
> Please check every function prototype and function call for this -- one
> space char before the opening paren is required, except in the
> definition of function-like macros (where the language standard requires
> the "(" to be directly adjacent).
> 
> 
> (6) According to the discussion above, this function should be called
> ProcessHotAddedCpus().
> 
> ... The best hint for this function name is actually the comment that
> sits atop the stretch of code you are extracting, namely:
> 
>    //
>    // Process hot-added CPUs.
>    //
> 
> 
>> +  IN APIC_ID                      *PluggedApicIds,
>> +  IN UINT32                       PluggedCount
>> +  )
>> +{
>> +  EFI_STATUS Status;
>> +  UINT32     PluggedIdx;
>> +  UINT32     NewSlot;
>> +
>> +  //
>> +  // Process hot-added CPUs.
> 
> (7) This short introductory comment is no longer needed, as it should be
> promoted to the name of the function.
> 
> 
>> +  //
>> +  // The Post-SMM Pen need not be reinstalled multiple times within a single
>> +  // root MMI handling. Even reinstalling once per root MMI is only prudence;
>> +  // in theory installing the pen in the driver's entry point function should
>> +  // suffice.
>> +  //
>> +  SmbaseReinstallPostSmmPen (mPostSmmPenAddress);
>> +
>> +  PluggedIdx = 0;
>> +  NewSlot = 0;
>> +  while (PluggedIdx < PluggedCount) {
>> +    APIC_ID NewApicId;
>> +    UINT32  CheckSlot;
>> +    UINTN   NewProcessorNumberByProtocol;
>> +
>> +    NewApicId = PluggedApicIds[PluggedIdx];
>> +
>> +    //
>> +    // Check if the supposedly hot-added CPU is already known to us.
>> +    //
>> +    for (CheckSlot = 0;
>> +         CheckSlot < mCpuHotPlugData->ArrayLength;
>> +         CheckSlot++) {
>> +      if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {
>> +        break;
>> +      }
>> +    }
>> +    if (CheckSlot < mCpuHotPlugData->ArrayLength) {
>> +      DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "
>> +        "before; ignoring it\n", __FUNCTION__, NewApicId));
>> +      PluggedIdx++;
>> +      continue;
>> +    }
>> +
>> +    //
>> +    // Find the first empty slot in CPU_HOT_PLUG_DATA.
>> +    //
>> +    while (NewSlot < mCpuHotPlugData->ArrayLength &&
>> +           mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) {
>> +      NewSlot++;
>> +    }
>> +    if (NewSlot == mCpuHotPlugData->ArrayLength) {
>> +      DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n",
>> +        __FUNCTION__, NewApicId));
>> +      goto Fatal;
> 
> (8) Please replace the "goto" with "return EFI_OUT_OF_RESOURCES".
> 
> 
>> +    }
>> +
>> +    //
>> +    // Store the APIC ID of the new processor to the slot.
>> +    //
>> +    mCpuHotPlugData->ApicId[NewSlot] = NewApicId;
>> +
>> +    //
>> +    // Relocate the SMBASE of the new CPU.
>> +    //
>> +    Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot],
>> +               mPostSmmPenAddress);
>> +    if (EFI_ERROR (Status)) {
>> +      goto RevokeNewSlot;
>> +    }
>> +
>> +    //
>> +    // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
>> +    //
>> +    Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId,
>> +                              &NewProcessorNumberByProtocol);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n",
>> +        __FUNCTION__, NewApicId, Status));
>> +      goto RevokeNewSlot;
>> +    }
>> +
>> +    DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, "
>> +      "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__,
>> +      NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot],
>> +      (UINT64)NewProcessorNumberByProtocol));
>> +
>> +    NewSlot++;
>> +    PluggedIdx++;
>> +  }
>> +
>> +  //
>> +  // We've handled this hotplug.
>> +  //
> 
> (9) I suggest: "We've processed this batch of hot-added CPUs.".
> 
> 
>> +  return EFI_SUCCESS;
>> +
>> +RevokeNewSlot:
>> +  mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
>> +
>> +Fatal:
> 
> (10) Please drop this label.
> 
> 
>> +  return EFI_INTERRUPT_PENDING;
> 
> (11) This should be "return Status".
> 
> 
>> +}
>>
>>   /**
>>     CPU Hotplug MMI handler function.
>> @@ -122,8 +240,6 @@ CpuHotplugMmi (
>>     UINT8      ApmControl;
>>     UINT32     PluggedCount;
>>     UINT32     ToUnplugCount;
>> -  UINT32     PluggedIdx;
>> -  UINT32     NewSlot;
>>
>>     //
>>     // Assert that we are entering this function due to our root MMI handler
>> @@ -179,87 +295,12 @@ CpuHotplugMmi (
>>       goto Fatal;
>>     }
>>
>> -  //
>> -  // Process hot-added CPUs.
>> -  //
>> -  // The Post-SMM Pen need not be reinstalled multiple times within a single
>> -  // root MMI handling. Even reinstalling once per root MMI is only prudence;
>> -  // in theory installing the pen in the driver's entry point function should
>> -  // suffice.
>> -  //
>> -  SmbaseReinstallPostSmmPen (mPostSmmPenAddress);
>> +  if (PluggedCount > 0) {
>> +    Status = PlugCpus(mPluggedApicIds, PluggedCount);
> 
> (12) Space missing before "(".
> 
> 
>> +  }
>>
>> -  PluggedIdx = 0;
>> -  NewSlot = 0;
>> -  while (PluggedIdx < PluggedCount) {
>> -    APIC_ID NewApicId;
>> -    UINT32  CheckSlot;
>> -    UINTN   NewProcessorNumberByProtocol;
>> -
>> -    NewApicId = mPluggedApicIds[PluggedIdx];
>> -
>> -    //
>> -    // Check if the supposedly hot-added CPU is already known to us.
>> -    //
>> -    for (CheckSlot = 0;
>> -         CheckSlot < mCpuHotPlugData->ArrayLength;
>> -         CheckSlot++) {
>> -      if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {
>> -        break;
>> -      }
>> -    }
>> -    if (CheckSlot < mCpuHotPlugData->ArrayLength) {
>> -      DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "
>> -        "before; ignoring it\n", __FUNCTION__, NewApicId));
>> -      PluggedIdx++;
>> -      continue;
>> -    }
>> -
>> -    //
>> -    // Find the first empty slot in CPU_HOT_PLUG_DATA.
>> -    //
>> -    while (NewSlot < mCpuHotPlugData->ArrayLength &&
>> -           mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) {
>> -      NewSlot++;
>> -    }
>> -    if (NewSlot == mCpuHotPlugData->ArrayLength) {
>> -      DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n",
>> -        __FUNCTION__, NewApicId));
>> -      goto Fatal;
>> -    }
>> -
>> -    //
>> -    // Store the APIC ID of the new processor to the slot.
>> -    //
>> -    mCpuHotPlugData->ApicId[NewSlot] = NewApicId;
>> -
>> -    //
>> -    // Relocate the SMBASE of the new CPU.
>> -    //
>> -    Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot],
>> -               mPostSmmPenAddress);
>> -    if (EFI_ERROR (Status)) {
>> -      goto RevokeNewSlot;
>> -    }
>> -
>> -    //
>> -    // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
>> -    //
>> -    Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId,
>> -                              &NewProcessorNumberByProtocol);
>> -    if (EFI_ERROR (Status)) {
>> -      DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n",
>> -        __FUNCTION__, NewApicId, Status));
>> -      goto RevokeNewSlot;
>> -    }
>> -
>> -    DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, "
>> -      "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__,
>> -      NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot],
>> -      (UINT64)NewProcessorNumberByProtocol));
>> -
>> -    NewSlot++;
>> -    PluggedIdx++;
>> +  if (EFI_ERROR(Status)) {
>> +    goto Fatal;
>>     }
> 
> (13) Without having seen the rest of the patches, I think this error
> check should be nested under the same (PluggedCount > 0) condition; in
> other words, I think it only makes sense to check Status after we
> actually call ProcessHotAddedCpus().
> 
> 
>>
>>     //
>> @@ -267,9 +308,6 @@ CpuHotplugMmi (
>>     //
>>     return EFI_SUCCESS;
>>
>> -RevokeNewSlot:
>> -  mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
>> -
>>   Fatal:
>>     ASSERT (FALSE);
>>     CpuDeadLoop ();
>>
> 
> I'll continue the review later this week.

Acking the comments above. Meanwhile let me reprocess the series
in light of the comments above.

Ankur

> 
> Thanks
> Laszlo
> 

Re: [edk2-devel] [PATCH v5 0/9] support CPU hot-unplug

[Ankur Arora]

On 2021-01-26 10:03 a.m., Laszlo Ersek wrote:
> Hi Ankur,
> 
> On 01/26/21 07:44, Ankur Arora wrote:
>> Hi,
>>
>> This series adds support for CPU hot-unplug with OVMF.
>>
>> Please see this in conjunction with the QEMU secureboot hot-unplug v2
>> series posted here (now upstreamed):
>>    https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
>>
>> Patches 1 and 3,
>>    ("OvmfPkg/CpuHotplugSmm: refactor hotplug logic")
>>    ("OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper")
>> are either refactors or add support functions.
>>
>>    OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
>>    OvmfPkg/CpuHotplugSmm: add CpuEject()
>>    OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
>>
>> Patch 2 and 9,
>>    ("OvmfPkg/CpuHotplugSmm: collect hot-unplug events")
>>    ("OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug")
>> handle the QEMU protocol logic for collection of CPU hot-unplug events
>> or the protocol negotiation.
>>
>> Patch 4,
>>    ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()")
>> adds the MMI logic for CPU hot-unplug handling and informing
>> the PiSmmCpuDxeSmm of CPU removal.
>>
>> Patches 5 and 6,
>>    ("OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA")
>>    ("OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state")
>> sets up state for doing the CPU ejection as part of hot-unplug.
>>
>> Patches 7, and 8,
>>    ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()")
>>    ("OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection")
>> add the CPU ejection logic.
>>
>> Testing (with QEMU 5.2.50):
>>   - Stable with randomized CPU plug/unplug (guest maxcpus=1,8,128)
>>   - Synthetic tests with simultaneous multi CPU hot-unplug
>>   - Negotiation with/without CPU hotplug enabled
>>
>> Also at:
>>    github.com/terminus/edk2/ hot-unplug-v5
>>
>> Changelog:
>> v5:
>>    - fixes ECC errors (all but one in "OvmfPkg/CpuHotplugSmm: add
>>      add Qemu Cpu Status helper").
>>
>> v4:
>>    - Gets rid of unnecessary UefiCpuPkg changes
>>    URL: https://patchew.org/EDK2/20210118063457.358581-1-ankur.a.arora@oracle.com/
>>
>> v3:
>>    - Use a saner PCD based interface to share state between PiSmmCpuDxeSmm
>>      and OvmfPkg/CpuHotplugSmm
>>    - Cleaner split of the hot-unplug code
>>    URL: https://patchew.org/EDK2/20210115074533.277448-1-ankur.a.arora@oracle.com/
>>
>> v2:
>>    - Do the ejection via SmmCpuFeaturesRendezvousExit()
>>    URL: https://patchew.org/EDK2/20210107195515.106158-1-ankur.a.arora@oracle.com/
>>
>> RFC:
>>    URL: https://patchew.org/EDK2/20201208053432.2690694-1-ankur.a.arora@oracle.com/
>>
>>
>> Please review.
>>
>> Thanks
>> Ankur
>>
>> Ankur Arora (9):
>>    OvmfPkg/CpuHotplugSmm: refactor hotplug logic
>>    OvmfPkg/CpuHotplugSmm: collect hot-unplug events
>>    OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
>>    OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
>>    OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA
>>    OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
>>    OvmfPkg/CpuHotplugSmm: add CpuEject()
>>    OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
>>    OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
>>
>>   OvmfPkg/OvmfPkg.dec                                |  10 +
>>   OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf            |   1 +
>>   .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   3 +
>>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                  |   6 +
>>   OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h  |   2 +
>>   OvmfPkg/Include/Library/CpuHotEjectData.h          |  32 ++
>>   OvmfPkg/CpuHotplugSmm/CpuHotplug.c                 | 450 ++++++++++++++++-----
>>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                  |  57 ++-
>>   .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  68 ++++
>>   OvmfPkg/SmmControl2Dxe/SmiFeatures.c               |  25 +-
>>   10 files changed, 552 insertions(+), 102 deletions(-)
>>   create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h
>>
> 
> This series is still mal-formatted; for some reason it has one too many
> CRs per line.
> 
> Did you try to set up the "base64" or the "8bit"
> content-transfer-encoding? The emails reflected through the list are
> still "quoted-printable".

Confirming that this was the problem. And, thanks for debugging this.

Ankur

> 
> Anyway, I managed to apply the patches with some trickery on a local branch.
> 
> Thanks
> Laszlo
> 

Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

[Laszlo Ersek]

On 01/26/21 20:15, Ankur Arora wrote:
> On 2021-01-26 11:01 a.m., Laszlo Ersek wrote:

>> I'll continue the review later this week.
> 
> Acking the comments above.

Thank you!

> Meanwhile let me reprocess the series in light of the comments above.

I'm at such a point now, during the v5 review, that I think I can easily
re-sync.

In general, I don't mind the posting of a new version of a series
mid-review, *IF* we agree about it in advance.

If you prefer to post a v6, for addressing the comments I've made thus
far, I'm OK with that. If you'd like me to continue reviewing v5, I'm
also OK with that.

So it's up to you -- please state your decision, so that I know if I
should proceed with v5 (later this week), or wait for v6.

Thanks!
Laszlo

Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

[Ankur Arora]

On 2021-01-26 1:07 p.m., Laszlo Ersek wrote:
> On 01/26/21 20:15, Ankur Arora wrote:
>> On 2021-01-26 11:01 a.m., Laszlo Ersek wrote:
> 
>>> I'll continue the review later this week.
>>
>> Acking the comments above.
> 
> Thank you!
> 
>> Meanwhile let me reprocess the series in light of the comments above.
> 
> I'm at such a point now, during the v5 review, that I think I can easily
> re-sync.
> 
> In general, I don't mind the posting of a new version of a series
> mid-review, *IF* we agree about it in advance.
> 
> If you prefer to post a v6, for addressing the comments I've made thus
> far, I'm OK with that. If you'd like me to continue reviewing v5, I'm
> also OK with that.
> 
> So it's up to you -- please state your decision, so that I know if I
> should proceed with v5 (later this week), or wait for v6.

I think I would prefer to send v6. Looking at the v5 comments so far, I'm
sure that there's a lot of non conforming coding style issues.
Addressing them now (or at least a hopefully significant subset) would
probably save time.

I'm looking at sending these out by Thursday morning PT, and given that
you plan to continue later this week, sounds like it might not lose too
much review time either.

Thanks
Ankur

> 
> Thanks!
> Laszlo
> 

Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

[Laszlo Ersek]

On 01/26/21 22:17, Ankur Arora wrote:
> On 2021-01-26 1:07 p.m., Laszlo Ersek wrote:
>> On 01/26/21 20:15, Ankur Arora wrote:
>>> On 2021-01-26 11:01 a.m., Laszlo Ersek wrote:
>>
>>>> I'll continue the review later this week.
>>>
>>> Acking the comments above.
>>
>> Thank you!
>>
>>> Meanwhile let me reprocess the series in light of the comments above.
>>
>> I'm at such a point now, during the v5 review, that I think I can easily
>> re-sync.
>>
>> In general, I don't mind the posting of a new version of a series
>> mid-review, *IF* we agree about it in advance.
>>
>> If you prefer to post a v6, for addressing the comments I've made thus
>> far, I'm OK with that. If you'd like me to continue reviewing v5, I'm
>> also OK with that.
>>
>> So it's up to you -- please state your decision, so that I know if I
>> should proceed with v5 (later this week), or wait for v6.
> 
> I think I would prefer to send v6. Looking at the v5 comments so far, I'm
> sure that there's a lot of non conforming coding style issues.
> Addressing them now (or at least a hopefully significant subset) would
> probably save time.

I agree; thank you.

(And, for all the yelling that ECC does, I'm really surprised it didn't
catch the "missing space between function designator and opening paren"
wart!)

> I'm looking at sending these out by Thursday morning PT, and given that
> you plan to continue later this week, sounds like it might not lose too
> much review time either.

Yes, that should work fine.

Thank you, Ankur!
Laszlo

Re: [PATCH v5 1/9] OvmfPkg/CpuHotplugSmm: refactor hotplug logic

[Ankur Arora]

On 2021-01-26 1:32 p.m., Laszlo Ersek wrote:
> On 01/26/21 22:17, Ankur Arora wrote:
>> On 2021-01-26 1:07 p.m., Laszlo Ersek wrote:
>>> On 01/26/21 20:15, Ankur Arora wrote:
>>>> On 2021-01-26 11:01 a.m., Laszlo Ersek wrote:
>>>
>>>>> I'll continue the review later this week.
>>>>
>>>> Acking the comments above.
>>>
>>> Thank you!
>>>
>>>> Meanwhile let me reprocess the series in light of the comments above.
>>>
>>> I'm at such a point now, during the v5 review, that I think I can easily
>>> re-sync.
>>>
>>> In general, I don't mind the posting of a new version of a series
>>> mid-review, *IF* we agree about it in advance.
>>>
>>> If you prefer to post a v6, for addressing the comments I've made thus
>>> far, I'm OK with that. If you'd like me to continue reviewing v5, I'm
>>> also OK with that.
>>>
>>> So it's up to you -- please state your decision, so that I know if I
>>> should proceed with v5 (later this week), or wait for v6.
>>
>> I think I would prefer to send v6. Looking at the v5 comments so far, I'm
>> sure that there's a lot of non conforming coding style issues.
>> Addressing them now (or at least a hopefully significant subset) would
>> probably save time.
> 
> I agree; thank you.
> 
> (And, for all the yelling that ECC does, I'm really surprised it didn't
> catch the "missing space between function designator and opening paren"
> wart!)

Heh. Yeah, I was surprised at how little fault ECC found in the code.

Thanks for reviewing.
Ankur

> 
>> I'm looking at sending these out by Thursday morning PT, and given that
>> you plan to continue later this week, sounds like it might not lose too
>> much review time either.
> 
> Yes, that should work fine.
> 
> Thank you, Ankur!
> Laszlo
>