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 <ler...@redhat.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
> Cc: Igor Mammedov <imamm...@redhat.com>
> Cc: Boris Ostrovsky <boris.ostrov...@oracle.com>
> Cc: Aaron Young <aaron.yo...@oracle.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora <ankur.a.ar...@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 <ler...@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.
> //
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72867): https://edk2.groups.io/g/devel/message/72867
Mute This Topic: https://groups.io/mt/81273611/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
