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.

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>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/CpuHotEjectData.h>
 #include <PiSmm.h>
 #include <Register/Intel/SmramSaveStateMap.h>
 #include <Register/QemuSmramSaveStateMap.h>
@@ -171,6 +173,70 @@ SmmCpuFeaturesHookReturnFromSmm (
   return OriginalInstructionPointer;
 }
 
+GLOBAL_REMOVE_IF_UNREFERENCED
+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.
+**/
+STATIC
+VOID
+SmmCpuFeaturesSmmInitHotEject (
+  VOID
+  )
+{
+  UINT32      mMaxNumberOfCpus;
+  EFI_STATUS  Status;
+
+  if (!FeaturePcdGet (PcdCpuHotPlugSupport)) {
+    return;
+  }
+
+  // PcdCpuHotPlugSupport => PcdCpuMaxLogicalProcessorNumber
+  mMaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+
+  // No spare CPUs to hot-eject
+  if (mMaxNumberOfCpus == 1) {
+    return;
+  }
+
+  mCpuHotEjectData =
+    (CPU_HOT_EJECT_DATA *)AllocatePool (sizeof (*mCpuHotEjectData));
+  ASSERT (mCpuHotEjectData != NULL);
+
+  //
+  // 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);
+
+  //
+  // Expose address of CPU Hot eject Data structure
+  //
+  Status = PcdSet64S (PcdCpuHotEjectDataAddress,
+                      (UINT64)(VOID *)mCpuHotEjectData);
+  ASSERT_EFI_ERROR (Status);
+}
+
 /**
   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);
 }
 
 /**
-- 
2.9.3



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70872): https://edk2.groups.io/g/devel/message/70872
Mute This Topic: https://groups.io/mt/80199922/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71016): https://edk2.groups.io/g/devel/message/71016
Mute This Topic: https://groups.io/mt/80199922/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2021-02-01 5:36 a.m., Laszlo Ersek wrote:
> 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.

Will drop. As you said, it's a NOP for !MSVC.

Just as a sidenote, I do see two copies of the mCpuHotEjectData in
the PiSmmCpuSmm and CpuHotplugSmm maps (which makes sense, given
that both include SmmCpuFeaturesLib):

.bss.mCpuHotEjectData
0x0000000000017d60        0x8 /tmp/PiSmmCpuDxeSmm.dll.0k4hl8.ltrans1.ltrans.o

.bss.mCpuHotEjectData
0x0000000000005110        0x8 /tmp/CpuHotplugSmm.dll.ixiN9a.ltrans0.ltrans.o

I imagine they do get unified in the build process later, but that's the point
my understanding stops.

> 
> (4b) On the other hand, STATIC it should be.

Yeah, that 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.

Will fix, and thanks for the very careful reading.

Thanks
Ankur

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

So I faintly see why using SafeInt might be a good (edge cases, overflow
etc) but given that the SMRAM region (and max CPU count)  is much smaller
than any overflow case, could you expand on the logic behind using
SafeIntLib?

Either way, will fix.

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

Makes sense.

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

The original reason was that I did not want to expose the CPU_EJECT_* values
outside CpuHotplugSmm. That's not really true any more -- given that they are
in a common header.

Will move that logic here.

> 
> 
>> +  //
>> +  // 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, these are quite helpful. Will fix.

Ankur

> 
> Thanks!
> Laszlo
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71091): https://edk2.groups.io/g/devel/message/71091
Mute This Topic: https://groups.io/mt/80199922/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 02/03/21 06:20, Ankur Arora wrote:

> Just as a sidenote, I do see two copies of the mCpuHotEjectData in
> the PiSmmCpuSmm and CpuHotplugSmm maps (which makes sense, given
> that both include SmmCpuFeaturesLib):
> 
> .bss.mCpuHotEjectData
> 0x0000000000017d60        0x8
> /tmp/PiSmmCpuDxeSmm.dll.0k4hl8.ltrans1.ltrans.o
> 
> .bss.mCpuHotEjectData
> 0x0000000000005110        0x8
> /tmp/CpuHotplugSmm.dll.ixiN9a.ltrans0.ltrans.o
> 
> I imagine they do get unified in the build process later, but that's the
> point my understanding stops.

The PiSmmCpuDxeSmm binary has a (static global) variable called
"mCpuHotEjectData" via OVMF's SmmCpuFeaturesLib instance, from this
patch (patch#6).

The CpuHotplugSmm binary has a (static global) variable called
"mCpuHotEjectData" because the "CpuHotplug.c" source file defines that
variable, from patch#7. (CpuHotplugSmm does not consume the
SmmCpuFeaturesLib class -- it has no reason to.)

In other words, there's nothing common between the two variables, beyond
the name. If you rename the first to mCpuHotEjectData1, and the second
to mCpuHotEjectData2, just for the experiment's sake, nothing will break.

PiSmmCpuDxeSmm and CpuHotplugSmm never get unified in the build process;
they are independent binaries.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71113): https://edk2.groups.io/g/devel/message/71113
Mute This Topic: https://groups.io/mt/80199922/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 2021-02-03 12:36 p.m., Laszlo Ersek wrote:
> On 02/03/21 06:20, Ankur Arora wrote:
> 
>> Just as a sidenote, I do see two copies of the mCpuHotEjectData in
>> the PiSmmCpuSmm and CpuHotplugSmm maps (which makes sense, given
>> that both include SmmCpuFeaturesLib):
>>
>> .bss.mCpuHotEjectData
>> 0x0000000000017d60        0x8
>> /tmp/PiSmmCpuDxeSmm.dll.0k4hl8.ltrans1.ltrans.o
>>
>> .bss.mCpuHotEjectData
>> 0x0000000000005110        0x8
>> /tmp/CpuHotplugSmm.dll.ixiN9a.ltrans0.ltrans.o
>>
>> I imagine they do get unified in the build process later, but that's the
>> point my understanding stops.
> 
> The PiSmmCpuDxeSmm binary has a (static global) variable called
> "mCpuHotEjectData" via OVMF's SmmCpuFeaturesLib instance, from this
> patch (patch#6).
> 
> The CpuHotplugSmm binary has a (static global) variable called
> "mCpuHotEjectData" because the "CpuHotplug.c" source file defines that
> variable, from patch#7. (CpuHotplugSmm does not consume the
> SmmCpuFeaturesLib class -- it has no reason to.)
> 
> In other words, there's nothing common between the two variables, beyond
> the name. If you rename the first to mCpuHotEjectData1, and the second
> to mCpuHotEjectData2, just for the experiment's sake, nothing will break.

Yeah you are right. I completely forgot that I had defined mCpuHotEjectData
in CpuHotplug.c and then assumed that it was because we link with
SmmCpuFeaturesLib.a

Sorry for the confusion.

Ankur

> 
> PiSmmCpuDxeSmm and CpuHotplugSmm never get unified in the build process;
> they are independent binaries.
> 
> Thanks
> Laszlo
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71129): https://edk2.groups.io/g/devel/message/71129
Mute This Topic: https://groups.io/mt/80199922/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-