Re: [edk2-devel] [PATCH v9 06/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state

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

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;
>    }
>