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

["Ankur Arora" <ankur.a.arora@oracle.com>]

On 2021-02-22 6:19 a.m., Laszlo Ersek wrote:
> On 02/22/21 08:19, 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 review comments:
>>       (1) Detail in commit message about context in which CPU_HOT_EJECT_DATA
>>        is inited.
>>       (2) Add in sorted order MemoryAllocationLib in LibraryClasses
>>       (3) Sort added includes in SmmCpuFeaturesLib.c
>>       (4a-4b) Fixup linkage directives for mCpuHotEjectData.
>>       (5) s/CpuHotEjectData/mCpuHotEjectData/
>>       (6,10a,10b) Remove dependence on PcdCpuHotPlugSupport
>>       (7) Make the tense structure consistent in block comment for
>>        InitCpuHotEject().
>>       (8) s/SmmCpuFeaturesSmmInitHotEject/InitCpuHotEject/
>>       (9) s/mMaxNumberOfCpus/MaxNumberOfCpus/
>>       (11) Remove a bunch of obvious comments.
>>       (14a,14b,14c) Use SafeUint functions and rework the allocation logic
>>        so we can just use a single allocation.
>>       (12) Remove the AllocatePool() cast.
>>       (13) Use a CpuDeadLoop() in case of failure; albeit via a goto, not
>>        inline.
>>       (15) Initialize the mCpuHotEjectData->QemuSelectorMap locally.
>>       (16) Fix indentation in PcdSet64S.
>>       (17) Change the cast in PcdSet64S() to UINTN.
>>       (18) Use RETURN_STATUS instead of EFI_STATUS.
>>       (19,20) Move the Handler logic in SmmCpuFeaturesRendezvousExit() into
>>        into a separate patch.
>>
>>   .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  4 +
>>   .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  | 92 ++++++++++++++++++++++
>>   2 files changed, 96 insertions(+)
> 
> Nice, I've only superficial comments:
> 
>>
>> 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..adbfc90ad46e 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,92 @@ 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          ArrayLen;
>> +  UINTN          BaseLen;
>> +  UINTN          TotalLen;
>> +  UINT32         Idx;
>> +  UINT32         MaxNumberOfCpus;
>> +  RETURN_STATUS  PcdStatus;
>> +
>> +  MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>> +
> 
> (1) This doesn't deserve an update in itself, but since I'll ask for
> some more below, I might as well name it: please remove the empty line
> here.
> 
>> +  if (MaxNumberOfCpus == 1) {
>> +    return;
>> +  }
>> +
>> +  //
>> +  // We want the following lay out for CPU_HOT_EJECT_DATA:
>> +  //  UINTN alignment:  CPU_HOT_EJECT_DATA
>> +  //                  --- padding if needed ---
>> +  //  UINT64 alignment:  CPU_HOT_EJECT_DATA->QemuSelectorMap[]
>> +  //
>> +  // Accordingly, we allocate:
>> +  //   sizeof(*mCpuHotEjectData) + (MaxNumberOfCpus *
>> +  //     sizeof(mCpuHotEjectData->QemuSelectorMap[0])).
>> +  // Add sizeof(UINT64) to use as padding if needed.
>> +  //
>> +
>> +  if (RETURN_ERROR (SafeUintnMult (sizeof (*mCpuHotEjectData), 1, &BaseLen)) ||
> 
> (2) This "multiplication" is somewhat awkward.
> 
> First, BaseLen is a UINTN, which always matches the return type of the
> sizeof operator, so a direct assignment would be OK.
> 
> But even that would be overkill -- I suggest removing the BaseLen
> variable, and just using "sizeof (*mCpuHotEjectData)" in its place.
> 
> 
>> +      RETURN_ERROR (SafeUintnMult (
>> +                      sizeof (mCpuHotEjectData->QemuSelectorMap[0]),
>> +                      MaxNumberOfCpus, &ArrayLen)) ||
>> +      RETURN_ERROR (SafeUintnAdd (BaseLen, ArrayLen, &TotalLen))||
> 
> (3) Missing space character before "||"
> 
> 
>> +      RETURN_ERROR (SafeUintnAdd (TotalLen, sizeof (UINT64), &TotalLen))) {
> 
> (4) So, I find the mixture of
> 
>    sizeof (UINT64)
> 
> and
> 
>    sizeof (mCpuHotEjectData->QemuSelectorMap[0])
> 
> confusing.
> 
> (And, this remark applies to both the code and the (otherwise very
> helpful!) comment above it.)
> 
> We use sizeof (UINT64) *because* that's the type of
> "mCpuHotEjectData->QemuSelectorMap[0]" -- we want to ensure natural
> alignment for the atomic accesses performed later.
> 
> So, given both kinds of sizeof expression stand for the same concept, we
> should use either "sizeof (UINT64)" exclusively, or "sizeof
> (mCpuHotEjectData->QemuSelectorMap[0])", exclusively.
> 
> I would vote "sizeof (UINT64)" here, because that could help us
> eliminate some of the unpleasant line breaks.
> 
> 
> (5) Not sure if I should obsess about this, but... an alignment to
> UINT64 may require up to (sizeof (UINT64) - 1) padding bytes. (sizeof
> (UINT64)) bytes are never needed, as that would imply the allocation is
> already correctly aligned -- but then we'd need 0 additional bytes.
> 
> 
> (6) So how about this formulation instead:
> 
>    if (RETURN_ERROR (SafeUintnMult (MaxNumberOfCpus, sizeof (UINT64), &Size)) ||
>        RETURN_ERROR (SafeUintnAdd (Size, sizeof (*mCpuHotEjectData), &Size)) ||
>        RETURN_ERROR (SafeUintnAdd (Size, sizeof (UINT64) - 1, &Size)) {
>      ...
>    }
> 
> The longest line (the first line) is 79 characters wide, and we only use
> one variable (called "Size").
> 
> If you don't like this variant, that's OK; but please at least unify the
> sizeof expressions (4).

I like this forumlation -- though I will get rid of the UINT64 -- I think
as you pointed out above the mixture is a little awkward.

Also acking comments above.
> 
> 
>> +    DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_EJECT_DATA\n", __FUNCTION__));
>> +    goto Fatal;
>> +  }
>> +
>> +  mCpuHotEjectData = AllocatePool (TotalLen);
>> +  if (mCpuHotEjectData == NULL) {
>> +    ASSERT (mCpuHotEjectData != NULL);
>> +    goto Fatal;
>> +  }
>> +
>> +  mCpuHotEjectData->Handler = NULL;
>> +  mCpuHotEjectData->ArrayLength = MaxNumberOfCpus;
>> +
>> +  mCpuHotEjectData->QemuSelectorMap = (void *)mCpuHotEjectData +
>> +                                        sizeof (*mCpuHotEjectData);
>> +  mCpuHotEjectData->QemuSelectorMap =
>> +    (void *)ALIGN_VALUE ((UINTN)mCpuHotEjectData->QemuSelectorMap,
>> +                           sizeof (UINT64));
> 
> (7) More idiomatic formulation, for setting the "QemuSelectorMap" field:
> 
>    mCpuHotEjectData->QemuSelectorMap = ALIGN_POINTER (mCpuHotEjectData + 1,
>                                          sizeof (UINT64));

Thanks. Yeah this is much better.

> 
>> +  //
>> +  // 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);
>> +  if (RETURN_ERROR (PcdStatus)) {
>> +    ASSERT_EFI_ERROR (PcdStatus);
> 
> (8) To be fully clean semantically, this should be
> ASSERT_RETURN_ERROR().

Yeah, that makes sense.
   
> ... Sorry about the bike-shedding; the purpose is two-fold:
> 
> - the new code should feel idiomatic,
> 
> - I hope you're going to stick around for further edk2 / OVMF
> development, and then these style hints shouldn't be useless in the long
> run.

Thanks. The project and working with you has decided been a mind
expanding experience. Hope to contribute in the future.

Ankur

> 
> Thanks!
> Laszlo
> 
> 
>> +    goto Fatal;
>> +  }
>> +
>> +  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 +277,9 @@ SmmCpuFeaturesSmmRelocationComplete (
>>     UINTN      MapPagesBase;
>>     UINTN      MapPagesCount;
>>
>> +
>> +  InitCpuHotEjectData ();
>> +
>>     if (!MemEncryptSevIsEnabled ()) {
>>       return;
>>     }
>>
>