Init CPU_HOT_EJECT_DATA, which will be used to share CPU ejection
state between SmmCpuFeaturesLib (via PiSmmCpuDxeSmm) and CpuHotPlugSmm.
The init happens via SmmCpuFeaturesSmmRelocationComplete(), and so it
will run as part of the PiSmmCpuDxeSmm entry point function,
PiCpuSmmEntry(). Once inited, CPU_HOT_EJECT_DATA is exposed via
PcdCpuHotEjectDataAddress.
The CPU hot-eject handler (CPU_HOT_EJECT_DATA->Handler) is setup when
there is an ejection request via CpuHotplugSmm.
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:
(1) Detail in commit message about context in which CPU_HOT_EJECT_DATA
is inited.
(2) Add in sorted order MemoryAllocationLib in LibraryClasses
(3) Sort added includes in SmmCpuFeaturesLib.c
(4a-4b) Fixup linkage directives for mCpuHotEjectData.
(5) s/CpuHotEjectData/mCpuHotEjectData/
(6,10a,10b) Remove dependence on PcdCpuHotPlugSupport
(7) Make the tense structure consistent in block comment for
InitCpuHotEject().
(8) s/SmmCpuFeaturesSmmInitHotEject/InitCpuHotEject/
(9) s/mMaxNumberOfCpus/MaxNumberOfCpus/
(11) Remove a bunch of obvious comments.
(14a,14b,14c) Use SafeUint functions and rework the allocation logic
so we can just use a single allocation.
(12) Remove the AllocatePool() cast.
(13) Use a CpuDeadLoop() in case of failure; albeit via a goto, not
inline.
(15) Initialize the mCpuHotEjectData->QemuSelectorMap locally.
(16) Fix indentation in PcdSet64S.
(17) Change the cast in PcdSet64S() to UINTN.
(18) Use RETURN_STATUS instead of EFI_STATUS.
(19,20) Move the Handler logic in SmmCpuFeaturesRendezvousExit() into
into a separate patch.
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
.../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 92 ++++++++++++++++++++++
2 files changed, 96 insertions(+)
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 97a10afb6e27..8a426a4c10fb 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -30,9 +30,13 @@ [LibraryClasses]
BaseMemoryLib
DebugLib
MemEncryptSevLib
+ MemoryAllocationLib
PcdLib
+ SafeIntLib
SmmServicesTableLib
UefiBootServicesTableLib
[Pcd]
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
+ gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 7ef7ed98342e..adbfc90ad46e 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -11,10 +11,13 @@
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/MemEncryptSevLib.h>
+#include <Library/MemoryAllocationLib.h>
#include <Library/PcdLib.h>
+#include <Library/SafeIntLib.h>
#include <Library/SmmCpuFeaturesLib.h>
#include <Library/SmmServicesTableLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Pcd/CpuHotEjectData.h>
#include <PiSmm.h>
#include <Register/Intel/SmramSaveStateMap.h>
#include <Register/QemuSmramSaveStateMap.h>
@@ -171,6 +174,92 @@ SmmCpuFeaturesHookReturnFromSmm (
return OriginalInstructionPointer;
}
+STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData = NULL;
+
+/**
+ Initialize mCpuHotEjectData if PcdCpuMaxLogicalProcessorNumber > 1.
+
+ Also setup the corresponding PcdCpuHotEjectDataAddress.
+**/
+STATIC
+VOID
+InitCpuHotEjectData (
+ VOID
+ )
+{
+ UINTN ArrayLen;
+ UINTN BaseLen;
+ UINTN TotalLen;
+ UINT32 Idx;
+ UINT32 MaxNumberOfCpus;
+ RETURN_STATUS PcdStatus;
+
+ MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+
+ if (MaxNumberOfCpus == 1) {
+ return;
+ }
+
+ //
+ // We want the following lay out for CPU_HOT_EJECT_DATA:
+ // UINTN alignment: CPU_HOT_EJECT_DATA
+ // --- padding if needed ---
+ // UINT64 alignment: CPU_HOT_EJECT_DATA->QemuSelectorMap[]
+ //
+ // Accordingly, we allocate:
+ // sizeof(*mCpuHotEjectData) + (MaxNumberOfCpus *
+ // sizeof(mCpuHotEjectData->QemuSelectorMap[0])).
+ // Add sizeof(UINT64) to use as padding if needed.
+ //
+
+ if (RETURN_ERROR (SafeUintnMult (sizeof (*mCpuHotEjectData), 1, &BaseLen)) ||
+ RETURN_ERROR (SafeUintnMult (
+ sizeof (mCpuHotEjectData->QemuSelectorMap[0]),
+ MaxNumberOfCpus, &ArrayLen)) ||
+ RETURN_ERROR (SafeUintnAdd (BaseLen, ArrayLen, &TotalLen))||
+ RETURN_ERROR (SafeUintnAdd (TotalLen, sizeof (UINT64), &TotalLen))) {
+ DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_EJECT_DATA\n", __FUNCTION__));
+ goto Fatal;
+ }
+
+ mCpuHotEjectData = AllocatePool (TotalLen);
+ if (mCpuHotEjectData == NULL) {
+ ASSERT (mCpuHotEjectData != NULL);
+ goto Fatal;
+ }
+
+ mCpuHotEjectData->Handler = NULL;
+ mCpuHotEjectData->ArrayLength = MaxNumberOfCpus;
+
+ mCpuHotEjectData->QemuSelectorMap = (void *)mCpuHotEjectData +
+ sizeof (*mCpuHotEjectData);
+ mCpuHotEjectData->QemuSelectorMap =
+ (void *)ALIGN_VALUE ((UINTN)mCpuHotEjectData->QemuSelectorMap,
+ sizeof (UINT64));
+ //
+ // We use mCpuHotEjectData->QemuSelectorMap to map
+ // ProcessorNum -> QemuSelector. Initialize to invalid values.
+ //
+ for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
+ mCpuHotEjectData->QemuSelectorMap[Idx] = CPU_EJECT_QEMU_SELECTOR_INVALID;
+ }
+
+ //
+ // Expose address of CPU Hot eject Data structure
+ //
+ PcdStatus = PcdSet64S (PcdCpuHotEjectDataAddress,
+ (UINTN)(VOID *)mCpuHotEjectData);
+ if (RETURN_ERROR (PcdStatus)) {
+ ASSERT_EFI_ERROR (PcdStatus);
+ goto Fatal;
+ }
+
+ return;
+
+Fatal:
+ CpuDeadLoop ();
+}
+
/**
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 +277,9 @@ SmmCpuFeaturesSmmRelocationComplete (
UINTN MapPagesBase;
UINTN MapPagesCount;
+
+ InitCpuHotEjectData ();
+
if (!MemEncryptSevIsEnabled ()) {
return;
}
--
2.9.3
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71917): https://edk2.groups.io/g/devel/message/71917
Mute This Topic: https://groups.io/mt/80819861/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:
> Init CPU_HOT_EJECT_DATA, which will be used to share CPU ejection
> state between SmmCpuFeaturesLib (via PiSmmCpuDxeSmm) and CpuHotPlugSmm.
>
> The init happens via SmmCpuFeaturesSmmRelocationComplete(), and so it
> will run as part of the PiSmmCpuDxeSmm entry point function,
> PiCpuSmmEntry(). Once inited, CPU_HOT_EJECT_DATA is exposed via
> PcdCpuHotEjectDataAddress.
>
> The CPU hot-eject handler (CPU_HOT_EJECT_DATA->Handler) is setup when
> there is an ejection request via CpuHotplugSmm.
>
> 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:
> (1) Detail in commit message about context in which CPU_HOT_EJECT_DATA
> is inited.
> (2) Add in sorted order MemoryAllocationLib in LibraryClasses
> (3) Sort added includes in SmmCpuFeaturesLib.c
> (4a-4b) Fixup linkage directives for mCpuHotEjectData.
> (5) s/CpuHotEjectData/mCpuHotEjectData/
> (6,10a,10b) Remove dependence on PcdCpuHotPlugSupport
> (7) Make the tense structure consistent in block comment for
> InitCpuHotEject().
> (8) s/SmmCpuFeaturesSmmInitHotEject/InitCpuHotEject/
> (9) s/mMaxNumberOfCpus/MaxNumberOfCpus/
> (11) Remove a bunch of obvious comments.
> (14a,14b,14c) Use SafeUint functions and rework the allocation logic
> so we can just use a single allocation.
> (12) Remove the AllocatePool() cast.
> (13) Use a CpuDeadLoop() in case of failure; albeit via a goto, not
> inline.
> (15) Initialize the mCpuHotEjectData->QemuSelectorMap locally.
> (16) Fix indentation in PcdSet64S.
> (17) Change the cast in PcdSet64S() to UINTN.
> (18) Use RETURN_STATUS instead of EFI_STATUS.
> (19,20) Move the Handler logic in SmmCpuFeaturesRendezvousExit() into
> into a separate patch.
>
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 92 ++++++++++++++++++++++
> 2 files changed, 96 insertions(+)
Nice, I've only superficial comments:
>
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> index 97a10afb6e27..8a426a4c10fb 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -30,9 +30,13 @@ [LibraryClasses]
> BaseMemoryLib
> DebugLib
> MemEncryptSevLib
> + MemoryAllocationLib
> PcdLib
> + SafeIntLib
> SmmServicesTableLib
> UefiBootServicesTableLib
>
> [Pcd]
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 7ef7ed98342e..adbfc90ad46e 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -11,10 +11,13 @@
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/MemEncryptSevLib.h>
> +#include <Library/MemoryAllocationLib.h>
> #include <Library/PcdLib.h>
> +#include <Library/SafeIntLib.h>
> #include <Library/SmmCpuFeaturesLib.h>
> #include <Library/SmmServicesTableLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> +#include <Pcd/CpuHotEjectData.h>
> #include <PiSmm.h>
> #include <Register/Intel/SmramSaveStateMap.h>
> #include <Register/QemuSmramSaveStateMap.h>
> @@ -171,6 +174,92 @@ SmmCpuFeaturesHookReturnFromSmm (
> return OriginalInstructionPointer;
> }
>
> +STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData = NULL;
> +
> +/**
> + Initialize mCpuHotEjectData if PcdCpuMaxLogicalProcessorNumber > 1.
> +
> + Also setup the corresponding PcdCpuHotEjectDataAddress.
> +**/
> +STATIC
> +VOID
> +InitCpuHotEjectData (
> + VOID
> + )
> +{
> + UINTN ArrayLen;
> + UINTN BaseLen;
> + UINTN TotalLen;
> + UINT32 Idx;
> + UINT32 MaxNumberOfCpus;
> + RETURN_STATUS PcdStatus;
> +
> + MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> +
(1) This doesn't deserve an update in itself, but since I'll ask for
some more below, I might as well name it: please remove the empty line
here.
> + if (MaxNumberOfCpus == 1) {
> + return;
> + }
> +
> + //
> + // We want the following lay out for CPU_HOT_EJECT_DATA:
> + // UINTN alignment: CPU_HOT_EJECT_DATA
> + // --- padding if needed ---
> + // UINT64 alignment: CPU_HOT_EJECT_DATA->QemuSelectorMap[]
> + //
> + // Accordingly, we allocate:
> + // sizeof(*mCpuHotEjectData) + (MaxNumberOfCpus *
> + // sizeof(mCpuHotEjectData->QemuSelectorMap[0])).
> + // Add sizeof(UINT64) to use as padding if needed.
> + //
> +
> + if (RETURN_ERROR (SafeUintnMult (sizeof (*mCpuHotEjectData), 1, &BaseLen)) ||
(2) This "multiplication" is somewhat awkward.
First, BaseLen is a UINTN, which always matches the return type of the
sizeof operator, so a direct assignment would be OK.
But even that would be overkill -- I suggest removing the BaseLen
variable, and just using "sizeof (*mCpuHotEjectData)" in its place.
> + RETURN_ERROR (SafeUintnMult (
> + sizeof (mCpuHotEjectData->QemuSelectorMap[0]),
> + MaxNumberOfCpus, &ArrayLen)) ||
> + RETURN_ERROR (SafeUintnAdd (BaseLen, ArrayLen, &TotalLen))||
(3) Missing space character before "||"
> + RETURN_ERROR (SafeUintnAdd (TotalLen, sizeof (UINT64), &TotalLen))) {
(4) So, I find the mixture of
sizeof (UINT64)
and
sizeof (mCpuHotEjectData->QemuSelectorMap[0])
confusing.
(And, this remark applies to both the code and the (otherwise very
helpful!) comment above it.)
We use sizeof (UINT64) *because* that's the type of
"mCpuHotEjectData->QemuSelectorMap[0]" -- we want to ensure natural
alignment for the atomic accesses performed later.
So, given both kinds of sizeof expression stand for the same concept, we
should use either "sizeof (UINT64)" exclusively, or "sizeof
(mCpuHotEjectData->QemuSelectorMap[0])", exclusively.
I would vote "sizeof (UINT64)" here, because that could help us
eliminate some of the unpleasant line breaks.
(5) Not sure if I should obsess about this, but... an alignment to
UINT64 may require up to (sizeof (UINT64) - 1) padding bytes. (sizeof
(UINT64)) bytes are never needed, as that would imply the allocation is
already correctly aligned -- but then we'd need 0 additional bytes.
(6) So how about this formulation instead:
if (RETURN_ERROR (SafeUintnMult (MaxNumberOfCpus, sizeof (UINT64), &Size)) ||
RETURN_ERROR (SafeUintnAdd (Size, sizeof (*mCpuHotEjectData), &Size)) ||
RETURN_ERROR (SafeUintnAdd (Size, sizeof (UINT64) - 1, &Size)) {
...
}
The longest line (the first line) is 79 characters wide, and we only use
one variable (called "Size").
If you don't like this variant, that's OK; but please at least unify the
sizeof expressions (4).
> + DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_EJECT_DATA\n", __FUNCTION__));
> + goto Fatal;
> + }
> +
> + mCpuHotEjectData = AllocatePool (TotalLen);
> + if (mCpuHotEjectData == NULL) {
> + ASSERT (mCpuHotEjectData != NULL);
> + goto Fatal;
> + }
> +
> + mCpuHotEjectData->Handler = NULL;
> + mCpuHotEjectData->ArrayLength = MaxNumberOfCpus;
> +
> + mCpuHotEjectData->QemuSelectorMap = (void *)mCpuHotEjectData +
> + sizeof (*mCpuHotEjectData);
> + mCpuHotEjectData->QemuSelectorMap =
> + (void *)ALIGN_VALUE ((UINTN)mCpuHotEjectData->QemuSelectorMap,
> + sizeof (UINT64));
(7) More idiomatic formulation, for setting the "QemuSelectorMap" field:
mCpuHotEjectData->QemuSelectorMap = ALIGN_POINTER (mCpuHotEjectData + 1,
sizeof (UINT64));
> + //
> + // We use mCpuHotEjectData->QemuSelectorMap to map
> + // ProcessorNum -> QemuSelector. Initialize to invalid values.
> + //
> + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
> + mCpuHotEjectData->QemuSelectorMap[Idx] = CPU_EJECT_QEMU_SELECTOR_INVALID;
> + }
> +
> + //
> + // Expose address of CPU Hot eject Data structure
> + //
> + PcdStatus = PcdSet64S (PcdCpuHotEjectDataAddress,
> + (UINTN)(VOID *)mCpuHotEjectData);
> + if (RETURN_ERROR (PcdStatus)) {
> + ASSERT_EFI_ERROR (PcdStatus);
(8) To be fully clean semantically, this should be
ASSERT_RETURN_ERROR().
... Sorry about the bike-shedding; the purpose is two-fold:
- the new code should feel idiomatic,
- I hope you're going to stick around for further edk2 / OVMF
development, and then these style hints shouldn't be useless in the long
run.
Thanks!
Laszlo
> + goto Fatal;
> + }
> +
> + return;
> +
> +Fatal:
> + CpuDeadLoop ();
> +}
> +
> /**
> 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 +277,9 @@ SmmCpuFeaturesSmmRelocationComplete (
> UINTN MapPagesBase;
> UINTN MapPagesCount;
>
> +
> + InitCpuHotEjectData ();
> +
> if (!MemEncryptSevIsEnabled ()) {
> return;
> }
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#71964): https://edk2.groups.io/g/devel/message/71964
Mute This Topic: https://groups.io/mt/80819861/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 2021-02-22 6:19 a.m., Laszlo Ersek wrote:
> On 02/22/21 08:19, Ankur Arora wrote:
>> Init CPU_HOT_EJECT_DATA, which will be used to share CPU ejection
>> state between SmmCpuFeaturesLib (via PiSmmCpuDxeSmm) and CpuHotPlugSmm.
>>
>> The init happens via SmmCpuFeaturesSmmRelocationComplete(), and so it
>> will run as part of the PiSmmCpuDxeSmm entry point function,
>> PiCpuSmmEntry(). Once inited, CPU_HOT_EJECT_DATA is exposed via
>> PcdCpuHotEjectDataAddress.
>>
>> The CPU hot-eject handler (CPU_HOT_EJECT_DATA->Handler) is setup when
>> there is an ejection request via CpuHotplugSmm.
>>
>> 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:
>> (1) Detail in commit message about context in which CPU_HOT_EJECT_DATA
>> is inited.
>> (2) Add in sorted order MemoryAllocationLib in LibraryClasses
>> (3) Sort added includes in SmmCpuFeaturesLib.c
>> (4a-4b) Fixup linkage directives for mCpuHotEjectData.
>> (5) s/CpuHotEjectData/mCpuHotEjectData/
>> (6,10a,10b) Remove dependence on PcdCpuHotPlugSupport
>> (7) Make the tense structure consistent in block comment for
>> InitCpuHotEject().
>> (8) s/SmmCpuFeaturesSmmInitHotEject/InitCpuHotEject/
>> (9) s/mMaxNumberOfCpus/MaxNumberOfCpus/
>> (11) Remove a bunch of obvious comments.
>> (14a,14b,14c) Use SafeUint functions and rework the allocation logic
>> so we can just use a single allocation.
>> (12) Remove the AllocatePool() cast.
>> (13) Use a CpuDeadLoop() in case of failure; albeit via a goto, not
>> inline.
>> (15) Initialize the mCpuHotEjectData->QemuSelectorMap locally.
>> (16) Fix indentation in PcdSet64S.
>> (17) Change the cast in PcdSet64S() to UINTN.
>> (18) Use RETURN_STATUS instead of EFI_STATUS.
>> (19,20) Move the Handler logic in SmmCpuFeaturesRendezvousExit() into
>> into a separate patch.
>>
>> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +
>> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 92 ++++++++++++++++++++++
>> 2 files changed, 96 insertions(+)
>
> Nice, I've only superficial comments:
>
>>
>> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> index 97a10afb6e27..8a426a4c10fb 100644
>> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
>> @@ -30,9 +30,13 @@ [LibraryClasses]
>> BaseMemoryLib
>> DebugLib
>> MemEncryptSevLib
>> + MemoryAllocationLib
>> PcdLib
>> + SafeIntLib
>> SmmServicesTableLib
>> UefiBootServicesTableLib
>>
>> [Pcd]
>> + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
>> + gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
>> gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
>> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> index 7ef7ed98342e..adbfc90ad46e 100644
>> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
>> @@ -11,10 +11,13 @@
>> #include <Library/BaseMemoryLib.h>
>> #include <Library/DebugLib.h>
>> #include <Library/MemEncryptSevLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> #include <Library/PcdLib.h>
>> +#include <Library/SafeIntLib.h>
>> #include <Library/SmmCpuFeaturesLib.h>
>> #include <Library/SmmServicesTableLib.h>
>> #include <Library/UefiBootServicesTableLib.h>
>> +#include <Pcd/CpuHotEjectData.h>
>> #include <PiSmm.h>
>> #include <Register/Intel/SmramSaveStateMap.h>
>> #include <Register/QemuSmramSaveStateMap.h>
>> @@ -171,6 +174,92 @@ SmmCpuFeaturesHookReturnFromSmm (
>> return OriginalInstructionPointer;
>> }
>>
>> +STATIC CPU_HOT_EJECT_DATA *mCpuHotEjectData = NULL;
>> +
>> +/**
>> + Initialize mCpuHotEjectData if PcdCpuMaxLogicalProcessorNumber > 1.
>> +
>> + Also setup the corresponding PcdCpuHotEjectDataAddress.
>> +**/
>> +STATIC
>> +VOID
>> +InitCpuHotEjectData (
>> + VOID
>> + )
>> +{
>> + UINTN ArrayLen;
>> + UINTN BaseLen;
>> + UINTN TotalLen;
>> + UINT32 Idx;
>> + UINT32 MaxNumberOfCpus;
>> + RETURN_STATUS PcdStatus;
>> +
>> + MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>> +
>
> (1) This doesn't deserve an update in itself, but since I'll ask for
> some more below, I might as well name it: please remove the empty line
> here.
>
>> + if (MaxNumberOfCpus == 1) {
>> + return;
>> + }
>> +
>> + //
>> + // We want the following lay out for CPU_HOT_EJECT_DATA:
>> + // UINTN alignment: CPU_HOT_EJECT_DATA
>> + // --- padding if needed ---
>> + // UINT64 alignment: CPU_HOT_EJECT_DATA->QemuSelectorMap[]
>> + //
>> + // Accordingly, we allocate:
>> + // sizeof(*mCpuHotEjectData) + (MaxNumberOfCpus *
>> + // sizeof(mCpuHotEjectData->QemuSelectorMap[0])).
>> + // Add sizeof(UINT64) to use as padding if needed.
>> + //
>> +
>> + if (RETURN_ERROR (SafeUintnMult (sizeof (*mCpuHotEjectData), 1, &BaseLen)) ||
>
> (2) This "multiplication" is somewhat awkward.
>
> First, BaseLen is a UINTN, which always matches the return type of the
> sizeof operator, so a direct assignment would be OK.
>
> But even that would be overkill -- I suggest removing the BaseLen
> variable, and just using "sizeof (*mCpuHotEjectData)" in its place.
>
>
>> + RETURN_ERROR (SafeUintnMult (
>> + sizeof (mCpuHotEjectData->QemuSelectorMap[0]),
>> + MaxNumberOfCpus, &ArrayLen)) ||
>> + RETURN_ERROR (SafeUintnAdd (BaseLen, ArrayLen, &TotalLen))||
>
> (3) Missing space character before "||"
>
>
>> + RETURN_ERROR (SafeUintnAdd (TotalLen, sizeof (UINT64), &TotalLen))) {
>
> (4) So, I find the mixture of
>
> sizeof (UINT64)
>
> and
>
> sizeof (mCpuHotEjectData->QemuSelectorMap[0])
>
> confusing.
>
> (And, this remark applies to both the code and the (otherwise very
> helpful!) comment above it.)
>
> We use sizeof (UINT64) *because* that's the type of
> "mCpuHotEjectData->QemuSelectorMap[0]" -- we want to ensure natural
> alignment for the atomic accesses performed later.
>
> So, given both kinds of sizeof expression stand for the same concept, we
> should use either "sizeof (UINT64)" exclusively, or "sizeof
> (mCpuHotEjectData->QemuSelectorMap[0])", exclusively.
>
> I would vote "sizeof (UINT64)" here, because that could help us
> eliminate some of the unpleasant line breaks.
>
>
> (5) Not sure if I should obsess about this, but... an alignment to
> UINT64 may require up to (sizeof (UINT64) - 1) padding bytes. (sizeof
> (UINT64)) bytes are never needed, as that would imply the allocation is
> already correctly aligned -- but then we'd need 0 additional bytes.
>
>
> (6) So how about this formulation instead:
>
> if (RETURN_ERROR (SafeUintnMult (MaxNumberOfCpus, sizeof (UINT64), &Size)) ||
> RETURN_ERROR (SafeUintnAdd (Size, sizeof (*mCpuHotEjectData), &Size)) ||
> RETURN_ERROR (SafeUintnAdd (Size, sizeof (UINT64) - 1, &Size)) {
> ...
> }
>
> The longest line (the first line) is 79 characters wide, and we only use
> one variable (called "Size").
>
> If you don't like this variant, that's OK; but please at least unify the
> sizeof expressions (4).
I like this forumlation -- though I will get rid of the UINT64 -- I think
as you pointed out above the mixture is a little awkward.
Also acking comments above.
>
>
>> + DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_EJECT_DATA\n", __FUNCTION__));
>> + goto Fatal;
>> + }
>> +
>> + mCpuHotEjectData = AllocatePool (TotalLen);
>> + if (mCpuHotEjectData == NULL) {
>> + ASSERT (mCpuHotEjectData != NULL);
>> + goto Fatal;
>> + }
>> +
>> + mCpuHotEjectData->Handler = NULL;
>> + mCpuHotEjectData->ArrayLength = MaxNumberOfCpus;
>> +
>> + mCpuHotEjectData->QemuSelectorMap = (void *)mCpuHotEjectData +
>> + sizeof (*mCpuHotEjectData);
>> + mCpuHotEjectData->QemuSelectorMap =
>> + (void *)ALIGN_VALUE ((UINTN)mCpuHotEjectData->QemuSelectorMap,
>> + sizeof (UINT64));
>
> (7) More idiomatic formulation, for setting the "QemuSelectorMap" field:
>
> mCpuHotEjectData->QemuSelectorMap = ALIGN_POINTER (mCpuHotEjectData + 1,
> sizeof (UINT64));
Thanks. Yeah this is much better.
>
>> + //
>> + // We use mCpuHotEjectData->QemuSelectorMap to map
>> + // ProcessorNum -> QemuSelector. Initialize to invalid values.
>> + //
>> + for (Idx = 0; Idx < mCpuHotEjectData->ArrayLength; Idx++) {
>> + mCpuHotEjectData->QemuSelectorMap[Idx] = CPU_EJECT_QEMU_SELECTOR_INVALID;
>> + }
>> +
>> + //
>> + // Expose address of CPU Hot eject Data structure
>> + //
>> + PcdStatus = PcdSet64S (PcdCpuHotEjectDataAddress,
>> + (UINTN)(VOID *)mCpuHotEjectData);
>> + if (RETURN_ERROR (PcdStatus)) {
>> + ASSERT_EFI_ERROR (PcdStatus);
>
> (8) To be fully clean semantically, this should be
> ASSERT_RETURN_ERROR().
Yeah, that makes sense.
> ... Sorry about the bike-shedding; the purpose is two-fold:
>
> - the new code should feel idiomatic,
>
> - I hope you're going to stick around for further edk2 / OVMF
> development, and then these style hints shouldn't be useless in the long
> run.
Thanks. The project and working with you has decided been a mind
expanding experience. Hope to contribute in the future.
Ankur
>
> Thanks!
> Laszlo
>
>
>> + goto Fatal;
>> + }
>> +
>> + return;
>> +
>> +Fatal:
>> + CpuDeadLoop ();
>> +}
>> +
>> /**
>> 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 +277,9 @@ SmmCpuFeaturesSmmRelocationComplete (
>> UINTN MapPagesBase;
>> UINTN MapPagesCount;
>>
>> +
>> + InitCpuHotEjectData ();
>> +
>> if (!MemEncryptSevIsEnabled ()) {
>> return;
>> }
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72034): https://edk2.groups.io/g/devel/message/72034
Mute This Topic: https://groups.io/mt/80819861/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.