Re: [edk2-devel] [PATCH v8 05/10] OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA

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

On 2021-02-22 5:06 a.m., Laszlo Ersek wrote:
> On 02/22/21 08:19, Ankur Arora wrote:
>> Define CPU_HOT_EJECT_DATA and add PCD PcdCpuHotEjectDataAddress, which
>> will be used to share CPU ejection state between OvmfPkg/CpuHotPlugSmm
>> and PiSmmCpuDxeSmm.
>>
>> 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 in v6:
>>       (1) Dropped modifications to LibraryClasses in OvmfPkg.dec
>>       (2,3) Cleanup comments around PCD PcdCpuHotEjectDataAddress.
>>       (4) Move PCD PcdCpuHotEjectDataAddress declaration in CpuHotplugSmm.inf
>>        to a patch-7 where it actually gets used.
>>       (5a,5b) Change the comment in the top block to use Laszlo's language.
>>        Also detail when the PCD would contain a valid value.
>>       (6) Move Library/CpuHotEjectData.h to Pcd/CpuHotEjectData.h
>>       (7,15,16) Fixup guard macro to be C namespace compliant. Also fixup the
>>        comment style near the endif guard.
>>       (8-10) Rename CPU_HOT_EJECT_FN to a more EDK2 compliant style. Also add
>>        a comment block and fix spacing.
>>       () Rename ApicIdMap -> QemuSelectorMap while keeping the type as UINT64.
>>        Related to a comment in patch-8 ("... add worker to do CPU ejection".)
>>       (11a,11b) Rename CPU_EJECT_INVALID to CPU_EJECT_QEMU_SELECTOR_INVALID
>>        and add a comment about it.
>>       () Remove CPU_EJECT_WORKER based on review comment on a patch 8.
>>       (12,14) Remove CPU_HOT_EJECT_DATA fields Revision and Reserved.
>>        Reorder CPU_HOT_EJECT_DATA to minimize internal padding
>>        and ensure elements are properly aligned.
>>       (13a,13b) Change CpuIndex->ApicId map to ProcessorNum -> QemuSelector
>>       () Make CPU_HOT_EJECT_HANDLER->Handler,
>>        CPU_HOT_EJECT_HANDLER->QemuSelectorMap volatile.
>>
>>   OvmfPkg/OvmfPkg.dec                   |  4 +++
>>   OvmfPkg/Include/Pcd/CpuHotEjectData.h | 52 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 56 insertions(+)
>>   create mode 100644 OvmfPkg/Include/Pcd/CpuHotEjectData.h
> 
> Very nice updates; I only have superficial comments this time:
> 
> (1) The patch -- correctly -- no longer touches CpuHotplugSmm, so please
> remove "/CpuHotplugSmm" from the subject line.
> 
> 
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 4348bb45c64a..9629707020ba 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -352,6 +352,10 @@ [PcdsDynamic, PcdsDynamicEx]
>>     #  This PCD is only accessed if PcdSmmSmramRequire is TRUE (see below).
>>     gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase|FALSE|BOOLEAN|0x34
>>
>> +  ## This PCD adds a communication channel between OVMF's SmmCpuFeaturesLib
>> +  #  instance in PiSmmCpuDxeSmm, and CpuHotplugSmm.
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46
>> +
>>   [PcdsFeatureFlag]
>>     gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
>>     gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
>> diff --git a/OvmfPkg/Include/Pcd/CpuHotEjectData.h b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
>> new file mode 100644
>> index 000000000000..024a92726869
>> --- /dev/null
>> +++ b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
>> @@ -0,0 +1,52 @@
>> +/** @file
>> +  Definition for the CPU_HOT_EJECT_DATA structure, which shares
>> +  CPU hot-eject state between OVMF's SmmCpuFeaturesLib instance in
>> +  PiSmmCpuDxeSmm, and CpuHotplugSmm.
>> +
>> +  CPU_HOT_EJECT_DATA is allocated in SMRAM, and pointed-to by
>> +  PcdCpuHotEjectDataAddress.
>> +
>> +  PcdCpuHotEjectDataAddress is valid when SMM_REQUIRE is TRUE
>> +  and MaxNumberOfCpus > 1.
> 
> (2) Looks good, thanks; please clarify it as follows though:
> 
> s/MaxNumberOfCpus/PcdCpuMaxLogicalProcessorNumber/
> 
> 
>> +
>> +  Copyright (C) 2021, Oracle Corporation.
>> +
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +#ifndef CPU_HOT_EJECT_DATA_H_
>> +#define CPU_HOT_EJECT_DATA_H_
>> +
>> +/**
>> +  CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
>> +  on each CPU at exit from SMM.
>> +
>> +  @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 in
>> +                               EFI_SMM_CPU_SERVICE_PROTOCOL.
>> +**/
>> +typedef
>> +VOID
>> +(EFIAPI *CPU_HOT_EJECT_HANDLER) (
>> +  IN UINTN  ProcessorNum
>> +  );
>> +
>> +//
>> +// CPU_EJECT_QEMU_SELECTOR_INVALID marks CPUs not being ejected in
>> +// CPU_HOT_EJECT_DATA->QemuSelectorMap.
>> +//
>> +// QEMU CPU Selector is UINT32, so we choose an invalid value larger
>> +// than that type.
>> +//
>> +#define CPU_EJECT_QEMU_SELECTOR_INVALID       (MAX_UINT64)
>> +
>> +typedef struct {
>> +  volatile UINT64       *QemuSelectorMap; // Maps ProcessorNum -> QemuSelector
>> +                                          // for pending hot-ejects
>> +  volatile CPU_HOT_EJECT_HANDLER Handler; // Handler to do the CPU ejection
>> +  UINT32                ArrayLength;      // Entries in the QemuSelectorMap
> 

Acking the comments above.

> (3) Please move "*QemuSelectorMap" and "ArrayLength" to the right, by 9
> spaces, so that they line up with "Handler".
> 
> Now... I realize that such an update would result in the awkwarly wide
> code snippet:
> 
>> typedef struct {
>>    volatile UINT64                *QemuSelectorMap; // Maps ProcessorNum -> QemuSelector
>>                                                     // for pending hot-ejects
>>    volatile CPU_HOT_EJECT_HANDLER Handler;          // Handler to do the CPU ejection
>>    UINT32                         ArrayLength;      // Entries in the QemuSelectorMap
>> };
> 
> where it is not really possible to re-wrap the comments usefully. With
> that in mind, the following is likely the best approach:
> 
>> typedef struct {
>>    //
>>    // Maps ProcessorNum -> QemuSelector for pending hot-ejects
>>    //
>>    volatile UINT64 *QemuSelectorMap;
>>    //
>>    // Handler to do the CPU ejection
>>    //
>>    volatile CPU_HOT_EJECT_HANDLER Handler;
>>    //
>>    // Entries in the QemuSelectorMap
>>    //
>>    UINT32 ArrayLength;
>> };
> 
> This way, you have ample room for the comments. Plus, the field names
> need not care about any visual alignment, because they are separated by
> the comments anyway.

Thanks for clarifying. I was wondering if this comment style is
fine for data structures or not. Should have checked other code.

Thanks
Ankur

> 
> Thanks!
> Laszlo
> 
>> +} CPU_HOT_EJECT_DATA;
>> +
>> +#endif // CPU_HOT_EJECT_DATA_H_
>>
>