[PATCH v3 00/10] support CPU hot-unplug

[PATCH v3 00/10] support CPU hot-unplug

[Ankur Arora]

[ Resending, again to the correct list. Apologies to everybody
  who got this twice. ]

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.

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

Patches 4 and 5,
  ("UefiCpuPkg: add CPU ejection support")
  ("OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state")
setup and init the PiSmmCpuDxeSmm CPU ejection state.

Patches 6, 7, and 8,
  ("OvmfPkg/CpuHotplugSmm: support CPU eject")
  ("OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()")
  ("OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection")
contain the core of the changes. The first one adds a NOP CPU ejection
handler, the second processing CPU ejection events and removing the
CPUs to be unplugged in a pen, and the third doing the actual eject.

Patch 10 ("MdePkg: use CpuPause() in CpuDeadLoop()") is unrelated to the
series and just adds a CpuPause() in the CpuDeadLoop((). 

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

Changelog:
 v3:
  - Use a saner PCD based interface to share state between PiSmmCpuDxeSmm
    and OvmfPkg/CpuHotplugSmm
  - Cleaner split of the hot-unplug code

 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 (10):
  OvmfPkg/CpuHotplugSmm: refactor hotplug logic
  OvmfPkg/CpuHotplugSmm: collect hot-unplug events
  OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
  UefiCpuPkg: add CPU ejection support
  OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
  OvmfPkg/CpuHotplugSmm: support CPU eject
  OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
  OvmfPkg/CpuHotplugSmm: add worker to do CPU ejection
  OvmfPkg/SmmControl2Dxe: negotiate hot-unplug
  MdePkg: use CpuPause() in CpuDeadLoop()


 MdePkg/Library/BaseLib/CpuDeadLoop.c               |   4 +-
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c                 | 424 ++++++++++++++++-----
 OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf            |   1 +
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c                  |  58 ++-
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h                  |   6 +
 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h  |   2 +
 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  |  68 ++++
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |   3 +
 OvmfPkg/SmmControl2Dxe/SmiFeatures.c               |  21 +-
 UefiCpuPkg/Include/CpuHotPlugData.h                |  21 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
 UefiCpuPkg/UefiCpuPkg.dec                          |   5 +
 UefiCpuPkg/UefiCpuPkg.uni                          |   4 +
 13 files changed, 515 insertions(+), 103 deletions(-)

-- 
2.9.3

[PATCH v3 01/10] 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..38c71bc11864 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] mPluggedApicIds     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                      *mPluggedApicIds,
+  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 = 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++;
+  }
+
+  //
+  // 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 v3 02/10] 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/CpuHotplugSmm/QemuCpuhp.c                 | 35 +++++++++++++++++------
 OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h |  2 ++
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
index 8d4a6693c8d6..16ecf8cc433e 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
diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
index a34a6d3fae61..bcae60f77ba6 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
 
-- 
2.9.3

[PATCH v3 03/10] 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.c | 23 +++++++++++++++++++++++
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
index 16ecf8cc433e..31c06b4d74dd 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
@@ -67,6 +67,29 @@ QemuCpuhpReadCpuStatus (
   return CpuStatus;
 }
 
+UINT8
+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 ();
+  }
+  return Status;
+}
+
 UINT32
 QemuCpuhpReadCommandData (
   IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
index 8adaa0ad91f0..3f31c9e4c0f4 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
@@ -30,6 +30,12 @@ QemuCpuhpReadCpuStatus (
   IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
   );
 
+UINT8
+QemuCpuhpWriteCpuStatus (
+  IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
+  IN UINT8                        CpuStatus
+  );
+
 UINT32
 QemuCpuhpReadCommandData (
   IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo
-- 
2.9.3

[PATCH v3 04/10] UefiCpuPkg: add CPU ejection support

[Ankur Arora]

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

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.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>
---
 UefiCpuPkg/Include/CpuHotPlugData.h          | 21 +++++++++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  1 +
 UefiCpuPkg/UefiCpuPkg.dec                    |  5 +++++
 UefiCpuPkg/UefiCpuPkg.uni                    |  4 ++++
 4 files changed, 31 insertions(+)

diff --git a/UefiCpuPkg/Include/CpuHotPlugData.h b/UefiCpuPkg/Include/CpuHotPlugData.h
index 6321a149fa52..86f964550655 100644
--- a/UefiCpuPkg/Include/CpuHotPlugData.h
+++ b/UefiCpuPkg/Include/CpuHotPlugData.h
@@ -2,6 +2,7 @@
 Definition for a structure sharing information for CPU hot plug.
 
 Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2021, Oracle Corporation.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -24,4 +25,24 @@ typedef struct {
   UINT32    SmrrSize;
 } CPU_HOT_PLUG_DATA;
 
+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;       // The entries number of the following ApicId array and SmBase array
+
+  UINT64           *ApicIdMap;        // Pointer to CpuIndex->ApicId map. Holds APIC IDs for pending ejects
+  CPU_HOT_EJECT_FN Handler;           // Handler for CPU ejection
+  UINT64           Reserved;
+} CPU_HOT_EJECT_DATA;
+
 #endif
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 76b146299679..f79c874d74f1 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -131,6 +131,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout                 ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                    ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress               ## SOMETIMES_PRODUCES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress              ## SOMETIMES_PRODUCES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable         ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode                      ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize               ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index d83c084467b3..704ccc05f662 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -339,6 +339,11 @@
   # @ValidList   0x80000001 | 0
   gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress|0x0|UINT64|0x60000011
 
+  ## Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported.
+  # @Prompt The pointer to CPU Hot Eject Data.
+  # @ValidList   0x80000001 | 0
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0x0|UINT64|0x60000017
+
   ## Indicates processor feature capabilities, each bit corresponding to a specific feature.
   # @Prompt Processor feature capabilities.
   # @ValidList   0x80000001 | 0
diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
index 219c1963bf08..b1721f256632 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -140,6 +140,10 @@
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotPlugDataAddress_HELP  #language en-US "Contains the pointer to a CPU Hot Plug Data structure if CPU hot-plug is supported."
 
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_PROMPT  #language en-US "The pointer to CPU Hot Eject Data"
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_HELP  #language en-US "Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported."
+
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_PROMPT  #language en-US "Number of reserved variable MTRRs"
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_HELP  #language en-US "Specifies the number of variable MTRRs reserved for OS use."
-- 
2.9.3

[PATCH v3 05/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state

[Ankur Arora]

Init CPU_HOT_EJECT_DATA, exposed via PcdCpuHotEjectDataAddress. This is
owned by PiSmmCpuDxeSmm,  managed in OvmfPkg/SmmCpuFeaturesLib, and is
consumed in OvmfPkg/CpuHotPlugSmm which sets up CPU_HOT_EJECT_DATA->Handler
which handles the mechanics of CPU ejection.

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

diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 7ef7ed98342e..5e7f2de25911 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -14,6 +14,7 @@
 #include <Library/PcdLib.h>
 #include <Library/SmmCpuFeaturesLib.h>
 #include <Library/SmmServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>                 // AllocatePool()
 #include <Library/UefiBootServicesTableLib.h>
 #include <PiSmm.h>
 #include <Register/Intel/SmramSaveStateMap.h>
@@ -171,6 +172,61 @@ SmmCpuFeaturesHookReturnFromSmm (
   return OriginalInstructionPointer;
 }
 
+//
+// The PCD corresponding to CPU_HOT_EJECT_DATA is owned by UefiCpuPkg
+// but we allocate and set it up here because it is needed from
+// SmmCpuFeaturesRendezvousExit() on all CPUs exiting SMI and
+// simultaneous PcdGet64() on multiple CPUs is not supported.
+//
+
+GLOBAL_REMOVE_IF_UNREFERENCED
+CPU_HOT_EJECT_DATA *mCpuHotEjectData;
+
+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);
 }
 
 /**
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 97a10afb6e27..3043f02b2216 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -36,3 +36,6 @@
 
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
-- 
2.9.3

[PATCH v3 06/10] OvmfPkg/CpuHotplugSmm: support CPU eject

[Ankur Arora]

PiSmmCpuDxeSmm exposes CPU_HOT_EJECT_DATA in SMRAM which we use as a
communication channel between the two CPU hot-unplug phases.

The first phase, unplug, happens by way of CpuHotplugMmi() where we
collect CPUs that need to be unplugged and mark them for removal in
SMM data structures.

The second phase, eject, via SmmCpuFeaturesRendezvousExit() does
the actual ejection once the CPU to be ejected is in the tail end
of its SMI handling.

This commit does not contain any of the ejection machinery, only
sets up state to enable us to do that.

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      | 49 +++++++++++++++++++++++++++++++--
 OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf |  1 +
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 38c71bc11864..f088049c7aef 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -32,11 +32,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 are 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.
@@ -317,6 +318,23 @@ Fatal:
   return EFI_INTERRUPT_PENDING;
 }
 
+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;
+  }
+}
 
 //
 // Entry point function of this driver.
@@ -368,8 +386,14 @@ CpuHotplugEntry (
   // Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm
   // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM.
   //
+  // Additionally, CPU HotUnplug 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);
-  if (mCpuHotPlugData == NULL) {
+  mCpuHotEjectData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotEjectDataAddress);
+
+  if (mCpuHotPlugData == NULL)  {
     Status = EFI_NOT_FOUND;
     DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_PLUG_DATA: %r\n", __FUNCTION__, Status));
     goto Fatal;
@@ -380,6 +404,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.
   //
@@ -462,6 +489,24 @@ CpuHotplugEntry (
   //
   SmbaseInstallFirstSmiHandler ();
 
+  if (mCpuHotEjectData) {
+  UINT32     Idx;
+    //
+    // To do CPU eject we need to map ProcessorNum -> APIC_ID. However, by the
+    // time CpuEject() is called (via SmmCpuFeaturesRendezvousExit()), we've
+    // already called RemoveProcessor() and so the APIC ID cannot be looked up
+    // from SMM data structures.
+    //
+    // So use mCpuHotEjectData->ApicIdMap to map from ProcessorNum -> APIC_ID.
+    //
+    // Initialize to known invalid values before installing the handler.
+    //
+    for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
+      mCpuHotEjectData->ApicIdMap[Idx] = CPU_EJECT_INVALID;
+    }
+    mCpuHotEjectData->Handler = CpuEject;
+  }
+
   return EFI_SUCCESS;
 
 ReleasePostSmmPen:
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
index 04322b0d7855..51d022e10416 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
@@ -54,6 +54,7 @@
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress                ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress               ## CONSUMES
   gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase             ## CONSUMES
 
 [FeaturePcd]
-- 
2.9.3

[PATCH v3 07/10] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()

[Ankur Arora]

Introduce UnplugCpus() which, for each APIC ID being unplugged:

  * informs PiSmmCpuDxeSmm by calling
    EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor()
  * stores the APIC ID in CPU_HOT_EJECT_DATA.ApicIdMap, such
    that it can later be used in the eject phase at SMI exit

Also add a holding area in CpuEject() where CPUs being ejected
can wait until ejected.

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 | 106 +++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index f088049c7aef..4048490783e4 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -183,6 +183,99 @@ Fatal:
 }
 
 /**
+  CPU Hot-unplug handler function.
+
+  @param[in] mUnplugApicIds      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                      *mUnplugApicIds,
+  IN UINT32                       ToUnplugCount
+  )
+{
+  EFI_STATUS Status = EFI_SUCCESS;
+  UINT32     ToUnplugIdx;
+  UINT32     EjectCount = 0;
+  UINTN      ProcessorNum;
+
+  ToUnplugIdx = 0;
+  while (ToUnplugIdx < ToUnplugCount) {
+    APIC_ID    RemoveApicId;
+
+    RemoveApicId = mUnplugApicIds[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;
+    } 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++;
+  }
+
+  //
+  // We've handled this unplug.
+  //
+  return EFI_SUCCESS;
+
+Fatal:
+  return EFI_INTERRUPT_PENDING;
+}
+
+/**
   CPU Hotplug MMI handler function.
 
   This is a root MMI handler.
@@ -298,6 +391,8 @@ CpuHotplugMmi (
 
   if (PluggedCount > 0) {
     Status = PlugCpus(mPluggedApicIds, PluggedCount);
+  } else if (ToUnplugCount > 0) {
+    Status = UnplugCpus(mToUnplugApicIds, ToUnplugCount);
   }
 
   if (EFI_ERROR(Status)) {
@@ -334,6 +429,17 @@ CpuEject(
   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 ();
 }
 
 //
-- 
2.9.3

[PATCH v3 08/10] 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 final CPU ejection. This happens via CpuEject().

On the worker CPU, CpuEject() calls QEMU to do the ejection for each CPU
that is unplugged. QEMU handles this by signalling the remote VCPU thread
which forces the SMI AP to context switch out of the SMI, ending with its
QEMU state destroyed.

On the AP, CpuEject() spins around in its holding area until the
context-switch happens. Given that the context switch would end up with
the AP state being cleaned up, this means that the AP will never
return to finish the SMI handling, and thus would not restore some
of the CPU state that it ordinarily would (in SmiRendezvous() and in
SmiEntry.nasm::CommonHandler).

This unrestored state includes FPU state, CET enable, stuffing of
RSB and the final RSM. Given that the CPU state is destroyed by
QEMU on unplug, 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 | 61 ++++++++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 4048490783e4..8aa52ebe5dd1 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -266,6 +266,20 @@ UnplugCpus(
     ToUnplugIdx++;
   }
 
+  if (EjectCount) {
+    UINTN  Worker;
+    Status = mMmCpuService->WhoAmI(mMmCpuService, &Worker);
+    ASSERT_EFI_ERROR(Status);
+    //
+    // UnplugCpus() is called via the root MMI handler and thus we are in the
+    // BSP context. Accordingly, mark ourselves as the ejecting CPU.
+    // Note that, the QEMU eject protocol does not specify that only the BSP
+    // can do the ejection, so this should be safe on any CPU (that is not itself
+    // being unplugged.)
+    //
+    mCpuHotEjectData->ApicIdMap[Worker] = CPU_EJECT_WORKER;
+  }
+
   //
   // We've handled this unplug.
   //
@@ -383,11 +397,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);
@@ -430,6 +439,48 @@ CpuEject(
     return;
   }
 
+  if (ApicId == CPU_EJECT_WORKER) {
+    UINT32 CpuIndex;
+    for (CpuIndex = 0; CpuIndex < mCpuHotEjectData->ArrayLength; CpuIndex++) {
+      UINT64 RemoveApicId = mCpuHotEjectData->ApicIdMap[CpuIndex];
+
+      if ((RemoveApicId != CPU_EJECT_INVALID && RemoveApicId != CPU_EJECT_WORKER)) {
+
+        //
+        // The CPUs to be unplugged have received the BSP's signal to exit the
+        // SMI and either will execute SmmCpuFeaturesSmiRendezvousExit()
+        // followed by this callback or are already waiting in the CpuDeadLoop()
+        // below.
+        //
+        // Tell QEMU to put them out of their misery.
+        //
+         QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
+         QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
+
+         //
+         // Compiler memory barrier to ensure the next store isn't reordered
+         //
+         MemoryFence();
+
+        // Clear the unplug status for CpuIndex to ensure that an invalid SMI
+        // later does not end up trying to unplug it or the newly hotplugged
+        // CpuIndex does not go into the dead loop.
+        //
+        mCpuHotEjectData->ApicIdMap[CpuIndex] = CPU_EJECT_INVALID;
+
+        DEBUG ((DEBUG_INFO, "%a: Unplugged CPU " FMT_APIC_ID "\n",
+               __FUNCTION__, RemoveApicId));
+       }
+     }
+
+    //
+    // Clear our own CPU status to ensure that we don't needlessly enter
+    // the this loop on the next SMI.
+    //
+    mCpuHotEjectData->ApicIdMap[ProcessorNum] = 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
-- 
2.9.3

[PATCH v3 09/10] OvmfPkg/SmmControl2Dxe: negotiate 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 | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
index c9d875543205..9c205af00a08 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.
@@ -120,8 +127,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-feat %x, requested mask %Lx\n",
+      __FUNCTION__, mSmiFeatures, RequestedFeaturesMask));
+
+      mSmiFeatures &= ~ICH9_LPC_SMI_F_CPU_HOT_UNPLUG;
+  }
+
   QemuFwCfgSelectItem (mRequestedFeaturesItem);
   QemuFwCfgWriteBytes (sizeof mSmiFeatures, &mSmiFeatures);
 
@@ -162,8 +179,8 @@ 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

[PATCH v3 10/10] MdePkg: use CpuPause() in CpuDeadLoop()

[Ankur Arora]

Use CpuPause() to allow the CPU to go into a lower power state
state while we spin wait.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Laszlo Ersek <lersek@redhat.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>
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
---
 MdePkg/Library/BaseLib/CpuDeadLoop.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/BaseLib/CpuDeadLoop.c b/MdePkg/Library/BaseLib/CpuDeadLoop.c
index 9e110cacbc96..8fa5a783e90e 100644
--- a/MdePkg/Library/BaseLib/CpuDeadLoop.c
+++ b/MdePkg/Library/BaseLib/CpuDeadLoop.c
@@ -28,5 +28,7 @@ CpuDeadLoop (
 {
   volatile UINTN  Index;
 
-  for (Index = 0; Index == 0;);
+  for (Index = 0; Index == 0;) {
+    CpuPause();
+  }
 }
-- 
2.9.3

Re: [edk2-devel] [PATCH v3 04/10] UefiCpuPkg: add CPU ejection support

[Laszlo Ersek]

Hi Ankur,

On 01/15/21 08:45, Ankur Arora wrote:
> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress,  
> which would be used to share CPU ejection state between
> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.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>
> ---
>  UefiCpuPkg/Include/CpuHotPlugData.h          | 21 +++++++++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  1 +
>  UefiCpuPkg/UefiCpuPkg.dec                    |  5 +++++
>  UefiCpuPkg/UefiCpuPkg.uni                    |  4 ++++
>  4 files changed, 31 insertions(+)
> 
> diff --git a/UefiCpuPkg/Include/CpuHotPlugData.h b/UefiCpuPkg/Include/CpuHotPlugData.h
> index 6321a149fa52..86f964550655 100644
> --- a/UefiCpuPkg/Include/CpuHotPlugData.h
> +++ b/UefiCpuPkg/Include/CpuHotPlugData.h
> @@ -2,6 +2,7 @@
>  Definition for a structure sharing information for CPU hot plug.
>  
>  Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2021, Oracle Corporation.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -24,4 +25,24 @@ typedef struct {
>    UINT32    SmrrSize;
>  } CPU_HOT_PLUG_DATA;
>  
> +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;       // The entries number of the following ApicId array and SmBase array
> +
> +  UINT64           *ApicIdMap;        // Pointer to CpuIndex->ApicId map. Holds APIC IDs for pending ejects
> +  CPU_HOT_EJECT_FN Handler;           // Handler for CPU ejection
> +  UINT64           Reserved;
> +} CPU_HOT_EJECT_DATA;
> +
>  #endif

I'm sorry, I still don't understand -- why is it necessary to modify
UefiCpuPkg *at all*, for this feature?

(1) The structure type for the data exchange should be in an OvmfPkg
header file.

(2) The dynamic PCD for transferring the address of the structure should
be declared in the "OvmfPkg.dec" file.

(3) The "handler" call is made
- from SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib,
- to CpuEject() in OvmfPkg/CpuHotplugSmm.

First, this call should not need a function pointer at all -- the
CpuEject() logic should be flattened into
OvmfPkg/Library/SmmCpuFeaturesLib).

Second, if that's not possible -- please explain why --, then a function
pointer might be justified after all, but the information channel still
seems to have zero relevance for UefiCpuPkg.

It's possible I'm not seeing something; please explain.

Thanks
Laszlo

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index 76b146299679..f79c874d74f1 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> @@ -131,6 +131,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout                 ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                    ## SOMETIMES_CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress               ## SOMETIMES_PRODUCES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress              ## SOMETIMES_PRODUCES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable         ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode                      ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize               ## SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index d83c084467b3..704ccc05f662 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -339,6 +339,11 @@
>    # @ValidList   0x80000001 | 0
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress|0x0|UINT64|0x60000011
>  
> +  ## Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported.
> +  # @Prompt The pointer to CPU Hot Eject Data.
> +  # @ValidList   0x80000001 | 0
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0x0|UINT64|0x60000017
> +
>    ## Indicates processor feature capabilities, each bit corresponding to a specific feature.
>    # @Prompt Processor feature capabilities.
>    # @ValidList   0x80000001 | 0
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index 219c1963bf08..b1721f256632 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -140,6 +140,10 @@
>  
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotPlugDataAddress_HELP  #language en-US "Contains the pointer to a CPU Hot Plug Data structure if CPU hot-plug is supported."
>  
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_PROMPT  #language en-US "The pointer to CPU Hot Eject Data"
> +
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_HELP  #language en-US "Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported."
> +
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_PROMPT  #language en-US "Number of reserved variable MTRRs"
>  
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_HELP  #language en-US "Specifies the number of variable MTRRs reserved for OS use."
> 

Re: [edk2-devel] [PATCH v3 04/10] UefiCpuPkg: add CPU ejection support

[Ankur Arora]

On 2021-01-15 12:17 a.m., Laszlo Ersek wrote:
> Hi Ankur,
> 
> On 01/15/21 08:45, Ankur Arora wrote:
>> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress,
>> which would be used to share CPU ejection state between
>> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.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>
>> ---
>>   UefiCpuPkg/Include/CpuHotPlugData.h          | 21 +++++++++++++++++++++
>>   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  1 +
>>   UefiCpuPkg/UefiCpuPkg.dec                    |  5 +++++
>>   UefiCpuPkg/UefiCpuPkg.uni                    |  4 ++++
>>   4 files changed, 31 insertions(+)
>>
>> diff --git a/UefiCpuPkg/Include/CpuHotPlugData.h b/UefiCpuPkg/Include/CpuHotPlugData.h
>> index 6321a149fa52..86f964550655 100644
>> --- a/UefiCpuPkg/Include/CpuHotPlugData.h
>> +++ b/UefiCpuPkg/Include/CpuHotPlugData.h
>> @@ -2,6 +2,7 @@
>>   Definition for a structure sharing information for CPU hot plug.
>>   
>>   Copyright (c) 2013 - 2015, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2021, Oracle Corporation.<BR>
>>   SPDX-License-Identifier: BSD-2-Clause-Patent
>>   
>>   **/
>> @@ -24,4 +25,24 @@ typedef struct {
>>     UINT32    SmrrSize;
>>   } CPU_HOT_PLUG_DATA;
>>   
>> +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;       // The entries number of the following ApicId array and SmBase array
>> +
>> +  UINT64           *ApicIdMap;        // Pointer to CpuIndex->ApicId map. Holds APIC IDs for pending ejects
>> +  CPU_HOT_EJECT_FN Handler;           // Handler for CPU ejection
>> +  UINT64           Reserved;
>> +} CPU_HOT_EJECT_DATA;
>> +
>>   #endif
> 
> I'm sorry, I still don't understand -- why is it necessary to modify
> UefiCpuPkg *at all*, for this feature?
> 
> (1) The structure type for the data exchange should be in an OvmfPkg
> header file.
> 
> (2) The dynamic PCD for transferring the address of the structure should
> be declared in the "OvmfPkg.dec" file.
> 
> (3) The "handler" call is made
> - from SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib,
> - to CpuEject() in OvmfPkg/CpuHotplugSmm.
> 
> First, this call should not need a function pointer at all -- the
> CpuEject() logic should be flattened into
> OvmfPkg/Library/SmmCpuFeaturesLib).

