[RFC PATCH 0/5] support CPU hot-unplug

[RFC PATCH 0/5] support CPU hot-unplug

[ankur.a.arora]

[ Resending to the correct edk2 alias this time. ]

Hi,

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

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

In particular, would be glad for comments on Patch 4, specifically
where we should be ejecting the CPU. 

Right now the ejection happens in UnplugCpus() (called from
CpuHotplugMmi()):
  +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
  +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);

That is way too early -- given that the actual unplug will happen
in SmmCpuUpdate() and given that the BSPHandler() would have waited
for the APs multiple times before then.

Another possibility is that the actual ejection be deferred to the
_EJ0 method after the return from the SMI. But, that seems like a
hack. Additionally, Igor points out here that this approach has problems:
  https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/

Please review.

Thanks
Ankur

Ankur Arora (5):
  OvmfPkg/CpuHotplugSmm: move CPU Hotplug into PlugCpus()
  OvmfPkg/CpuHotplugSmm: handle fw_remove
  OvmfPkg/CpuHotplugSmm: add CpuStatus helper function
  OvmfPkg/CpuHotplugSmm: handle CPU hot-unplug
  OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOT_UNPLUG

 OvmfPkg/CpuHotplugSmm/CpuHotplug.c            | 293 ++++++++++++------
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c             |  58 +++-
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.h             |   6 +
 .../Include/IndustryStandard/QemuCpuHotplug.h |   2 +
 OvmfPkg/SmmControl2Dxe/SmiFeatures.c          |  21 +-
 5 files changed, 280 insertions(+), 100 deletions(-)

-- 
2.25.4

[RFC PATCH 1/5] OvmfPkg/CpuHotplugSmm: move CPU Hotplug into PlugCpus()

[Ankur Arora]

Move the CPU Hotplug logic into PlugCpus() in preparation for
adding corresponding unplug logic.

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..0f8f210d0ecf 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.25.4

[RFC PATCH 2/5] OvmfPkg/CpuHotplugSmm: handle fw_remove

[Ankur Arora]

Limit collection of APIC IDs for hot-unplug events to CPUs with
fw_remove set. Additionally we now ignore CPUs which have only
remove set.

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.

Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
 OvmfPkg/CpuHotplugSmm/QemuCpuhp.c             | 35 ++++++++++++++-----
 .../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..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
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
 
-- 
2.25.4

[RFC PATCH 3/5] OvmfPkg/CpuHotplugSmm: add CpuStatus helper function

[Ankur Arora]

Add QemuCpuhpWriteCpuStatus() which writes to the QEMU CPU status register.
On error, it hangs in similar fashion to the other helper functions.

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 2dd783ebf42e..14145696aa50 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..9f084d199fd4 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.25.4

[RFC PATCH 4/5] OvmfPkg/CpuHotplugSmm: handle CPU hot-unplug

[Ankur Arora]

Introduce a new function UnplugCpus() which, for each
unplugged CPU:

  - find the slot for APIC ID in CPU_HOT_PLUG_DATA.

  - inform PiSmmCPuDxeSmm by calling
    EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor().

  - update the QEMU_CPUHP_STAT_EJECTED bit in the
    QEMU_CPUHP_R_CPU_STAT regiser.

Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 80 ++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 0f8f210d0ecf..0a839ae52215 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -181,6 +181,84 @@ Fatal:
   return EFI_INTERRUPT_PENDING;
 }
 
