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>
---
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 000000000000..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
+ UINT32 ArrayLength; // 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <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> > --- > > 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 000000000000..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 ejection > + UINT32 ArrayLength; // Entries in the QemuSelectorMap (3) Please move "*QemuSelectorMap" and "ArrayLength" to the right, by 9 spaces, so that they line up with "Handler". Now... I realize that such an update would result in the awkwarly wide code snippet: > typedef struct { > volatile UINT64 *QemuSelectorMap; // Maps ProcessorNum -> QemuSelector > // for pending hot-ejects > volatile CPU_HOT_EJECT_HANDLER Handler; // Handler to do the CPU ejection > UINT32 ArrayLength; // Entries in the QemuSelectorMap > }; where it is not really possible to re-wrap the comments usefully. With that in mind, the following is likely the best approach: > typedef struct { > // > // Maps ProcessorNum -> QemuSelector for pending hot-ejects > // > volatile UINT64 *QemuSelectorMap; > // > // Handler to do the CPU ejection > // > volatile CPU_HOT_EJECT_HANDLER Handler; > // > // Entries in the QemuSelectorMap > // > UINT32 ArrayLength; > }; This way, you have ample room for the comments. Plus, the field names need not care about any visual alignment, because they are separated by the comments anyway. Thanks! Laszlo > +} CPU_HOT_EJECT_DATA; > + > +#endif // CPU_HOT_EJECT_DATA_H_ > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#71954): https://edk2.groups.io/g/devel/message/71954 Mute This Topic: https://groups.io/mt/80819860/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 <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> >> --- >> >> 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 000000000000..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 ejection >> + UINT32 ArrayLength; // Entries in the QemuSelectorMap > Acking the comments above. > (3) Please move "*QemuSelectorMap" and "ArrayLength" to the right, by 9 > spaces, so that they line up with "Handler". > > Now... I realize that such an update would result in the awkwarly wide > code snippet: > >> typedef struct { >> volatile UINT64 *QemuSelectorMap; // Maps ProcessorNum -> QemuSelector >> // for pending hot-ejects >> volatile CPU_HOT_EJECT_HANDLER Handler; // Handler to do the CPU ejection >> UINT32 ArrayLength; // Entries in the QemuSelectorMap >> }; > > where it is not really possible to re-wrap the comments usefully. With > that in mind, the following is likely the best approach: > >> typedef struct { >> // >> // Maps ProcessorNum -> QemuSelector for pending hot-ejects >> // >> volatile UINT64 *QemuSelectorMap; >> // >> // Handler to do the CPU ejection >> // >> volatile CPU_HOT_EJECT_HANDLER Handler; >> // >> // Entries in the QemuSelectorMap >> // >> UINT32 ArrayLength; >> }; > > This way, you have ample room for the comments. Plus, the field names > need not care about any visual alignment, because they are separated by > the comments anyway. Thanks for clarifying. I was wondering if this comment style is fine for data structures or not. Should have checked other code. Thanks Ankur > > Thanks! > Laszlo > >> +} CPU_HOT_EJECT_DATA; >> + >> +#endif // CPU_HOT_EJECT_DATA_H_ >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#72006): https://edk2.groups.io/g/devel/message/72006 Mute This Topic: https://groups.io/mt/80819860/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.