Sure, it can be flattened -- but it's out of place in SmmCpuFeaturesLib.
All of the logic pertaining to the unplug is in CpuHotPlugSmm, so it seems to
make sense to locate the related ejection code along with it.

If you are concerned about paying the additional cost of an indirect call
then I think it should be possible to install the handler only when there
is an actual unplug to be done and remove it after.

> 
> Second, if that's not possible -- please explain why --, then a function
> pointer might be justified after all, but the information channel still
> seems to have zero relevance for UefiCpuPkg.

The reason for keeping the PCD in UefiCpuPkg was that its declaration and
init are in SmmCpuFeaturesLib which gets folded into the UefiCpuPkg/PiSmmCpuDxe
execution context and so the export happening via OvmfPkg.dec seemed off.

That said, I guess your underlying point is that this adds unnecessary state to
non OVMF builds (?), which it does, so I can move the PCD to OvmfPkg.dec.


Thanks
Ankur

> 
> It's possible I'm not seeing something; please explain.
> 
> Thanks
> Laszlo
> 
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
>> index 76b146299679..f79c874d74f1 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
>> @@ -131,6 +131,7 @@
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout                 ## CONSUMES
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                    ## SOMETIMES_CONSUMES
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress               ## SOMETIMES_PRODUCES
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress              ## SOMETIMES_PRODUCES
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable         ## CONSUMES
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode                      ## CONSUMES
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize               ## SOMETIMES_CONSUMES
>> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
>> index d83c084467b3..704ccc05f662 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.dec
>> +++ b/UefiCpuPkg/UefiCpuPkg.dec
>> @@ -339,6 +339,11 @@
>>     # @ValidList   0x80000001 | 0
>>     gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress|0x0|UINT64|0x60000011
>>   
>> +  ## Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported.
>> +  # @Prompt The pointer to CPU Hot Eject Data.
>> +  # @ValidList   0x80000001 | 0
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0x0|UINT64|0x60000017
>> +
>>     ## Indicates processor feature capabilities, each bit corresponding to a specific feature.
>>     # @Prompt Processor feature capabilities.
>>     # @ValidList   0x80000001 | 0
>> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
>> index 219c1963bf08..b1721f256632 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.uni
>> +++ b/UefiCpuPkg/UefiCpuPkg.uni
>> @@ -140,6 +140,10 @@
>>   
>>   #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotPlugDataAddress_HELP  #language en-US "Contains the pointer to a CPU Hot Plug Data structure if CPU hot-plug is supported."
>>   
>> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_PROMPT  #language en-US "The pointer to CPU Hot Eject Data"
>> +
>> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuHotEjectDataAddress_HELP  #language en-US "Contains the pointer to a CPU Hot Eject Data structure if CPU hot-plug is supported."
>> +
>>   #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_PROMPT  #language en-US "Number of reserved variable MTRRs"
>>   
>>   #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuNumberOfReservedVariableMtrrs_HELP  #language en-US "Specifies the number of variable MTRRs reserved for OS use."
>>
> 

Re: [edk2-devel] [PATCH v3 04/10] UefiCpuPkg: add CPU ejection support

[Laszlo Ersek]

On 01/15/21 19:16, Ankur Arora wrote:
> On 2021-01-15 12:17 a.m., Laszlo Ersek wrote:
>> Hi Ankur,
>>
>> On 01/15/21 08:45, Ankur Arora wrote:
>>> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress,
>>> which would be used to share CPU ejection state between
>>> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.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>
>>> ---
>>>   UefiCpuPkg/Include/CpuHotPlugData.h          | 21
>>> +++++++++++++++++++++
>>>   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  1 +
>>>   UefiCpuPkg/UefiCpuPkg.dec                    |  5 +++++
>>>   UefiCpuPkg/UefiCpuPkg.uni                    |  4 ++++
>>>   4 files changed, 31 insertions(+)
>>>
>>> diff --git a/UefiCpuPkg/Include/CpuHotPlugData.h
>>> b/UefiCpuPkg/Include/CpuHotPlugData.h
>>> index 6321a149fa52..86f964550655 100644
>>> --- a/UefiCpuPkg/Include/CpuHotPlugData.h
>>> +++ b/UefiCpuPkg/Include/CpuHotPlugData.h
>>> @@ -2,6 +2,7 @@
>>>   Definition for a structure sharing information for CPU hot plug.
>>>     Copyright (c) 2013 - 2015, Intel Corporation. All rights
>>> reserved.<BR>
>>> +Copyright (c) 2021, Oracle Corporation.<BR>
>>>   SPDX-License-Identifier: BSD-2-Clause-Patent
>>>     **/
>>> @@ -24,4 +25,24 @@ typedef struct {
>>>     UINT32    SmrrSize;
>>>   } CPU_HOT_PLUG_DATA;
>>>   +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;       // The entries number of the
>>> following ApicId array and SmBase array
>>> +
>>> +  UINT64           *ApicIdMap;        // Pointer to CpuIndex->ApicId
>>> map. Holds APIC IDs for pending ejects
>>> +  CPU_HOT_EJECT_FN Handler;           // Handler for CPU ejection
>>> +  UINT64           Reserved;
>>> +} CPU_HOT_EJECT_DATA;
>>> +
>>>   #endif
>>
>> I'm sorry, I still don't understand -- why is it necessary to modify
>> UefiCpuPkg *at all*, for this feature?
>>
>> (1) The structure type for the data exchange should be in an OvmfPkg
>> header file.
>>
>> (2) The dynamic PCD for transferring the address of the structure should
>> be declared in the "OvmfPkg.dec" file.
>>
>> (3) The "handler" call is made
>> - from SmmCpuFeaturesRendezvousExit() in
>> OvmfPkg/Library/SmmCpuFeaturesLib,
>> - to CpuEject() in OvmfPkg/CpuHotplugSmm.
>>
>> First, this call should not need a function pointer at all -- the
>> CpuEject() logic should be flattened into
>> OvmfPkg/Library/SmmCpuFeaturesLib).
> 
> Sure, it can be flattened -- but it's out of place in SmmCpuFeaturesLib.
> All of the logic pertaining to the unplug is in CpuHotPlugSmm, so it
> seems to
> make sense to locate the related ejection code along with it.

OK. Having a (possibly NULL) function pointer also doubles as a "control
knob" to see whether the feature is enabled or not. I'm fine with the
function pointer, then.

> If you are concerned about paying the additional cost of an indirect call
> then I think it should be possible to install the handler only when there
> is an actual unplug to be done and remove it after.

No, that wasn't my concern.

>>
>> Second, if that's not possible -- please explain why --, then a function
>> pointer might be justified after all, but the information channel still
>> seems to have zero relevance for UefiCpuPkg.
> 
> The reason for keeping the PCD in UefiCpuPkg was that its declaration and
> init are in SmmCpuFeaturesLib which gets folded into the
> UefiCpuPkg/PiSmmCpuDxe
> execution context and so the export happening via OvmfPkg.dec seemed off.

No, it's not off, that's precisely the goal. SmmCpuFeaturesLib is a
dedicated platform customization interface for PiSmmCpuDxeSmm. (By
platform, I mean "firmware platform"; such as OvmfPkg.)
SmmCpuFeaturesLib exists becuase PiSmmCpuDxeSmm wants it to exist.

If we can solve a task by hiding it entirely in SmmCpuFeaturesLib, in
connection with other parts of the firmware platform, we should do that.
That's why the SmmCpuFeaturesLib class was invented, with carefully
selected hook points. UefiCpuPkg in general is a core package, not a
firmware platform package, so we only modify UefiCpuPkg for platform
needs if that is absolutely unavoidable.

We are modifying the SmmCpuFeaturesLib instance provided by OvmfPkg, so
we should strive for keeping the internals of that solution internal to
OvmfPkg -- such as a PCD declared in OvmfPkg.dec, a structure type
defined in an OvmfPkg include file, and so on. We're welcome to stuff as
much platform logic into PiSmmCpuDxeSmm through our platform's
SmmCpuFeaturesLib instance as possible, so long as we have an actual
justified purpose with that "stuffing", and we honor the interface
contracts.

> That said, I guess your underlying point is that this adds unnecessary
> state to non OVMF builds (?), which it does, so I can move the PCD to OvmfPkg.dec.

Yes, that's my point. Ideally, the diffstat of the series should
entirely stay within OvmfPkg.

I would suggest even splitting off the last patch (for CpuDeadLoop())
into its own submission. That patch could be merged sooner than, and
independently of, the unplug feature for OVMF.

Is it OK with you if I ask you to submit a v4 like that, before I start
going through the series in detail?

A bit more feedback on folding this UefiCpuPkg content into OvmfPkg:

- "OvmfPkg/Include/CpuHotUnplug.h" looks good to me, for the header file
(feel free to replace Unplug with Eject though, if you like the latter more)

- in INF files, in every section, such as [Sources], [Pcds] etc, please
keep entries (filenames, PCD names) alphabetically sorted -- unless the
preexistent order already breaks this property

- don't bother about a .uni file under OvmfPkg

- in "OvmfPkg.dec", please find the PCD with the highest token value,
and for introducing the new PCD, use a new token value that's one
greater than the current maximum.

Thank you!
Laszlo

Re: [edk2-devel] [PATCH v3 04/10] UefiCpuPkg: add CPU ejection support

[Ankur Arora]

On 2021-01-15 10:44 a.m., Laszlo Ersek wrote:
> On 01/15/21 19:16, Ankur Arora wrote:
>> On 2021-01-15 12:17 a.m., Laszlo Ersek wrote:
>>> Hi Ankur,
>>>
>>> On 01/15/21 08:45, Ankur Arora wrote:
>>>> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress,
>>>> which would be used to share CPU ejection state between
>>>> PiSmmCpuDxeSmm and OvmfPkg/CpuHotPlugSmm.
>>>>
[...]
>>>
>>> Second, if that's not possible -- please explain why --, then a function
>>> pointer might be justified after all, but the information channel still
>>> seems to have zero relevance for UefiCpuPkg.
>>
>> The reason for keeping the PCD in UefiCpuPkg was that its declaration and
>> init are in SmmCpuFeaturesLib which gets folded into the
>> UefiCpuPkg/PiSmmCpuDxe
>> execution context and so the export happening via OvmfPkg.dec seemed off.
> 
> No, it's not off, that's precisely the goal. SmmCpuFeaturesLib is a
> dedicated platform customization interface for PiSmmCpuDxeSmm. (By
> platform, I mean "firmware platform"; such as OvmfPkg.)
> SmmCpuFeaturesLib exists becuase PiSmmCpuDxeSmm wants it to exist.
> 
> If we can solve a task by hiding it entirely in SmmCpuFeaturesLib, in
> connection with other parts of the firmware platform, we should do that.
> That's why the SmmCpuFeaturesLib class was invented, with carefully
> selected hook points. UefiCpuPkg in general is a core package, not a
> firmware platform package, so we only modify UefiCpuPkg for platform
> needs if that is absolutely unavoidable.
> 
> We are modifying the SmmCpuFeaturesLib instance provided by OvmfPkg, so
> we should strive for keeping the internals of that solution internal to
> OvmfPkg -- such as a PCD declared in OvmfPkg.dec, a structure type
> defined in an OvmfPkg include file, and so on. We're welcome to stuff as
> much platform logic into PiSmmCpuDxeSmm through our platform's
> SmmCpuFeaturesLib instance as possible, so long as we have an actual
> justified purpose with that "stuffing", and we honor the interface
> contracts.
> 
>> That said, I guess your underlying point is that this adds unnecessary
>> state to non OVMF builds (?), which it does, so I can move the PCD to OvmfPkg.dec.
> 
> Yes, that's my point. Ideally, the diffstat of the series should
> entirely stay within OvmfPkg.
> 
> I would suggest even splitting off the last patch (for CpuDeadLoop())
> into its own submission. That patch could be merged sooner than, and
> independently of, the unplug feature for OVMF.
> 
> Is it OK with you if I ask you to submit a v4 like that, before I start
> going through the series in detail?

Sure. Let me send a v4 with these changes.

Ankur

> 
> A bit more feedback on folding this UefiCpuPkg content into OvmfPkg:
> 
> - "OvmfPkg/Include/CpuHotUnplug.h" looks good to me, for the header file
> (feel free to replace Unplug with Eject though, if you like the latter more)
> 
> - in INF files, in every section, such as [Sources], [Pcds] etc, please
> keep entries (filenames, PCD names) alphabetically sorted -- unless the
> preexistent order already breaks this property
> 
> - don't bother about a .uni file under OvmfPkg
> 
> - in "OvmfPkg.dec", please find the PCD with the highest token value,
> and for introducing the new PCD, use a new token value that's one
> greater than the current maximum.
> 
> Thank you!
> Laszlo
>