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
(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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
