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

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

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.

(4b) On the other hand, STATIC 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.


> +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.

(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.


(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.)


> +  //
> +  // 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!
Laszlo