Re: [edk2-devel] [PATCH v9 08/10] OvmfPkg/CpuHotplugSmm: add EjectCpu()

["Laszlo Ersek" <lersek@redhat.com>]

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.
>    //
>