+/**
+  CPU Hot-unplug handler function.
+
+  @param[in] mUnplugApicIds      List of APIC IDs to be plugged.
+
+  @param[in] ToUnplugCount       Count of APIC IDs to be plugged.
+
+  @retval EFI_SUCCESS	         Some of the requested APIC IDs were 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;
+
+    //
+    // Remove the CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
+    //
+
+  ToUnplugIdx = 0;
+  while (ToUnplugIdx < ToUnplugCount) {
+    APIC_ID    RemoveApicId;
+    UINT32     ProcessorNum;
+
+    RemoveApicId = mUnplugApicIds[ToUnplugIdx];
+
+    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_VERBOSE, "%a: did not find APIC ID " FMT_APIC_ID " to unplug\n",
+        __FUNCTION__, RemoveApicId));
+      ToUnplugIdx++;
+      continue;
+    }
+
+    Status = mMmCpuService->RemoveProcessor (mMmCpuService, ProcessorNum);
+
+    if (EFI_ERROR(Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: RemoveProcessor(" FMT_APIC_ID "): %r\n",
+        __FUNCTION__, RemoveApicId, Status));
+      goto Fatal;
+    }
+
+      //
+      // Tell the host that the firmware is done.
+      //
+    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
+    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
+
+    ToUnplugIdx++;
+  }
+
+  //
+  // We've handled this unplug.
+  //
+  return EFI_SUCCESS;
+
+Fatal:
+  return EFI_INTERRUPT_PENDING;
+}
+
 /**
   CPU Hotplug MMI handler function.
 
@@ -297,6 +375,8 @@ CpuHotplugMmi (
 
   if (PluggedCount > 0) {
     Status = PlugCpus(mPluggedApicIds, PluggedCount);
+  } else if (ToUnplugCount > 0) {
+    Status = UnplugCpus(mToUnplugApicIds, ToUnplugCount);
   }
 
   if (EFI_ERROR(Status)) {
-- 
2.25.4

[RFC PATCH 5/5] OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOT_UNPLUG

[Ankur Arora]

We treat ICH9_LPC_SMI_F_CPU_HOT_UNPLUG as a subset of the
ICH9_LPC_SMI_F_CPU_HOTPLUG feature flag.

Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
 OvmfPkg/CpuHotplugSmm/CpuHotplug.c   |  5 -----
 OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 21 +++++++++++++++++++--
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 0a839ae52215..cdf8e55b8d91 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -367,11 +367,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);
diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
index c9d875543205..b95827051c2b 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.25.4

Re: [RFC PATCH 0/5] support CPU hot-unplug

[Laszlo Ersek]

Hi Ankur,

On 12/08/20 06:34, Ankur Arora wrote:
> [ Resending to the correct edk2 alias this time. ]
> 
> Hi,
> 
> This series adds support for CPU hot-unplug with OVMF.
> 
> Please see this in conjunction with the QEMU v2 series posted here: 
>   https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
> 
> In particular, would be glad for comments on Patch 4, specifically
> where we should be ejecting the CPU. 
> 
> Right now the ejection happens in UnplugCpus() (called from
> CpuHotplugMmi()):
>   +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
>   +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
> 
> That is way too early -- given that the actual unplug will happen
> in SmmCpuUpdate() and given that the BSPHandler() would have waited
> for the APs multiple times before then.
> 
> Another possibility is that the actual ejection be deferred to the
> _EJ0 method after the return from the SMI. But, that seems like a
> hack. Additionally, Igor points out here that this approach has problems:
>   https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/
> 
> Please review.

thanks for the patches; I'm confirming I've got them.

I'll need a non-trivial amount of time before I come to these patches
(and to the QEMU patches posted by Igor).

I'm working very busily on
<https://bugzilla.tianocore.org/show_bug.cgi?id=3097> and my brain is
full of other stuff.

We had the reverse situation earlier this year, I think, when -- in
relation to hotplug -- Igor was occupied with a more pressing QEMU
development (NUMA I think?), for a significant amount of time.

What's important is that I want to do both Igor's patches and your
patches *justice*, with my review, and at this time I just cannot sit
down with them alone for a day. These patches deserve a deep looking-at,
rather than a skim, and I cannot afford the former at the moment. I
prefer doing a (hopefully) thorough review, later, to rushing a review now.

I hope that's tolerable.

Thank you,
Laszlo

Re: [RFC PATCH 0/5] support CPU hot-unplug

[Ankur Arora]

On 2020-12-10 1:21 a.m., Laszlo Ersek wrote:
> Hi Ankur,
> 
> On 12/08/20 06:34, Ankur Arora wrote:
>> [ Resending to the correct edk2 alias this time. ]
>>
>> Hi,
>>
>> This series adds support for CPU hot-unplug with OVMF.
>>
>> Please see this in conjunction with the QEMU v2 series posted here:
>>    https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
>>
>> In particular, would be glad for comments on Patch 4, specifically
>> where we should be ejecting the CPU.
>>
>> Right now the ejection happens in UnplugCpus() (called from
>> CpuHotplugMmi()):
>>    +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
>>    +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
>>
>> That is way too early -- given that the actual unplug will happen
>> in SmmCpuUpdate() and given that the BSPHandler() would have waited
>> for the APs multiple times before then.
>>
>> Another possibility is that the actual ejection be deferred to the
>> _EJ0 method after the return from the SMI. But, that seems like a
>> hack. Additionally, Igor points out here that this approach has problems:
>>    https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/
>>
>> Please review.
> 
> thanks for the patches; I'm confirming I've got them.
> 
> I'll need a non-trivial amount of time before I come to these patches
> (and to the QEMU patches posted by Igor).
> 
> I'm working very busily on
> <https://bugzilla.tianocore.org/show_bug.cgi?id=3097> and my brain is
> full of other stuff.

Thanks for letting me know. I empathize with not wanting to context
switch out all of your built up virtio-fs/ARM state.

> 
> We had the reverse situation earlier this year, I think, when -- in
> relation to hotplug -- Igor was occupied with a more pressing QEMU
> development (NUMA I think?), for a significant amount of time.
> 
> What's important is that I want to do both Igor's patches and your
> patches *justice*, with my review, and at this time I just cannot sit
> down with them alone for a day. These patches deserve a deep looking-at,
> rather than a skim, and I cannot afford the former at the moment. I
> prefer doing a (hopefully) thorough review, later, to rushing a review now.

I'll look forward to it. Anyway I think a deep look at these patches might
be wasted at the current stage. In particular there's a glaring hole in this
patch set which is how to handle the actual unplug (setting of
QEMU_CPUHP_STAT_EJECTED).

That's one thing I would be glad for a comment on: not right away, please
come back to this when you have thinking room.

So the problem is that my current approach -- setting QEMU_CPUHP_STAT_EJECTED
via the CpuHotplugMmi() handler definitely doesn't work given that it removes
an AP immediately while the SMI handler is still using it.

The two alternatives are:
  - do this in SmmCpuFeaturesLib::SmmCpuFeaturesRendezvousExit() while exiting
    SMI. That means that the only thing we will not do on the AP being unplugged
    is restoring debug registers and a bunch of MSRs. Which AFAICS would be
    okay, since the next time this AP is plugged in it will start from a clean
    slate anyway.

  - Qemu marks the hot-unplug when QEMU_CPUHP_STAT_EJECTED is set and defers it
    until the SMI exit.

AFAICS, both ought to work. But, assuming it works (I haven't tried it out yet)
the first seems cleaner.

Ankur


> 
> I hope that's tolerable.
> 
> Thank you,
> Laszlo
> 

Re: [edk2-devel] [RFC PATCH 0/5] support CPU hot-unplug

[Igor Mammedov]

On Thu, 10 Dec 2020 12:08:13 -0800
"Ankur Arora" <ankur.a.arora@oracle.com> wrote:

> On 2020-12-10 1:21 a.m., Laszlo Ersek wrote:
> > Hi Ankur,
> > 
> > On 12/08/20 06:34, Ankur Arora wrote:  
> >> [ Resending to the correct edk2 alias this time. ]
> >>
> >> Hi,
> >>
> >> This series adds support for CPU hot-unplug with OVMF.
> >>
> >> Please see this in conjunction with the QEMU v2 series posted here:
> >>    https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
> >>
> >> In particular, would be glad for comments on Patch 4, specifically
> >> where we should be ejecting the CPU.
> >>
> >> Right now the ejection happens in UnplugCpus() (called from
> >> CpuHotplugMmi()):
> >>    +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
> >>    +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
> >>
> >> That is way too early -- given that the actual unplug will happen
> >> in SmmCpuUpdate() and given that the BSPHandler() would have waited
> >> for the APs multiple times before then.
> >>
> >> Another possibility is that the actual ejection be deferred to the
> >> _EJ0 method after the return from the SMI. But, that seems like a
> >> hack. Additionally, Igor points out here that this approach has problems:
> >>    https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/
> >>
> >> Please review.  
> > 
> > thanks for the patches; I'm confirming I've got them.
> > 
> > I'll need a non-trivial amount of time before I come to these patches
> > (and to the QEMU patches posted by Igor).
> > 
> > I'm working very busily on
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=3097> and my brain is
> > full of other stuff.  
> 
> Thanks for letting me know. I empathize with not wanting to context
> switch out all of your built up virtio-fs/ARM state.
> 
> > 
> > We had the reverse situation earlier this year, I think, when -- in
> > relation to hotplug -- Igor was occupied with a more pressing QEMU
> > development (NUMA I think?), for a significant amount of time.
> > 
> > What's important is that I want to do both Igor's patches and your
> > patches *justice*, with my review, and at this time I just cannot sit
> > down with them alone for a day. These patches deserve a deep looking-at,
> > rather than a skim, and I cannot afford the former at the moment. I
> > prefer doing a (hopefully) thorough review, later, to rushing a review now.  
> 
> I'll look forward to it. Anyway I think a deep look at these patches might
> be wasted at the current stage. In particular there's a glaring hole in this
> patch set which is how to handle the actual unplug (setting of
> QEMU_CPUHP_STAT_EJECTED).
> 
> That's one thing I would be glad for a comment on: not right away, please
> come back to this when you have thinking room.
> 
> So the problem is that my current approach -- setting QEMU_CPUHP_STAT_EJECTED
> via the CpuHotplugMmi() handler definitely doesn't work given that it removes
> an AP immediately while the SMI handler is still using it.
> 
> The two alternatives are:
>   - do this in SmmCpuFeaturesLib::SmmCpuFeaturesRendezvousExit() while exiting
>     SMI. That means that the only thing we will not do on the AP being unplugged
>     is restoring debug registers and a bunch of MSRs. Which AFAICS would be
>     okay, since the next time this AP is plugged in it will start from a clean
>     slate anyway.
> 

>   - Qemu marks the hot-unplug when QEMU_CPUHP_STAT_EJECTED is set and defers it
>     until the SMI exit.
I don't like implementing workarounds on hw side for guest software sake.
(it's occasionally done but only if there is no way to fix guest side,
for example closed sources OS. So there shall be very good reason to do so)

> AFAICS, both ought to work. But, assuming it works (I haven't tried it out yet)
> the first seems cleaner.
> 
> Ankur
> 
> 
> > 
> > I hope that's tolerable.
> > 
> > Thank you,
> > Laszlo
> >   
> 
> 
> 
> 
> 

Re: [edk2-devel] [RFC PATCH 0/5] support CPU hot-unplug

[Laszlo Ersek]

Hi Ankur,

On 12/08/20 06:34, Ankur Arora wrote:
> [ Resending to the correct edk2 alias this time. ]
> 
> Hi,
> 
> This series adds support for CPU hot-unplug with OVMF.
> 
> Please see this in conjunction with the QEMU v2 series posted here: 
>   https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
> 
> In particular, would be glad for comments on Patch 4, specifically
> where we should be ejecting the CPU. 
> 
> Right now the ejection happens in UnplugCpus() (called from
> CpuHotplugMmi()):
>   +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
>   +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
> 
> That is way too early -- given that the actual unplug will happen
> in SmmCpuUpdate() and given that the BSPHandler() would have waited
> for the APs multiple times before then.
> 
> Another possibility is that the actual ejection be deferred to the
> _EJ0 method after the return from the SMI. But, that seems like a
> hack. Additionally, Igor points out here that this approach has problems:
>   https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/

I've filed:

  https://bugzilla.tianocore.org/show_bug.cgi?id=3132

Can you please register an account in the TianoCore Bugzilla at
<https://bugzilla.tianocore.org/>, and assign the above ticket to yourself?

And then, the URL of the new BZ ticket should be included in every
commit message, like this:

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132

just above your S-o-b.

No need to repost just because of this; I'll review the RFC series later.

Thanks,
Laszlo

> 
> Please review.
> 
> Thanks
> Ankur
> 
> Ankur Arora (5):
>   OvmfPkg/CpuHotplugSmm: move CPU Hotplug into PlugCpus()
>   OvmfPkg/CpuHotplugSmm: handle fw_remove
>   OvmfPkg/CpuHotplugSmm: add CpuStatus helper function
>   OvmfPkg/CpuHotplugSmm: handle CPU hot-unplug
>   OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOT_UNPLUG
> 
>  OvmfPkg/CpuHotplugSmm/CpuHotplug.c            | 293 ++++++++++++------
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.c             |  58 +++-
>  OvmfPkg/CpuHotplugSmm/QemuCpuhp.h             |   6 +
>  .../Include/IndustryStandard/QemuCpuHotplug.h |   2 +
>  OvmfPkg/SmmControl2Dxe/SmiFeatures.c          |  21 +-
>  5 files changed, 280 insertions(+), 100 deletions(-)
> 

Re: [edk2-devel] [RFC PATCH 0/5] support CPU hot-unplug

[Laszlo Ersek]

On 12/21/20 15:44, Laszlo Ersek wrote:
> Hi Ankur,
> 
> On 12/08/20 06:34, Ankur Arora wrote:
>> [ Resending to the correct edk2 alias this time. ]
>>
>> Hi,
>>
>> This series adds support for CPU hot-unplug with OVMF.
>>
>> Please see this in conjunction with the QEMU v2 series posted here: 
>>   https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
>>
>> In particular, would be glad for comments on Patch 4, specifically
>> where we should be ejecting the CPU. 
>>
>> Right now the ejection happens in UnplugCpus() (called from
>> CpuHotplugMmi()):
>>   +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
>>   +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
>>
>> That is way too early -- given that the actual unplug will happen
>> in SmmCpuUpdate() and given that the BSPHandler() would have waited
>> for the APs multiple times before then.
>>
>> Another possibility is that the actual ejection be deferred to the
>> _EJ0 method after the return from the SMI. But, that seems like a
>> hack. Additionally, Igor points out here that this approach has problems:
>>   https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/
> 
> I've filed:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> 
> Can you please register an account in the TianoCore Bugzilla at
> <https://bugzilla.tianocore.org/>, and assign the above ticket to yourself?
> 
> And then, the URL of the new BZ ticket should be included in every
> commit message, like this:
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> 
> just above your S-o-b.
> 
> No need to repost just because of this; I'll review the RFC series later.

I've also listed <https://bugzilla.tianocore.org/show_bug.cgi?id=3132>
under
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning#edk2-stable202102-tag-planning>.

Thanks
Laszlo

Re: [edk2-devel] [RFC PATCH 0/5] support CPU hot-unplug

[Igor Mammedov]

On Mon, 21 Dec 2020 15:44:35 +0100
"Laszlo Ersek" <lersek@redhat.com> wrote:

> Hi Ankur,
> 
> On 12/08/20 06:34, Ankur Arora wrote:
> > [ Resending to the correct edk2 alias this time. ]
> > 
> > Hi,
> > 
> > This series adds support for CPU hot-unplug with OVMF.
> > 
> > Please see this in conjunction with the QEMU v2 series posted here: 
> >   https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
> > 
> > In particular, would be glad for comments on Patch 4, specifically
> > where we should be ejecting the CPU. 
> > 
> > Right now the ejection happens in UnplugCpus() (called from
> > CpuHotplugMmi()):
> >   +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
> >   +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
> > 
> > That is way too early -- given that the actual unplug will happen
> > in SmmCpuUpdate() and given that the BSPHandler() would have waited
> > for the APs multiple times before then.
> > 
> > Another possibility is that the actual ejection be deferred to the
> > _EJ0 method after the return from the SMI. But, that seems like a
> > hack. Additionally, Igor points out here that this approach has problems:
> >   https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/  
> 
> I've filed:
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> 
> Can you please register an account in the TianoCore Bugzilla at
> <https://bugzilla.tianocore.org/>, and assign the above ticket to yourself?
> 
> And then, the URL of the new BZ ticket should be included in every
> commit message, like this:
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> 
> just above your S-o-b.
> 
> No need to repost just because of this; I'll review the RFC series later.
BTW:

meanwhile, QEMU part got merged so one doesn't need to hunt for it anymore.
If something is broken there, we will have to fix it on top.

> 
> Thanks,
> Laszlo
> 
> > 
> > Please review.
> > 
> > Thanks
> > Ankur
> > 
> > Ankur Arora (5):
> >   OvmfPkg/CpuHotplugSmm: move CPU Hotplug into PlugCpus()
> >   OvmfPkg/CpuHotplugSmm: handle fw_remove
> >   OvmfPkg/CpuHotplugSmm: add CpuStatus helper function
> >   OvmfPkg/CpuHotplugSmm: handle CPU hot-unplug
> >   OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOT_UNPLUG
> > 
> >  OvmfPkg/CpuHotplugSmm/CpuHotplug.c            | 293 ++++++++++++------
> >  OvmfPkg/CpuHotplugSmm/QemuCpuhp.c             |  58 +++-
> >  OvmfPkg/CpuHotplugSmm/QemuCpuhp.h             |   6 +
> >  .../Include/IndustryStandard/QemuCpuHotplug.h |   2 +
> >  OvmfPkg/SmmControl2Dxe/SmiFeatures.c          |  21 +-
> >  5 files changed, 280 insertions(+), 100 deletions(-)
> >   
> 
> 
> 
> 
> 
> 

Re: [edk2-devel] [RFC PATCH 0/5] support CPU hot-unplug

[Laszlo Ersek]

On 12/21/20 16:46, Igor Mammedov wrote:
> On Mon, 21 Dec 2020 15:44:35 +0100
> "Laszlo Ersek" <lersek@redhat.com> wrote:
> 
>> Hi Ankur,
>>
>> On 12/08/20 06:34, Ankur Arora wrote:
>>> [ Resending to the correct edk2 alias this time. ]
>>>
>>> Hi,
>>>
>>> This series adds support for CPU hot-unplug with OVMF.
>>>
>>> Please see this in conjunction with the QEMU v2 series posted here: 
>>>   https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
>>>
>>> In particular, would be glad for comments on Patch 4, specifically
>>> where we should be ejecting the CPU. 
>>>
>>> Right now the ejection happens in UnplugCpus() (called from
>>> CpuHotplugMmi()):
>>>   +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
>>>   +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
>>>
>>> That is way too early -- given that the actual unplug will happen
>>> in SmmCpuUpdate() and given that the BSPHandler() would have waited
>>> for the APs multiple times before then.
>>>
>>> Another possibility is that the actual ejection be deferred to the
>>> _EJ0 method after the return from the SMI. But, that seems like a
>>> hack. Additionally, Igor points out here that this approach has problems:
>>>   https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/  
>>
>> I've filed:
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>>
>> Can you please register an account in the TianoCore Bugzilla at
>> <https://bugzilla.tianocore.org/>, and assign the above ticket to yourself?
>>
>> And then, the URL of the new BZ ticket should be included in every
>> commit message, like this:
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>>
>> just above your S-o-b.
>>
>> No need to repost just because of this; I'll review the RFC series later.
> BTW:
> 
> meanwhile, QEMU part got merged so one doesn't need to hunt for it anymore.
> If something is broken there, we will have to fix it on top.

Thank you very much for the info!
Laszlo

Re: [edk2-devel] [RFC PATCH 0/5] support CPU hot-unplug

[Ankur Arora]

On 2020-12-21 6:44 a.m., Laszlo Ersek wrote:
> Hi Ankur,
> 
> On 12/08/20 06:34, Ankur Arora wrote:
>> [ Resending to the correct edk2 alias this time. ]
>>
>> Hi,
>>
>> This series adds support for CPU hot-unplug with OVMF.
>>
>> Please see this in conjunction with the QEMU v2 series posted here:
>>    https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
>>
>> In particular, would be glad for comments on Patch 4, specifically
>> where we should be ejecting the CPU.
>>
>> Right now the ejection happens in UnplugCpus() (called from
>> CpuHotplugMmi()):
>>    +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
>>    +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
>>
>> That is way too early -- given that the actual unplug will happen
>> in SmmCpuUpdate() and given that the BSPHandler() would have waited
>> for the APs multiple times before then.
>>
>> Another possibility is that the actual ejection be deferred to the
>> _EJ0 method after the return from the SMI. But, that seems like a
>> hack. Additionally, Igor points out here that this approach has problems:
>>    https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/
> 
> I've filed:
> 
>    https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> 
> Can you please register an account in the TianoCore Bugzilla at
> <https://bugzilla.tianocore.org/>, and assign the above ticket to yourself?
> 
> And then, the URL of the new BZ ticket should be included in every
> commit message, like this:
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> 
> just above your S-o-b.

Sure. Will do.
  
> No need to repost just because of this; I'll review the RFC series later.

Actually could you hold off before reviewing the RFC series. I'll be sending
a v2 series shortly. That does the actual unplug via SmmCpuFeaturesRendezvousExit()
(I had described the approach elsewhere in this thread.)

Thanks
Ankur

> Thanks,
> Laszlo
> 
>>
>> Please review.
>>
>> Thanks
>> Ankur
>>
>> Ankur Arora (5):
>>    OvmfPkg/CpuHotplugSmm: move CPU Hotplug into PlugCpus()
>>    OvmfPkg/CpuHotplugSmm: handle fw_remove
>>    OvmfPkg/CpuHotplugSmm: add CpuStatus helper function
>>    OvmfPkg/CpuHotplugSmm: handle CPU hot-unplug
>>    OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOT_UNPLUG
>>
>>   OvmfPkg/CpuHotplugSmm/CpuHotplug.c            | 293 ++++++++++++------
>>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.c             |  58 +++-
>>   OvmfPkg/CpuHotplugSmm/QemuCpuhp.h             |   6 +
>>   .../Include/IndustryStandard/QemuCpuHotplug.h |   2 +
>>   OvmfPkg/SmmControl2Dxe/SmiFeatures.c          |  21 +-
>>   5 files changed, 280 insertions(+), 100 deletions(-)
>>
> 

Re: [edk2-devel] [RFC PATCH 0/5] support CPU hot-unplug

[Laszlo Ersek]

On 12/21/20 20:09, Ankur Arora wrote:
> On 2020-12-21 6:44 a.m., Laszlo Ersek wrote:
>> Hi Ankur,
>>
>> On 12/08/20 06:34, Ankur Arora wrote:
>>> [ Resending to the correct edk2 alias this time. ]
>>>
>>> Hi,
>>>
>>> This series adds support for CPU hot-unplug with OVMF.
>>>
>>> Please see this in conjunction with the QEMU v2 series posted here:
>>>   
>>> https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
>>>
>>>
>>> In particular, would be glad for comments on Patch 4, specifically
>>> where we should be ejecting the CPU.
>>>
>>> Right now the ejection happens in UnplugCpus() (called from
>>> CpuHotplugMmi()):
>>>    +    QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
>>>    +    QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
>>>
>>> That is way too early -- given that the actual unplug will happen
>>> in SmmCpuUpdate() and given that the BSPHandler() would have waited
>>> for the APs multiple times before then.
>>>
>>> Another possibility is that the actual ejection be deferred to the
>>> _EJ0 method after the return from the SMI. But, that seems like a
>>> hack. Additionally, Igor points out here that this approach has
>>> problems:
>>>   
>>> https://lore.kernel.org/qemu-devel/20201204170939.1815522-1-imammedo@redhat.com/
>>>
>>
>> I've filed:
>>
>>    https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>>
>> Can you please register an account in the TianoCore Bugzilla at
>> <https://bugzilla.tianocore.org/>, and assign the above ticket to
>> yourself?
>>
>> And then, the URL of the new BZ ticket should be included in every
>> commit message, like this:
>>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>>
>> just above your S-o-b.
> 
> Sure. Will do.
>  
>> No need to repost just because of this; I'll review the RFC series later.
> 
> Actually could you hold off before reviewing the RFC series. I'll be
> sending
> a v2 series shortly. That does the actual unplug via
> SmmCpuFeaturesRendezvousExit()
> (I had described the approach elsewhere in this thread.)

Sure, I can de-queue the RFC series. I planned to review it anyway in
the new year; I've just done some admin stuff around the feature now.

Thanks
Laszlo