Re: [edk2-devel] [PATCH v8 05/10] OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA

2021-02-22 Thread Ankur Arora 

On 2021-02-22 5:06 a.m., Laszlo Ersek wrote:

On 02/22/21 08:19, 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 
Cc: Jordan Justen 
Cc: Ard Biesheuvel 
Cc: Igor Mammedov 
Cc: Boris Ostrovsky 
Cc: Aaron Young 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora 
---

Notes:
 Addresses the following review comments in v6:
  (1) Dropped modifications to LibraryClasses in OvmfPkg.dec
  (2,3) Cleanup comments around PCD PcdCpuHotEjectDataAddress.
  (4) Move PCD PcdCpuHotEjectDataAddress declaration in CpuHotplugSmm.inf
   to a patch-7 where it actually gets used.
  (5a,5b) Change the comment in the top block to use Laszlo's language.
   Also detail when the PCD would contain a valid value.
  (6) Move Library/CpuHotEjectData.h to Pcd/CpuHotEjectData.h
  (7,15,16) Fixup guard macro to be C namespace compliant. Also fixup the
   comment style near the endif guard.
  (8-10) Rename CPU_HOT_EJECT_FN to a more EDK2 compliant style. Also add
   a comment block and fix spacing.
  () Rename ApicIdMap -> QemuSelectorMap while keeping the type as UINT64.
   Related to a comment in patch-8 ("... add worker to do CPU ejection".)
  (11a,11b) Rename CPU_EJECT_INVALID to CPU_EJECT_QEMU_SELECTOR_INVALID
   and add a comment about it.
  () Remove CPU_EJECT_WORKER based on review comment on a patch 8.
  (12,14) Remove CPU_HOT_EJECT_DATA fields Revision and Reserved.
   Reorder CPU_HOT_EJECT_DATA to minimize internal padding
   and ensure elements are properly aligned.
  (13a,13b) Change CpuIndex->ApicId map to ProcessorNum -> QemuSelector
  () Make CPU_HOT_EJECT_HANDLER->Handler,
   CPU_HOT_EJECT_HANDLER->QemuSelectorMap volatile.

  OvmfPkg/OvmfPkg.dec   |  4 +++
  OvmfPkg/Include/Pcd/CpuHotEjectData.h | 52 +++
  2 files changed, 56 insertions(+)
  create mode 100644 OvmfPkg/Include/Pcd/CpuHotEjectData.h


Very nice updates; I only have superficial comments this time:

(1) The patch -- correctly -- no longer touches CpuHotplugSmm, so please
remove "/CpuHotplugSmm" from the subject line.




diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4348bb45c64a..9629707020ba 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -352,6 +352,10 @@ [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 OVMF's SmmCpuFeaturesLib
+  #  instance in PiSmmCpuDxeSmm, and CpuHotplugSmm.
+  gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46
+
  [PcdsFeatureFlag]
gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c

gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
diff --git a/OvmfPkg/Include/Pcd/CpuHotEjectData.h 
b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
new file mode 100644
index ..024a92726869
--- /dev/null
+++ b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
@@ -0,0 +1,52 @@
+/** @file
+  Definition for 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.
+
+  PcdCpuHotEjectDataAddress is valid when SMM_REQUIRE is TRUE
+  and MaxNumberOfCpus > 1.


(2) Looks good, thanks; please clarify it as follows though:

s/MaxNumberOfCpus/PcdCpuMaxLogicalProcessorNumber/



+
+  Copyright (C) 2021, Oracle Corporation.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef CPU_HOT_EJECT_DATA_H_
+#define CPU_HOT_EJECT_DATA_H_
+
+/**
+  CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
+  on each CPU at exit from SMM.
+
+  @param[in] ProcessorNum  ProcessorNum denotes the CPU exiting SMM,
+   and will be used as an index into
+   CPU_HOT_EJECT_DATA->QemuSelectorMap. It is
+   identical to the processor handle in
+   EFI_SMM_CPU_SERVICE_PROTOCOL.
+**/
+typedef
+VOID
+(EFIAPI *CPU_HOT_EJECT_HANDLER) (
+  IN UINTN  ProcessorNum
+  );
+
+//
+// CPU_EJECT_QEMU_SELECTOR_INVALID marks CPUs not being ejected in
+// CPU_HOT_EJECT_DATA->QemuSelectorMap.
+//
+// QEMU CPU Selector is UINT32, so we choose an invalid value larger
+// than that type.
+//
+#define CPU_EJECT_QEMU_SELECTOR_INVALID   (MAX_UINT64)
+
+typedef struct {
+  volatile UINT64   *QemuSelectorMap; // Maps ProcessorNum -> QemuSelector
+  // for pending hot-ejects
+  volatile CPU_HOT_EJECT_HANDLER Handler; // Handler to do the CPU

Re: [edk2-devel] [PATCH v8 05/10] OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA

2021-02-22 Thread Laszlo Ersek 
On 02/22/21 08:19, 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 
> Cc: Jordan Justen 
> Cc: Ard Biesheuvel 
> Cc: Igor Mammedov 
> Cc: Boris Ostrovsky 
> Cc: Aaron Young 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
> Signed-off-by: Ankur Arora 
> ---
>
> Notes:
> Addresses the following review comments in v6:
>  (1) Dropped modifications to LibraryClasses in OvmfPkg.dec
>  (2,3) Cleanup comments around PCD PcdCpuHotEjectDataAddress.
>  (4) Move PCD PcdCpuHotEjectDataAddress declaration in CpuHotplugSmm.inf
>   to a patch-7 where it actually gets used.
>  (5a,5b) Change the comment in the top block to use Laszlo's language.
>   Also detail when the PCD would contain a valid value.
>  (6) Move Library/CpuHotEjectData.h to Pcd/CpuHotEjectData.h
>  (7,15,16) Fixup guard macro to be C namespace compliant. Also fixup the
>   comment style near the endif guard.
>  (8-10) Rename CPU_HOT_EJECT_FN to a more EDK2 compliant style. Also add
>   a comment block and fix spacing.
>  () Rename ApicIdMap -> QemuSelectorMap while keeping the type as UINT64.
>   Related to a comment in patch-8 ("... add worker to do CPU ejection".)
>  (11a,11b) Rename CPU_EJECT_INVALID to CPU_EJECT_QEMU_SELECTOR_INVALID
>   and add a comment about it.
>  () Remove CPU_EJECT_WORKER based on review comment on a patch 8.
>  (12,14) Remove CPU_HOT_EJECT_DATA fields Revision and Reserved.
>   Reorder CPU_HOT_EJECT_DATA to minimize internal padding
>   and ensure elements are properly aligned.
>  (13a,13b) Change CpuIndex->ApicId map to ProcessorNum -> QemuSelector
>  () Make CPU_HOT_EJECT_HANDLER->Handler,
>   CPU_HOT_EJECT_HANDLER->QemuSelectorMap volatile.
>
>  OvmfPkg/OvmfPkg.dec   |  4 +++
>  OvmfPkg/Include/Pcd/CpuHotEjectData.h | 52 
> +++
>  2 files changed, 56 insertions(+)
>  create mode 100644 OvmfPkg/Include/Pcd/CpuHotEjectData.h

Very nice updates; I only have superficial comments this time:

(1) The patch -- correctly -- no longer touches CpuHotplugSmm, so please
remove "/CpuHotplugSmm" from the subject line.


>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 4348bb45c64a..9629707020ba 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -352,6 +352,10 @@ [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 OVMF's SmmCpuFeaturesLib
> +  #  instance in PiSmmCpuDxeSmm, and CpuHotplugSmm.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46
> +
>  [PcdsFeatureFlag]
>gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
>
> gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
> diff --git a/OvmfPkg/Include/Pcd/CpuHotEjectData.h 
> b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
> new file mode 100644
> index ..024a92726869
> --- /dev/null
> +++ b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
> @@ -0,0 +1,52 @@
> +/** @file
> +  Definition for 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.
> +
> +  PcdCpuHotEjectDataAddress is valid when SMM_REQUIRE is TRUE
> +  and MaxNumberOfCpus > 1.

(2) Looks good, thanks; please clarify it as follows though:

s/MaxNumberOfCpus/PcdCpuMaxLogicalProcessorNumber/


> +
> +  Copyright (C) 2021, Oracle Corporation.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef CPU_HOT_EJECT_DATA_H_
> +#define CPU_HOT_EJECT_DATA_H_
> +
> +/**
> +  CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
> +  on each CPU at exit from SMM.
> +
> +  @param[in] ProcessorNum  ProcessorNum denotes the CPU exiting SMM,
> +   and will be used as an index into
> +   CPU_HOT_EJECT_DATA->QemuSelectorMap. It is
> +   identical to the processor handle in
> +   EFI_SMM_CPU_SERVICE_PROTOCOL.
> +**/
> +typedef
> +VOID
> +(EFIAPI *CPU_HOT_EJECT_HANDLER) (
> +  IN UINTN  ProcessorNum
> +  );
> +
> +//
> +// CPU_EJECT_QEMU_SELECTOR_INVALID marks CPUs not being ejected in
> +// CPU_HOT_EJECT_DATA->QemuSelectorMap.
> +//
> +// QEMU CPU Selector is UINT32, so we choose an invalid value larger
> +// than that type.
> +//
> +#define CPU_EJECT_QEMU_SELECTOR_INVALID   (MAX_UINT64)
> +
> +typedef struct {
> +  volatile UINT64   *QemuSelectorMap; // Maps ProcessorNum -> 
> QemuSelector
>

[edk2-devel] [PATCH v8 05/10] OvmfPkg/CpuHotplugSmm: define CPU_HOT_EJECT_DATA

2021-02-21 Thread Ankur Arora 
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 
Cc: Jordan Justen 
Cc: Ard Biesheuvel 
Cc: Igor Mammedov 
Cc: Boris Ostrovsky 
Cc: Aaron Young 
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora 
---

Notes:
Addresses the following review comments in v6:
 (1) Dropped modifications to LibraryClasses in OvmfPkg.dec
 (2,3) Cleanup comments around PCD PcdCpuHotEjectDataAddress.
 (4) Move PCD PcdCpuHotEjectDataAddress declaration in CpuHotplugSmm.inf
  to a patch-7 where it actually gets used.
 (5a,5b) Change the comment in the top block to use Laszlo's language.
  Also detail when the PCD would contain a valid value.
 (6) Move Library/CpuHotEjectData.h to Pcd/CpuHotEjectData.h
 (7,15,16) Fixup guard macro to be C namespace compliant. Also fixup the
  comment style near the endif guard.
 (8-10) Rename CPU_HOT_EJECT_FN to a more EDK2 compliant style. Also add
  a comment block and fix spacing.
 () Rename ApicIdMap -> QemuSelectorMap while keeping the type as UINT64.
  Related to a comment in patch-8 ("... add worker to do CPU ejection".)
 (11a,11b) Rename CPU_EJECT_INVALID to CPU_EJECT_QEMU_SELECTOR_INVALID
  and add a comment about it.
 () Remove CPU_EJECT_WORKER based on review comment on a patch 8.
 (12,14) Remove CPU_HOT_EJECT_DATA fields Revision and Reserved.
  Reorder CPU_HOT_EJECT_DATA to minimize internal padding
  and ensure elements are properly aligned.
 (13a,13b) Change CpuIndex->ApicId map to ProcessorNum -> QemuSelector
 () Make CPU_HOT_EJECT_HANDLER->Handler,
  CPU_HOT_EJECT_HANDLER->QemuSelectorMap volatile.

 OvmfPkg/OvmfPkg.dec   |  4 +++
 OvmfPkg/Include/Pcd/CpuHotEjectData.h | 52 +++
 2 files changed, 56 insertions(+)
 create mode 100644 OvmfPkg/Include/Pcd/CpuHotEjectData.h

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 4348bb45c64a..9629707020ba 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -352,6 +352,10 @@ [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 OVMF's SmmCpuFeaturesLib
+  #  instance in PiSmmCpuDxeSmm, and CpuHotplugSmm.
+  gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress|0|UINT64|0x46
+
 [PcdsFeatureFlag]
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderPciTranslation|TRUE|BOOLEAN|0x1c
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuBootOrderMmioTranslation|FALSE|BOOLEAN|0x1d
diff --git a/OvmfPkg/Include/Pcd/CpuHotEjectData.h 
b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
new file mode 100644
index ..024a92726869
--- /dev/null
+++ b/OvmfPkg/Include/Pcd/CpuHotEjectData.h
@@ -0,0 +1,52 @@
+/** @file
+  Definition for 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.
+
+  PcdCpuHotEjectDataAddress is valid when SMM_REQUIRE is TRUE
+  and MaxNumberOfCpus > 1.
+
+  Copyright (C) 2021, Oracle Corporation.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef CPU_HOT_EJECT_DATA_H_
+#define CPU_HOT_EJECT_DATA_H_
+
+/**
+  CPU Hot-eject handler, called from SmmCpuFeaturesRendezvousExit()
+  on each CPU at exit from SMM.
+
+  @param[in] ProcessorNum  ProcessorNum denotes the CPU exiting SMM,
+   and will be used as an index into
+   CPU_HOT_EJECT_DATA->QemuSelectorMap. It is
+   identical to the processor handle in
+   EFI_SMM_CPU_SERVICE_PROTOCOL.
+**/
+typedef
+VOID
+(EFIAPI *CPU_HOT_EJECT_HANDLER) (
+  IN UINTN  ProcessorNum
+  );
+
+//
+// CPU_EJECT_QEMU_SELECTOR_INVALID marks CPUs not being ejected in
+// CPU_HOT_EJECT_DATA->QemuSelectorMap.
+//
+// QEMU CPU Selector is UINT32, so we choose an invalid value larger
+// than that type.
+//
+#define CPU_EJECT_QEMU_SELECTOR_INVALID   (MAX_UINT64)
+
+typedef struct {
+  volatile UINT64   *QemuSelectorMap; // Maps ProcessorNum -> QemuSelector
+  // for pending hot-ejects
+  volatile CPU_HOT_EJECT_HANDLER Handler; // Handler to do the CPU ejection
+  UINT32ArrayLength;  // Entries in the QemuSelectorMap
+} CPU_HOT_EJECT_DATA;
+
+#endif // CPU_HOT_EJECT_DATA_H_
-- 
2.9.3



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71916): https://edk2.groups.io/g/devel/message/71916
Mute This Topic: https://groups.io/mt/80819860/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: