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 <ler...@redhat.com> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Ard Biesheuvel <ard.biesheu...@arm.com> > Cc: Igor Mammedov <imamm...@redhat.com> > Cc: Boris Ostrovsky <boris.ostrov...@oracle.com> > Cc: Aaron Young <aaron.yo...@oracle.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132 > Signed-off-by: Ankur Arora <ankur.a.ar...@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. > + 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 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. > + 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. (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. 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 (#71003): https://edk2.groups.io/g/devel/message/71003 Mute This Topic: https://groups.io/mt/80199916/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-