* [PATCH v2 01/10] OvmfPkg/CpuHotplugSmm: move CPU Hotplug into PlugCpus()
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 19:55 ` [PATCH v2 02/10] OvmfPkg/CpuHotplugSmm: handle Hot-unplug events Ankur Arora
` (8 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
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..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.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 02/10] OvmfPkg/CpuHotplugSmm: handle Hot-unplug events
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
2021-01-07 19:55 ` [PATCH v2 01/10] OvmfPkg/CpuHotplugSmm: move CPU Hotplug into PlugCpus() Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 19:55 ` [PATCH v2 03/10] OvmfPkg/CpuHotplugSmm: add Qemu CpuStatus helper Ankur Arora
` (7 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Process fw_remove events in QemuCpuhpCollectApicIds() and collect
corresponding APIC IDs for CPUs that are being hot-unplugged.
Additionally we now ignore CPUs which have only 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..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.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 03/10] OvmfPkg/CpuHotplugSmm: add Qemu CpuStatus helper
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
2021-01-07 19:55 ` [PATCH v2 01/10] OvmfPkg/CpuHotplugSmm: move CPU Hotplug into PlugCpus() Ankur Arora
2021-01-07 19:55 ` [PATCH v2 02/10] OvmfPkg/CpuHotplugSmm: handle Hot-unplug events Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 19:55 ` [PATCH v2 04/10] OvmfPkg/CpuHotplugSmm: handle CPU hot-unplug Ankur Arora
` (6 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Add QemuCpuhpWriteCpuStatus() which will be used to update the QEMU
CPU status register. On error, it hangs in similar fashion to the
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 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.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 04/10] OvmfPkg/CpuHotplugSmm: handle CPU hot-unplug
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
` (2 preceding siblings ...)
2021-01-07 19:55 ` [PATCH v2 03/10] OvmfPkg/CpuHotplugSmm: add Qemu CpuStatus helper Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 19:55 ` [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface() Ankur Arora
` (5 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Introduce a new function UnplugCpus() which, for each unplugged CPU:
* finds the slot for APIC ID in CPU_HOT_PLUG_DATA
* informs PiSmmCpuDxeSmm by calling
EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor()
* caches the APIC ID, such that it can be ejected at SMI exit
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 | 135 ++++++++++++++++++++++++++++++++++++-
1 file changed, 134 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 0f8f210d0ecf..20d92a35da39 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -53,6 +53,14 @@ STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
STATIC APIC_ID *mPluggedApicIds;
STATIC APIC_ID *mToUnplugApicIds;
//
+// Similar to the SMRAM arrays above mHotunplugWork stores APIC IDs of
+// processors with pending unplugs for the duration of the MMI.
+//
+// This array maps ProcessorNum -> APIC ID and so has room for possible
+// CPU count.
+//
+STATIC APIC_ID *mHotUnplugWork;
+//
// Address of the non-SMRAM reserved memory page that contains the Post-SMM Pen
// for hot-added CPUs.
//
@@ -182,6 +190,100 @@ 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.
+
+ @param[out] mHotUnplugWork List mapping ProcessorNum -> APIC ID for later unplug.
+ Invalid entries are specified as MAX_UINT32.
+
+ @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,
+ OUT APIC_ID *mHotUnplugWork
+ )
+{
+ 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 is 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;
+ } else {
+ //
+ // Stash the APIC IDs so we can do the actual unplug later.
+ //
+
+ if (mHotUnplugWork[ProcessorNum] != MAX_UINT32) {
+ //
+ // Since ProcessorNum and APIC-ID are a 1-1 mapping, so an already
+ // filled mHotUnplugWork[ProcessorNum] is a fatal error.
+ //
+ DEBUG ((DEBUG_ERROR, "%a: ProcessorNum %u maps to " FMT_APIC_ID ", cannot map to " FMT_APIC_ID "\n",
+ __FUNCTION__, ProcessorNum, mHotUnplugWork[ProcessorNum], RemoveApicId));
+ goto Fatal;
+ }
+
+ DEBUG ((DEBUG_INFO, "%a: Caching ProcessorNum %u -> " FMT_APIC_ID " for unplugging\n",
+ __FUNCTION__, ProcessorNum, RemoveApicId));
+ mHotUnplugWork[ProcessorNum] = RemoveApicId;
+ }
+
+ ToUnplugIdx++;
+ }
+
+ //
+ // We've handled this unplug.
+ //
+ return EFI_SUCCESS;
+
+Fatal:
+ return EFI_INTERRUPT_PENDING;
+}
+
+/**
CPU Hotplug MMI handler function.
This is a root MMI handler.
@@ -297,6 +399,8 @@ CpuHotplugMmi (
if (PluggedCount > 0) {
Status = PlugCpus(mPluggedApicIds, PluggedCount);
+ } else if (ToUnplugCount > 0) {
+ Status = UnplugCpus(mToUnplugApicIds, ToUnplugCount, mHotUnplugWork);
}
if (EFI_ERROR(Status)) {
@@ -330,6 +434,7 @@ CpuHotplugEntry (
{
EFI_STATUS Status;
UINTN Size;
+ UINTN Idx;
//
// This module should only be included when SMM support is required.
@@ -403,12 +508,36 @@ CpuHotplugEntry (
}
//
+ // Allocate for the full CPU count. We index to-be-unplugged APIC IDs
+ // with ProcessorNum.
+ //
+ if (RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), mCpuHotPlugData->ArrayLength, &Size))) {
+ Status = EFI_ABORTED;
+ DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_PLUG_DATA\n", __FUNCTION__));
+ goto ReleaseToUnplugApicIds;
+ }
+ Status = gMmst->MmAllocatePool (EfiRuntimeServicesData, Size,
+ (VOID **)&mHotUnplugWork);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status));
+ goto ReleaseToUnplugApicIds;
+ }
+
+ //
+ // We will use mHotUnplugWork to map from ProcessorNum -> APIC_ID.
+ // Initialize to known invalid values.
+ //
+ for (Idx = 0; Idx < mCpuHotPlugData->ArrayLength; Idx++) {
+ mHotUnplugWork[Idx] = MAX_UINT32;
+ }
+
+ //
// Allocate the Post-SMM Pen for hot-added CPUs.
//
Status = SmbaseAllocatePostSmmPen (&mPostSmmPenAddress,
SystemTable->BootServices);
if (EFI_ERROR (Status)) {
- goto ReleaseToUnplugApicIds;
+ goto ReleaseToHotUnplugWork;
}
//
@@ -468,6 +597,10 @@ ReleasePostSmmPen:
SmbaseReleasePostSmmPen (mPostSmmPenAddress, SystemTable->BootServices);
mPostSmmPenAddress = 0;
+ReleaseToHotUnplugWork:
+ gMmst->MmFreePool (mHotUnplugWork);
+ mHotUnplugWork = NULL;
+
ReleaseToUnplugApicIds:
gMmst->MmFreePool (mToUnplugApicIds);
mToUnplugApicIds = NULL;
--
2.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
` (3 preceding siblings ...)
2021-01-07 19:55 ` [PATCH v2 04/10] OvmfPkg/CpuHotplugSmm: handle CPU hot-unplug Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 20:48 ` [edk2-devel] " Laszlo Ersek
2021-01-07 19:55 ` [PATCH v2 06/10] UefiCpuPkg/PiSmmCpuDxeSmm: initialize IsBsp Ankur Arora
` (4 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jian J Wang,
Michael D Kinney, Liming Gao, Zhiguang Liu, Eric Dong, Ray Ni,
Rahul Kumar, Aaron Young
Add MmRegisterShutdownInterface(), which is used to register a callback,
that gets used to do the final ejection as part of CPU hot-unplug.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
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: 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>
---
Not sure this is the right way to register a callback to be called
from SmmCpuFeaturesRendezvousExit(). Happy to hear suggestions for
a better way to accomplish this.
---
MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 1 +
MdePkg/Include/Pi/PiMmCis.h | 16 ++++++++++++++++
MdePkg/Include/Pi/PiSmmCis.h | 2 ++
MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c | 11 +++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 +
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 1 +
6 files changed, 32 insertions(+)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index cfa9922cbdb5..9d883bb06633 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -39,6 +39,7 @@ EFI_SMM_SYSTEM_TABLE2 gSmmCoreSmst = {
SmmFreePool,
SmmAllocatePages,
SmmFreePages,
+ NULL, // SmmShutdownAp
NULL, // SmmStartupThisAp
0, // CurrentlyExecutingCpu
0, // NumberOfCpus
diff --git a/MdePkg/Include/Pi/PiMmCis.h b/MdePkg/Include/Pi/PiMmCis.h
index fdf0591a03d6..237bd8dcba76 100644
--- a/MdePkg/Include/Pi/PiMmCis.h
+++ b/MdePkg/Include/Pi/PiMmCis.h
@@ -77,6 +77,14 @@ EFI_STATUS
IN OUT VOID *ProcArguments OPTIONAL
);
+typedef
+EFI_STATUS
+(EFIAPI *EFI_MM_SHUTDOWN_AP)(
+ IN UINTN CpuNumber,
+ IN BOOLEAN IsBSP
+ );
+
+
/**
Function prototype for protocol install notification.
@@ -242,6 +250,13 @@ VOID
IN CONST EFI_MM_ENTRY_CONTEXT *MmEntryContext
);
+EFI_STATUS
+EFIAPI
+MmRegisterShutdownInterface (
+ IN EFI_MM_SHUTDOWN_AP Procedure
+ );
+
+
///
/// Management Mode System Table (MMST)
///
@@ -282,6 +297,7 @@ struct _EFI_MM_SYSTEM_TABLE {
///
/// MP service
///
+ EFI_MM_SHUTDOWN_AP MmShutdownAp;
EFI_MM_STARTUP_THIS_AP MmStartupThisAp;
///
diff --git a/MdePkg/Include/Pi/PiSmmCis.h b/MdePkg/Include/Pi/PiSmmCis.h
index 06ef4aecd7b5..296dc01f6703 100644
--- a/MdePkg/Include/Pi/PiSmmCis.h
+++ b/MdePkg/Include/Pi/PiSmmCis.h
@@ -49,6 +49,7 @@ EFI_STATUS
IN UINTN TableSize
);
+typedef EFI_MM_SHUTDOWN_AP EFI_SMM_SHUTDOWN_AP;
typedef EFI_MM_STARTUP_THIS_AP EFI_SMM_STARTUP_THIS_AP;
typedef EFI_MM_NOTIFY_FN EFI_SMM_NOTIFY_FN;
typedef EFI_MM_REGISTER_PROTOCOL_NOTIFY EFI_SMM_REGISTER_PROTOCOL_NOTIFY;
@@ -137,6 +138,7 @@ struct _EFI_SMM_SYSTEM_TABLE2 {
///
/// MP service
///
+ EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
EFI_SMM_STARTUP_THIS_AP SmmStartupThisAp;
///
diff --git a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
index 27f9d526e396..c7d81a0dc193 100644
--- a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
+++ b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
@@ -55,3 +55,14 @@ MmServicesTableLibConstructor (
return EFI_SUCCESS;
}
+
+EFI_STATUS
+EFIAPI
+MmRegisterShutdownInterface (
+ IN EFI_MM_SHUTDOWN_AP Procedure
+ )
+{
+ gMmst->MmShutdownAp = Procedure;
+
+ return EFI_SUCCESS;
+}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index db68e1316ec5..f2f67e85e5e9 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -22,6 +22,7 @@ SMM_CPU_PRIVATE_DATA mSmmCpuPrivateData = {
NULL, // Pointer to CpuSaveStateSize array
NULL, // Pointer to CpuSaveState array
{ {0} }, // SmmReservedSmramRegion
+ NULL, // SmmShutdownAp
{
SmmStartupThisAp, // SmmCoreEntryContext.SmmStartupThisAp
0, // SmmCoreEntryContext.CurrentlyExecutingCpu
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index b8aa9e1769d3..7672834a2f70 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -247,6 +247,7 @@ typedef struct {
VOID **CpuSaveState;
EFI_SMM_RESERVED_SMRAM_REGION SmmReservedSmramRegion[1];
+ EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
EFI_SMM_ENTRY_CONTEXT SmmCoreEntryContext;
EFI_SMM_ENTRY_POINT SmmCoreEntry;
--
2.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()
2021-01-07 19:55 ` [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface() Ankur Arora
@ 2021-01-07 20:48 ` Laszlo Ersek
2021-01-07 21:00 ` Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Laszlo Ersek @ 2021-01-07 20:48 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jian J Wang, Michael D Kinney,
Liming Gao, Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar,
Aaron Young
On 01/07/21 20:55, Ankur Arora wrote:
> Add MmRegisterShutdownInterface(), which is used to register a callback,
> that gets used to do the final ejection as part of CPU hot-unplug.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> 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: 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>
> ---
>
> Not sure this is the right way to register a callback to be called
> from SmmCpuFeaturesRendezvousExit(). Happy to hear suggestions for
> a better way to accomplish this.
No, it's not.
SmmCpuFeaturesRendezvousExit() is an interface in the
"UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h" class. Each platform is
supposed to create its own instance of this library class. For example,
OVMF's instance is at:
OvmfPkg/Library/SmmCpuFeaturesLib
You can modify the SmmCpuFeaturesRendezvousExit() function
implementation in that library instance.
I don't think you can easily modify the function *declaration* though,
as that would break a whole lot of platforms that exist outside of edk2.
Additionally, I don't think you can modify EFI_MM_SYSTEM_TABLE. That's a
structure defined in the Platform Init specification. You'd first need
to standardize (via the Platform Init Working Group of the UEFI Forum)
the proposed change, and a compatible way to upgrade would have to be
found. An alternative would be the "code first" procedure, which exists
so that code can be contributed ahead of changing the published
standards. But I'm unsure (I don't remember) how that procedure works in
practice.
Either way, I would advise against this approach. We should limit
ourselves to modifying only the contents of
SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib.
You should basically migrate the contents of CpuUnplugExitWork() (in
patch#8) into SmmCpuFeaturesRendezvousExit(), without changing the
prototype of SmmCpuFeaturesRendezvousExit(). This might require you to
calculate "IsBSP" on the spot, possibly with the help of some global
data that you set up in advance.
*However*, regarding "IsBSP" itself -- on the QEMU platform, the BSP is
neither unpluggable, nor switchable with any one of the APs. A removal
for the BSP will never be attempted, so it's fine to add much simpler
code that recognizes such an attempt (as a sanity check), and hangs hard
on it. If that would lead to grave complications, then don't bother with
it; just assume (or enforce elsewhere) that the BSP is never being
unplugged.
Please see the apic_designate_bsp() call sites in the QEMU source code
-- there is exactly one of them, in "target/i386/cpu.c":
/* We hard-wire the BSP to the first CPU. */
apic_designate_bsp(cpu->apic_state, s->cpu_index == 0);
By adding the business logic to SmmCpuFeaturesRendezvousExit() instead
of CpuUnplugExitWork, said logic will actually be included in the
PiSmmCpuDxeSmm binary -- but that's fine. That's why the
SmmCpuFeaturesRendezvousExit() API exists, as a platform customization
hook for PiSmmCpuDxeSmm.
A formal comment in closing -- modifying two packages in the same patch,
such as OvmfPkg and UefiCpuPkg, is almost always forbidden. It is
permitted exceptionally, when there is absolutely no way to solve an
issue with a gradual approach (i.e., by modifying the packages in
question with separate patches).
I think I'll skip reviewing this version beyond this email, as the
required changes appear quite intrusive.
Thanks
Laszlo
>
> ---
> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 1 +
> MdePkg/Include/Pi/PiMmCis.h | 16 ++++++++++++++++
> MdePkg/Include/Pi/PiSmmCis.h | 2 ++
> MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c | 11 +++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 +
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 1 +
> 6 files changed, 32 insertions(+)
>
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> index cfa9922cbdb5..9d883bb06633 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
> @@ -39,6 +39,7 @@ EFI_SMM_SYSTEM_TABLE2 gSmmCoreSmst = {
> SmmFreePool,
> SmmAllocatePages,
> SmmFreePages,
> + NULL, // SmmShutdownAp
> NULL, // SmmStartupThisAp
> 0, // CurrentlyExecutingCpu
> 0, // NumberOfCpus
> diff --git a/MdePkg/Include/Pi/PiMmCis.h b/MdePkg/Include/Pi/PiMmCis.h
> index fdf0591a03d6..237bd8dcba76 100644
> --- a/MdePkg/Include/Pi/PiMmCis.h
> +++ b/MdePkg/Include/Pi/PiMmCis.h
> @@ -77,6 +77,14 @@ EFI_STATUS
> IN OUT VOID *ProcArguments OPTIONAL
> );
>
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_MM_SHUTDOWN_AP)(
> + IN UINTN CpuNumber,
> + IN BOOLEAN IsBSP
> + );
> +
> +
> /**
> Function prototype for protocol install notification.
>
> @@ -242,6 +250,13 @@ VOID
> IN CONST EFI_MM_ENTRY_CONTEXT *MmEntryContext
> );
>
> +EFI_STATUS
> +EFIAPI
> +MmRegisterShutdownInterface (
> + IN EFI_MM_SHUTDOWN_AP Procedure
> + );
> +
> +
> ///
> /// Management Mode System Table (MMST)
> ///
> @@ -282,6 +297,7 @@ struct _EFI_MM_SYSTEM_TABLE {
> ///
> /// MP service
> ///
> + EFI_MM_SHUTDOWN_AP MmShutdownAp;
> EFI_MM_STARTUP_THIS_AP MmStartupThisAp;
>
> ///
> diff --git a/MdePkg/Include/Pi/PiSmmCis.h b/MdePkg/Include/Pi/PiSmmCis.h
> index 06ef4aecd7b5..296dc01f6703 100644
> --- a/MdePkg/Include/Pi/PiSmmCis.h
> +++ b/MdePkg/Include/Pi/PiSmmCis.h
> @@ -49,6 +49,7 @@ EFI_STATUS
> IN UINTN TableSize
> );
>
> +typedef EFI_MM_SHUTDOWN_AP EFI_SMM_SHUTDOWN_AP;
> typedef EFI_MM_STARTUP_THIS_AP EFI_SMM_STARTUP_THIS_AP;
> typedef EFI_MM_NOTIFY_FN EFI_SMM_NOTIFY_FN;
> typedef EFI_MM_REGISTER_PROTOCOL_NOTIFY EFI_SMM_REGISTER_PROTOCOL_NOTIFY;
> @@ -137,6 +138,7 @@ struct _EFI_SMM_SYSTEM_TABLE2 {
> ///
> /// MP service
> ///
> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
> EFI_SMM_STARTUP_THIS_AP SmmStartupThisAp;
>
> ///
> diff --git a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
> index 27f9d526e396..c7d81a0dc193 100644
> --- a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
> +++ b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
> @@ -55,3 +55,14 @@ MmServicesTableLibConstructor (
>
> return EFI_SUCCESS;
> }
> +
> +EFI_STATUS
> +EFIAPI
> +MmRegisterShutdownInterface (
> + IN EFI_MM_SHUTDOWN_AP Procedure
> + )
> +{
> + gMmst->MmShutdownAp = Procedure;
> +
> + return EFI_SUCCESS;
> +}
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index db68e1316ec5..f2f67e85e5e9 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -22,6 +22,7 @@ SMM_CPU_PRIVATE_DATA mSmmCpuPrivateData = {
> NULL, // Pointer to CpuSaveStateSize array
> NULL, // Pointer to CpuSaveState array
> { {0} }, // SmmReservedSmramRegion
> + NULL, // SmmShutdownAp
> {
> SmmStartupThisAp, // SmmCoreEntryContext.SmmStartupThisAp
> 0, // SmmCoreEntryContext.CurrentlyExecutingCpu
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index b8aa9e1769d3..7672834a2f70 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -247,6 +247,7 @@ typedef struct {
> VOID **CpuSaveState;
>
> EFI_SMM_RESERVED_SMRAM_REGION SmmReservedSmramRegion[1];
> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
> EFI_SMM_ENTRY_CONTEXT SmmCoreEntryContext;
> EFI_SMM_ENTRY_POINT SmmCoreEntry;
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()
2021-01-07 20:48 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-07 21:00 ` Laszlo Ersek
2021-01-07 21:19 ` Ankur Arora
2021-01-07 21:45 ` Laszlo Ersek
2 siblings, 0 replies; 19+ messages in thread
From: Laszlo Ersek @ 2021-01-07 21:00 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jian J Wang, Michael D Kinney,
Liming Gao, Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar,
Aaron Young
On 01/07/21 21:48, Laszlo Ersek wrote:
> On 01/07/21 20:55, Ankur Arora wrote:
>> Add MmRegisterShutdownInterface(), which is used to register a callback,
>> that gets used to do the final ejection as part of CPU hot-unplug.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> 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: 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>
>> ---
>>
>> Not sure this is the right way to register a callback to be called
>> from SmmCpuFeaturesRendezvousExit(). Happy to hear suggestions for
>> a better way to accomplish this.
>
> No, it's not.
>
> SmmCpuFeaturesRendezvousExit() is an interface in the
> "UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h" class. Each platform is
> supposed to create its own instance of this library class. For example,
> OVMF's instance is at:
>
> OvmfPkg/Library/SmmCpuFeaturesLib
>
> You can modify the SmmCpuFeaturesRendezvousExit() function
> implementation in that library instance.
>
> I don't think you can easily modify the function *declaration* though,
> as that would break a whole lot of platforms that exist outside of edk2.
>
> Additionally, I don't think you can modify EFI_MM_SYSTEM_TABLE. That's a
> structure defined in the Platform Init specification. You'd first need
> to standardize (via the Platform Init Working Group of the UEFI Forum)
> the proposed change, and a compatible way to upgrade would have to be
> found. An alternative would be the "code first" procedure, which exists
> so that code can be contributed ahead of changing the published
> standards. But I'm unsure (I don't remember) how that procedure works in
> practice.
>
> Either way, I would advise against this approach. We should limit
> ourselves to modifying only the contents of
> SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib.
>
> You should basically migrate the contents of CpuUnplugExitWork() (in
> patch#8) into SmmCpuFeaturesRendezvousExit(), without changing the
> prototype of SmmCpuFeaturesRendezvousExit(). This might require you to
> calculate "IsBSP" on the spot, possibly with the help of some global
> data that you set up in advance.
If you need "IsBSP" for a *different purpose* than unplugging the BSP
itself, then you can fetch that easily: please see the
PlatformSmmBspElection() function in
"OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c".
The logic there is so simple that you can simply copy it into
SmmCpuFeaturesRendezvousExit()
[OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c],
to calculate IsBSP on the spot (rather than taking it from a parameter):
MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr;
ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1);
Thanks
Laszlo
>
> *However*, regarding "IsBSP" itself -- on the QEMU platform, the BSP is
> neither unpluggable, nor switchable with any one of the APs. A removal
> for the BSP will never be attempted, so it's fine to add much simpler
> code that recognizes such an attempt (as a sanity check), and hangs hard
> on it. If that would lead to grave complications, then don't bother with
> it; just assume (or enforce elsewhere) that the BSP is never being
> unplugged.
>
> Please see the apic_designate_bsp() call sites in the QEMU source code
> -- there is exactly one of them, in "target/i386/cpu.c":
>
> /* We hard-wire the BSP to the first CPU. */
> apic_designate_bsp(cpu->apic_state, s->cpu_index == 0);
>
>
> By adding the business logic to SmmCpuFeaturesRendezvousExit() instead
> of CpuUnplugExitWork, said logic will actually be included in the
> PiSmmCpuDxeSmm binary -- but that's fine. That's why the
> SmmCpuFeaturesRendezvousExit() API exists, as a platform customization
> hook for PiSmmCpuDxeSmm.
>
>
> A formal comment in closing -- modifying two packages in the same patch,
> such as OvmfPkg and UefiCpuPkg, is almost always forbidden. It is
> permitted exceptionally, when there is absolutely no way to solve an
> issue with a gradual approach (i.e., by modifying the packages in
> question with separate patches).
>
> I think I'll skip reviewing this version beyond this email, as the
> required changes appear quite intrusive.
>
> Thanks
> Laszlo
>
>>
>> ---
>> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 1 +
>> MdePkg/Include/Pi/PiMmCis.h | 16 ++++++++++++++++
>> MdePkg/Include/Pi/PiSmmCis.h | 2 ++
>> MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c | 11 +++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 +
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 1 +
>> 6 files changed, 32 insertions(+)
>>
>> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> index cfa9922cbdb5..9d883bb06633 100644
>> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> @@ -39,6 +39,7 @@ EFI_SMM_SYSTEM_TABLE2 gSmmCoreSmst = {
>> SmmFreePool,
>> SmmAllocatePages,
>> SmmFreePages,
>> + NULL, // SmmShutdownAp
>> NULL, // SmmStartupThisAp
>> 0, // CurrentlyExecutingCpu
>> 0, // NumberOfCpus
>> diff --git a/MdePkg/Include/Pi/PiMmCis.h b/MdePkg/Include/Pi/PiMmCis.h
>> index fdf0591a03d6..237bd8dcba76 100644
>> --- a/MdePkg/Include/Pi/PiMmCis.h
>> +++ b/MdePkg/Include/Pi/PiMmCis.h
>> @@ -77,6 +77,14 @@ EFI_STATUS
>> IN OUT VOID *ProcArguments OPTIONAL
>> );
>>
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *EFI_MM_SHUTDOWN_AP)(
>> + IN UINTN CpuNumber,
>> + IN BOOLEAN IsBSP
>> + );
>> +
>> +
>> /**
>> Function prototype for protocol install notification.
>>
>> @@ -242,6 +250,13 @@ VOID
>> IN CONST EFI_MM_ENTRY_CONTEXT *MmEntryContext
>> );
>>
>> +EFI_STATUS
>> +EFIAPI
>> +MmRegisterShutdownInterface (
>> + IN EFI_MM_SHUTDOWN_AP Procedure
>> + );
>> +
>> +
>> ///
>> /// Management Mode System Table (MMST)
>> ///
>> @@ -282,6 +297,7 @@ struct _EFI_MM_SYSTEM_TABLE {
>> ///
>> /// MP service
>> ///
>> + EFI_MM_SHUTDOWN_AP MmShutdownAp;
>> EFI_MM_STARTUP_THIS_AP MmStartupThisAp;
>>
>> ///
>> diff --git a/MdePkg/Include/Pi/PiSmmCis.h b/MdePkg/Include/Pi/PiSmmCis.h
>> index 06ef4aecd7b5..296dc01f6703 100644
>> --- a/MdePkg/Include/Pi/PiSmmCis.h
>> +++ b/MdePkg/Include/Pi/PiSmmCis.h
>> @@ -49,6 +49,7 @@ EFI_STATUS
>> IN UINTN TableSize
>> );
>>
>> +typedef EFI_MM_SHUTDOWN_AP EFI_SMM_SHUTDOWN_AP;
>> typedef EFI_MM_STARTUP_THIS_AP EFI_SMM_STARTUP_THIS_AP;
>> typedef EFI_MM_NOTIFY_FN EFI_SMM_NOTIFY_FN;
>> typedef EFI_MM_REGISTER_PROTOCOL_NOTIFY EFI_SMM_REGISTER_PROTOCOL_NOTIFY;
>> @@ -137,6 +138,7 @@ struct _EFI_SMM_SYSTEM_TABLE2 {
>> ///
>> /// MP service
>> ///
>> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
>> EFI_SMM_STARTUP_THIS_AP SmmStartupThisAp;
>>
>> ///
>> diff --git a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> index 27f9d526e396..c7d81a0dc193 100644
>> --- a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> +++ b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> @@ -55,3 +55,14 @@ MmServicesTableLibConstructor (
>>
>> return EFI_SUCCESS;
>> }
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +MmRegisterShutdownInterface (
>> + IN EFI_MM_SHUTDOWN_AP Procedure
>> + )
>> +{
>> + gMmst->MmShutdownAp = Procedure;
>> +
>> + return EFI_SUCCESS;
>> +}
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index db68e1316ec5..f2f67e85e5e9 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -22,6 +22,7 @@ SMM_CPU_PRIVATE_DATA mSmmCpuPrivateData = {
>> NULL, // Pointer to CpuSaveStateSize array
>> NULL, // Pointer to CpuSaveState array
>> { {0} }, // SmmReservedSmramRegion
>> + NULL, // SmmShutdownAp
>> {
>> SmmStartupThisAp, // SmmCoreEntryContext.SmmStartupThisAp
>> 0, // SmmCoreEntryContext.CurrentlyExecutingCpu
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> index b8aa9e1769d3..7672834a2f70 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> @@ -247,6 +247,7 @@ typedef struct {
>> VOID **CpuSaveState;
>>
>> EFI_SMM_RESERVED_SMRAM_REGION SmmReservedSmramRegion[1];
>> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
>> EFI_SMM_ENTRY_CONTEXT SmmCoreEntryContext;
>> EFI_SMM_ENTRY_POINT SmmCoreEntry;
>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()
2021-01-07 20:48 ` [edk2-devel] " Laszlo Ersek
2021-01-07 21:00 ` Laszlo Ersek
@ 2021-01-07 21:19 ` Ankur Arora
2021-01-07 21:50 ` Laszlo Ersek
2021-01-07 21:45 ` Laszlo Ersek
2 siblings, 1 reply; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 21:19 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: imammedo, boris.ostrovsky, Jian J Wang, Michael D Kinney,
Liming Gao, Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar,
Aaron Young
On 2021-01-07 12:48 p.m., Laszlo Ersek wrote:
> On 01/07/21 20:55, Ankur Arora wrote:
>> Add MmRegisterShutdownInterface(), which is used to register a callback,
>> that gets used to do the final ejection as part of CPU hot-unplug.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> 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: 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>
>> ---
>>
>> Not sure this is the right way to register a callback to be called
>> from SmmCpuFeaturesRendezvousExit(). Happy to hear suggestions for
>> a better way to accomplish this.
>
> No, it's not.
>
> SmmCpuFeaturesRendezvousExit() is an interface in the
> "UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h" class. Each platform is
> supposed to create its own instance of this library class. For example,
> OVMF's instance is at:
>
> OvmfPkg/Library/SmmCpuFeaturesLib
>
> You can modify the SmmCpuFeaturesRendezvousExit() function
> implementation in that library instance.
>
> I don't think you can easily modify the function *declaration* though,
> as that would break a whole lot of platforms that exist outside of edk2.
>
> Additionally, I don't think you can modify EFI_MM_SYSTEM_TABLE. That's a
> structure defined in the Platform Init specification. You'd first need
> to standardize (via the Platform Init Working Group of the UEFI Forum)
> the proposed change, and a compatible way to upgrade would have to be
> found. An alternative would be the "code first" procedure, which exists
> so that code can be contributed ahead of changing the published
> standards. But I'm unsure (I don't remember) how that procedure works in
> practice.
Thanks, yeah I'm definitely not looking to do that. I did look through
the spec but not enough to get any kind of familiarity with it.
>
> Either way, I would advise against this approach. We should limit
> ourselves to modifying only the contents of
> SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib.
That was my initial approach. The problem that ran into was that which
CPUs to eject depends on state held in OvmfPkg/CpuHotPlugSmm (see patch 8)
which, I could not figure out a way to make that available given that the
libraries are linked statically.
The other way I can think of doing this would be to just reprobe this state
from QEMU. The problem with that is that then there's zero shared state
between the removal and the ejection.
>
> You should basically migrate the contents of CpuUnplugExitWork() (in
> patch#8) into SmmCpuFeaturesRendezvousExit(), without changing the
> prototype of SmmCpuFeaturesRendezvousExit(). This might require you to
> calculate "IsBSP" on the spot, possibly with the help of some global
> data that you set up in advance.
The reason for trying to capture IsBSP was that mSmmMpSyncData->BspIndex
has gone at this point. If, as you say, there are significant problems
with changing the prototype, then I can figure out a way to avoid that,
especially given that we get the BSP by reading an APIC MSR anyway.
>
> *However*, regarding "IsBSP" itself -- on the QEMU platform, the BSP is
> neither unpluggable, nor switchable with any one of the APs. A removal
> for the BSP will never be attempted, so it's fine to add much simpler
> code that recognizes such an attempt (as a sanity check), and hangs hard
> on it. If that would lead to grave complications, then don't bother with
> it; just assume (or enforce elsewhere) that the BSP is never being
> unplugged.
>
> Please see the apic_designate_bsp() call sites in the QEMU source code
> -- there is exactly one of them, in "target/i386/cpu.c":
>
> /* We hard-wire the BSP to the first CPU. */
> apic_designate_bsp(cpu->apic_state, s->cpu_index == 0);
>
>
> By adding the business logic to SmmCpuFeaturesRendezvousExit() instead
> of CpuUnplugExitWork, said logic will actually be included in the
> PiSmmCpuDxeSmm binary -- but that's fine. That's why the
> SmmCpuFeaturesRendezvousExit() API exists, as a platform customization
> hook for PiSmmCpuDxeSmm.
>
>
> A formal comment in closing -- modifying two packages in the same patch,
> such as OvmfPkg and UefiCpuPkg, is almost always forbidden. It is
> permitted exceptionally, when there is absolutely no way to solve an
> issue with a gradual approach (i.e., by modifying the packages in
> question with separate patches).
Thanks for letting me know. I'll avoid that in future versions.
>
> I think I'll skip reviewing this version beyond this email, as the
> required changes appear quite intrusive.
Could you take a look at patch 8? That's really the crux of the series and
your comments on that would be helpful.
Thanks
Ankur
>
> Thanks
> Laszlo
>
>>
>> ---
>> MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 1 +
>> MdePkg/Include/Pi/PiMmCis.h | 16 ++++++++++++++++
>> MdePkg/Include/Pi/PiSmmCis.h | 2 ++
>> MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c | 11 +++++++++++
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 1 +
>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 1 +
>> 6 files changed, 32 insertions(+)
>>
>> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> index cfa9922cbdb5..9d883bb06633 100644
>> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
>> @@ -39,6 +39,7 @@ EFI_SMM_SYSTEM_TABLE2 gSmmCoreSmst = {
>> SmmFreePool,
>> SmmAllocatePages,
>> SmmFreePages,
>> + NULL, // SmmShutdownAp
>> NULL, // SmmStartupThisAp
>> 0, // CurrentlyExecutingCpu
>> 0, // NumberOfCpus
>> diff --git a/MdePkg/Include/Pi/PiMmCis.h b/MdePkg/Include/Pi/PiMmCis.h
>> index fdf0591a03d6..237bd8dcba76 100644
>> --- a/MdePkg/Include/Pi/PiMmCis.h
>> +++ b/MdePkg/Include/Pi/PiMmCis.h
>> @@ -77,6 +77,14 @@ EFI_STATUS
>> IN OUT VOID *ProcArguments OPTIONAL
>> );
>>
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *EFI_MM_SHUTDOWN_AP)(
>> + IN UINTN CpuNumber,
>> + IN BOOLEAN IsBSP
>> + );
>> +
>> +
>> /**
>> Function prototype for protocol install notification.
>>
>> @@ -242,6 +250,13 @@ VOID
>> IN CONST EFI_MM_ENTRY_CONTEXT *MmEntryContext
>> );
>>
>> +EFI_STATUS
>> +EFIAPI
>> +MmRegisterShutdownInterface (
>> + IN EFI_MM_SHUTDOWN_AP Procedure
>> + );
>> +
>> +
>> ///
>> /// Management Mode System Table (MMST)
>> ///
>> @@ -282,6 +297,7 @@ struct _EFI_MM_SYSTEM_TABLE {
>> ///
>> /// MP service
>> ///
>> + EFI_MM_SHUTDOWN_AP MmShutdownAp;
>> EFI_MM_STARTUP_THIS_AP MmStartupThisAp;
>>
>> ///
>> diff --git a/MdePkg/Include/Pi/PiSmmCis.h b/MdePkg/Include/Pi/PiSmmCis.h
>> index 06ef4aecd7b5..296dc01f6703 100644
>> --- a/MdePkg/Include/Pi/PiSmmCis.h
>> +++ b/MdePkg/Include/Pi/PiSmmCis.h
>> @@ -49,6 +49,7 @@ EFI_STATUS
>> IN UINTN TableSize
>> );
>>
>> +typedef EFI_MM_SHUTDOWN_AP EFI_SMM_SHUTDOWN_AP;
>> typedef EFI_MM_STARTUP_THIS_AP EFI_SMM_STARTUP_THIS_AP;
>> typedef EFI_MM_NOTIFY_FN EFI_SMM_NOTIFY_FN;
>> typedef EFI_MM_REGISTER_PROTOCOL_NOTIFY EFI_SMM_REGISTER_PROTOCOL_NOTIFY;
>> @@ -137,6 +138,7 @@ struct _EFI_SMM_SYSTEM_TABLE2 {
>> ///
>> /// MP service
>> ///
>> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
>> EFI_SMM_STARTUP_THIS_AP SmmStartupThisAp;
>>
>> ///
>> diff --git a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> index 27f9d526e396..c7d81a0dc193 100644
>> --- a/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> +++ b/MdePkg/Library/MmServicesTableLib/MmServicesTableLib.c
>> @@ -55,3 +55,14 @@ MmServicesTableLibConstructor (
>>
>> return EFI_SUCCESS;
>> }
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +MmRegisterShutdownInterface (
>> + IN EFI_MM_SHUTDOWN_AP Procedure
>> + )
>> +{
>> + gMmst->MmShutdownAp = Procedure;
>> +
>> + return EFI_SUCCESS;
>> +}
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> index db68e1316ec5..f2f67e85e5e9 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
>> @@ -22,6 +22,7 @@ SMM_CPU_PRIVATE_DATA mSmmCpuPrivateData = {
>> NULL, // Pointer to CpuSaveStateSize array
>> NULL, // Pointer to CpuSaveState array
>> { {0} }, // SmmReservedSmramRegion
>> + NULL, // SmmShutdownAp
>> {
>> SmmStartupThisAp, // SmmCoreEntryContext.SmmStartupThisAp
>> 0, // SmmCoreEntryContext.CurrentlyExecutingCpu
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> index b8aa9e1769d3..7672834a2f70 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
>> @@ -247,6 +247,7 @@ typedef struct {
>> VOID **CpuSaveState;
>>
>> EFI_SMM_RESERVED_SMRAM_REGION SmmReservedSmramRegion[1];
>> + EFI_SMM_SHUTDOWN_AP SmmShutdownAp;
>> EFI_SMM_ENTRY_CONTEXT SmmCoreEntryContext;
>> EFI_SMM_ENTRY_POINT SmmCoreEntry;
>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()
2021-01-07 21:19 ` Ankur Arora
@ 2021-01-07 21:50 ` Laszlo Ersek
0 siblings, 0 replies; 19+ messages in thread
From: Laszlo Ersek @ 2021-01-07 21:50 UTC (permalink / raw)
To: Ankur Arora, devel
Cc: imammedo, boris.ostrovsky, Jian J Wang, Michael D Kinney,
Liming Gao, Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar,
Aaron Young
On 01/07/21 22:19, Ankur Arora wrote:
> On 2021-01-07 12:48 p.m., Laszlo Ersek wrote:
>> We should limit ourselves to modifying only the contents of
>> SmmCpuFeaturesRendezvousExit() in OvmfPkg/Library/SmmCpuFeaturesLib.
>
> That was my initial approach. The problem that ran into was that which
> CPUs to eject depends on state held in OvmfPkg/CpuHotPlugSmm (see patch 8)
> which, I could not figure out a way to make that available given that the
> libraries are linked statically.
[...]
> Could you take a look at patch 8? That's really the crux of the series
Indeed.
The proposal I just sent elsewhere in this patch#5 sub-thread addresses
exactly that.
It's a frequent cross-driver communication pattern in edk2, utilizing
dynamic PCDs (platform configuration database entries).
The tricky part with this pattern is always proper serialization (make
sure the publishing driver always performs the PcdSet before the
consuming driver performs the PcdGet). This is often achieved with a
DEPEX. Luckily, in this case, such a depex (and indeed an earlier
instance of this exact same pattern) is already in place between
PiSmmCpuDxeSmm and CpuHotplugSmm, so you can imitate it. In the other
email, I laid out the suggested steps.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()
2021-01-07 20:48 ` [edk2-devel] " Laszlo Ersek
2021-01-07 21:00 ` Laszlo Ersek
2021-01-07 21:19 ` Ankur Arora
@ 2021-01-07 21:45 ` Laszlo Ersek
2021-01-07 23:42 ` Ankur Arora
2 siblings, 1 reply; 19+ messages in thread
From: Laszlo Ersek @ 2021-01-07 21:45 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jian J Wang, Michael D Kinney,
Liming Gao, Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar,
Aaron Young
On 01/07/21 21:48, Laszlo Ersek wrote:
> You should basically migrate the contents of CpuUnplugExitWork() (in
> patch#8) into SmmCpuFeaturesRendezvousExit(), without changing the
> prototype of SmmCpuFeaturesRendezvousExit(). This might require you to
> calculate "IsBSP" on the spot, possibly with the help of some global
> data that you set up in advance.
[...]
> By adding the business logic to SmmCpuFeaturesRendezvousExit() instead
> of CpuUnplugExitWork, said logic will actually be included in the
> PiSmmCpuDxeSmm binary -- but that's fine. That's why the
> SmmCpuFeaturesRendezvousExit() API exists, as a platform customization
> hook for PiSmmCpuDxeSmm.
If you need PiSmmCpuDxeSmm and CpuHotplugSmm to collaborate on a
dynamically sized array, such as "mHotUnplugWork", here's a possible
approach.
Consider how the somewhat similar "mCpuHotPlugData" object is allocated,
and then shared between both drivers, in SMRAM.
Line numbers at commit 85b8eac59b8c.
(1) in the PiCpuSmmEntry() function, PiSmmCpuDxeSmm first sets
"mMaxNumberOfCpus" to the value of "PcdCpuMaxLogicalProcessorNumber".
(PcdCpuHotPlugSupport is TRUE in our case.) Line 624.
(2) "mCpuHotPlugData.ArrayLength" is set to "mMaxNumberOfCpus". Line
824.
(3) SmmCpuFeaturesSmmRelocationComplete() is called
[OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c]. Line 930.
(4) The address of "mCpuHotPlugData" is published through
"PcdCpuHotPlugDataAddress", on line 1021. (Again, PcdCpuHotPlugSupport
is TRUE.)
(5) "OvmfPkg/CpuHotplugSmm/CpuHotplug.c" then locates "mCpuHotPlugData"
as follows, in the CpuHotplugEntry() function:
17cb8ddba39b5 329) //
17cb8ddba39b5 330) // Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm
17cb8ddba39b5 331) // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM.
17cb8ddba39b5 332) //
17cb8ddba39b5 333) mCpuHotPlugData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotPlugDataAddress);
The same pattern can be repeated for the new data structure that you
(might) need:
(a) Extend the SmmCpuFeaturesSmmRelocationComplete() function in
"OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c" with the
following logic -- probably implemented as a new STATIC helper function:
- fetch "PcdCpuMaxLogicalProcessorNumber", so that you know how to size
the "mHotUnplugWork" array
- allocate the "mHotUnplugWork" array in SMRAM. For the allocation, you
can simply use the AllocatePool() function from MemoryAllocationLib, as
in this SMM library instance, it will allocate SMRAM.
- if the allocation fails, hang hard (there's really nothing else to do)
- publish the array's address via a new UINT64 PCD -- introduce a new
PCD in "OvmfPkg.dec", as dynamic-only, set a default 0 value in the DSC
files, and then use a PcdSet64S() call in the code.
(b) The same DEPEX guarantee continues working in the CpuHotplugSmm
driver as before. Thus, in the CpuHotplugEntry() function, you can
locate "mHotUnplugWork" with another PcdGet64() function call, referring
to the new PCD.
(c) In the SmmCpuFeaturesRendezvousExit() function -- which is part of
the same library instance as SmmCpuFeaturesSmmRelocationComplete() --,
you can simply refer to "mHotUnplugWork" by name.
In summary, the ownership of of "mHotUnplugWork" moves from
CpuHotplugSmm to PiSmmCpuSmmDxe. PiSmmCpuSmmDxe itself does not know
about this, because all the new logic is hooked into it through existent
hook functions in "OvmfPkg/Library/SmmCpuFeaturesLib". And CpuHotplugSmm
retains access to "mHotUnplugWork" by reusing the dynamic PCD pattern
that's already employed for "mCpuHotPlugData".
(Note that the PCD itself exists in normal RAM, but this is safe,
because the transfer of the "mHotUnplugWork" address through the PCD
occurs way before such code could execute that is not part of the
platform firmware.)
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [edk2-devel] [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface()
2021-01-07 21:45 ` Laszlo Ersek
@ 2021-01-07 23:42 ` Ankur Arora
0 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 23:42 UTC (permalink / raw)
To: devel, lersek
Cc: imammedo, boris.ostrovsky, Jian J Wang, Michael D Kinney,
Liming Gao, Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar,
Aaron Young
On 2021-01-07 1:45 p.m., Laszlo Ersek wrote:
> On 01/07/21 21:48, Laszlo Ersek wrote:
>
>> You should basically migrate the contents of CpuUnplugExitWork() (in
>> patch#8) into SmmCpuFeaturesRendezvousExit(), without changing the
>> prototype of SmmCpuFeaturesRendezvousExit(). This might require you to
>> calculate "IsBSP" on the spot, possibly with the help of some global
>> data that you set up in advance.
>
> [...]
>
>> By adding the business logic to SmmCpuFeaturesRendezvousExit() instead
>> of CpuUnplugExitWork, said logic will actually be included in the
>> PiSmmCpuDxeSmm binary -- but that's fine. That's why the
>> SmmCpuFeaturesRendezvousExit() API exists, as a platform customization
>> hook for PiSmmCpuDxeSmm.
>
> If you need PiSmmCpuDxeSmm and CpuHotplugSmm to collaborate on a
> dynamically sized array, such as "mHotUnplugWork", here's a possible
> approach.
>
> Consider how the somewhat similar "mCpuHotPlugData" object is allocated,
> and then shared between both drivers, in SMRAM.
>
> Line numbers at commit 85b8eac59b8c.
>
> (1) in the PiCpuSmmEntry() function, PiSmmCpuDxeSmm first sets
> "mMaxNumberOfCpus" to the value of "PcdCpuMaxLogicalProcessorNumber".
> (PcdCpuHotPlugSupport is TRUE in our case.) Line 624.
>
> (2) "mCpuHotPlugData.ArrayLength" is set to "mMaxNumberOfCpus". Line
> 824.
>
> (3) SmmCpuFeaturesSmmRelocationComplete() is called
> [OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c]. Line 930.
>
> (4) The address of "mCpuHotPlugData" is published through
> "PcdCpuHotPlugDataAddress", on line 1021. (Again, PcdCpuHotPlugSupport
> is TRUE.)
Aah, yeah. Thanks, this is exactly what I needed to do.
>
> (5) "OvmfPkg/CpuHotplugSmm/CpuHotplug.c" then locates "mCpuHotPlugData"
> as follows, in the CpuHotplugEntry() function:
>
> 17cb8ddba39b5 329) //
> 17cb8ddba39b5 330) // Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm
> 17cb8ddba39b5 331) // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM.
> 17cb8ddba39b5 332) //
> 17cb8ddba39b5 333) mCpuHotPlugData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotPlugDataAddress);
>
>
> The same pattern can be repeated for the new data structure that you
> (might) need:
>
> (a) Extend the SmmCpuFeaturesSmmRelocationComplete() function in
> "OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c" with the
> following logic -- probably implemented as a new STATIC helper function:
>
> - fetch "PcdCpuMaxLogicalProcessorNumber", so that you know how to size
> the "mHotUnplugWork" array
>
> - allocate the "mHotUnplugWork" array in SMRAM. For the allocation, you
> can simply use the AllocatePool() function from MemoryAllocationLib, as
> in this SMM library instance, it will allocate SMRAM.
>
> - if the allocation fails, hang hard (there's really nothing else to do)
>
> - publish the array's address via a new UINT64 PCD -- introduce a new
> PCD in "OvmfPkg.dec", as dynamic-only, set a default 0 value in the DSC
> files, and then use a PcdSet64S() call in the code.
>
> (b) The same DEPEX guarantee continues working in the CpuHotplugSmm
> driver as before. Thus, in the CpuHotplugEntry() function, you can
> locate "mHotUnplugWork" with another PcdGet64() function call, referring
> to the new PCD.
>
> (c) In the SmmCpuFeaturesRendezvousExit() function -- which is part of
> the same library instance as SmmCpuFeaturesSmmRelocationComplete() --,
> you can simply refer to "mHotUnplugWork" by name.
>
> In summary, the ownership of of "mHotUnplugWork" moves from
> CpuHotplugSmm to PiSmmCpuSmmDxe. PiSmmCpuSmmDxe itself does not know
> about this, because all the new logic is hooked into it through existent
> hook functions in "OvmfPkg/Library/SmmCpuFeaturesLib". And CpuHotplugSmm
> retains access to "mHotUnplugWork" by reusing the dynamic PCD pattern
> that's already employed for "mCpuHotPlugData".
Right, this makes perfect sense now.
Let me fix up the patches and resend in light of your comments.
Thanks
Ankur
>
> (Note that the PCD itself exists in normal RAM, but this is safe,
> because the transfer of the "mHotUnplugWork" address through the PCD
> occurs way before such code could execute that is not part of the
> platform firmware.)
>
> Thanks,
> Laszlo
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 06/10] UefiCpuPkg/PiSmmCpuDxeSmm: initialize IsBsp
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
` (4 preceding siblings ...)
2021-01-07 19:55 ` [PATCH v2 05/10] MdePkg: add MmRegisterShutdownInterface() Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 19:55 ` [PATCH v2 07/10] UefiCpuPkg/SmmCpuFeaturesLib: add IsBsp as a param to SmmCpuFeaturesRendezvousExit() Ankur Arora
` (3 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Eric Dong, Ray Ni,
Rahul Kumar, Aaron Young
Initialize IsBsp early to ensure that this variable reflects the BSP
status correctly.
Also replace this check:
if (mSmmMpSyncData->BspIndex == (UINT32)CpuIndex)
with:
if (IsBsp)
Note that there's a window of time when these two diverge, when the BSP,
at exit from BSPHandler(), resets mSmmMpSyncData->BspIndex.
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: 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>
---
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 4bcd217917d7..e7ea44eb86fc 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1565,7 +1565,12 @@ SmiRendezvous (
{
EFI_STATUS Status;
BOOLEAN ValidSmi;
- BOOLEAN IsBsp;
+
+ //
+ // IsBsp starts out as false. Once a CPU gets elected as BSP,
+ // it will transition its value to true.
+ //
+ BOOLEAN IsBsp = FALSE;
BOOLEAN BspInProgress;
UINTN Index;
UINTN Cr2;
@@ -1656,7 +1661,6 @@ SmiRendezvous (
//
// Elect BSP
//
- IsBsp = FALSE;
if (FeaturePcdGet (PcdCpuSmmEnableBspElection)) {
if (!mSmmMpSyncData->SwitchBsp || mSmmMpSyncData->CandidateBsp[CpuIndex]) {
//
@@ -1679,6 +1683,8 @@ SmiRendezvous (
(UINT32)-1,
(UINT32)CpuIndex
);
+
+ IsBsp = mSmmMpSyncData->BspIndex == (UINT32)CpuIndex;
}
}
}
@@ -1686,7 +1692,7 @@ SmiRendezvous (
//
// "mSmmMpSyncData->BspIndex == CpuIndex" means this is the BSP
//
- if (mSmmMpSyncData->BspIndex == CpuIndex) {
+ if (IsBsp) {
//
// Clear last request for SwitchBsp.
--
2.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 07/10] UefiCpuPkg/SmmCpuFeaturesLib: add IsBsp as a param to SmmCpuFeaturesRendezvousExit()
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
` (5 preceding siblings ...)
2021-01-07 19:55 ` [PATCH v2 06/10] UefiCpuPkg/PiSmmCpuDxeSmm: initialize IsBsp Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 19:55 ` [PATCH v2 08/10] OvmfCpuPkg/CpuHotplug: add a hot-unplug handler called at SMI exit Ankur Arora
` (2 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Eric Dong, Ray Ni, Rahul Kumar, Aaron Young
Add IsBsp as a parameter to SmmCpuFeaturesRendezvousExit(). This'll
be used to disambiguate the BSP so it can eject hot-unplugged CPUs.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.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/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 6 +++++-
UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h | 4 +++-
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 4 +++-
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 2 +-
4 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 7ef7ed98342e..4d78bbfe7634 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -368,13 +368,17 @@ SmmCpuFeaturesRendezvousEntry (
@param[in] CpuIndex The index of the CPU that is exiting SMM. The value
must be between 0 and the NumberOfCpus field in the
System Management System Table (SMST).
+ @param[in] IsBSP Is this CPU the SMM BSP?
**/
VOID
EFIAPI
SmmCpuFeaturesRendezvousExit (
- IN UINTN CpuIndex
+ IN UINTN CpuIndex,
+ IN BOOLEAN IsBSP
)
{
+ if(gSmst->SmmShutdownAp)
+ gSmst->SmmShutdownAp(CpuIndex, IsBSP);
}
/**
diff --git a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
index dbcd57e0ad42..5e8c4000f010 100644
--- a/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
+++ b/UefiCpuPkg/Include/Library/SmmCpuFeaturesLib.h
@@ -259,11 +259,13 @@ SmmCpuFeaturesRendezvousEntry (
@param[in] CpuIndex The index of the CPU that is exiting SMM. The value must
be between 0 and the NumberOfCpus field in the System
Management System Table (SMST).
+ @param[in] IsBSP Is this CPU the SMM BSP?
**/
VOID
EFIAPI
SmmCpuFeaturesRendezvousExit (
- IN UINTN CpuIndex
+ IN UINTN CpuIndex,
+ IN BOOLEAN IsBSP
);
/**
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 8fed18cf0e17..325518d3de3e 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -443,11 +443,13 @@ SmmCpuFeaturesRendezvousEntry (
@param[in] CpuIndex The index of the CPU that is exiting SMM. The value must
be between 0 and the NumberOfCpus field in the System
Management System Table (SMST).
+ @param[in] IsBSP Is this CPU the SMM BSP?
**/
VOID
EFIAPI
SmmCpuFeaturesRendezvousExit (
- IN UINTN CpuIndex
+ IN UINTN CpuIndex,
+ IN BOOLEAN IsBSP
)
{
}
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index e7ea44eb86fc..fb6aab17de37 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1728,7 +1728,7 @@ SmiRendezvous (
}
Exit:
- SmmCpuFeaturesRendezvousExit (CpuIndex);
+ SmmCpuFeaturesRendezvousExit (CpuIndex, IsBsp);
//
// Restore Cr2
--
2.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 08/10] OvmfCpuPkg/CpuHotplug: add a hot-unplug handler called at SMI exit
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
` (6 preceding siblings ...)
2021-01-07 19:55 ` [PATCH v2 07/10] UefiCpuPkg/SmmCpuFeaturesLib: add IsBsp as a param to SmmCpuFeaturesRendezvousExit() Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 19:55 ` [PATCH v2 09/10] OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOT_UNPLUG Ankur Arora
2021-01-07 19:55 ` [PATCH v2 10/10] MdePkg: use CpuPause() in CpuDeadLoop() Ankur Arora
9 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Eric Dong, Ray Ni, Rahul Kumar, Aaron Young
Add CpuUnplugExitWork(), to be called from SmmCpuFeaturesRendezvousExit()
to do the final ejection as part of CPU hot-unplug.
On the BSP, CpuUnplugExitWork() 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 and
with its QEMU state destroyed.
On the AP, CpuUnplugExitWork() provides a holding area where the CPU
spins until context switched out by QEMU via the BSP. Given that
the context switch would end up with the AP state being cleaned up,
this means that the AP CPU 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: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.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 | 68 +++++++++++++++++++++++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 6 ++++
2 files changed, 74 insertions(+)
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 20d92a35da39..379c9a66f261 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -421,6 +421,69 @@ Fatal:
return EFI_INTERRUPT_PENDING;
}
+EFI_STATUS
+EFIAPI
+CpuUnplugExitWork(
+ IN UINTN CpuIndex,
+ IN BOOLEAN IsBSP
+ )
+{
+ APIC_ID RemoveApicId;
+
+ RemoveApicId = mHotUnplugWork[CpuIndex];
+
+ if (!IsBSP && RemoveApicId == MAX_UINT32) {
+ return EFI_SUCCESS;
+ }
+
+ if (IsBSP) {
+ UINT32 Idx;
+ for (Idx = 0; Idx < mCpuHotPlugData->ArrayLength; Idx++) {
+ RemoveApicId = mHotUnplugWork[Idx];
+
+ if (RemoveApicId != MAX_UINT32) {
+ //
+ // The CPU(s) to be unplugged have received the BSP's signal to exit the
+ // SMI and either will execute SmmCpuFeaturesSmiRendezvousExit() and this
+ // callback or are waiting here.
+ //
+ // Tell HW to put it out of its misery.
+ //
+ QemuCpuhpWriteCpuSelector (mMmCpuIo, RemoveApicId);
+ QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECTED);
+
+ //
+ // Barrier to ensure that the compiler doesn't reorder the next store
+ //
+ MemoryFence();
+
+ //
+ // Clear the unplug status to make sure that an invalid SMI later
+ // does not try to do an unplug or go to the dead loop.
+ //
+ mHotUnplugWork[Idx] = MAX_UINT32;
+
+ DEBUG ((DEBUG_INFO, "%a: Unplugged CPU " FMT_APIC_ID "\n",
+ __FUNCTION__, RemoveApicId));
+ }
+ }
+ return EFI_SUCCESS;
+ }
+
+ //
+ // 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 ();
+
+ return EFI_ABORTED;
+}
+
//
// Entry point function of this driver.
@@ -573,6 +636,11 @@ CpuHotplugEntry (
}
//
+ // Register handler for hot-unplugging an AP.
+ //
+ MmRegisterShutdownInterface(CpuUnplugExitWork);
+
+ //
// Register the handler for the CPU Hotplug MMI.
//
Status = gMmst->MmiHandlerRegister (
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index fb6aab17de37..f246d730d1e2 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1727,6 +1727,12 @@ SmiRendezvous (
}
}
+ //
+ // Note that the BSP will unplug any CPUs that have been marked for
+ // hot-unplug at any point after it sets AllCpusInSync = FALSE
+ // so it cannot depend on an AP executing code post that point.
+ //
+
Exit:
SmmCpuFeaturesRendezvousExit (CpuIndex, IsBsp);
--
2.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 09/10] OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOT_UNPLUG
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
` (7 preceding siblings ...)
2021-01-07 19:55 ` [PATCH v2 08/10] OvmfCpuPkg/CpuHotplug: add a hot-unplug handler called at SMI exit Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 19:55 ` [PATCH v2 10/10] MdePkg: use CpuPause() in CpuDeadLoop() Ankur Arora
9 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Treat ICH9_LPC_SMI_F_CPU_HOT_UNPLUG as a subfeature of the 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/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 379c9a66f261..1f9fd31dfe43 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -391,11 +391,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.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 10/10] MdePkg: use CpuPause() in CpuDeadLoop()
2021-01-07 19:55 [PATCH v2 00/10] support CPU hot-unplug Ankur Arora
` (8 preceding siblings ...)
2021-01-07 19:55 ` [PATCH v2 09/10] OvmfPkg/SmmControl2Dxe: negotiate ICH9_LPC_SMI_F_CPU_HOT_UNPLUG Ankur Arora
@ 2021-01-07 19:55 ` Ankur Arora
2021-01-07 22:30 ` [edk2-devel] " Michael D Kinney
9 siblings, 1 reply; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 19:55 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Michael D Kinney,
Liming Gao, Zhiguang Liu, Aaron Young
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>
---
MdePkg/Library/BaseLib/CpuDeadLoop.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/BaseLib/CpuDeadLoop.c b/MdePkg/Library/BaseLib/CpuDeadLoop.c
index 9e110cacbc96..e76312de7b4e 100644
--- a/MdePkg/Library/BaseLib/CpuDeadLoop.c
+++ b/MdePkg/Library/BaseLib/CpuDeadLoop.c
@@ -28,5 +28,6 @@ CpuDeadLoop (
{
volatile UINTN Index;
- for (Index = 0; Index == 0;);
+ for (Index = 0; Index == 0;)
+ CpuPause();
}
--
2.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v2 10/10] MdePkg: use CpuPause() in CpuDeadLoop()
2021-01-07 19:55 ` [PATCH v2 10/10] MdePkg: use CpuPause() in CpuDeadLoop() Ankur Arora
@ 2021-01-07 22:30 ` Michael D Kinney
2021-01-07 23:43 ` Ankur Arora
0 siblings, 1 reply; 19+ messages in thread
From: Michael D Kinney @ 2021-01-07 22:30 UTC (permalink / raw)
To: devel@edk2.groups.io, ankur.a.arora@oracle.com, Kinney, Michael D
Cc: lersek@redhat.com, imammedo@redhat.com,
boris.ostrovsky@oracle.com, Liming Gao, Liu, Zhiguang,
Aaron Young
Looks like a good improvement. One minor comment is that the loop
should have {} to follow coding style:
for (Index = 0; Index == 0;) {
CpuPause();
}
With that change:
Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ankur Arora
> Sent: Thursday, January 7, 2021 11:55 AM
> To: devel@edk2.groups.io
> Cc: lersek@redhat.com; imammedo@redhat.com; boris.ostrovsky@oracle.com; Ankur Arora <ankur.a.arora@oracle.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Aaron Young <aaron.young@oracle.com>
> Subject: [edk2-devel] [PATCH v2 10/10] MdePkg: use CpuPause() in CpuDeadLoop()
>
> 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>
> ---
> MdePkg/Library/BaseLib/CpuDeadLoop.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/MdePkg/Library/BaseLib/CpuDeadLoop.c b/MdePkg/Library/BaseLib/CpuDeadLoop.c
> index 9e110cacbc96..e76312de7b4e 100644
> --- a/MdePkg/Library/BaseLib/CpuDeadLoop.c
> +++ b/MdePkg/Library/BaseLib/CpuDeadLoop.c
> @@ -28,5 +28,6 @@ CpuDeadLoop (
> {
>
> volatile UINTN Index;
>
>
>
> - for (Index = 0; Index == 0;);
>
> + for (Index = 0; Index == 0;)
>
> + CpuPause();>
> }
>
> --
> 2.9.3
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#69980): https://edk2.groups.io/g/devel/message/69980
> Mute This Topic: https://groups.io/mt/79507581/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v2 10/10] MdePkg: use CpuPause() in CpuDeadLoop()
2021-01-07 22:30 ` [edk2-devel] " Michael D Kinney
@ 2021-01-07 23:43 ` Ankur Arora
0 siblings, 0 replies; 19+ messages in thread
From: Ankur Arora @ 2021-01-07 23:43 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: lersek@redhat.com, imammedo@redhat.com,
boris.ostrovsky@oracle.com, Liming Gao, Liu, Zhiguang,
Aaron Young
On 2021-01-07 2:30 p.m., Kinney, Michael D wrote:
> Looks like a good improvement. One minor comment is that the loop
> should have {} to follow coding style:
>
> for (Index = 0; Index == 0;) {
> CpuPause();
> }
>
> With that change:
>
> Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Thanks. Will fix up the coding style and resend with the rest of the series.
Ankur
>
> Mike
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ankur Arora
>> Sent: Thursday, January 7, 2021 11:55 AM
>> To: devel@edk2.groups.io
>> Cc: lersek@redhat.com; imammedo@redhat.com; boris.ostrovsky@oracle.com; Ankur Arora <ankur.a.arora@oracle.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>;
>> Aaron Young <aaron.young@oracle.com>
>> Subject: [edk2-devel] [PATCH v2 10/10] MdePkg: use CpuPause() in CpuDeadLoop()
>>
>> 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>
>> ---
>> MdePkg/Library/BaseLib/CpuDeadLoop.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdePkg/Library/BaseLib/CpuDeadLoop.c b/MdePkg/Library/BaseLib/CpuDeadLoop.c
>> index 9e110cacbc96..e76312de7b4e 100644
>> --- a/MdePkg/Library/BaseLib/CpuDeadLoop.c
>> +++ b/MdePkg/Library/BaseLib/CpuDeadLoop.c
>> @@ -28,5 +28,6 @@ CpuDeadLoop (
>> {
>>
>> volatile UINTN Index;
>>
>>
>>
>> - for (Index = 0; Index == 0;);
>>
>> + for (Index = 0; Index == 0;)
>>
>> + CpuPause();>
>> }
>>
>> --
>> 2.9.3
>>
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>> View/Reply Online (#69980): https://edk2.groups.io/g/devel/message/69980
>> Mute This Topic: https://groups.io/mt/79507581/1643496
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
>> -=-=-=-=-=-=
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread