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 <ler...@redhat.com>
Cc: Jordan Justen <jordan.l.jus...@intel.com>
Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
Cc: Igor Mammedov <imamm...@redhat.com>
Cc: Boris Ostrovsky <boris.ostrov...@oracle.com>
Cc: Aaron Young <aaron.yo...@oracle.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3132
Signed-off-by: Ankur Arora <ankur.a.ar...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
