Re: [PATCH v6 5/9] OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA

["Laszlo Ersek" <lersek@redhat.com>]

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@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>
> ---
>  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