* [PATCH v9 00/10] support CPU hot-unplug
@ 2021-03-12 6:26 Ankur Arora
2021-03-12 6:26 ` [PATCH v9 01/10] OvmfPkg/CpuHotplugSmm: refactor hotplug logic Ankur Arora
` (10 more replies)
0 siblings, 11 replies; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 UTC (permalink / raw)
To: devel; +Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora
Hi,
This series adds OVMF support for CPU hot-unplug.
QEMU secureboot hot-unplug logic corresponding to this is in upstream.
Also posted here:
https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
Testing (with QEMU 5.2.50):
- Stable with randomized CPU plug/unplug (guest maxcpus=33,128)
- Synthetic tests with simultaneous multi CPU hot-unplug
Also at:
github.com/terminus/edk2/ hot-unplug-v9
Changelog:
v9:
- Rebased on top of edd46cd407ea
- Clarify comments around memory-barriers in patches 7, 8, 9
- Address other review comments from v8
v8:
- Fixes a couple of ECC issues in the code (in patches 7, 9)
URL: https://patchew.org/EDK2/20210222071928.1401820-1-ankur.a.arora@oracle.com/
v7:
- Address review comments from v6.
- Fix ejection bug where we were using APIC ID to do the ejection
rather than the Qemu Selector.
- Describes safety properties and ordering needed for concurrent
accesses to CPU_HOT_EJECT_DATA->QemuSelectorMap, and
CPU_HOT_EJECT_DATA->Handler.
URL: https://patchew.org/EDK2/20210219090444.1332380-1-ankur.a.arora@oracle.com/
v6:
- addresses v5 review comments.
URL: https://patchew.org/EDK2/20210129005950.467638-1-ankur.a.arora@oracle.com/
v5:
- fixes ECC errors (all but one in "OvmfPkg/CpuHotplugSmm: add
add Qemu Cpu Status helper").
URL: https://patchew.org/EDK2/20210126064440.299596-1-ankur.a.arora@oracle.com/
v4:
- Gets rid of unnecessary UefiCpuPkg changes
URL: https://patchew.org/EDK2/20210118063457.358581-1-ankur.a.arora@oracle.com/
v3:
- Use a saner PCD based interface to share state between PiSmmCpuDxeSmm
and OvmfPkg/CpuHotplugSmm
- Cleaner split of the hot-unplug code
URL: https://patchew.org/EDK2/20210115074533.277448-1-ankur.a.arora@oracle.com/
v2:
- Do the ejection via SmmCpuFeaturesRendezvousExit()
URL: https://patchew.org/EDK2/20210107195515.106158-1-ankur.a.arora@oracle.com/
RFC:
URL: https://patchew.org/EDK2/20201208053432.2690694-1-ankur.a.arora@oracle.com/
Please review.
Thanks
Ankur
Ankur Arora (10):
OvmfPkg/CpuHotplugSmm: refactor hotplug logic
OvmfPkg/CpuHotplugSmm: collect hot-unplug events
OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
OvmfPkg: define CPU_HOT_EJECT_DATA
OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler
OvmfPkg/CpuHotplugSmm: add EjectCpu()
OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject
OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
OvmfPkg/OvmfPkg.dec | 4 +
OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 2 +
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
OvmfPkg/CpuHotplugSmm/QemuCpuhp.h | 7 +
OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 2 +
OvmfPkg/Include/Pcd/CpuHotEjectData.h | 60 +++
OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 576 +++++++++++++++++----
OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 123 +++--
.../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 111 ++++
OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 18 +-
10 files changed, 775 insertions(+), 132 deletions(-)
create mode 100644 OvmfPkg/Include/Pcd/CpuHotEjectData.h
--
2.9.3
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v9 01/10] OvmfPkg/CpuHotplugSmm: refactor hotplug logic
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-12 6:26 ` [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events Ankur Arora
` (9 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 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
ProcessHotAddedCpus(). 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 210 ++++++++++++++++++++++---------------
1 file changed, 126 insertions(+), 84 deletions(-)
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index cfe698ed2b5e..bf68fcd42914 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -62,6 +62,129 @@ STATIC UINT32 mPostSmmPenAddress;
//
STATIC EFI_HANDLE mDispatchHandle;
+/**
+ Process CPUs that have been hot-added, per QemuCpuhpCollectApicIds().
+
+ For each such CPU, relocate the SMBASE, and report the CPU to PiSmmCpuDxeSmm
+ via EFI_SMM_CPU_SERVICE_PROTOCOL. If the supposedly hot-added CPU is already
+ known, skip it silently.
+
+ @param[in] PluggedApicIds The APIC IDs of the CPUs that have been
+ hot-plugged.
+
+ @param[in] PluggedCount The number of filled-in APIC IDs in
+ PluggedApicIds.
+
+ @retval EFI_SUCCESS CPUs corresponding to all the APIC IDs are
+ populated.
+
+ @retval EFI_OUT_OF_RESOURCES Out of APIC ID space in "mCpuHotPlugData".
+
+ @return Error codes propagated from SmbaseRelocate()
+ and mMmCpuService->AddProcessor().
+**/
+STATIC
+EFI_STATUS
+ProcessHotAddedCpus (
+ IN APIC_ID *PluggedApicIds,
+ IN UINT32 PluggedCount
+ )
+{
+ EFI_STATUS Status;
+ UINT32 PluggedIdx;
+ UINT32 NewSlot;
+
+ //
+ // The Post-SMM Pen need not be reinstalled multiple times within a single
+ // root MMI handling. Even reinstalling once per root MMI is only prudence;
+ // in theory installing the pen in the driver's entry point function should
+ // suffice.
+ //
+ SmbaseReinstallPostSmmPen (mPostSmmPenAddress);
+
+ PluggedIdx = 0;
+ NewSlot = 0;
+ while (PluggedIdx < PluggedCount) {
+ APIC_ID NewApicId;
+ UINT32 CheckSlot;
+ UINTN NewProcessorNumberByProtocol;
+
+ NewApicId = PluggedApicIds[PluggedIdx];
+
+ //
+ // Check if the supposedly hot-added CPU is already known to us.
+ //
+ for (CheckSlot = 0;
+ CheckSlot < mCpuHotPlugData->ArrayLength;
+ CheckSlot++) {
+ if (mCpuHotPlugData->ApicId[CheckSlot] == NewApicId) {
+ break;
+ }
+ }
+ if (CheckSlot < mCpuHotPlugData->ArrayLength) {
+ DEBUG ((DEBUG_VERBOSE, "%a: APIC ID " FMT_APIC_ID " was hot-plugged "
+ "before; ignoring it\n", __FUNCTION__, NewApicId));
+ PluggedIdx++;
+ continue;
+ }
+
+ //
+ // Find the first empty slot in CPU_HOT_PLUG_DATA.
+ //
+ while (NewSlot < mCpuHotPlugData->ArrayLength &&
+ mCpuHotPlugData->ApicId[NewSlot] != MAX_UINT64) {
+ NewSlot++;
+ }
+ if (NewSlot == mCpuHotPlugData->ArrayLength) {
+ DEBUG ((DEBUG_ERROR, "%a: no room for APIC ID " FMT_APIC_ID "\n",
+ __FUNCTION__, NewApicId));
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ //
+ // Store the APIC ID of the new processor to the slot.
+ //
+ mCpuHotPlugData->ApicId[NewSlot] = NewApicId;
+
+ //
+ // Relocate the SMBASE of the new CPU.
+ //
+ Status = SmbaseRelocate (NewApicId, mCpuHotPlugData->SmBase[NewSlot],
+ mPostSmmPenAddress);
+ if (EFI_ERROR (Status)) {
+ goto RevokeNewSlot;
+ }
+
+ //
+ // Add the new CPU with EFI_SMM_CPU_SERVICE_PROTOCOL.
+ //
+ Status = mMmCpuService->AddProcessor (mMmCpuService, NewApicId,
+ &NewProcessorNumberByProtocol);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: AddProcessor(" FMT_APIC_ID "): %r\n",
+ __FUNCTION__, NewApicId, Status));
+ goto RevokeNewSlot;
+ }
+
+ DEBUG ((DEBUG_INFO, "%a: hot-added APIC ID " FMT_APIC_ID ", SMBASE 0x%Lx, "
+ "EFI_SMM_CPU_SERVICE_PROTOCOL assigned number %Lu\n", __FUNCTION__,
+ NewApicId, (UINT64)mCpuHotPlugData->SmBase[NewSlot],
+ (UINT64)NewProcessorNumberByProtocol));
+
+ NewSlot++;
+ PluggedIdx++;
+ }
+
+ //
+ // We've processed this batch of hot-added CPUs.
+ //
+ return EFI_SUCCESS;
+
+RevokeNewSlot:
+ mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
+
+ return Status;
+}
/**
CPU Hotplug MMI handler function.
@@ -122,8 +245,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 +300,11 @@ 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);
-
- 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));
+ if (PluggedCount > 0) {
+ Status = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
+ if (EFI_ERROR (Status)) {
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++;
}
//
@@ -267,9 +312,6 @@ CpuHotplugMmi (
//
return EFI_SUCCESS;
-RevokeNewSlot:
- mCpuHotPlugData->ApicId[NewSlot] = MAX_UINT64;
-
Fatal:
ASSERT (FALSE);
CpuDeadLoop ();
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
2021-03-12 6:26 ` [PATCH v9 01/10] OvmfPkg/CpuHotplugSmm: refactor hotplug logic Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-16 9:56 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 03/10] OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper Ankur Arora
` (8 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 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 APIC IDs
and QEMU CPU Selectors for CPUs being hot-unplugged.
In addition, we now ignore CPUs which only have remove set. These
CPUs haven't been processed by OSPM yet.
This is based on the QEMU hot-unplug protocol documented here:
https://lore.kernel.org/qemu-devel/20201204170939.1815522-3-imammedo@redhat.com/
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
Notes:
Addresses the following comments from v8:
(1) Fix commit message to mention that we collect cpu-selectors as well.
(2,3,6) s/UnplugSelector/UnplugSelectors/ in CpuHotplug.c, QemuCpuhp.c
(4) Fix comment above the declaration of the now renamed mToUnplugSelector.
(5) Fix spacing around "||".
(7) Fix QemuCpuCollectApicIds() comments to line up descriptions for
ToUnplugSelectors and other params.
(8) s/ExtendSel/ExtendSels/.
(9) Add the (ExtendSels => ExtendIds) assert.
(10) Fix the missing CurrentSelector++ bug.
OvmfPkg/CpuHotplugSmm/QemuCpuhp.h | 1 +
OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 1 +
OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 29 +++++--
OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 101 +++++++++++++++-------
4 files changed, 93 insertions(+), 39 deletions(-)
diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
index 8adaa0ad91f0..3e2c2192e1c0 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
@@ -55,6 +55,7 @@ QemuCpuhpCollectApicIds (
OUT APIC_ID *PluggedApicIds,
OUT UINT32 *PluggedCount,
OUT APIC_ID *ToUnplugApicIds,
+ OUT UINT32 *ToUnplugSelectors,
OUT UINT32 *ToUnplugCount
);
diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
index a34a6d3fae61..2ec7a107a64d 100644
--- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
+++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
@@ -34,6 +34,7 @@
#define QEMU_CPUHP_STAT_ENABLED BIT0
#define QEMU_CPUHP_STAT_INSERT BIT1
#define QEMU_CPUHP_STAT_REMOVE BIT2
+#define QEMU_CPUHP_STAT_FW_REMOVE BIT4
#define QEMU_CPUHP_RW_CMD_DATA 0x8
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index bf68fcd42914..ee1497b93140 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -45,13 +45,16 @@ STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
// don't want to allocate SMRAM at OS runtime, and potentially fail (or
// fragment the SMRAM map).
//
-// These arrays provide room for ("possible CPU count" minus one) APIC IDs
-// each, as we don't expect every possible CPU to appear, or disappear, in a
-// single MMI. The numbers of used (populated) elements in the arrays are
+// The first array stores APIC IDs for hot-plug events, the second and the
+// third store APIC IDs and QEMU CPU Selectors (both indexed similarly) for
+// hot-unplug events. All of these provide room for "possible CPU count" minus
+// one elements as we don't expect every possible CPU to appear, or disappear,
+// in a single MMI. The numbers of used (populated) elements in the arrays are
// determined on every MMI separately.
//
STATIC APIC_ID *mPluggedApicIds;
STATIC APIC_ID *mToUnplugApicIds;
+STATIC UINT32 *mToUnplugSelectors;
//
// Address of the non-SMRAM reserved memory page that contains the Post-SMM Pen
// for hot-added CPUs.
@@ -289,6 +292,7 @@ CpuHotplugMmi (
mPluggedApicIds,
&PluggedCount,
mToUnplugApicIds,
+ mToUnplugSelectors,
&ToUnplugCount
);
if (EFI_ERROR (Status)) {
@@ -333,7 +337,9 @@ CpuHotplugEntry (
)
{
EFI_STATUS Status;
+ UINTN Len;
UINTN Size;
+ UINTN SizeSel;
//
// This module should only be included when SMM support is required.
@@ -387,8 +393,9 @@ CpuHotplugEntry (
//
// Allocate the data structures that depend on the possible CPU count.
//
- if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Size)) ||
- RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Size, &Size))) {
+ if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Len)) ||
+ RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Len, &Size)) ||
+ RETURN_ERROR (SafeUintnMult (sizeof (UINT32), Len, &SizeSel))) {
Status = EFI_ABORTED;
DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_PLUG_DATA\n", __FUNCTION__));
goto Fatal;
@@ -405,6 +412,12 @@ CpuHotplugEntry (
DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status));
goto ReleasePluggedApicIds;
}
+ Status = gMmst->MmAllocatePool (EfiRuntimeServicesData, SizeSel,
+ (VOID **)&mToUnplugSelectors);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status));
+ goto ReleaseToUnplugApicIds;
+ }
//
// Allocate the Post-SMM Pen for hot-added CPUs.
@@ -412,7 +425,7 @@ CpuHotplugEntry (
Status = SmbaseAllocatePostSmmPen (&mPostSmmPenAddress,
SystemTable->BootServices);
if (EFI_ERROR (Status)) {
- goto ReleaseToUnplugApicIds;
+ goto ReleaseToUnplugSelectors;
}
//
@@ -472,6 +485,10 @@ ReleasePostSmmPen:
SmbaseReleasePostSmmPen (mPostSmmPenAddress, SystemTable->BootServices);
mPostSmmPenAddress = 0;
+ReleaseToUnplugSelectors:
+ gMmst->MmFreePool (mToUnplugSelectors);
+ mToUnplugSelectors = NULL;
+
ReleaseToUnplugApicIds:
gMmst->MmFreePool (mToUnplugApicIds);
mToUnplugApicIds = NULL;
diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
index 8d4a6693c8d6..8434dd446b96 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
@@ -145,27 +145,30 @@ QemuCpuhpWriteCommand (
On error, the contents of the output parameters are undefined.
- @param[in] MmCpuIo The EFI_MM_CPU_IO_PROTOCOL instance for
- accessing IO Ports.
+ @param[in] MmCpuIo The EFI_MM_CPU_IO_PROTOCOL instance for
+ accessing IO Ports.
- @param[in] PossibleCpuCount The number of possible CPUs in the system. Must
- be positive.
+ @param[in] PossibleCpuCount The number of possible CPUs in the system. Must
+ be positive.
- @param[in] ApicIdCount The number of elements each one of the
- PluggedApicIds and ToUnplugApicIds arrays can
- accommodate. Must be positive.
+ @param[in] ApicIdCount The number of elements each one of the
+ PluggedApicIds and ToUnplugApicIds arrays can
+ accommodate. Must be positive.
- @param[out] PluggedApicIds The APIC IDs of the CPUs that have been
- hot-plugged.
+ @param[out] PluggedApicIds The APIC IDs of the CPUs that have been
+ hot-plugged.
- @param[out] PluggedCount The number of filled-in APIC IDs in
- PluggedApicIds.
+ @param[out] PluggedCount The number of filled-in APIC IDs in
+ PluggedApicIds.
- @param[out] ToUnplugApicIds The APIC IDs of the CPUs that are about to be
- hot-unplugged.
+ @param[out] ToUnplugApicIds The APIC IDs of the CPUs that are about to be
+ hot-unplugged.
- @param[out] ToUnplugCount The number of filled-in APIC IDs in
- ToUnplugApicIds.
+ @param[out] ToUnplugSelectors The QEMU Selectors of the CPUs that are about
+ to be hot-unplugged.
+
+ @param[out] ToUnplugCount The number of filled-in APIC IDs in
+ ToUnplugApicIds.
@retval EFI_INVALID_PARAMETER PossibleCpuCount is zero, or ApicIdCount is
zero.
@@ -187,6 +190,7 @@ QemuCpuhpCollectApicIds (
OUT APIC_ID *PluggedApicIds,
OUT UINT32 *PluggedCount,
OUT APIC_ID *ToUnplugApicIds,
+ OUT UINT32 *ToUnplugSelectors,
OUT UINT32 *ToUnplugCount
)
{
@@ -204,6 +208,7 @@ QemuCpuhpCollectApicIds (
UINT32 PendingSelector;
UINT8 CpuStatus;
APIC_ID *ExtendIds;
+ UINT32 *ExtendSels;
UINT32 *ExtendCount;
APIC_ID NewApicId;
@@ -245,10 +250,10 @@ QemuCpuhpCollectApicIds (
if ((CpuStatus & QEMU_CPUHP_STAT_INSERT) != 0) {
//
// The "insert" event guarantees the "enabled" status; plus it excludes
- // the "remove" event.
+ // the "fw_remove" event.
//
if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0 ||
- (CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
+ (CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) {
DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: "
"inconsistent CPU status\n", __FUNCTION__, CurrentSelector,
CpuStatus));
@@ -259,33 +264,63 @@ QemuCpuhpCollectApicIds (
CurrentSelector));
ExtendIds = PluggedApicIds;
+ ExtendSels = NULL;
ExtendCount = PluggedCount;
- } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
- DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", __FUNCTION__,
- CurrentSelector));
+ } else if ((CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) {
+ //
+ // "fw_remove" event guarantees "enabled".
+ //
+ if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0) {
+ DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: "
+ "inconsistent CPU status\n", __FUNCTION__, CurrentSelector,
+ CpuStatus));
+ return EFI_PROTOCOL_ERROR;
+ }
+
+ DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: fw_remove\n",
+ __FUNCTION__, CurrentSelector));
ExtendIds = ToUnplugApicIds;
+ ExtendSels = ToUnplugSelectors;
ExtendCount = ToUnplugCount;
+ } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
+ //
+ // Let the OSPM deal with the "remove" event.
+ //
+ DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove (ignored)\n",
+ __FUNCTION__, CurrentSelector));
+
+ ExtendIds = NULL;
+ ExtendSels = NULL;
+ ExtendCount = NULL;
} else {
DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: no event\n",
__FUNCTION__, CurrentSelector));
break;
}
- //
- // Save the APIC ID of the CPU with the pending event, to the corresponding
- // APIC ID array.
- //
- if (*ExtendCount == ApicIdCount) {
- DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__));
- return EFI_BUFFER_TOO_SMALL;
- }
- QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);
- NewApicId = QemuCpuhpReadCommandData (MmCpuIo);
- DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,
- NewApicId));
- ExtendIds[(*ExtendCount)++] = NewApicId;
+ ASSERT ((ExtendIds == NULL) == (ExtendCount == NULL));
+ ASSERT ((ExtendSels == NULL) || (ExtendIds != NULL));
+ if (ExtendIds != NULL) {
+ //
+ // Save the APIC ID of the CPU with the pending event, to the
+ // corresponding APIC ID array.
+ // For unplug events, also save the CurrentSelector.
+ //
+ if (*ExtendCount == ApicIdCount) {
+ DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__));
+ return EFI_BUFFER_TOO_SMALL;
+ }
+ QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);
+ NewApicId = QemuCpuhpReadCommandData (MmCpuIo);
+ DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,
+ NewApicId));
+ if (ExtendSels != NULL) {
+ ExtendSels[(*ExtendCount)] = CurrentSelector;
+ }
+ ExtendIds[(*ExtendCount)++] = NewApicId;
+ }
//
// We've processed the CPU with (known) pending events, but we must never
// clear events. Therefore we need to advance past this CPU manually;
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v9 03/10] OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
2021-03-12 6:26 ` [PATCH v9 01/10] OvmfPkg/CpuHotplugSmm: refactor hotplug logic Ankur Arora
2021-03-12 6:26 ` [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-12 6:26 ` [PATCH v9 04/10] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus() Ankur Arora
` (7 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 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 a similar fashion as
other helper functions.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/CpuHotplugSmm/QemuCpuhp.h | 6 ++++++
OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 22 ++++++++++++++++++++++
2 files changed, 28 insertions(+)
diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
index 3e2c2192e1c0..8bb3c66e9b44 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
@@ -42,6 +42,12 @@ QemuCpuhpWriteCpuSelector (
);
VOID
+QemuCpuhpWriteCpuStatus (
+ IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
+ IN UINT8 CpuStatus
+ );
+
+VOID
QemuCpuhpWriteCommand (
IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
IN UINT8 Command
diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
index 8434dd446b96..dc86ab96777a 100644
--- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
+++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
@@ -114,6 +114,28 @@ QemuCpuhpWriteCpuSelector (
}
VOID
+QemuCpuhpWriteCpuStatus (
+ IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
+ IN UINT8 CpuStatus
+ )
+{
+ EFI_STATUS Status;
+
+ Status = MmCpuIo->Io.Write (
+ MmCpuIo,
+ MM_IO_UINT8,
+ ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_R_CPU_STAT,
+ 1,
+ &CpuStatus
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %r\n", __FUNCTION__, Status));
+ ASSERT (FALSE);
+ CpuDeadLoop ();
+ }
+}
+
+VOID
QemuCpuhpWriteCommand (
IN CONST EFI_MM_CPU_IO_PROTOCOL *MmCpuIo,
IN UINT8 Command
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v9 04/10] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
` (2 preceding siblings ...)
2021-03-12 6:26 ` [PATCH v9 03/10] OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-12 6:26 ` [PATCH v9 05/10] OvmfPkg: define CPU_HOT_EJECT_DATA Ankur Arora
` (6 subsequent siblings)
10 siblings, 0 replies; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Introduce UnplugCpus() which maps each APIC ID being unplugged
onto the hardware ID of the processor and informs PiSmmCpuDxeSmm
of removal by calling EFI_SMM_CPU_SERVICE_PROTOCOL.RemoveProcessor().
With this change we handle the first phase of unplug where we collect
the CPUs that need to be unplugged and mark them for removal in SMM
data structures.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 84 ++++++++++++++++++++++++++++++++++++++
1 file changed, 84 insertions(+)
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index ee1497b93140..59f000eb7886 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -190,6 +190,83 @@ RevokeNewSlot:
}
/**
+ Process to be hot-unplugged CPUs, per QemuCpuhpCollectApicIds().
+
+ For each such CPU, report the CPU to PiSmmCpuDxeSmm via
+ EFI_SMM_CPU_SERVICE_PROTOCOL. If the to be hot-unplugged CPU is
+ unknown, skip it silently.
+
+ @param[in] ToUnplugApicIds The APIC IDs of the CPUs that are about to be
+ hot-unplugged.
+
+ @param[in] ToUnplugCount The number of filled-in APIC IDs in
+ ToUnplugApicIds.
+
+ @retval EFI_SUCCESS Known APIC IDs have been removed from SMM data
+ structures.
+
+ @return Error codes propagated from
+ mMmCpuService->RemoveProcessor().
+**/
+STATIC
+EFI_STATUS
+UnplugCpus (
+ IN APIC_ID *ToUnplugApicIds,
+ IN UINT32 ToUnplugCount
+ )
+{
+ EFI_STATUS Status;
+ UINT32 ToUnplugIdx;
+ UINTN ProcessorNum;
+
+ ToUnplugIdx = 0;
+ while (ToUnplugIdx < ToUnplugCount) {
+ APIC_ID RemoveApicId;
+
+ RemoveApicId = ToUnplugApicIds[ToUnplugIdx];
+
+ //
+ // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find
+ // the ProcessorNum for the APIC ID to be removed.
+ //
+ for (ProcessorNum = 0;
+ ProcessorNum < mCpuHotPlugData->ArrayLength;
+ ProcessorNum++) {
+ if (mCpuHotPlugData->ApicId[ProcessorNum] == RemoveApicId) {
+ break;
+ }
+ }
+
+ //
+ // Ignore the unplug if APIC ID not found
+ //
+ if (ProcessorNum == mCpuHotPlugData->ArrayLength) {
+ DEBUG ((DEBUG_VERBOSE, "%a: did not find APIC ID " FMT_APIC_ID
+ " to unplug\n", __FUNCTION__, RemoveApicId));
+ ToUnplugIdx++;
+ continue;
+ }
+
+ //
+ // Mark ProcessorNum for removal from SMM data structures
+ //
+ Status = mMmCpuService->RemoveProcessor (mMmCpuService, ProcessorNum);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: RemoveProcessor(" FMT_APIC_ID "): %r\n",
+ __FUNCTION__, RemoveApicId, Status));
+ return Status;
+ }
+
+ ToUnplugIdx++;
+ }
+
+ //
+ // We've removed this set of APIC IDs from SMM data structures.
+ //
+ return EFI_SUCCESS;
+}
+
+/**
CPU Hotplug MMI handler function.
This is a root MMI handler.
@@ -311,6 +388,13 @@ CpuHotplugMmi (
}
}
+ if (ToUnplugCount > 0) {
+ Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
+ if (EFI_ERROR (Status)) {
+ goto Fatal;
+ }
+ }
+
//
// We've handled this MMI.
//
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v9 05/10] OvmfPkg: define CPU_HOT_EJECT_DATA
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
` (3 preceding siblings ...)
2021-03-12 6:26 ` [PATCH v9 04/10] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus() Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-16 10:03 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 06/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state Ankur Arora
` (5 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress, which
will be used to share CPU ejection state between OvmfPkg/CpuHotPlugSmm
and PiSmmCpuDxeSmm.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
Notes:
Addresses the following comments from v8:
(1) Get rid of the unnecessary commit specifier from the subject.
(2) s/MaxNumberOfCpus/PcdCpuMaxLogicalProcessorNumber/
(3) Shifted the comments to be above each structure field.
OvmfPkg/OvmfPkg.dec | 4 +++
OvmfPkg/Include/Pcd/CpuHotEjectData.h | 60 +++++++++++++++++++++++++++++++++++
2 files changed, 64 insertions(+)
create mode 100644 OvmfPkg/Include/Pcd/CpuHotEjectData.h
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4348bb45c64a..9629707020ba 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -352,6 +352,10 @@ [PcdsDynamic, PcdsDynamicEx]
# This PCD is only accessed if PcdSmmSmramRequire is TRUE (see below).
gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE|BOOLEAN|0x34
+ ## This PCD adds a communication channel between OVMF's SmmCpuFeaturesLib
+ # instance in PiSmmCpuDxeSmm, and CpuHotplugSmm.
+ gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46
+
[PcdsFeatureFlag]
gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
diff --git a/OvmfPkg/Include/Pcd/CpuHotEjectData.h b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
new file mode 100644
index 000000000000..06714375526c
--- /dev/null
+++ b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
@@ -0,0 +1,60 @@
+/** @file
+ Definition for the CPU_HOT_EJECT_DATA structure, which shares
+ CPU hot-eject state between OVMF's SmmCpuFeaturesLib instance in
+ PiSmmCpuDxeSmm, and CpuHotplugSmm.
+
+ CPU_HOT_EJECT_DATA is allocated in SMRAM, and pointed-to by
+ PcdCpuHotEjectDataAddress.
+
+ PcdCpuHotEjectDataAddress is valid when SMM_REQUIRE is TRUE
+ and PcdCpuMaxLogicalProcessorNumber > 1.
+
+ Copyright (C) 2021, Oracle Corporation.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef CPU_HOT_EJECT_DATA_H_
+#define CPU_HOT_EJECT_DATA_H_
+
+/**
+ CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
+ on each CPU at exit from SMM.
+
+ @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM,
+ and will be used as an index into
+ CPU_HOT_EJECT_DATA->QemuSelectorMap. It is
+ identical to the processor handle in
+ EFI_SMM_CPU_SERVICE_PROTOCOL.
+**/
+typedef
+VOID
+(EFIAPI *CPU_HOT_EJECT_HANDLER) (
+ IN UINTN ProcessorNum
+ );
+
+//
+// CPU_EJECT_QEMU_SELECTOR_INVALID marks CPUs not being ejected in
+// CPU_HOT_EJECT_DATA->QemuSelectorMap.
+//
+// QEMU CPU Selector is UINT32, so we choose an invalid value larger
+// than that type.
+//
+#define CPU_EJECT_QEMU_SELECTOR_INVALID (MAX_UINT64)
+
+typedef struct {
+ //
+ // Maps ProcessorNum -> QemuSelector for pending hot-ejects
+ //
+ volatile UINT64 *QemuSelectorMap;
+ //
+ // Handler to do the CPU ejection
+ //
+ volatile CPU_HOT_EJECT_HANDLER Handler;
+ //
+ // Entries in the QemuSelectorMap
+ //
+ UINT32 ArrayLength;
+} CPU_HOT_EJECT_DATA;
+
+#endif // CPU_HOT_EJECT_DATA_H_
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v9 06/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
` (4 preceding siblings ...)
2021-03-12 6:26 ` [PATCH v9 05/10] OvmfPkg: define CPU_HOT_EJECT_DATA Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-16 10:12 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler Ankur Arora
` (4 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Init CPU_HOT_EJECT_DATA, which will be used to share CPU ejection
state between SmmCpuFeaturesLib (via PiSmmCpuDxeSmm) and CpuHotPlugSmm.
The init happens via SmmCpuFeaturesSmmRelocationComplete(), and so it
will run as part of the PiSmmCpuDxeSmm entry point function,
PiCpuSmmEntry(). Once inited, CPU_HOT_EJECT_DATA is exposed via
PcdCpuHotEjectDataAddress.
The CPU hot-eject handler (CPU_HOT_EJECT_DATA->Handler) is setup when
there is an ejection request via CpuHotplugSmm.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
Notes:
Addresses the following comments from v8:
(1) Remove line before the "if (MaxNumberofCpus == 1)" check.
(3) Fixup the space around "||".
(2,6) Simplify the three SafeInt multiplication into the ones suggested
by Laszlo.
(4) Get rid of the mixed sizeof(mCpuHotEjectData->QemuSelectorMap[0]) and
sizeof(UINT64) in favour of UINT64 everywhere. I was planning to use
the first, but describing the alignment needed is easier in terms of the
second.
Also, as Laszlo's comments on v8-patch-9 mention, we don't really need
this alignment for correctness reasons. This patch retains it, so we
don't pay access penalty for unaligned access.
(5) Change alignment from UINT64 to UINT64-1.
(7) Use the more idiomatic ALIGN_POINTER instead of ALIGN_VALUE.
(8) RETURN_ERROR -> ASSERT_RETURN_ERROR.
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 ++
.../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 77 ++++++++++++++++++++++
2 files changed, 81 insertions(+)
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 97a10afb6e27..8a426a4c10fb 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -30,9 +30,13 @@ [LibraryClasses]
BaseMemoryLib
DebugLib
MemEncryptSevLib
+ MemoryAllocationLib
PcdLib
+ SafeIntLib
SmmServicesTableLib
UefiBootServicesTableLib
[Pcd]
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
+ gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 7ef7ed98342e..5c025bc717c3 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -11,10 +11,13 @@
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/MemEncryptSevLib.h>
+#include <Library/MemoryAllocationLib.h>
#include <Library/PcdLib.h>
+#include <Library/SafeIntLib.h>
#include <Library/SmmCpuFeaturesLib.h>
#include <Library/SmmServicesTableLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Pcd/CpuHotEjectData.h>
#include <PiSmm.h>
#include <Register/Intel/SmramSaveStateMap.h>
#include <Register/QemuSmramSaveStateMap.h>
@@ -171,6 +174,77 @@ SmmCpuFeaturesHookReturnFromSmm (
return OriginalInstructionPointer;
}
+STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData = NULL;
+
+/**
+ Initialize mCpuHotEjectData if PcdCpuMaxLogicalProcessorNumber > 1.
+
+ Also setup the corresponding PcdCpuHotEjectDataAddress.
+**/
+STATIC
+VOID
+InitCpuHotEjectData (
+ VOID
+ )
+{
+ UINTN Size;
+ UINT32 Idx;
+ UINT32 MaxNumberOfCpus;
+ RETURN_STATUS PcdStatus;
+
+ MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+ if (MaxNumberOfCpus == 1) {
+ return;
+ }
+
+ //
+ // We allocate CPU_HOT_EJECT_DATA and CPU_HOT_EJECT_DATA->QemuSelectorMap[]
+ // in a single allocation, and explicitly align the QemuSelectorMap[] (which
+ // is a UINT64 array) at its natural boundary.
+ // Accordingly, allocate:
+ // sizeof(*mCpuHotEjectData) + (MaxNumberOfCpus * sizeof(UINT64))
+ // and, add sizeof(UINT64) - 1 to use as padding if needed.
+ //
+
+ if (RETURN_ERROR (SafeUintnMult (MaxNumberOfCpus, sizeof (UINT64), &Size)) ||
+ RETURN_ERROR (SafeUintnAdd (Size, sizeof (*mCpuHotEjectData), &Size)) ||
+ RETURN_ERROR (SafeUintnAdd (Size, sizeof (UINT64) - 1, &Size))) {
+ DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_EJECT_DATA\n", __FUNCTION__));
+ goto Fatal;
+ }
+
+ mCpuHotEjectData = AllocatePool (Size);
+ if (mCpuHotEjectData == NULL) {
+ ASSERT (mCpuHotEjectData != NULL);
+ goto Fatal;
+ }
+
+ mCpuHotEjectData->Handler = NULL;
+ mCpuHotEjectData->ArrayLength = MaxNumberOfCpus;
+
+ mCpuHotEjectData->QemuSelectorMap = ALIGN_POINTER (mCpuHotEjectData + 1,
+ sizeof (UINT64));
+ //
+ // We use mCpuHotEjectData->QemuSelectorMap to map
+ // ProcessorNum -> QemuSelector. Initialize to invalid values.
+ //
+ for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
+ mCpuHotEjectData->QemuSelectorMap[Idx] = CPU_EJECT_QEMU_SELECTOR_INVALID;
+ }
+
+ //
+ // Expose address of CPU Hot eject Data structure
+ //
+ PcdStatus = PcdSet64S (PcdCpuHotEjectDataAddress,
+ (UINTN)(VOID *)mCpuHotEjectData);
+ ASSERT_RETURN_ERROR (PcdStatus);
+
+ return;
+
+Fatal:
+ CpuDeadLoop ();
+}
+
/**
Hook point in normal execution mode that allows the one CPU that was elected
as monarch during System Management Mode initialization to perform additional
@@ -188,6 +262,9 @@ SmmCpuFeaturesSmmRelocationComplete (
UINTN MapPagesBase;
UINTN MapPagesCount;
+
+ InitCpuHotEjectData ();
+
if (!MemEncryptSevIsEnabled ()) {
return;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v9 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
` (5 preceding siblings ...)
2021-03-12 6:26 ` [PATCH v9 06/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-16 10:20 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 08/10] OvmfPkg/CpuHotplugSmm: add EjectCpu() Ankur Arora
` (3 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Call the CPU hot-eject handler if one is installed. The condition for
installation is (PcdCpuMaxLogicalProcessorNumber > 1), and there's
a hot-unplug request.
The handler is called from SmmCpuFeaturesRendezvousExit(), which is
in-turn called at the tail-end of SmiRendezvous() after the BSP has
signalled an SMI exit via the "AllCpusInSync" loop.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
Notes:
Addresses the following comments from v8:
(1) Add a MemoryFence() before accessing mCpuHotEjctData->Handler
(and comment to that effect.)
.../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 5c025bc717c3..fdf2380974fa 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -452,6 +452,40 @@ SmmCpuFeaturesRendezvousExit (
IN UINTN CpuIndex
)
{
+ //
+ // We only call the Handler if CPU hot-eject is enabled
+ // (PcdCpuMaxLogicalProcessorNumber > 1), and hot-eject is needed
+ // in this SMI exit (otherwise mCpuHotEjectData->Handler is not armed.)
+ //
+
+ if (mCpuHotEjectData != NULL) {
+ CPU_HOT_EJECT_HANDLER Handler;
+
+ //
+ // As the comment above mentions, mCpuHotEjectData->Handler might be
+ // written to on the BSP as part of handling of the CPU-ejection.
+ //
+ // We know that any initial assignment to mCpuHotEjectData->Handler
+ // (on the BSP, in the CpuHotplugMmi() context) is ordered-before the
+ // load below, since it is guaranteed to happen before the
+ // control-dependency of the BSP's SMI exit signal -- by way of a store
+ // to AllCpusInSync (on the BSP, in BspHandler()) and the corresponding
+ // AllCpusInSync loop (on the APs, in SmiRendezvous()) which depends on
+ // that store.
+ //
+ // This guarantees that these pieces of code can never execute
+ // simultaneously. In addition, we ensure that the following load is
+ // ordered-after the AllCpusInSync loop by using a MemoryFence() with
+ // acquire semantics.
+ //
+ MemoryFence();
+
+ Handler = mCpuHotEjectData->Handler;
+
+ if (Handler != NULL) {
+ Handler (CpuIndex);
+ }
+ }
}
/**
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v9 08/10] OvmfPkg/CpuHotplugSmm: add EjectCpu()
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
` (6 preceding siblings ...)
2021-03-12 6:26 ` [PATCH v9 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-16 10:35 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject Ankur Arora
` (2 subsequent siblings)
10 siblings, 1 reply; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Add EjectCpu(), which handles the CPU ejection, and provides a holding
area for said CPUs. It is called via SmmCpuFeaturesRendezvousExit(),
at the tail end of the SMI handling.
Also UnplugCpus() now stashes QEMU Selectors of CPUs which need to be
ejected in CPU_HOT_EJECT_DATA.QemuSelectorMap. This is used by
EjectCpu() to identify CPUs marked for ejection.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
Notes:
Addresses the following comments from v8:
(1) Fixup the coment about UnplugCpus() to reference stashing QEMU
Cpu Selectors instead of APIC IDs.
(2) s/ToUnplugSelector/ToUnplugSelectors/
(3) Use plural for APIC ID in comment describing retval EFI_ALREADY_STARTED.
(4) Fixup indentation in check against CPU_EJECT_QEMU_SELECTOR_INVALID.
(5) Clarify comment:
- // never match more than one APIC ID and by transitivity, more than one
- // QemuSelector in a single invocation of UnplugCpus().
+ // never match more than one APIC ID -- nor, by transitivity, designate
+ // more than one QemuSelector -- in a single invocation of UnplugCpus().
(6a) Remove unnecessary UINT64 cast for mCpuHotEjectData->QemuSelectorMap[ProcessorNum].
(6b) Switch from 0x%Lx -> %Lu for QemuSelectorMap[ProcessorNum].
(6c) Switch from 0x%Lx -> %u for QemuSelector
(7) Switch to "return EFI_ALREADY_STARTED".
(8a) Replace "QemuSelector 0x%Lx" with "QemuSelector %u".
(8b) Replace the mCpuHotEjectData->QemuSelectorMap[ProcessorNum] argument
with just QemuSelector in DEBUG call.
(9) Clarify comment and make the language complementary to that in patch-7
Explicitly mention release memory fence.
OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 2 +
OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 154 ++++++++++++++++++++++++++++++--
2 files changed, 148 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
index 04322b0d7855..ebcc7e2ac63a 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
@@ -40,6 +40,7 @@ [Packages]
[LibraryClasses]
BaseLib
BaseMemoryLib
+ CpuLib
DebugLib
LocalApicLib
MmServicesTableLib
@@ -54,6 +55,7 @@ [Protocols]
[Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## CONSUMES
+ gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## CONSUMES
gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## CONSUMES
[FeaturePcd]
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 59f000eb7886..2eeb4567a262 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -10,10 +10,12 @@
#include <IndustryStandard/Q35MchIch9.h> // ICH9_APM_CNT
#include <IndustryStandard/QemuCpuHotplug.h> // QEMU_CPUHP_CMD_GET_PENDING
#include <Library/BaseLib.h> // CpuDeadLoop()
+#include <Library/CpuLib.h> // CpuSleep()
#include <Library/DebugLib.h> // ASSERT()
#include <Library/MmServicesTableLib.h> // gMmst
#include <Library/PcdLib.h> // PcdGetBool()
#include <Library/SafeIntLib.h> // SafeUintnSub()
+#include <Pcd/CpuHotEjectData.h> // CPU_HOT_EJECT_DATA
#include <Protocol/MmCpuIo.h> // EFI_MM_CPU_IO_PROTOCOL
#include <Protocol/SmmCpuService.h> // EFI_SMM_CPU_SERVICE_PROTOCOL
#include <Uefi/UefiBaseType.h> // EFI_STATUS
@@ -32,11 +34,12 @@ STATIC EFI_MM_CPU_IO_PROTOCOL *mMmCpuIo;
//
STATIC EFI_SMM_CPU_SERVICE_PROTOCOL *mMmCpuService;
//
-// This structure is a communication side-channel between the
+// These structures serve as communication side-channels between the
// EFI_SMM_CPU_SERVICE_PROTOCOL consumer (i.e., this driver) and provider
// (i.e., PiSmmCpuDxeSmm).
//
STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
+STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData;
//
// SMRAM arrays for fetching the APIC IDs of processors with pending events (of
// known event types), for the time of just one MMI.
@@ -190,18 +193,71 @@ RevokeNewSlot:
}
/**
+ CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
+ on each CPU at exit from SMM.
+
+ If, the executing CPU is not being ejected, nothing to be done.
+ If, the executing CPU is being ejected, wait in a halted loop
+ until ejected.
+
+ @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM,
+ and will be used as an index into
+ CPU_HOT_EJECT_DATA->QemuSelectorMap. It is
+ identical to the processor handle number in
+ EFI_SMM_CPU_SERVICE_PROTOCOL.
+**/
+VOID
+EFIAPI
+EjectCpu (
+ IN UINTN ProcessorNum
+ )
+{
+ UINT64 QemuSelector;
+
+ QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum];
+ if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) {
+ return;
+ }
+
+ //
+ // APs being unplugged get here from SmmCpuFeaturesRendezvousExit()
+ // after having been cleared to exit the SMI and so have no SMM
+ // processing remaining.
+ //
+ // Keep them penned here until the BSP tells QEMU to eject them.
+ //
+ for (;;) {
+ DisableInterrupts ();
+ CpuSleep ();
+ }
+}
+
+/**
Process to be hot-unplugged CPUs, per QemuCpuhpCollectApicIds().
For each such CPU, report the CPU to PiSmmCpuDxeSmm via
- EFI_SMM_CPU_SERVICE_PROTOCOL. If the to be hot-unplugged CPU is
- unknown, skip it silently.
+ EFI_SMM_CPU_SERVICE_PROTOCOL and stash the QEMU Cpu Selectors for later
+ ejection. If the to be hot-unplugged CPU is unknown, skip it silently.
+
+ Additonally, if we do stash any Cpu Selectors, also install a CPU eject
+ handler which would handle the ejection.
@param[in] ToUnplugApicIds The APIC IDs of the CPUs that are about to be
hot-unplugged.
+ @param[in] ToUnplugSelectors The QEMU Selectors of the CPUs that are about to
+ be hot-unplugged.
+
@param[in] ToUnplugCount The number of filled-in APIC IDs in
ToUnplugApicIds.
+ @retval EFI_ALREADY_STARTED For the ProcessorNum that
+ EFI_SMM_CPU_SERVICE_PROTOCOL had assigned to
+ one of the APIC IDs in ToUnplugApicIds,
+ mCpuHotEjectData->QemuSelectorMap already has
+ the QemuSelector value stashed. (This should
+ never happen.)
+
@retval EFI_SUCCESS Known APIC IDs have been removed from SMM data
structures.
@@ -212,23 +268,36 @@ STATIC
EFI_STATUS
UnplugCpus (
IN APIC_ID *ToUnplugApicIds,
+ IN UINT32 *ToUnplugSelectors,
IN UINT32 ToUnplugCount
)
{
EFI_STATUS Status;
UINT32 ToUnplugIdx;
+ UINT32 EjectCount;
UINTN ProcessorNum;
ToUnplugIdx = 0;
+ EjectCount = 0;
while (ToUnplugIdx < ToUnplugCount) {
APIC_ID RemoveApicId;
+ UINT32 QemuSelector;
RemoveApicId = ToUnplugApicIds[ToUnplugIdx];
+ QemuSelector = ToUnplugSelectors[ToUnplugIdx];
//
- // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find
- // the ProcessorNum for the APIC ID to be removed.
+ // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use RemoveApicId
+ // to find the corresponding ProcessorNum for the CPU to be removed.
//
+ // With this we can establish a 3 way mapping:
+ // APIC_ID -- ProcessorNum -- QemuSelector
+ //
+ // We stash the ProcessorNum -> QemuSelector mapping so it can later be
+ // used for CPU hot-eject in SmmCpuFeaturesRendezvousExit() context (where
+ // we only have ProcessorNum available.)
+ //
+
for (ProcessorNum = 0;
ProcessorNum < mCpuHotPlugData->ArrayLength;
ProcessorNum++) {
@@ -257,11 +326,62 @@ UnplugCpus (
return Status;
}
+ if (mCpuHotEjectData->QemuSelectorMap[ProcessorNum] !=
+ CPU_EJECT_QEMU_SELECTOR_INVALID) {
+ //
+ // mCpuHotEjectData->QemuSelectorMap[ProcessorNum] is set to
+ // CPU_EJECT_QEMU_SELECTOR_INVALID when mCpuHotEjectData->QemuSelectorMap
+ // is allocated, and once the subject processsor is ejected.
+ //
+ // Additionally, mMmCpuService->RemoveProcessor(ProcessorNum) invalidates
+ // mCpuHotPlugData->ApicId[ProcessorNum], so a given ProcessorNum can
+ // never match more than one APIC ID -- nor, by transitivity, designate
+ // more than one QemuSelector -- in a single invocation of UnplugCpus().
+ //
+ DEBUG ((DEBUG_ERROR, "%a: ProcessorNum %Lu maps to QemuSelector %Lu, "
+ "cannot also map to %u\n", __FUNCTION__, (UINT64)ProcessorNum,
+ mCpuHotEjectData->QemuSelectorMap[ProcessorNum], QemuSelector));
+
+ return EFI_ALREADY_STARTED;
+ }
+
+ //
+ // Stash the QemuSelector so we can do the actual ejection later.
+ //
+ mCpuHotEjectData->QemuSelectorMap[ProcessorNum] = (UINT64)QemuSelector;
+
+ DEBUG ((DEBUG_INFO, "%a: Started hot-unplug on ProcessorNum %Lu, APIC ID "
+ FMT_APIC_ID ", QemuSelector %u\n", __FUNCTION__, (UINT64)ProcessorNum,
+ RemoveApicId, QemuSelector));
+
+ EjectCount++;
ToUnplugIdx++;
}
+ if (EjectCount != 0) {
+ //
+ // We have processors to be ejected; install the handler.
+ //
+ mCpuHotEjectData->Handler = EjectCpu;
+
+ //
+ // The BSP and APs load mCpuHotEjectData->Handler, and
+ // mCpuHotEjectData->QemuSelectorMap[] in SmmCpuFeaturesRendezvousExit()
+ // and EjectCpu().
+ //
+ // The comment in SmmCpuFeaturesRendezvousExit() details how we use
+ // the AllCpusInSync control-dependency to ensure that any loads are
+ // ordered-after the stores above.
+ //
+ // Ensure that the stores above are ordered-before the AllCpusInSync store
+ // by using a MemoryFence() with release semantics.
+ //
+ MemoryFence ();
+ }
+
//
- // We've removed this set of APIC IDs from SMM data structures.
+ // We've removed this set of APIC IDs from SMM data structures and
+ // have installed an ejection handler if needed.
//
return EFI_SUCCESS;
}
@@ -389,7 +509,7 @@ CpuHotplugMmi (
}
if (ToUnplugCount > 0) {
- Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
+ Status = UnplugCpus (mToUnplugApicIds, mToUnplugSelectors, ToUnplugCount);
if (EFI_ERROR (Status)) {
goto Fatal;
}
@@ -460,9 +580,14 @@ CpuHotplugEntry (
//
// Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm
- // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM.
+ // has pointed:
+ // - PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM,
+ // - PcdCpuHotEjectDataAddress to CPU_HOT_EJECT_DATA in SMRAM, if the
+ // possible CPU count is greater than 1.
//
mCpuHotPlugData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotPlugDataAddress);
+ mCpuHotEjectData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotEjectDataAddress);
+
if (mCpuHotPlugData == NULL) {
Status = EFI_NOT_FOUND;
DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_PLUG_DATA: %r\n", __FUNCTION__, Status));
@@ -474,6 +599,19 @@ CpuHotplugEntry (
if (mCpuHotPlugData->ArrayLength == 1) {
return EFI_UNSUPPORTED;
}
+
+ if (mCpuHotEjectData == NULL) {
+ Status = EFI_NOT_FOUND;
+ } else if (mCpuHotPlugData->ArrayLength != mCpuHotEjectData->ArrayLength) {
+ Status = EFI_INVALID_PARAMETER;
+ } else {
+ Status = EFI_SUCCESS;
+ }
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_EJECT_DATA: %r\n", __FUNCTION__, Status));
+ goto Fatal;
+ }
+
//
// Allocate the data structures that depend on the possible CPU count.
//
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v9 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
` (7 preceding siblings ...)
2021-03-12 6:26 ` [PATCH v9 08/10] OvmfPkg/CpuHotplugSmm: add EjectCpu() Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-16 11:27 ` [edk2-devel] " Laszlo Ersek
2021-03-16 12:52 ` Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 10/10] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug Ankur Arora
2021-03-16 14:07 ` [PATCH v9 00/10] support " Laszlo Ersek
10 siblings, 2 replies; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Add logic in EjectCpu() to do the actual the CPU ejection.
On the BSP, ejection happens by first selecting the CPU via
its QemuSelector and then sending the QEMU "eject" command.
QEMU in-turn signals the remote VCPU thread which context-switches
the CPU out of the SMI handler.
Meanwhile the CPU being ejected, waits around in its holding
area until it is context-switched out. Note that it is possible
that a slow CPU gets ejected before it reaches the wait loop.
However, this would never happen before it has executed the
"AllCpusInSync" loop in SmiRendezvous().
It can mean that an ejected CPU does not execute code after
that point but given that the CPU state will be destroyed by
QEMU, the missed cleanup is no great loss.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
Notes:
Addresses the following comments from v8:
(1a,1b) CheckIfBsp(): get rid of ProcessorNum, document retval.
(2) Line up IsBsp and ApicBaseMsr
(3) s/ongoing SMI iteration/ongoing SMI/
(4) Get rid of the allusions to alignment in the comment in EjectCpu().
() Also reduce some of the repetitive detail in this comment.
(5) EjectCpu(): reorder logic to cleanly separate the AP and the BSP portions.
(6) Get rid of unnecessary MemoryFence() between QemuCpuhpWrite
and clearing of the eject status.
(7) Change type of QemuSelector to %Lu in DEBUG statement
(8) Get rid of the repetitive comment in SmmCpuFeaturesRendezvousExit().
The necessary parts of this got moved to patch-7.
OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 1 +
OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 113 ++++++++++++++++++++--
2 files changed, 108 insertions(+), 6 deletions(-)
diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
index 2ec7a107a64d..d0e83102c13f 100644
--- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
+++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
@@ -34,6 +34,7 @@
#define QEMU_CPUHP_STAT_ENABLED BIT0
#define QEMU_CPUHP_STAT_INSERT BIT1
#define QEMU_CPUHP_STAT_REMOVE BIT2
+#define QEMU_CPUHP_STAT_EJECT BIT3
#define QEMU_CPUHP_STAT_FW_REMOVE BIT4
#define QEMU_CPUHP_RW_CMD_DATA 0x8
diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
index 2eeb4567a262..ae3abd525900 100644
--- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
+++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
@@ -18,6 +18,7 @@
#include <Pcd/CpuHotEjectData.h> // CPU_HOT_EJECT_DATA
#include <Protocol/MmCpuIo.h> // EFI_MM_CPU_IO_PROTOCOL
#include <Protocol/SmmCpuService.h> // EFI_SMM_CPU_SERVICE_PROTOCOL
+#include <Register/Intel/ArchitecturalMsr.h> // MSR_IA32_APIC_BASE_REGISTER
#include <Uefi/UefiBaseType.h> // EFI_STATUS
#include "ApicId.h" // APIC_ID
@@ -193,12 +194,40 @@ RevokeNewSlot:
}
/**
+ EjectCpu needs to know the BSP at SMI exit at a point when
+ some of the EFI_SMM_CPU_SERVICE_PROTOCOL state has been torn
+ down.
+ Reuse the logic from OvmfPkg::PlatformSmmBspElection() to
+ do that.
+
+ @retval TRUE If the CPU executing this function is the BSP.
+
+ @retval FALSE If the CPU executing this function is an AP.
+**/
+STATIC
+BOOLEAN
+CheckIfBsp (
+ VOID
+ )
+{
+ MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr;
+ BOOLEAN IsBsp;
+
+ ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
+ IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1);
+ return IsBsp;
+}
+
+/**
CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
on each CPU at exit from SMM.
- If, the executing CPU is not being ejected, nothing to be done.
+ If, the executing CPU is neither the BSP, nor being ejected, nothing
+ to be done.
If, the executing CPU is being ejected, wait in a halted loop
until ejected.
+ If, the executing CPU is the BSP, set QEMU CPU status to eject
+ for CPUs being ejected.
@param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM,
and will be used as an index into
@@ -214,6 +243,83 @@ EjectCpu (
{
UINT64 QemuSelector;
+ if (CheckIfBsp ()) {
+ UINT32 Idx;
+
+ for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
+ UINT64 QemuSelector;
+
+ QemuSelector = mCpuHotEjectData->QemuSelectorMap[Idx];
+
+ if (QemuSelector != CPU_EJECT_QEMU_SELECTOR_INVALID) {
+ //
+ // This to-be-ejected-CPU has already received the BSP's SMI exit
+ // signal and will execute SmmCpuFeaturesRendezvousExit()
+ // followed by this callback or is already penned in the
+ // CpuSleep() loop below.
+ //
+ // Tell QEMU to context-switch it out.
+ //
+ QemuCpuhpWriteCpuSelector (mMmCpuIo, (UINT32) QemuSelector);
+ QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECT);
+
+ //
+ // Now that we've ejected the CPU corresponding to QemuSelectorMap[Idx],
+ // clear its eject status to ensure that an invalid future SMI does
+ // not end up trying a spurious eject or a newly hotplugged CPU does
+ // not get penned in the CpuSleep() loop.
+ //
+ // Note that the QemuCpuhpWriteCpuStatus() command above is a write to
+ // a different address space and uses the EFI_MM_CPU_IO_PROTOCOL.
+ //
+ // This means that we are guaranteed that the following assignment
+ // will not be reordered before the eject. And, so we can safely
+ // do this write here.
+ //
+ mCpuHotEjectData->QemuSelectorMap[Idx] =
+ CPU_EJECT_QEMU_SELECTOR_INVALID;
+
+ DEBUG ((DEBUG_INFO, "%a: Unplugged ProcessorNum %u, "
+ "QemuSelector %Lu\n", __FUNCTION__, Idx, QemuSelector));
+ }
+ }
+
+ //
+ // We are done until the next hot-unplug; clear the handler.
+ //
+ // mCpuHotEjectData->Handler is a NOP for any CPU not under ejection.
+ // So, once we are done with all the ejections, we can safely reset it
+ // here since any CPU dereferencing it would only see either the old
+ // or the new value (since it is aligned at a natural boundary.)
+ //
+ mCpuHotEjectData->Handler = NULL;
+ return;
+ }
+
+ //
+ // Reached only on APs
+ //
+
+ //
+ // mCpuHotEjectData->QemuSelectorMap[ProcessorNum] is updated
+ // on the BSP in the ongoing SMI at two places:
+ //
+ // - UnplugCpus() where the BSP determines if a CPU is under ejection
+ // or not. As a comment in UnplugCpus() at set-up, and in
+ // SmmCpuFeaturesRendezvousExit() where it is dereferenced describe,
+ // any such updates are guaranteed to be ordered-before the
+ // dereference below.
+ //
+ // - EjectCpu() on the BSP (above) updates QemuSelectorMap[ProcessorNum]
+ // for a CPU once it's ejected.
+ //
+ // The CPU under ejection: might be executing anywhere between the
+ // AllCpusInSync loop in SmiRendezvous(), to about to dereference
+ // QemuSelectorMap[ProcessorNum].
+ // As described in the comment above where we do the reset, this
+ // is not a problem since the ejected CPU never sees the after value.
+ // CPUs not-under ejection: never see any changes so they are fine.
+ //
QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum];
if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) {
return;
@@ -495,11 +601,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 = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v9 10/10] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
` (8 preceding siblings ...)
2021-03-12 6:26 ` [PATCH v9 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject Ankur Arora
@ 2021-03-12 6:26 ` Ankur Arora
2021-03-16 11:40 ` [edk2-devel] " Laszlo Ersek
2021-03-16 14:07 ` [PATCH v9 00/10] support " Laszlo Ersek
10 siblings, 1 reply; 21+ messages in thread
From: Ankur Arora @ 2021-03-12 6:26 UTC (permalink / raw)
To: devel
Cc: lersek, imammedo, boris.ostrovsky, Ankur Arora, Jordan Justen,
Ard Biesheuvel, Aaron Young
Advertise OVMF support for CPU hot-unplug and negotiate it
if QEMU requests the feature.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Aaron Young <aaron.young@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
---
Notes:
Addresses the following comments from v8:
(1) Remove inconsistent comment style (and stray newline) around the newly
added ICH9_LPC_SMI_F_CPU_HOT_UNPLUG.
(2) Remove spurious empty line.
OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
index c9d875543205..3e2e61e4dbd0 100644
--- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
+++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
@@ -28,7 +28,12 @@
// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
//
#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_HOTPLUG upon hot-unplug", in the
+// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
+//
+#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG BIT2
//
// Provides a scratch buffer (allocated in EfiReservedMemoryType type memory)
// for the S3 boot script fragment to write to and read from.
@@ -112,7 +117,8 @@ NegotiateSmiFeatures (
QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures);
//
- // We want broadcast SMI, SMI on CPU hotplug, and nothing else.
+ // We want broadcast SMI, SMI on CPU hotplug, SMI on CPU hot-unplug
+ // and nothing else.
//
RequestedFeaturesMask = ICH9_LPC_SMI_F_BROADCAST;
if (!MemEncryptSevIsEnabled ()) {
@@ -120,6 +126,7 @@ 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;
QemuFwCfgSelectItem (mRequestedFeaturesItem);
@@ -166,6 +173,13 @@ NegotiateSmiFeatures (
__FUNCTION__));
}
+ if ((mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOT_UNPLUG) == 0) {
+ DEBUG ((DEBUG_INFO, "%a: CPU hot-unplug not negotiated\n", __FUNCTION__));
+ } else {
+ DEBUG ((DEBUG_INFO, "%a: CPU hot-unplug with SMI negotiated\n",
+ __FUNCTION__));
+ }
+
//
// Negotiation successful (although we may not have gotten the optimal
// feature set).
--
2.9.3
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events
2021-03-12 6:26 ` [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events Ankur Arora
@ 2021-03-16 9:56 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2021-03-16 9:56 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jordan Justen, Ard Biesheuvel,
Aaron Young
On 03/12/21 07:26, Ankur Arora wrote:
> Process fw_remove events in QemuCpuhpCollectApicIds(), and collect APIC IDs
> and QEMU CPU Selectors for CPUs being hot-unplugged.
>
> In addition, we now ignore CPUs which only have remove set. These
> CPUs haven't been processed by OSPM yet.
>
> This is based on the QEMU hot-unplug protocol documented here:
> https://lore.kernel.org/qemu-devel/20201204170939.1815522-3-imammedo@redhat.com/
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>
> Notes:
> Addresses the following comments from v8:
> (1) Fix commit message to mention that we collect cpu-selectors as well.
> (2,3,6) s/UnplugSelector/UnplugSelectors/ in CpuHotplug.c, QemuCpuhp.c
> (4) Fix comment above the declaration of the now renamed mToUnplugSelector.
> (5) Fix spacing around "||".
> (7) Fix QemuCpuCollectApicIds() comments to line up descriptions for
> ToUnplugSelectors and other params.
> (8) s/ExtendSel/ExtendSels/.
> (9) Add the (ExtendSels => ExtendIds) assert.
> (10) Fix the missing CurrentSelector++ bug.
>
> OvmfPkg/CpuHotplugSmm/QemuCpuhp.h | 1 +
> OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 1 +
> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 29 +++++--
> OvmfPkg/CpuHotplugSmm/QemuCpuhp.c | 101 +++++++++++++++-------
> 4 files changed, 93 insertions(+), 39 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
>
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> index 8adaa0ad91f0..3e2c2192e1c0 100644
> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.h
> @@ -55,6 +55,7 @@ QemuCpuhpCollectApicIds (
> OUT APIC_ID *PluggedApicIds,
> OUT UINT32 *PluggedCount,
> OUT APIC_ID *ToUnplugApicIds,
> + OUT UINT32 *ToUnplugSelectors,
> OUT UINT32 *ToUnplugCount
> );
>
> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> index a34a6d3fae61..2ec7a107a64d 100644
> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> @@ -34,6 +34,7 @@
> #define QEMU_CPUHP_STAT_ENABLED BIT0
> #define QEMU_CPUHP_STAT_INSERT BIT1
> #define QEMU_CPUHP_STAT_REMOVE BIT2
> +#define QEMU_CPUHP_STAT_FW_REMOVE BIT4
>
> #define QEMU_CPUHP_RW_CMD_DATA 0x8
>
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index bf68fcd42914..ee1497b93140 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -45,13 +45,16 @@ STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
> // don't want to allocate SMRAM at OS runtime, and potentially fail (or
> // fragment the SMRAM map).
> //
> -// These arrays provide room for ("possible CPU count" minus one) APIC IDs
> -// each, as we don't expect every possible CPU to appear, or disappear, in a
> -// single MMI. The numbers of used (populated) elements in the arrays are
> +// The first array stores APIC IDs for hot-plug events, the second and the
> +// third store APIC IDs and QEMU CPU Selectors (both indexed similarly) for
> +// hot-unplug events. All of these provide room for "possible CPU count" minus
> +// one elements as we don't expect every possible CPU to appear, or disappear,
> +// in a single MMI. The numbers of used (populated) elements in the arrays are
> // determined on every MMI separately.
> //
> STATIC APIC_ID *mPluggedApicIds;
> STATIC APIC_ID *mToUnplugApicIds;
> +STATIC UINT32 *mToUnplugSelectors;
> //
> // Address of the non-SMRAM reserved memory page that contains the Post-SMM Pen
> // for hot-added CPUs.
> @@ -289,6 +292,7 @@ CpuHotplugMmi (
> mPluggedApicIds,
> &PluggedCount,
> mToUnplugApicIds,
> + mToUnplugSelectors,
> &ToUnplugCount
> );
> if (EFI_ERROR (Status)) {
> @@ -333,7 +337,9 @@ CpuHotplugEntry (
> )
> {
> EFI_STATUS Status;
> + UINTN Len;
> UINTN Size;
> + UINTN SizeSel;
>
> //
> // This module should only be included when SMM support is required.
> @@ -387,8 +393,9 @@ CpuHotplugEntry (
> //
> // Allocate the data structures that depend on the possible CPU count.
> //
> - if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Size)) ||
> - RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Size, &Size))) {
> + if (RETURN_ERROR (SafeUintnSub (mCpuHotPlugData->ArrayLength, 1, &Len)) ||
> + RETURN_ERROR (SafeUintnMult (sizeof (APIC_ID), Len, &Size)) ||
> + RETURN_ERROR (SafeUintnMult (sizeof (UINT32), Len, &SizeSel))) {
> Status = EFI_ABORTED;
> DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_PLUG_DATA\n", __FUNCTION__));
> goto Fatal;
> @@ -405,6 +412,12 @@ CpuHotplugEntry (
> DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status));
> goto ReleasePluggedApicIds;
> }
> + Status = gMmst->MmAllocatePool (EfiRuntimeServicesData, SizeSel,
> + (VOID **)&mToUnplugSelectors);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: MmAllocatePool(): %r\n", __FUNCTION__, Status));
> + goto ReleaseToUnplugApicIds;
> + }
>
> //
> // Allocate the Post-SMM Pen for hot-added CPUs.
> @@ -412,7 +425,7 @@ CpuHotplugEntry (
> Status = SmbaseAllocatePostSmmPen (&mPostSmmPenAddress,
> SystemTable->BootServices);
> if (EFI_ERROR (Status)) {
> - goto ReleaseToUnplugApicIds;
> + goto ReleaseToUnplugSelectors;
> }
>
> //
> @@ -472,6 +485,10 @@ ReleasePostSmmPen:
> SmbaseReleasePostSmmPen (mPostSmmPenAddress, SystemTable->BootServices);
> mPostSmmPenAddress = 0;
>
> +ReleaseToUnplugSelectors:
> + gMmst->MmFreePool (mToUnplugSelectors);
> + mToUnplugSelectors = NULL;
> +
> ReleaseToUnplugApicIds:
> gMmst->MmFreePool (mToUnplugApicIds);
> mToUnplugApicIds = NULL;
> diff --git a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> index 8d4a6693c8d6..8434dd446b96 100644
> --- a/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> +++ b/OvmfPkg/CpuHotplugSmm/QemuCpuhp.c
> @@ -145,27 +145,30 @@ QemuCpuhpWriteCommand (
>
> On error, the contents of the output parameters are undefined.
>
> - @param[in] MmCpuIo The EFI_MM_CPU_IO_PROTOCOL instance for
> - accessing IO Ports.
> + @param[in] MmCpuIo The EFI_MM_CPU_IO_PROTOCOL instance for
> + accessing IO Ports.
>
> - @param[in] PossibleCpuCount The number of possible CPUs in the system. Must
> - be positive.
> + @param[in] PossibleCpuCount The number of possible CPUs in the system. Must
> + be positive.
>
> - @param[in] ApicIdCount The number of elements each one of the
> - PluggedApicIds and ToUnplugApicIds arrays can
> - accommodate. Must be positive.
> + @param[in] ApicIdCount The number of elements each one of the
> + PluggedApicIds and ToUnplugApicIds arrays can
> + accommodate. Must be positive.
>
> - @param[out] PluggedApicIds The APIC IDs of the CPUs that have been
> - hot-plugged.
> + @param[out] PluggedApicIds The APIC IDs of the CPUs that have been
> + hot-plugged.
>
> - @param[out] PluggedCount The number of filled-in APIC IDs in
> - PluggedApicIds.
> + @param[out] PluggedCount The number of filled-in APIC IDs in
> + PluggedApicIds.
>
> - @param[out] ToUnplugApicIds The APIC IDs of the CPUs that are about to be
> - hot-unplugged.
> + @param[out] ToUnplugApicIds The APIC IDs of the CPUs that are about to be
> + hot-unplugged.
>
> - @param[out] ToUnplugCount The number of filled-in APIC IDs in
> - ToUnplugApicIds.
> + @param[out] ToUnplugSelectors The QEMU Selectors of the CPUs that are about
> + to be hot-unplugged.
> +
> + @param[out] ToUnplugCount The number of filled-in APIC IDs in
> + ToUnplugApicIds.
>
> @retval EFI_INVALID_PARAMETER PossibleCpuCount is zero, or ApicIdCount is
> zero.
> @@ -187,6 +190,7 @@ QemuCpuhpCollectApicIds (
> OUT APIC_ID *PluggedApicIds,
> OUT UINT32 *PluggedCount,
> OUT APIC_ID *ToUnplugApicIds,
> + OUT UINT32 *ToUnplugSelectors,
> OUT UINT32 *ToUnplugCount
> )
> {
> @@ -204,6 +208,7 @@ QemuCpuhpCollectApicIds (
> UINT32 PendingSelector;
> UINT8 CpuStatus;
> APIC_ID *ExtendIds;
> + UINT32 *ExtendSels;
> UINT32 *ExtendCount;
> APIC_ID NewApicId;
>
> @@ -245,10 +250,10 @@ QemuCpuhpCollectApicIds (
> if ((CpuStatus & QEMU_CPUHP_STAT_INSERT) != 0) {
> //
> // The "insert" event guarantees the "enabled" status; plus it excludes
> - // the "remove" event.
> + // the "fw_remove" event.
> //
> if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0 ||
> - (CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
> + (CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) {
> DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: "
> "inconsistent CPU status\n", __FUNCTION__, CurrentSelector,
> CpuStatus));
> @@ -259,33 +264,63 @@ QemuCpuhpCollectApicIds (
> CurrentSelector));
>
> ExtendIds = PluggedApicIds;
> + ExtendSels = NULL;
> ExtendCount = PluggedCount;
> - } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
> - DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove\n", __FUNCTION__,
> - CurrentSelector));
> + } else if ((CpuStatus & QEMU_CPUHP_STAT_FW_REMOVE) != 0) {
> + //
> + // "fw_remove" event guarantees "enabled".
> + //
> + if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) == 0) {
> + DEBUG ((DEBUG_ERROR, "%a: CurrentSelector=%u CpuStatus=0x%x: "
> + "inconsistent CPU status\n", __FUNCTION__, CurrentSelector,
> + CpuStatus));
> + return EFI_PROTOCOL_ERROR;
> + }
> +
> + DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: fw_remove\n",
> + __FUNCTION__, CurrentSelector));
>
> ExtendIds = ToUnplugApicIds;
> + ExtendSels = ToUnplugSelectors;
> ExtendCount = ToUnplugCount;
> + } else if ((CpuStatus & QEMU_CPUHP_STAT_REMOVE) != 0) {
> + //
> + // Let the OSPM deal with the "remove" event.
> + //
> + DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: remove (ignored)\n",
> + __FUNCTION__, CurrentSelector));
> +
> + ExtendIds = NULL;
> + ExtendSels = NULL;
> + ExtendCount = NULL;
> } else {
> DEBUG ((DEBUG_VERBOSE, "%a: CurrentSelector=%u: no event\n",
> __FUNCTION__, CurrentSelector));
> break;
> }
>
> - //
> - // Save the APIC ID of the CPU with the pending event, to the corresponding
> - // APIC ID array.
> - //
> - if (*ExtendCount == ApicIdCount) {
> - DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__));
> - return EFI_BUFFER_TOO_SMALL;
> - }
> - QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);
> - NewApicId = QemuCpuhpReadCommandData (MmCpuIo);
> - DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,
> - NewApicId));
> - ExtendIds[(*ExtendCount)++] = NewApicId;
> + ASSERT ((ExtendIds == NULL) == (ExtendCount == NULL));
> + ASSERT ((ExtendSels == NULL) || (ExtendIds != NULL));
>
> + if (ExtendIds != NULL) {
> + //
> + // Save the APIC ID of the CPU with the pending event, to the
> + // corresponding APIC ID array.
> + // For unplug events, also save the CurrentSelector.
> + //
> + if (*ExtendCount == ApicIdCount) {
> + DEBUG ((DEBUG_ERROR, "%a: APIC ID array too small\n", __FUNCTION__));
> + return EFI_BUFFER_TOO_SMALL;
> + }
> + QemuCpuhpWriteCommand (MmCpuIo, QEMU_CPUHP_CMD_GET_ARCH_ID);
> + NewApicId = QemuCpuhpReadCommandData (MmCpuIo);
> + DEBUG ((DEBUG_VERBOSE, "%a: ApicId=" FMT_APIC_ID "\n", __FUNCTION__,
> + NewApicId));
> + if (ExtendSels != NULL) {
> + ExtendSels[(*ExtendCount)] = CurrentSelector;
> + }
> + ExtendIds[(*ExtendCount)++] = NewApicId;
> + }
> //
> // We've processed the CPU with (known) pending events, but we must never
> // clear events. Therefore we need to advance past this CPU manually;
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v9 05/10] OvmfPkg: define CPU_HOT_EJECT_DATA
2021-03-12 6:26 ` [PATCH v9 05/10] OvmfPkg: define CPU_HOT_EJECT_DATA Ankur Arora
@ 2021-03-16 10:03 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2021-03-16 10:03 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jordan Justen, Ard Biesheuvel,
Aaron Young
On 03/12/21 07:26, Ankur Arora wrote:
> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress, which
> will be used to share CPU ejection state between OvmfPkg/CpuHotPlugSmm
> and PiSmmCpuDxeSmm.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>
> Notes:
> Addresses the following comments from v8:
>
> (1) Get rid of the unnecessary commit specifier from the subject.
> (2) s/MaxNumberOfCpus/PcdCpuMaxLogicalProcessorNumber/
> (3) Shifted the comments to be above each structure field.
>
> OvmfPkg/OvmfPkg.dec | 4 +++
> OvmfPkg/Include/Pcd/CpuHotEjectData.h | 60 +++++++++++++++++++++++++++++++++++
> 2 files changed, 64 insertions(+)
> create mode 100644 OvmfPkg/Include/Pcd/CpuHotEjectData.h
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Laszlo
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 4348bb45c64a..9629707020ba 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -352,6 +352,10 @@ [PcdsDynamic, PcdsDynamicEx]
> # This PCD is only accessed if PcdSmmSmramRequire is TRUE (see below).
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE|BOOLEAN|0x34
>
> + ## This PCD adds a communication channel between OVMF's SmmCpuFeaturesLib
> + # instance in PiSmmCpuDxeSmm, and CpuHotplugSmm.
> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46
> +
> [PcdsFeatureFlag]
> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
> diff --git a/OvmfPkg/Include/Pcd/CpuHotEjectData.h b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
> new file mode 100644
> index 000000000000..06714375526c
> --- /dev/null
> +++ b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
> @@ -0,0 +1,60 @@
> +/** @file
> + Definition for the CPU_HOT_EJECT_DATA structure, which shares
> + CPU hot-eject state between OVMF's SmmCpuFeaturesLib instance in
> + PiSmmCpuDxeSmm, and CpuHotplugSmm.
> +
> + CPU_HOT_EJECT_DATA is allocated in SMRAM, and pointed-to by
> + PcdCpuHotEjectDataAddress.
> +
> + PcdCpuHotEjectDataAddress is valid when SMM_REQUIRE is TRUE
> + and PcdCpuMaxLogicalProcessorNumber > 1.
> +
> + Copyright (C) 2021, Oracle Corporation.
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef CPU_HOT_EJECT_DATA_H_
> +#define CPU_HOT_EJECT_DATA_H_
> +
> +/**
> + CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
> + on each CPU at exit from SMM.
> +
> + @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM,
> + and will be used as an index into
> + CPU_HOT_EJECT_DATA->QemuSelectorMap. It is
> + identical to the processor handle in
> + EFI_SMM_CPU_SERVICE_PROTOCOL.
> +**/
> +typedef
> +VOID
> +(EFIAPI *CPU_HOT_EJECT_HANDLER) (
> + IN UINTN ProcessorNum
> + );
> +
> +//
> +// CPU_EJECT_QEMU_SELECTOR_INVALID marks CPUs not being ejected in
> +// CPU_HOT_EJECT_DATA->QemuSelectorMap.
> +//
> +// QEMU CPU Selector is UINT32, so we choose an invalid value larger
> +// than that type.
> +//
> +#define CPU_EJECT_QEMU_SELECTOR_INVALID (MAX_UINT64)
> +
> +typedef struct {
> + //
> + // Maps ProcessorNum -> QemuSelector for pending hot-ejects
> + //
> + volatile UINT64 *QemuSelectorMap;
> + //
> + // Handler to do the CPU ejection
> + //
> + volatile CPU_HOT_EJECT_HANDLER Handler;
> + //
> + // Entries in the QemuSelectorMap
> + //
> + UINT32 ArrayLength;
> +} CPU_HOT_EJECT_DATA;
> +
> +#endif // CPU_HOT_EJECT_DATA_H_
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v9 06/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
2021-03-12 6:26 ` [PATCH v9 06/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state Ankur Arora
@ 2021-03-16 10:12 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2021-03-16 10:12 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jordan Justen, Ard Biesheuvel,
Aaron Young
On 03/12/21 07:26, Ankur Arora wrote:
> Init CPU_HOT_EJECT_DATA, which will be used to share CPU ejection
> state between SmmCpuFeaturesLib (via PiSmmCpuDxeSmm) and CpuHotPlugSmm.
>
> The init happens via SmmCpuFeaturesSmmRelocationComplete(), and so it
> will run as part of the PiSmmCpuDxeSmm entry point function,
> PiCpuSmmEntry(). Once inited, CPU_HOT_EJECT_DATA is exposed via
> PcdCpuHotEjectDataAddress.
>
> The CPU hot-eject handler (CPU_HOT_EJECT_DATA->Handler) is setup when
> there is an ejection request via CpuHotplugSmm.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>
> Notes:
> Addresses the following comments from v8:
>
> (1) Remove line before the "if (MaxNumberofCpus == 1)" check.
> (3) Fixup the space around "||".
> (2,6) Simplify the three SafeInt multiplication into the ones suggested
> by Laszlo.
> (4) Get rid of the mixed sizeof(mCpuHotEjectData->QemuSelectorMap[0]) and
> sizeof(UINT64) in favour of UINT64 everywhere. I was planning to use
> the first, but describing the alignment needed is easier in terms of the
> second.
> Also, as Laszlo's comments on v8-patch-9 mention, we don't really need
> this alignment for correctness reasons. This patch retains it, so we
> don't pay access penalty for unaligned access.
> (5) Change alignment from UINT64 to UINT64-1.
> (7) Use the more idiomatic ALIGN_POINTER instead of ALIGN_VALUE.
> (8) RETURN_ERROR -> ASSERT_RETURN_ERROR.
>
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 ++
> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 77 ++++++++++++++++++++++
> 2 files changed, 81 insertions(+)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
>
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> index 97a10afb6e27..8a426a4c10fb 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -30,9 +30,13 @@ [LibraryClasses]
> BaseMemoryLib
> DebugLib
> MemEncryptSevLib
> + MemoryAllocationLib
> PcdLib
> + SafeIntLib
> SmmServicesTableLib
> UefiBootServicesTableLib
>
> [Pcd]
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 7ef7ed98342e..5c025bc717c3 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -11,10 +11,13 @@
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/MemEncryptSevLib.h>
> +#include <Library/MemoryAllocationLib.h>
> #include <Library/PcdLib.h>
> +#include <Library/SafeIntLib.h>
> #include <Library/SmmCpuFeaturesLib.h>
> #include <Library/SmmServicesTableLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> +#include <Pcd/CpuHotEjectData.h>
> #include <PiSmm.h>
> #include <Register/Intel/SmramSaveStateMap.h>
> #include <Register/QemuSmramSaveStateMap.h>
> @@ -171,6 +174,77 @@ SmmCpuFeaturesHookReturnFromSmm (
> return OriginalInstructionPointer;
> }
>
> +STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData = NULL;
> +
> +/**
> + Initialize mCpuHotEjectData if PcdCpuMaxLogicalProcessorNumber > 1.
> +
> + Also setup the corresponding PcdCpuHotEjectDataAddress.
> +**/
> +STATIC
> +VOID
> +InitCpuHotEjectData (
> + VOID
> + )
> +{
> + UINTN Size;
> + UINT32 Idx;
> + UINT32 MaxNumberOfCpus;
> + RETURN_STATUS PcdStatus;
> +
> + MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> + if (MaxNumberOfCpus == 1) {
> + return;
> + }
> +
> + //
> + // We allocate CPU_HOT_EJECT_DATA and CPU_HOT_EJECT_DATA->QemuSelectorMap[]
> + // in a single allocation, and explicitly align the QemuSelectorMap[] (which
> + // is a UINT64 array) at its natural boundary.
> + // Accordingly, allocate:
> + // sizeof(*mCpuHotEjectData) + (MaxNumberOfCpus * sizeof(UINT64))
> + // and, add sizeof(UINT64) - 1 to use as padding if needed.
> + //
> +
> + if (RETURN_ERROR (SafeUintnMult (MaxNumberOfCpus, sizeof (UINT64), &Size)) ||
> + RETURN_ERROR (SafeUintnAdd (Size, sizeof (*mCpuHotEjectData), &Size)) ||
> + RETURN_ERROR (SafeUintnAdd (Size, sizeof (UINT64) - 1, &Size))) {
> + DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_EJECT_DATA\n", __FUNCTION__));
> + goto Fatal;
> + }
> +
> + mCpuHotEjectData = AllocatePool (Size);
> + if (mCpuHotEjectData == NULL) {
> + ASSERT (mCpuHotEjectData != NULL);
> + goto Fatal;
> + }
> +
> + mCpuHotEjectData->Handler = NULL;
> + mCpuHotEjectData->ArrayLength = MaxNumberOfCpus;
> +
> + mCpuHotEjectData->QemuSelectorMap = ALIGN_POINTER (mCpuHotEjectData + 1,
> + sizeof (UINT64));
> + //
> + // We use mCpuHotEjectData->QemuSelectorMap to map
> + // ProcessorNum -> QemuSelector. Initialize to invalid values.
> + //
> + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
> + mCpuHotEjectData->QemuSelectorMap[Idx] = CPU_EJECT_QEMU_SELECTOR_INVALID;
> + }
> +
> + //
> + // Expose address of CPU Hot eject Data structure
> + //
> + PcdStatus = PcdSet64S (PcdCpuHotEjectDataAddress,
> + (UINTN)(VOID *)mCpuHotEjectData);
> + ASSERT_RETURN_ERROR (PcdStatus);
> +
> + return;
> +
> +Fatal:
> + CpuDeadLoop ();
> +}
> +
> /**
> Hook point in normal execution mode that allows the one CPU that was elected
> as monarch during System Management Mode initialization to perform additional
> @@ -188,6 +262,9 @@ SmmCpuFeaturesSmmRelocationComplete (
> UINTN MapPagesBase;
> UINTN MapPagesCount;
>
> +
> + InitCpuHotEjectData ();
> +
> if (!MemEncryptSevIsEnabled ()) {
> return;
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v9 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler
2021-03-12 6:26 ` [PATCH v9 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler Ankur Arora
@ 2021-03-16 10:20 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2021-03-16 10:20 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jordan Justen, Ard Biesheuvel,
Aaron Young
On 03/12/21 07:26, Ankur Arora wrote:
> Call the CPU hot-eject handler if one is installed. The condition for
> installation is (PcdCpuMaxLogicalProcessorNumber > 1), and there's
> a hot-unplug request.
>
> The handler is called from SmmCpuFeaturesRendezvousExit(), which is
> in-turn called at the tail-end of SmiRendezvous() after the BSP has
> signalled an SMI exit via the "AllCpusInSync" loop.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>
> Notes:
> Addresses the following comments from v8:
>
> (1) Add a MemoryFence() before accessing mCpuHotEjctData->Handler
> (and comment to that effect.)
>
> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 5c025bc717c3..fdf2380974fa 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -452,6 +452,40 @@ SmmCpuFeaturesRendezvousExit (
> IN UINTN CpuIndex
> )
> {
> + //
> + // We only call the Handler if CPU hot-eject is enabled
> + // (PcdCpuMaxLogicalProcessorNumber > 1), and hot-eject is needed
> + // in this SMI exit (otherwise mCpuHotEjectData->Handler is not armed.)
> + //
> +
> + if (mCpuHotEjectData != NULL) {
> + CPU_HOT_EJECT_HANDLER Handler;
> +
> + //
> + // As the comment above mentions, mCpuHotEjectData->Handler might be
> + // written to on the BSP as part of handling of the CPU-ejection.
> + //
> + // We know that any initial assignment to mCpuHotEjectData->Handler
> + // (on the BSP, in the CpuHotplugMmi() context) is ordered-before the
> + // load below, since it is guaranteed to happen before the
> + // control-dependency of the BSP's SMI exit signal -- by way of a store
> + // to AllCpusInSync (on the BSP, in BspHandler()) and the corresponding
> + // AllCpusInSync loop (on the APs, in SmiRendezvous()) which depends on
> + // that store.
> + //
> + // This guarantees that these pieces of code can never execute
> + // simultaneously. In addition, we ensure that the following load is
> + // ordered-after the AllCpusInSync loop by using a MemoryFence() with
> + // acquire semantics.
> + //
> + MemoryFence();
> +
> + Handler = mCpuHotEjectData->Handler;
> +
> + if (Handler != NULL) {
> + Handler (CpuIndex);
> + }
> + }
> }
>
> /**
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v9 08/10] OvmfPkg/CpuHotplugSmm: add EjectCpu()
2021-03-12 6:26 ` [PATCH v9 08/10] OvmfPkg/CpuHotplugSmm: add EjectCpu() Ankur Arora
@ 2021-03-16 10:35 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2021-03-16 10:35 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jordan Justen, Ard Biesheuvel,
Aaron Young
On 03/12/21 07:26, Ankur Arora wrote:
> Add EjectCpu(), which handles the CPU ejection, and provides a holding
> area for said CPUs. It is called via SmmCpuFeaturesRendezvousExit(),
> at the tail end of the SMI handling.
>
> Also UnplugCpus() now stashes QEMU Selectors of CPUs which need to be
> ejected in CPU_HOT_EJECT_DATA.QemuSelectorMap. This is used by
> EjectCpu() to identify CPUs marked for ejection.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>
> Notes:
> Addresses the following comments from v8:
>
> (1) Fixup the coment about UnplugCpus() to reference stashing QEMU
> Cpu Selectors instead of APIC IDs.
> (2) s/ToUnplugSelector/ToUnplugSelectors/
> (3) Use plural for APIC ID in comment describing retval EFI_ALREADY_STARTED.
> (4) Fixup indentation in check against CPU_EJECT_QEMU_SELECTOR_INVALID.
> (5) Clarify comment:
> - // never match more than one APIC ID and by transitivity, more than one
> - // QemuSelector in a single invocation of UnplugCpus().
> + // never match more than one APIC ID -- nor, by transitivity, designate
> + // more than one QemuSelector -- in a single invocation of UnplugCpus().
> (6a) Remove unnecessary UINT64 cast for mCpuHotEjectData->QemuSelectorMap[ProcessorNum].
> (6b) Switch from 0x%Lx -> %Lu for QemuSelectorMap[ProcessorNum].
> (6c) Switch from 0x%Lx -> %u for QemuSelector
> (7) Switch to "return EFI_ALREADY_STARTED".
> (8a) Replace "QemuSelector 0x%Lx" with "QemuSelector %u".
> (8b) Replace the mCpuHotEjectData->QemuSelectorMap[ProcessorNum] argument
> with just QemuSelector in DEBUG call.
> (9) Clarify comment and make the language complementary to that in patch-7
> Explicitly mention release memory fence.
>
> OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 2 +
> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 154 ++++++++++++++++++++++++++++++--
> 2 files changed, 148 insertions(+), 8 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> index 04322b0d7855..ebcc7e2ac63a 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> @@ -40,6 +40,7 @@ [Packages]
> [LibraryClasses]
> BaseLib
> BaseMemoryLib
> + CpuLib
> DebugLib
> LocalApicLib
> MmServicesTableLib
> @@ -54,6 +55,7 @@ [Protocols]
>
> [Pcd]
> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## CONSUMES
> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## CONSUMES
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## CONSUMES
>
> [FeaturePcd]
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index 59f000eb7886..2eeb4567a262 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -10,10 +10,12 @@
> #include <IndustryStandard/Q35MchIch9.h> // ICH9_APM_CNT
> #include <IndustryStandard/QemuCpuHotplug.h> // QEMU_CPUHP_CMD_GET_PENDING
> #include <Library/BaseLib.h> // CpuDeadLoop()
> +#include <Library/CpuLib.h> // CpuSleep()
> #include <Library/DebugLib.h> // ASSERT()
> #include <Library/MmServicesTableLib.h> // gMmst
> #include <Library/PcdLib.h> // PcdGetBool()
> #include <Library/SafeIntLib.h> // SafeUintnSub()
> +#include <Pcd/CpuHotEjectData.h> // CPU_HOT_EJECT_DATA
> #include <Protocol/MmCpuIo.h> // EFI_MM_CPU_IO_PROTOCOL
> #include <Protocol/SmmCpuService.h> // EFI_SMM_CPU_SERVICE_PROTOCOL
> #include <Uefi/UefiBaseType.h> // EFI_STATUS
> @@ -32,11 +34,12 @@ STATIC EFI_MM_CPU_IO_PROTOCOL *mMmCpuIo;
> //
> STATIC EFI_SMM_CPU_SERVICE_PROTOCOL *mMmCpuService;
> //
> -// This structure is a communication side-channel between the
> +// These structures serve as communication side-channels between the
> // EFI_SMM_CPU_SERVICE_PROTOCOL consumer (i.e., this driver) and provider
> // (i.e., PiSmmCpuDxeSmm).
> //
> STATIC CPU_HOT_PLUG_DATA *mCpuHotPlugData;
> +STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData;
> //
> // SMRAM arrays for fetching the APIC IDs of processors with pending events (of
> // known event types), for the time of just one MMI.
> @@ -190,18 +193,71 @@ RevokeNewSlot:
> }
>
> /**
> + CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
> + on each CPU at exit from SMM.
> +
> + If, the executing CPU is not being ejected, nothing to be done.
> + If, the executing CPU is being ejected, wait in a halted loop
> + until ejected.
> +
> + @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM,
> + and will be used as an index into
> + CPU_HOT_EJECT_DATA->QemuSelectorMap. It is
> + identical to the processor handle number in
> + EFI_SMM_CPU_SERVICE_PROTOCOL.
> +**/
> +VOID
> +EFIAPI
> +EjectCpu (
> + IN UINTN ProcessorNum
> + )
> +{
> + UINT64 QemuSelector;
> +
> + QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum];
> + if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) {
> + return;
> + }
> +
> + //
> + // APs being unplugged get here from SmmCpuFeaturesRendezvousExit()
> + // after having been cleared to exit the SMI and so have no SMM
> + // processing remaining.
> + //
> + // Keep them penned here until the BSP tells QEMU to eject them.
> + //
> + for (;;) {
> + DisableInterrupts ();
> + CpuSleep ();
> + }
> +}
> +
> +/**
> Process to be hot-unplugged CPUs, per QemuCpuhpCollectApicIds().
>
> For each such CPU, report the CPU to PiSmmCpuDxeSmm via
> - EFI_SMM_CPU_SERVICE_PROTOCOL. If the to be hot-unplugged CPU is
> - unknown, skip it silently.
> + EFI_SMM_CPU_SERVICE_PROTOCOL and stash the QEMU Cpu Selectors for later
> + ejection. If the to be hot-unplugged CPU is unknown, skip it silently.
> +
> + Additonally, if we do stash any Cpu Selectors, also install a CPU eject
> + handler which would handle the ejection.
>
> @param[in] ToUnplugApicIds The APIC IDs of the CPUs that are about to be
> hot-unplugged.
>
> + @param[in] ToUnplugSelectors The QEMU Selectors of the CPUs that are about to
> + be hot-unplugged.
> +
> @param[in] ToUnplugCount The number of filled-in APIC IDs in
> ToUnplugApicIds.
>
> + @retval EFI_ALREADY_STARTED For the ProcessorNum that
> + EFI_SMM_CPU_SERVICE_PROTOCOL had assigned to
> + one of the APIC IDs in ToUnplugApicIds,
> + mCpuHotEjectData->QemuSelectorMap already has
> + the QemuSelector value stashed. (This should
> + never happen.)
> +
> @retval EFI_SUCCESS Known APIC IDs have been removed from SMM data
> structures.
>
> @@ -212,23 +268,36 @@ STATIC
> EFI_STATUS
> UnplugCpus (
> IN APIC_ID *ToUnplugApicIds,
> + IN UINT32 *ToUnplugSelectors,
> IN UINT32 ToUnplugCount
> )
> {
> EFI_STATUS Status;
> UINT32 ToUnplugIdx;
> + UINT32 EjectCount;
> UINTN ProcessorNum;
>
> ToUnplugIdx = 0;
> + EjectCount = 0;
> while (ToUnplugIdx < ToUnplugCount) {
> APIC_ID RemoveApicId;
> + UINT32 QemuSelector;
>
> RemoveApicId = ToUnplugApicIds[ToUnplugIdx];
> + QemuSelector = ToUnplugSelectors[ToUnplugIdx];
>
> //
> - // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use it to find
> - // the ProcessorNum for the APIC ID to be removed.
> + // mCpuHotPlugData->ApicId maps ProcessorNum -> ApicId. Use RemoveApicId
> + // to find the corresponding ProcessorNum for the CPU to be removed.
> //
> + // With this we can establish a 3 way mapping:
> + // APIC_ID -- ProcessorNum -- QemuSelector
> + //
> + // We stash the ProcessorNum -> QemuSelector mapping so it can later be
> + // used for CPU hot-eject in SmmCpuFeaturesRendezvousExit() context (where
> + // we only have ProcessorNum available.)
> + //
> +
> for (ProcessorNum = 0;
> ProcessorNum < mCpuHotPlugData->ArrayLength;
> ProcessorNum++) {
> @@ -257,11 +326,62 @@ UnplugCpus (
> return Status;
> }
>
> + if (mCpuHotEjectData->QemuSelectorMap[ProcessorNum] !=
> + CPU_EJECT_QEMU_SELECTOR_INVALID) {
> + //
> + // mCpuHotEjectData->QemuSelectorMap[ProcessorNum] is set to
> + // CPU_EJECT_QEMU_SELECTOR_INVALID when mCpuHotEjectData->QemuSelectorMap
> + // is allocated, and once the subject processsor is ejected.
> + //
> + // Additionally, mMmCpuService->RemoveProcessor(ProcessorNum) invalidates
> + // mCpuHotPlugData->ApicId[ProcessorNum], so a given ProcessorNum can
> + // never match more than one APIC ID -- nor, by transitivity, designate
> + // more than one QemuSelector -- in a single invocation of UnplugCpus().
> + //
> + DEBUG ((DEBUG_ERROR, "%a: ProcessorNum %Lu maps to QemuSelector %Lu, "
> + "cannot also map to %u\n", __FUNCTION__, (UINT64)ProcessorNum,
> + mCpuHotEjectData->QemuSelectorMap[ProcessorNum], QemuSelector));
> +
> + return EFI_ALREADY_STARTED;
> + }
> +
> + //
> + // Stash the QemuSelector so we can do the actual ejection later.
> + //
> + mCpuHotEjectData->QemuSelectorMap[ProcessorNum] = (UINT64)QemuSelector;
> +
> + DEBUG ((DEBUG_INFO, "%a: Started hot-unplug on ProcessorNum %Lu, APIC ID "
> + FMT_APIC_ID ", QemuSelector %u\n", __FUNCTION__, (UINT64)ProcessorNum,
> + RemoveApicId, QemuSelector));
> +
> + EjectCount++;
> ToUnplugIdx++;
> }
>
> + if (EjectCount != 0) {
> + //
> + // We have processors to be ejected; install the handler.
> + //
> + mCpuHotEjectData->Handler = EjectCpu;
> +
> + //
> + // The BSP and APs load mCpuHotEjectData->Handler, and
> + // mCpuHotEjectData->QemuSelectorMap[] in SmmCpuFeaturesRendezvousExit()
> + // and EjectCpu().
> + //
> + // The comment in SmmCpuFeaturesRendezvousExit() details how we use
> + // the AllCpusInSync control-dependency to ensure that any loads are
> + // ordered-after the stores above.
> + //
> + // Ensure that the stores above are ordered-before the AllCpusInSync store
> + // by using a MemoryFence() with release semantics.
> + //
> + MemoryFence ();
> + }
> +
> //
> - // We've removed this set of APIC IDs from SMM data structures.
> + // We've removed this set of APIC IDs from SMM data structures and
> + // have installed an ejection handler if needed.
> //
> return EFI_SUCCESS;
> }
> @@ -389,7 +509,7 @@ CpuHotplugMmi (
> }
>
> if (ToUnplugCount > 0) {
> - Status = UnplugCpus (mToUnplugApicIds, ToUnplugCount);
> + Status = UnplugCpus (mToUnplugApicIds, mToUnplugSelectors, ToUnplugCount);
> if (EFI_ERROR (Status)) {
> goto Fatal;
> }
> @@ -460,9 +580,14 @@ CpuHotplugEntry (
>
> //
> // Our DEPEX on EFI_SMM_CPU_SERVICE_PROTOCOL guarantees that PiSmmCpuDxeSmm
> - // has pointed PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM.
> + // has pointed:
> + // - PcdCpuHotPlugDataAddress to CPU_HOT_PLUG_DATA in SMRAM,
> + // - PcdCpuHotEjectDataAddress to CPU_HOT_EJECT_DATA in SMRAM, if the
> + // possible CPU count is greater than 1.
> //
> mCpuHotPlugData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotPlugDataAddress);
> + mCpuHotEjectData = (VOID *)(UINTN)PcdGet64 (PcdCpuHotEjectDataAddress);
> +
> if (mCpuHotPlugData == NULL) {
> Status = EFI_NOT_FOUND;
> DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_PLUG_DATA: %r\n", __FUNCTION__, Status));
> @@ -474,6 +599,19 @@ CpuHotplugEntry (
> if (mCpuHotPlugData->ArrayLength == 1) {
> return EFI_UNSUPPORTED;
> }
> +
> + if (mCpuHotEjectData == NULL) {
> + Status = EFI_NOT_FOUND;
> + } else if (mCpuHotPlugData->ArrayLength != mCpuHotEjectData->ArrayLength) {
> + Status = EFI_INVALID_PARAMETER;
> + } else {
> + Status = EFI_SUCCESS;
> + }
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "%a: CPU_HOT_EJECT_DATA: %r\n", __FUNCTION__, Status));
> + goto Fatal;
> + }
> +
> //
> // Allocate the data structures that depend on the possible CPU count.
> //
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v9 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject
2021-03-12 6:26 ` [PATCH v9 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject Ankur Arora
@ 2021-03-16 11:27 ` Laszlo Ersek
2021-03-16 12:52 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2021-03-16 11:27 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jordan Justen, Ard Biesheuvel,
Aaron Young
On 03/12/21 07:26, Ankur Arora wrote:
> Add logic in EjectCpu() to do the actual the CPU ejection.
>
> On the BSP, ejection happens by first selecting the CPU via
> its QemuSelector and then sending the QEMU "eject" command.
> QEMU in-turn signals the remote VCPU thread which context-switches
> the CPU out of the SMI handler.
>
> Meanwhile the CPU being ejected, waits around in its holding
> area until it is context-switched out. Note that it is possible
> that a slow CPU gets ejected before it reaches the wait loop.
> However, this would never happen before it has executed the
> "AllCpusInSync" loop in SmiRendezvous().
> It can mean that an ejected CPU does not execute code after
> that point but given that the CPU state will be destroyed by
> QEMU, the missed cleanup is no great loss.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>
> Notes:
> Addresses the following comments from v8:
>
> (1a,1b) CheckIfBsp(): get rid of ProcessorNum, document retval.
> (2) Line up IsBsp and ApicBaseMsr
> (3) s/ongoing SMI iteration/ongoing SMI/
> (4) Get rid of the allusions to alignment in the comment in EjectCpu().
> () Also reduce some of the repetitive detail in this comment.
> (5) EjectCpu(): reorder logic to cleanly separate the AP and the BSP portions.
> (6) Get rid of unnecessary MemoryFence() between QemuCpuhpWrite
> and clearing of the eject status.
> (7) Change type of QemuSelector to %Lu in DEBUG statement
> (8) Get rid of the repetitive comment in SmmCpuFeaturesRendezvousExit().
> The necessary parts of this got moved to patch-7.
>
> OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h | 1 +
> OvmfPkg/CpuHotplugSmm/CpuHotplug.c | 113 ++++++++++++++++++++--
> 2 files changed, 108 insertions(+), 6 deletions(-)
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
>
> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> index 2ec7a107a64d..d0e83102c13f 100644
> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> +++ b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> @@ -34,6 +34,7 @@
> #define QEMU_CPUHP_STAT_ENABLED BIT0
> #define QEMU_CPUHP_STAT_INSERT BIT1
> #define QEMU_CPUHP_STAT_REMOVE BIT2
> +#define QEMU_CPUHP_STAT_EJECT BIT3
> #define QEMU_CPUHP_STAT_FW_REMOVE BIT4
>
> #define QEMU_CPUHP_RW_CMD_DATA 0x8
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> index 2eeb4567a262..ae3abd525900 100644
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplug.c
> @@ -18,6 +18,7 @@
> #include <Pcd/CpuHotEjectData.h> // CPU_HOT_EJECT_DATA
> #include <Protocol/MmCpuIo.h> // EFI_MM_CPU_IO_PROTOCOL
> #include <Protocol/SmmCpuService.h> // EFI_SMM_CPU_SERVICE_PROTOCOL
> +#include <Register/Intel/ArchitecturalMsr.h> // MSR_IA32_APIC_BASE_REGISTER
> #include <Uefi/UefiBaseType.h> // EFI_STATUS
>
> #include "ApicId.h" // APIC_ID
> @@ -193,12 +194,40 @@ RevokeNewSlot:
> }
>
> /**
> + EjectCpu needs to know the BSP at SMI exit at a point when
> + some of the EFI_SMM_CPU_SERVICE_PROTOCOL state has been torn
> + down.
> + Reuse the logic from OvmfPkg::PlatformSmmBspElection() to
> + do that.
> +
> + @retval TRUE If the CPU executing this function is the BSP.
> +
> + @retval FALSE If the CPU executing this function is an AP.
> +**/
> +STATIC
> +BOOLEAN
> +CheckIfBsp (
> + VOID
> + )
> +{
> + MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr;
> + BOOLEAN IsBsp;
> +
> + ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
> + IsBsp = (BOOLEAN)(ApicBaseMsr.Bits.BSP == 1);
> + return IsBsp;
> +}
> +
> +/**
> CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
> on each CPU at exit from SMM.
>
> - If, the executing CPU is not being ejected, nothing to be done.
> + If, the executing CPU is neither the BSP, nor being ejected, nothing
> + to be done.
> If, the executing CPU is being ejected, wait in a halted loop
> until ejected.
> + If, the executing CPU is the BSP, set QEMU CPU status to eject
> + for CPUs being ejected.
>
> @param[in] ProcessorNum ProcessorNum denotes the CPU exiting SMM,
> and will be used as an index into
> @@ -214,6 +243,83 @@ EjectCpu (
> {
> UINT64 QemuSelector;
>
> + if (CheckIfBsp ()) {
> + UINT32 Idx;
> +
> + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
> + UINT64 QemuSelector;
> +
> + QemuSelector = mCpuHotEjectData->QemuSelectorMap[Idx];
> +
> + if (QemuSelector != CPU_EJECT_QEMU_SELECTOR_INVALID) {
> + //
> + // This to-be-ejected-CPU has already received the BSP's SMI exit
> + // signal and will execute SmmCpuFeaturesRendezvousExit()
> + // followed by this callback or is already penned in the
> + // CpuSleep() loop below.
> + //
> + // Tell QEMU to context-switch it out.
> + //
> + QemuCpuhpWriteCpuSelector (mMmCpuIo, (UINT32) QemuSelector);
> + QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECT);
> +
> + //
> + // Now that we've ejected the CPU corresponding to QemuSelectorMap[Idx],
> + // clear its eject status to ensure that an invalid future SMI does
> + // not end up trying a spurious eject or a newly hotplugged CPU does
> + // not get penned in the CpuSleep() loop.
> + //
> + // Note that the QemuCpuhpWriteCpuStatus() command above is a write to
> + // a different address space and uses the EFI_MM_CPU_IO_PROTOCOL.
> + //
> + // This means that we are guaranteed that the following assignment
> + // will not be reordered before the eject. And, so we can safely
> + // do this write here.
> + //
> + mCpuHotEjectData->QemuSelectorMap[Idx] =
> + CPU_EJECT_QEMU_SELECTOR_INVALID;
> +
> + DEBUG ((DEBUG_INFO, "%a: Unplugged ProcessorNum %u, "
> + "QemuSelector %Lu\n", __FUNCTION__, Idx, QemuSelector));
> + }
> + }
> +
> + //
> + // We are done until the next hot-unplug; clear the handler.
> + //
> + // mCpuHotEjectData->Handler is a NOP for any CPU not under ejection.
> + // So, once we are done with all the ejections, we can safely reset it
> + // here since any CPU dereferencing it would only see either the old
> + // or the new value (since it is aligned at a natural boundary.)
> + //
> + mCpuHotEjectData->Handler = NULL;
> + return;
> + }
> +
> + //
> + // Reached only on APs
> + //
> +
> + //
> + // mCpuHotEjectData->QemuSelectorMap[ProcessorNum] is updated
> + // on the BSP in the ongoing SMI at two places:
> + //
> + // - UnplugCpus() where the BSP determines if a CPU is under ejection
> + // or not. As a comment in UnplugCpus() at set-up, and in
> + // SmmCpuFeaturesRendezvousExit() where it is dereferenced describe,
> + // any such updates are guaranteed to be ordered-before the
> + // dereference below.
> + //
> + // - EjectCpu() on the BSP (above) updates QemuSelectorMap[ProcessorNum]
> + // for a CPU once it's ejected.
> + //
> + // The CPU under ejection: might be executing anywhere between the
> + // AllCpusInSync loop in SmiRendezvous(), to about to dereference
> + // QemuSelectorMap[ProcessorNum].
> + // As described in the comment above where we do the reset, this
> + // is not a problem since the ejected CPU never sees the after value.
> + // CPUs not-under ejection: never see any changes so they are fine.
> + //
> QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum];
> if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) {
> return;
> @@ -495,11 +601,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 = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v9 10/10] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
2021-03-12 6:26 ` [PATCH v9 10/10] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug Ankur Arora
@ 2021-03-16 11:40 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2021-03-16 11:40 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jordan Justen, Ard Biesheuvel,
Aaron Young
On 03/12/21 07:26, Ankur Arora wrote:
> Advertise OVMF support for CPU hot-unplug and negotiate it
> if QEMU requests the feature.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Aaron Young <aaron.young@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> ---
>
> Notes:
> Addresses the following comments from v8:
>
> (1) Remove inconsistent comment style (and stray newline) around the newly
> added ICH9_LPC_SMI_F_CPU_HOT_UNPLUG.
> (2) Remove spurious empty line.
>
> OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
> index c9d875543205..3e2e61e4dbd0 100644
> --- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
> +++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
> @@ -28,7 +28,12 @@
> // "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
> //
> #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_HOTPLUG upon hot-unplug", in the
> +// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
> +//
> +#define ICH9_LPC_SMI_F_CPU_HOT_UNPLUG BIT2
Per my v8 comment, this hunk is not supposed to change the amount of
vertical whitespace, in either direction. v8 added an empty line that
wasn't called for, and this version removes one, which is also unjustified.
But I'll fix that up when I merge this series (= soon).
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
> //
> // Provides a scratch buffer (allocated in EfiReservedMemoryType type memory)
> // for the S3 boot script fragment to write to and read from.
> @@ -112,7 +117,8 @@ NegotiateSmiFeatures (
> QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures);
>
> //
> - // We want broadcast SMI, SMI on CPU hotplug, and nothing else.
> + // We want broadcast SMI, SMI on CPU hotplug, SMI on CPU hot-unplug
> + // and nothing else.
> //
> RequestedFeaturesMask = ICH9_LPC_SMI_F_BROADCAST;
> if (!MemEncryptSevIsEnabled ()) {
> @@ -120,6 +126,7 @@ 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;
> QemuFwCfgSelectItem (mRequestedFeaturesItem);
> @@ -166,6 +173,13 @@ NegotiateSmiFeatures (
> __FUNCTION__));
> }
>
> + if ((mSmiFeatures & ICH9_LPC_SMI_F_CPU_HOT_UNPLUG) == 0) {
> + DEBUG ((DEBUG_INFO, "%a: CPU hot-unplug not negotiated\n", __FUNCTION__));
> + } else {
> + DEBUG ((DEBUG_INFO, "%a: CPU hot-unplug with SMI negotiated\n",
> + __FUNCTION__));
> + }
> +
> //
> // Negotiation successful (although we may not have gotten the optimal
> // feature set).
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH v9 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject
2021-03-12 6:26 ` [PATCH v9 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject Ankur Arora
2021-03-16 11:27 ` [edk2-devel] " Laszlo Ersek
@ 2021-03-16 12:52 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2021-03-16 12:52 UTC (permalink / raw)
To: devel, ankur.a.arora
Cc: imammedo, boris.ostrovsky, Jordan Justen, Ard Biesheuvel,
Aaron Young
On 03/12/21 07:26, Ankur Arora wrote:
> @@ -214,6 +243,83 @@ EjectCpu (
> {
> UINT64 QemuSelector;
>
> + if (CheckIfBsp ()) {
> + UINT32 Idx;
> +
> + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
> + UINT64 QemuSelector;
Visual Studio warns that the inner QemuSelector declaration shadows the
outer one. I'm going to remove the inner declaration, due to:
> +
> + QemuSelector = mCpuHotEjectData->QemuSelectorMap[Idx];
> +
> + if (QemuSelector != CPU_EJECT_QEMU_SELECTOR_INVALID) {
> + //
> + // This to-be-ejected-CPU has already received the BSP's SMI exit
> + // signal and will execute SmmCpuFeaturesRendezvousExit()
> + // followed by this callback or is already penned in the
> + // CpuSleep() loop below.
> + //
> + // Tell QEMU to context-switch it out.
> + //
> + QemuCpuhpWriteCpuSelector (mMmCpuIo, (UINT32) QemuSelector);
> + QemuCpuhpWriteCpuStatus (mMmCpuIo, QEMU_CPUHP_STAT_EJECT);
> +
> + //
> + // Now that we've ejected the CPU corresponding to QemuSelectorMap[Idx],
> + // clear its eject status to ensure that an invalid future SMI does
> + // not end up trying a spurious eject or a newly hotplugged CPU does
> + // not get penned in the CpuSleep() loop.
> + //
> + // Note that the QemuCpuhpWriteCpuStatus() command above is a write to
> + // a different address space and uses the EFI_MM_CPU_IO_PROTOCOL.
> + //
> + // This means that we are guaranteed that the following assignment
> + // will not be reordered before the eject. And, so we can safely
> + // do this write here.
> + //
> + mCpuHotEjectData->QemuSelectorMap[Idx] =
> + CPU_EJECT_QEMU_SELECTOR_INVALID;
> +
> + DEBUG ((DEBUG_INFO, "%a: Unplugged ProcessorNum %u, "
> + "QemuSelector %Lu\n", __FUNCTION__, Idx, QemuSelector));
> + }
> + }
> +
> + //
> + // We are done until the next hot-unplug; clear the handler.
> + //
> + // mCpuHotEjectData->Handler is a NOP for any CPU not under ejection.
> + // So, once we are done with all the ejections, we can safely reset it
> + // here since any CPU dereferencing it would only see either the old
> + // or the new value (since it is aligned at a natural boundary.)
> + //
> + mCpuHotEjectData->Handler = NULL;
> + return;
> + }
> +
> + //
> + // Reached only on APs
> + //
> +
> + //
> + // mCpuHotEjectData->QemuSelectorMap[ProcessorNum] is updated
> + // on the BSP in the ongoing SMI at two places:
> + //
> + // - UnplugCpus() where the BSP determines if a CPU is under ejection
> + // or not. As a comment in UnplugCpus() at set-up, and in
> + // SmmCpuFeaturesRendezvousExit() where it is dereferenced describe,
> + // any such updates are guaranteed to be ordered-before the
> + // dereference below.
> + //
> + // - EjectCpu() on the BSP (above) updates QemuSelectorMap[ProcessorNum]
> + // for a CPU once it's ejected.
> + //
> + // The CPU under ejection: might be executing anywhere between the
> + // AllCpusInSync loop in SmiRendezvous(), to about to dereference
> + // QemuSelectorMap[ProcessorNum].
> + // As described in the comment above where we do the reset, this
> + // is not a problem since the ejected CPU never sees the after value.
> + // CPUs not-under ejection: never see any changes so they are fine.
> + //
> QemuSelector = mCpuHotEjectData->QemuSelectorMap[ProcessorNum];
this reference being to the outer one.
Thanks
Laszlo
> if (QemuSelector == CPU_EJECT_QEMU_SELECTOR_INVALID) {
> return;
> @@ -495,11 +601,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 = ProcessHotAddedCpus (mPluggedApicIds, PluggedCount);
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 00/10] support CPU hot-unplug
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
` (9 preceding siblings ...)
2021-03-12 6:26 ` [PATCH v9 10/10] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug Ankur Arora
@ 2021-03-16 14:07 ` Laszlo Ersek
2021-03-16 17:56 ` Ankur Arora
10 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2021-03-16 14:07 UTC (permalink / raw)
To: Ankur Arora, devel; +Cc: imammedo, boris.ostrovsky
On 03/12/21 07:26, Ankur Arora wrote:
> Hi,
>
> This series adds OVMF support for CPU hot-unplug.
>
> QEMU secureboot hot-unplug logic corresponding to this is in upstream.
> Also posted here:
> https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
>
> Testing (with QEMU 5.2.50):
> - Stable with randomized CPU plug/unplug (guest maxcpus=33,128)
> - Synthetic tests with simultaneous multi CPU hot-unplug
>
> Also at:
> github.com/terminus/edk2/ hot-unplug-v9
>
> Changelog:
>
> v9:
> - Rebased on top of edd46cd407ea
> - Clarify comments around memory-barriers in patches 7, 8, 9
> - Address other review comments from v8
Merged as commit range 4751a48aeb2a..f3bdfc41866e, via
<https://github.com/tianocore/edk2/pull/1494>, with the following R-b's
/ comments / light tweaks added:
> 1: 4b4466ed7c44 = 1: 0cb242e33693 OvmfPkg/CpuHotplugSmm: refactor hotplug logic
> 2: 719828efdebe ! 2: a752dd07466c OvmfPkg/CpuHotplugSmm: collect hot-unplug events
> @@ -20,6 +20,7 @@
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> Message-Id: <20210312062656.2477515-3-ankur.a.arora@oracle.com>
> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> 3: 7d732efcb7af = 3: 2d92e052c3af OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
> 4: 64649eeeee42 = 4: 15e6ae8ea400 OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
> 5: 7190a5d134a7 ! 5: 8ade9d425a6e OvmfPkg: define CPU_HOT_EJECT_DATA
> @@ -15,6 +15,7 @@
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> Message-Id: <20210312062656.2477515-6-ankur.a.arora@oracle.com>
> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> --- a/OvmfPkg/OvmfPkg.dec
> 6: 1fd019dac476 ! 6: b6d5996706dd OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
> @@ -22,6 +22,7 @@
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> Message-Id: <20210312062656.2477515-7-ankur.a.arora@oracle.com>
> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> 7: c8758d9b9764 ! 7: af9c77e151fa OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler
> @@ -19,6 +19,7 @@
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> Message-Id: <20210312062656.2477515-8-ankur.a.arora@oracle.com>
> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> 8: 5ff8e8700678 ! 8: 30c69d2cfa63 OvmfPkg/CpuHotplugSmm: add EjectCpu()
> @@ -19,6 +19,7 @@
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> Message-Id: <20210312062656.2477515-9-ankur.a.arora@oracle.com>
> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
> 9: d6660f8f2f14 ! 9: f05328886302 OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject
> @@ -27,6 +27,9 @@
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> Message-Id: <20210312062656.2477515-10-ankur.a.arora@oracle.com>
> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> + [lersek@redhat.com: unneeded inner QemuSelector declaration in EjectCpu()
> + triggers VS warning #4456 (local variable shadowed); remove it]
>
> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
> @@ -102,8 +105,6 @@
> + UINT32 Idx;
> +
> + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
> -+ UINT64 QemuSelector;
> -+
> + QemuSelector = mCpuHotEjectData->QemuSelectorMap[Idx];
> +
> + if (QemuSelector != CPU_EJECT_QEMU_SELECTOR_INVALID) {
> 10: 0a9a1cd40b98 ! 10: f3bdfc41866e OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
> @@ -14,6 +14,9 @@
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
> Message-Id: <20210312062656.2477515-11-ankur.a.arora@oracle.com>
> + [lersek@redhat.com: preserve the empty line between the ICH9_LPC_SMI_F_*
> + group of macro definitions and the SCRATCH_BUFFER type definition]
> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
> --- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
> @@ -22,16 +25,15 @@
> // "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
> //
> #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_HOTPLUG upon hot-unplug", in the
> +// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
> +//
> +#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.
> @@
> QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures);
>
Thanks for the contribution,
Laszlo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v9 00/10] support CPU hot-unplug
2021-03-16 14:07 ` [PATCH v9 00/10] support " Laszlo Ersek
@ 2021-03-16 17:56 ` Ankur Arora
0 siblings, 0 replies; 21+ messages in thread
From: Ankur Arora @ 2021-03-16 17:56 UTC (permalink / raw)
To: Laszlo Ersek, devel; +Cc: imammedo, boris.ostrovsky
On 2021-03-16 7:07 a.m., Laszlo Ersek wrote:
> On 03/12/21 07:26, Ankur Arora wrote:
>> Hi,
>>
>> This series adds OVMF support for CPU hot-unplug.
>>
>> QEMU secureboot hot-unplug logic corresponding to this is in upstream.
>> Also posted here:
>> https://lore.kernel.org/qemu-devel/20201207140739.3829993-1-imammedo@redhat.com/
>>
>> Testing (with QEMU 5.2.50):
>> - Stable with randomized CPU plug/unplug (guest maxcpus=33,128)
>> - Synthetic tests with simultaneous multi CPU hot-unplug
>>
>> Also at:
>> github.com/terminus/edk2/ hot-unplug-v9
>>
>> Changelog:
>>
>> v9:
>> - Rebased on top of edd46cd407ea
>> - Clarify comments around memory-barriers in patches 7, 8, 9
>> - Address other review comments from v8
>
> Merged as commit range 4751a48aeb2a..f3bdfc41866e, via
> <https://github.com/tianocore/edk2/pull/1494>, with the following R-b's
> / comments / light tweaks added:
Awesome! Thanks Laszlo.
And, thanks especially for the thorough reviews.
Ankur
>
>> 1: 4b4466ed7c44 = 1: 0cb242e33693 OvmfPkg/CpuHotplugSmm: refactor hotplug logic
>> 2: 719828efdebe ! 2: a752dd07466c OvmfPkg/CpuHotplugSmm: collect hot-unplug events
>> @@ -20,6 +20,7 @@
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> Message-Id: <20210312062656.2477515-3-ankur.a.arora@oracle.com>
>> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
>> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
>> 3: 7d732efcb7af = 3: 2d92e052c3af OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper
>> 4: 64649eeeee42 = 4: 15e6ae8ea400 OvmfPkg/CpuHotplugSmm: introduce UnplugCpus()
>> 5: 7190a5d134a7 ! 5: 8ade9d425a6e OvmfPkg: define CPU_HOT_EJECT_DATA
>> @@ -15,6 +15,7 @@
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> Message-Id: <20210312062656.2477515-6-ankur.a.arora@oracle.com>
>> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> --- a/OvmfPkg/OvmfPkg.dec
>> 6: 1fd019dac476 ! 6: b6d5996706dd OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
>> @@ -22,6 +22,7 @@
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> Message-Id: <20210312062656.2477515-7-ankur.a.arora@oracle.com>
>> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> 7: c8758d9b9764 ! 7: af9c77e151fa OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler
>> @@ -19,6 +19,7 @@
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> Message-Id: <20210312062656.2477515-8-ankur.a.arora@oracle.com>
>> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> 8: 5ff8e8700678 ! 8: 30c69d2cfa63 OvmfPkg/CpuHotplugSmm: add EjectCpu()
>> @@ -19,6 +19,7 @@
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> Message-Id: <20210312062656.2477515-9-ankur.a.arora@oracle.com>
>> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
>> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
>> 9: d6660f8f2f14 ! 9: f05328886302 OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject
>> @@ -27,6 +27,9 @@
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> Message-Id: <20210312062656.2477515-10-ankur.a.arora@oracle.com>
>> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> + [lersek@redhat.com: unneeded inner QemuSelector declaration in EjectCpu()
>> + triggers VS warning #4456 (local variable shadowed); remove it]
>>
>> diff --git a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h b/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
>> --- a/OvmfPkg/Include/IndustryStandard/QemuCpuHotplug.h
>> @@ -102,8 +105,6 @@
>> + UINT32 Idx;
>> +
>> + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
>> -+ UINT64 QemuSelector;
>> -+
>> + QemuSelector = mCpuHotEjectData->QemuSelectorMap[Idx];
>> +
>> + if (QemuSelector != CPU_EJECT_QEMU_SELECTOR_INVALID) {
>> 10: 0a9a1cd40b98 ! 10: f3bdfc41866e OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug
>> @@ -14,6 +14,9 @@
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora@oracle.com>
>> Message-Id: <20210312062656.2477515-11-ankur.a.arora@oracle.com>
>> + [lersek@redhat.com: preserve the empty line between the ICH9_LPC_SMI_F_*
>> + group of macro definitions and the SCRATCH_BUFFER type definition]
>> + Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
>> --- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
>> @@ -22,16 +25,15 @@
>> // "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
>> //
>> #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_HOTPLUG upon hot-unplug", in the
>> +// "etc/smi/supported-features" and "etc/smi/requested-features" fw_cfg files.
>> +//
>> +#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.
>> @@
>> QemuFwCfgReadBytes (sizeof mSmiFeatures, &mSmiFeatures);
>>
>
> Thanks for the contribution,
> Laszlo
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-03-16 17:56 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-12 6:26 [PATCH v9 00/10] support CPU hot-unplug Ankur Arora
2021-03-12 6:26 ` [PATCH v9 01/10] OvmfPkg/CpuHotplugSmm: refactor hotplug logic Ankur Arora
2021-03-12 6:26 ` [PATCH v9 02/10] OvmfPkg/CpuHotplugSmm: collect hot-unplug events Ankur Arora
2021-03-16 9:56 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 03/10] OvmfPkg/CpuHotplugSmm: add Qemu Cpu Status helper Ankur Arora
2021-03-12 6:26 ` [PATCH v9 04/10] OvmfPkg/CpuHotplugSmm: introduce UnplugCpus() Ankur Arora
2021-03-12 6:26 ` [PATCH v9 05/10] OvmfPkg: define CPU_HOT_EJECT_DATA Ankur Arora
2021-03-16 10:03 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 06/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state Ankur Arora
2021-03-16 10:12 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 07/10] OvmfPkg/SmmCpuFeaturesLib: call CPU hot-eject handler Ankur Arora
2021-03-16 10:20 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 08/10] OvmfPkg/CpuHotplugSmm: add EjectCpu() Ankur Arora
2021-03-16 10:35 ` [edk2-devel] " Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 09/10] OvmfPkg/CpuHotplugSmm: do actual CPU hot-eject Ankur Arora
2021-03-16 11:27 ` [edk2-devel] " Laszlo Ersek
2021-03-16 12:52 ` Laszlo Ersek
2021-03-12 6:26 ` [PATCH v9 10/10] OvmfPkg/SmmControl2Dxe: negotiate CPU hot-unplug Ankur Arora
2021-03-16 11:40 ` [edk2-devel] " Laszlo Ersek
2021-03-16 14:07 ` [PATCH v9 00/10] support " Laszlo Ersek
2021-03-16 17:56 ` Ankur Arora
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox