Re: [PATCH v6 6/9] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state

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

On 2021-02-01 5:36 a.m., Laszlo Ersek wrote:
> On 01/29/21 01:59, Ankur Arora wrote:
>> Init CPU_HOT_EJECT_DATA, which will be used to share CPU ejection state
>> between SmmCpuFeaturesLib (via PiSmmCpuDxeSmm) and CpuHotPlugSmm.
>> CpuHotplugSmm also sets up the CPU ejection mechanism via
>> CPU_HOT_EJECT_DATA->Handler.
>>
>> Additionally, expose CPU_HOT_EJECT_DATA via PcdCpuHotEjectDataAddress.
> 
> (1) Please mention that the logic is added to
> SmmCpuFeaturesSmmRelocationComplete(), and so it will run as part of the
> PiSmmCpuDxeSmm entry point function, PiCpuSmmEntry().
> 
> 
>>
>> 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>
>> ---
>>   .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  3 +
>>   .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  | 78 ++++++++++++++++++++++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> index 97a10afb6e27..32c63722ee62 100644
>> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> @@ -35,4 +35,7 @@ [LibraryClasses]
>>     UefiBootServicesTableLib
>>
>>   [Pcd]
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugSupport
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
>>     gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
>> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> index 7ef7ed98342e..33dd5da92432 100644
>> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> @@ -14,7 +14,9 @@
>>   #include <Library/PcdLib.h>
>>   #include <Library/SmmCpuFeaturesLib.h>
>>   #include <Library/SmmServicesTableLib.h>
>> +#include <Library/MemoryAllocationLib.h>
> 
> (2) The MemoryAllocationLib class is not listed in the [LibraryClasses]
> section of "OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf"; so
> please list it there as well.
> 
> (Please keep the [LibraryClasses] section in the INF file sorted, while
> at it.)
> 
> 
>>   #include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/CpuHotEjectData.h>
> 
> (3) This will change, once you move the header file under
> "OvmfPkg/Include/Pcd/"; either way, please keep the #include directives
> alphabetically sorted.
> 
> (Before this patch, the #include list is well-sorted.)
> 
> 
>>   #include <PiSmm.h>
>>   #include <Register/Intel/SmramSaveStateMap.h>
>>   #include <Register/QemuSmramSaveStateMap.h>
>> @@ -171,6 +173,70 @@ SmmCpuFeaturesHookReturnFromSmm (
>>     return OriginalInstructionPointer;
>>   }
>>
>> +GLOBAL_REMOVE_IF_UNREFERENCED
> 
> (4a) This is useless unless building with MSVC; I don't really remember
> introducing any instance of this macro myself, ever. I suggest dropping
> it.

Will drop. As you said, it's a NOP for !MSVC.

Just as a sidenote, I do see two copies of the mCpuHotEjectData in
the PiSmmCpuSmm and CpuHotplugSmm maps (which makes sense, given
that both include SmmCpuFeaturesLib):

.bss.mCpuHotEjectData
0x0000000000017d60        0x8 /tmp/PiSmmCpuDxeSmm.dll.0k4hl8.ltrans1.ltrans.o

.bss.mCpuHotEjectData
0x0000000000005110        0x8 /tmp/CpuHotplugSmm.dll.ixiN9a.ltrans0.ltrans.o

I imagine they do get unified in the build process later, but that's the point
my understanding stops.

> 
> (4b) On the other hand, STATIC it should be.

Yeah, that it should be.
  
> 
>> +CPU_HOT_EJECT_DATA *mCpuHotEjectData = NULL;
>> +
>> +/**
>> +  Initialize CpuHotEjectData if PcdCpuHotPlugSupport is enabled
>> +  and, if more than 1 CPU is configured.
>> +
>> +  Also sets up the corresponding PcdCpuHotEjectDataAddress.
>> +**/
> 
> (5) typo: s/CpuHotEjectData/mCpuHotEjectData/
> 
> 
> (6) As requested elsewhere under v6, there's no need to make this
> dependent on PcdCpuHotPlugSupport.
> 
> 
> (7) "Initialize" is imperative mood, "sets up" is indicative mood.
> Either one is fine, just be consistent please.

Will fix, and thanks for the very careful reading.

Thanks
Ankur

> 
> 
>> +STATIC
>> +VOID
>> +SmmCpuFeaturesSmmInitHotEject (
> 
> (8) This is a STATIC function (i.e., it has internal linkage); there's
> no need to complicate its name with the "SmmCpuFeatures..." prefix.
> 
> I suggest "InitCpuHotEjectData".
> 
> 
>> +  VOID
>> +  )
>> +{
>> +  UINT32      mMaxNumberOfCpus;
> 
> (9) This is a variable with automatic storage duration, so the "m"
> prefix is invalid.
> 
> 
>> +  EFI_STATUS  Status;
>> +
>> +  if (!FeaturePcdGet (PcdCpuHotPlugSupport)) {
>> +    return;
>> +  }
> 
> (10a) Please drop this, per prior discussion.
> 
> (10b) Please drop the PCD from the INF file too.
> 
> 
> (11) In the rest of this function, the comment style is incorrect in
> several spots. The idiomatic style is:
> 
>    //
>    // Blah.
>    //
> 
> I.e., normally we'd need leading and trailing empty comment lines.
> 
> *However*, most of those comments don't really explain much beyond
> what's emergent from the code anyway, to me anyway, thus, I would simply
> suggest dropping those comments.
> 
> 
>> +
>> +  // PcdCpuHotPlugSupport => PcdCpuMaxLogicalProcessorNumber
>> +  mMaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>> +
>> +  // No spare CPUs to hot-eject
>> +  if (mMaxNumberOfCpus == 1) {
>> +    return;
>> +  }
>> +
>> +  mCpuHotEjectData =
>> +    (CPU_HOT_EJECT_DATA *)AllocatePool (sizeof (*mCpuHotEjectData));
> 
> (12) The cast is superfluous (it only wastes screen real estate), as
> AllocatePool() returns (VOID *).
> 
> (Hopefully this will also let us avoid the somewhat awkward line break.)
> 
> 
>> +  ASSERT (mCpuHotEjectData != NULL);
> 
> (13) Here we need to hang harder than this -- even in a RELEASE build,
> in case AllocatePool() fails. The following should work:
> 
>    if (mCpuHotEjectData == NULL) {
>      ASSERT (mCpuHotEjectData != NULL);
>      CpuDeadLoop ();
>    }
> 
> I'll have another comment on this, below...
> 
> 
>> +
>> +  //
>> +  // Allocate buffer for pointers to array in CPU_HOT_EJECT_DATA.
>> +  //
>> +
>> +  // Revision
>> +  mCpuHotEjectData->Revision = CPU_HOT_EJECT_DATA_REVISION_1;
>> +
>> +  // Array Length of APIC ID
>> +  mCpuHotEjectData->ArrayLength = mMaxNumberOfCpus;
>> +
>> +  // CpuIndex -> APIC ID map
>> +  mCpuHotEjectData->ApicIdMap = (UINT64 *)AllocatePool
>> +      (sizeof (*mCpuHotEjectData->ApicIdMap) * mCpuHotEjectData->ArrayLength);
>> +
>> +  // Hot-eject handler
>> +  mCpuHotEjectData->Handler = NULL;
>> +
>> +  // Reserved
>> +  mCpuHotEjectData->Reserved = 0;
>> +
>> +  ASSERT (mCpuHotEjectData->ApicIdMap != NULL);
>> +
> 
> (14) I would propose the following:
> 
> (14a) Add SafeIntLib to both the #include directive list, and the
> [LibraryClasses] section in the INF file.
> 
> (14b) Use SafeIntLib functions to calculate the cumulative size for both
> CPU_HOT_EJECT_DATA, and the ApicIdMap placed right after it, in a local
> UINTN variable.

So I faintly see why using SafeInt might be a good (edge cases, overflow
etc) but given that the SMRAM region (and max CPU count)  is much smaller
than any overflow case, could you expand on the logic behind using
SafeIntLib?

Either way, will fix.

> 
> (14c) Use a single AllocatePool() call. This simplifies error handling
> -- you'll need just one instance of point (13) above --, plus it might
> even reduce SMRAM fragmentation a tiny bit.

Makes sense.

> 
> 
> (15) The following initialization logic, from patch v6 7/9
> ("OvmfPkg/CpuHotplugSmm: add CpuEject()"), belongs in the present patch,
> in my opinion:
> 
>      //
>      // For CPU ejection we need to map ProcessorNum -> APIC_ID. By the time
>      // we need the mapping, however, the Processor's APIC ID has already been
>      // removed from SMM data structures. So we will maintain a local map
>      // in mCpuHotEjectData->ApicIdMap.
>      //
>      for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
>        mCpuHotEjectData->ApicIdMap[Idx] = CPU_EJECT_INVALID;
>      }
> 
> If necessary, feel free to trim or reword the comment; I just think the
> data structure is not ready for publishing via the PCD until the
> "ApicIdMap" elements have been set to INVALID. (IOW, I'm kind of making
> a "RAII" argument.)

The original reason was that I did not want to expose the CPU_EJECT_* values
outside CpuHotplugSmm. That's not really true any more -- given that they are
in a common header.

Will move that logic here.

> 
> 
>> +  //
>> +  // Expose address of CPU Hot eject Data structure
>> +  //
> 
> (this comment is helpful, please keep it)
> 
> 
>> +  Status = PcdSet64S (PcdCpuHotEjectDataAddress,
>> +                      (UINT64)(VOID *)mCpuHotEjectData);
> 
> (16) Incorrect indentation on the second line.
> 
> 
> (17) The (UINT64) cast could trigger a warning in an IA32 build (casting
> between integer and pointer types should keep the width); please replace
> (UINT64) with (UINTN).
> 
> 
>> +  ASSERT_EFI_ERROR (Status);
> 
> (18) Given that we don't use the "Status" variable for anything else in
> this function, it's more idiomatic for "Status" to directly match the
> type returned by PcdSet64S() -- RETURN_STATUS. In such cases, we
> generally call the variable "PcdStatus". So the idea is
> 
>    RETURN_STATUS PcdStatus;
> 
>    PcdStatus = PcdSet64S (...);
>    ASSERT_RETURN_ERROR (PcdStatus);
> 
> RETURN_STATUS and EFI_STATUS behave identically in practice, but (again)
> if we use the status variable *only* for a PcdSet retval, then
> RETURN_STATUS is more elegant.
> 
> (RETURN_STATUS is basically a BASE type, while EFI_STATUS exists in
> connection with the PI and UEFI specs; IOW, RETURN_STATUS is
> semantically more primitive / foundational.)
> 
> The usage of an ASSERT is fine here, BTW; we don't expect this PcdSet
> call to ever fail.
> 
> 
>> +}
>> +
>>   /**
>>     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 +254,9 @@ SmmCpuFeaturesSmmRelocationComplete (
>>     UINTN      MapPagesBase;
>>     UINTN      MapPagesCount;
>>
>> +
>> +  SmmCpuFeaturesSmmInitHotEject ();
>> +
>>     if (!MemEncryptSevIsEnabled ()) {
>>       return;
>>     }
>> @@ -375,6 +444,15 @@ SmmCpuFeaturesRendezvousExit (
>>     IN UINTN  CpuIndex
>>     )
>>   {
>> +  //
>> +  // CPU Hot-eject not enabled.
>> +  //
>> +  if (mCpuHotEjectData == NULL ||
>> +      mCpuHotEjectData->Handler == NULL) {
>> +    return;
>> +  }
>> +
>> +  mCpuHotEjectData->Handler (CpuIndex);
>>   }
>>
>>   /**
>>
> 
> (19a) Please split the SmmCpuFeaturesRendezvousExit() change to a
> separate patch.
> 
> In particular, "init CPU ejection state" in the subject does not cover
> the SmmCpuFeaturesRendezvousExit() change at all.
> 
> (19b) In the separate patch's commit message, it would be nice to
> mention the *call site* of SmmCpuFeaturesRendezvousExit(), such as "one
> of the last actions in SmiRendezvous()".
> 
> 
> (20) I think we should refine the comment "CPU Hot-eject not enabled".
> That comment covers the (mCpuHotEjectData == NULL) case, yes; but it
> doesn't cover (mCpuHotEjectData->Handler == NULL).
> 
> The latter condition certainly seems valid, because:
> 
> - some SMIs are likely handled before the SMM driver dispatch reaches
>    the CpuHotplugSmm driver, and the latter gets a chance to set up the
>    callback, as a part of erecting the CPU hot-(un)plug support,
> 
> - and even after CpuHotplugSmm is loaded, an unplug request may never
>    happen.
> 
> However, we should document this particular state, with a dedicated
> comment -- perhaps just say, "hot-eject has not been requested yet".

Thanks, these are quite helpful. Will fix.

Ankur

> 
> Thanks!
> Laszlo
>