[edk2-devel] [PATCH v6 5/9] OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA
Ankur Arora
ankur.a.arora at oracle.com
Tue Feb 2 06:15:55 UTC 2021
On 2021-01-31 8:53 p.m., Laszlo Ersek wrote:
> On 01/29/21 01:59, 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 at redhat.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Cc: Igor Mammedov <imammedo at redhat.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky at oracle.com>
>> Cc: Aaron Young <aaron.young at oracle.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
>> Signed-off-by: Ankur Arora <ankur.a.arora at oracle.com>
>> ---
>> OvmfPkg/OvmfPkg.dec | 10 +++++++++
>> OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf | 1 +
>> OvmfPkg/Include/Library/CpuHotEjectData.h | 35 +++++++++++++++++++++++++++++++
>> 3 files changed, 46 insertions(+)
>> create mode 100644 OvmfPkg/Include/Library/CpuHotEjectData.h
>>
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 4348bb45c64a..1a2debb821d7 100644
>> --- a/OvmfPkg/OvmfPkg.dec
>> +++ b/OvmfPkg/OvmfPkg.dec
>> @@ -106,6 +106,10 @@ [LibraryClasses]
>> #
>> XenPlatformLib|Include/Library/XenPlatformLib.h
>>
>> + ## @libraryclass Share CPU hot-eject state
>> + #
>> + CpuHotEjectData|Include/Library/CpuHotEjectData.h
>> +
>> [Guids]
>> gUefiOvmfPkgTokenSpaceGuid = {0x93bb96af, 0xb9f2, 0x4eb8, {0x94, 0x62, 0xe0, 0xba, 0x74, 0x56, 0x42, 0x36}}
>> gEfiXenInfoGuid = {0xd3b46f3b, 0xd441, 0x1244, {0x9a, 0x12, 0x0, 0x12, 0x27, 0x3f, 0xc1, 0x4d}}
>
> (1) Please drop this hunk -- the [LibraryClasses] section should not be
> modified, as we're not introducing a new library class.
>
>
>> @@ -352,6 +356,12 @@ [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 PiSmmCpuDxeSmm and
>> + # CpuHotplugSmm.
>
> (2) I suggest:
>
> ## This PCD adds a communication channel between OVMF's SmmCpuFeaturesLib
> # instance in PiSmmCpuDxeSmm, and CpuHotplugSmm.
>
>
>> + #
>> + # Only accessed if PcdCpuHotPlugSupport is TRUE
>
> (3) This statement is technically true, but I suggest dropping it. In my
> opinion, it is not useful (it's a superfluous statement). Here's why:
>
> - We set the "PcdCpuHotPlugSupport" feature flag to TRUE in the OVMF DSC
> files exactly when the SMM_REQUIRE feature test macro is set on the
> "build" command line.
>
> - The whole SMM infrastructure is included in the firmware binary
> exactly when SMM_REQUIRE is set.
>
> In other words, PcdCpuHotPlugSupport is *equivalent* with
> SmmCpuFeaturesLib, PiSmmCpuDxeSmm, and CpuHotplugSmm being included in
> the firmware binary.
>
> Given that the first comment already declares the PCD as an info channel
> between SmmCpuFeaturesLib (as built into PiSmmCpuDxeSmm) and
> CpuHotplugSmm, the second comment adds nothing.
That makes sense.
>
>
>> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46
>> +
>> [PcdsFeatureFlag]
>> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
>> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
>> diff --git a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
>> index 04322b0d7855..e08b572ef169 100644
>> --- a/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
>> +++ b/OvmfPkg/CpuHotplugSmm/CpuHotplugSmm.inf
>> @@ -54,6 +54,7 @@ [Protocols]
>>
>> [Pcd]
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress ## CONSUMES
>> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress ## CONSUMES
>> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase ## CONSUMES
>>
>> [FeaturePcd]
>
> (4) Please move this hunk to patch#7 (subject: "OvmfPkg/CpuHotplugSmm:
> add CpuEject()"). That's where CpuHotplugSmm first needs the new PCD.
>
>
>> diff --git a/OvmfPkg/Include/Library/CpuHotEjectData.h b/OvmfPkg/Include/Library/CpuHotEjectData.h
>> new file mode 100644
>> index 000000000000..b6fb629a1283
>> --- /dev/null
>> +++ b/OvmfPkg/Include/Library/CpuHotEjectData.h
>> @@ -0,0 +1,35 @@
>> +/** @file
>> + Definition for a CPU hot-eject state sharing structure.
>> +
>
> (5a) I suggest the following language:
>
> Definition of 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.
>
> (5b) Please append at least one more sentence to state the condition
> when the PCD is *not* NULL.
>
>
> (6) This new header file should be located at:
>
> OvmfPkg/Include/Pcd/CpuHotEjectData.h
Your explanation below makes sense. OvmfPkg/Include/Library did not
quite seem like the right place but I wasn't sure what would be
better.
Will fix.
>
> please.
>
> The (more or less) general rule is this:
>
> - if you have a macro definition or a structure type that is accessible
> through a Pcd, a Protocol, a Guid -- HOB, VenHw() devpath node etc --,
> a Library, a Register, etc,
>
> - and the Pcd, Protocol, Guid, Library etc in question is declared in
> "WhateverPkg/WhateverPkg.dec",
>
> - then the header file defining the structure or macro should be placed
> in the following directory (according to the access type):
>
> WhateverPkg/Include/Pcd/
> WhateverPkg/Include/Protocol/
> WhateverPkg/Include/Guid/
> WhateverPkg/Include/Library/
> WhateverPkg/Include/Register/
>
> Admittedly, while this rule is universally honored in edk2 in the
> Protocol, Guid, and Library cases, the Register case is somewhat less
> frequently followed, and the Pcd case is almost nonexistent. For
> example, "UefiCpuPkg/Include/CpuHotPlugData.h" itself does not follow
> the rule (no "Pcd" subdir). However, there are examples that do follow
> the rule:
>
> CryptoPkg/Include/Pcd/PcdCryptoServiceFamilyEnable.h
> RedfishPkg/Include/Pcd/RestExServiceDevicePath.h
>
>
>> + Copyright (C) 2021, Oracle Corporation.
>> +
>> + SPDX-License-Identifier: BSD-2-Clause-Patent
>> +**/
>> +
>> +#ifndef _CPU_HOT_EJECT_DATA_H_
>> +#define _CPU_HOT_EJECT_DATA_H_
>
> (7) Please use the following guard macro:
>
> CPU_HOT_EJECT_DATA_H_
>
> (i.e., please drop the leading underscore).
>
> Although the leading underscore is widely used in edk2, in include guard
> macros, it's a bad practice (it creates identifiers that are reserved by
> the C standard), so we should not introduce more of it.
>
>
>> +
>> +typedef
>> +VOID
>> +(EFIAPI *CPU_HOT_EJECT_FN)(
>
> (8) Please replace _FN with _HANDLER or _FUNCTION.
>
> In edk2, we tend to avoid abbreviations. (Yes, the practice has not
> entirely been consistent, and sometimes it's actually *annoying* that
> our type names are too long. But that's what we got.)
>
> ... _HANDLER would be better, as you call the related field "Handler" in
> the structure.
>
>
> (9) Missing space character before the last parenthesis on the line.
>
>
> (10) Please add a leading comment block on this function prototype.
> (Well, yes, I realize it is technically a *pointer* type, but still.)
>
> This is not just a formality; I'd really like the "ProcessorNum"
> parameter to be described, for example its relationship with the
> "ProcessorNumber" parameter of EFI_SMM_CPU_SERVICE_PROTOCOL member
> functions, and/or the "CPU_HOT_PLUG_DATA.ApicId" array.
>
>
>> + IN UINTN ProcessorNum
>> + );
>> +
>> +#define CPU_EJECT_INVALID (MAX_UINT64)
>> +#define CPU_EJECT_WORKER (MAX_UINT64-1)
>
> (11a) If these are meant as special values for the "ApicIdMap" array,
> then I'd suggest something like:
>
> CPU_EJECT_APIC_ID_INVALID
> CPU_EJECT_APIC_ID_WORKER
>
> (11b) Can you add a single-sentence comment to each macro? (Observe the
> comment style while at it, please.)
>
>
>> +
>> +#define CPU_HOT_EJECT_DATA_REVISION_1 0x00000001
>> +
>> +typedef struct {
>> + UINT32 Revision; // Used for version identification of
>> + // this structure
>
> (12) Please drop both the "CPU_HOT_EJECT_DATA_REVISION_1" macro and the
> "Revision" field.
>
> The "CPU_HOT_PLUG_DATA" structure, from
> "UefiCpuPkg/Include/CpuHotPlugData.h", is different. That structure is
> versioned because it establishes a communication channel between a core
> module (PiSmmCpuDxeSmm) and a platform module (such as
> OvmfPkg/CpuHotplugSmm); what's more, those modules could even be built
> separately, and be available in binary-only form.
>
> (Side note: we ignore "CPU_HOT_PLUG_DATA.Revision" in
> "OvmfPkg/CpuHotplugSmm" because the OVMF platforms exist in the exact
> same repository as PiSmmCpuDxeSmm, so we can keep them in sync. This is
> BTW one reason why I absolutely want OVMF to live in the core edk2
> repository. Anyway, digression ends.)
>
> However, the same versioning idea (or requirement) does not hold for the
> present use case. The new communication channel is between:
>
> - OVMF's SmmCpuFeaturesLib instance in PiSmmCpuDxeSmm,
> - and CpuHotplugSmm.
>
> Both of those are OVMF platform modules, and we never build one without
> building the other. (Put differently, we never build PiSmmCpuDxeSmm and
> CpuHotplugSmm separately, for any particular OVMF binary.)
>
> Thus, the "Revision" field is useless.
Agreed. I was unsure about adding this field -- but wasn't sure if there
might be situations when CpuHotplugSmm and PiSmmCpuDxeSmm might come
separately or not.
Thanks for clarifying it.
>
>
>> + UINT32 ArrayLength; // Entries in the ApicIdMap array
>> +
>> + UINT64 *ApicIdMap; // Pointer to CpuIndex->ApicId map for
>> + // pending hot-ejects
>
> (13a) "CpuIndex" is yet another name here; if you mean
> "ProcessorNum[ber]" -- see point (10) above --, then please use that
> word.
I did. Will fix.
>
> (13b) Also, the "->" arrow is a bit confusing (is "CpuIndex" a
> pointer???), so please either use " -> " (spaces on both sides) or write
> "ProcessorNumber-to-ApicId".
>
>
>> + CPU_HOT_EJECT_FN Handler; // Handler to do the CPU ejection
>> +
>> + UINT64 Reserved;
>
> (14) Please drop the "Reserved" field as well, with the justification
> given in (12).
>
>
>> +} CPU_HOT_EJECT_DATA;
>> +
>> +#endif /* _CPU_HOT_EJECT_DATA_H_ */
>>
>
> (15) Comment style is wrong; should be //.
>
> (I admit that you may find many examples for the wrong comment style,
> near such "#endif" directives, under OvmfPkg/Include; sorry about that.)
>
>
> (16) Please drop the leading underscore here too.
>
Will fix and acking all the other comments that I did not explicitly
address as well.
Thanks
Ankur
>
> I plan to continue the review either today, or sometime later this week.
>
> Thanks!
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71049): https://edk2.groups.io/g/devel/message/71049
Mute This Topic: https://groups.io/mt/80199916/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list