Re: [edk2-devel] [PATCH v9 06/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
On 03/12/21 07:26, 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
> 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 comments from v8:
>
> (1) Remove line before the "if (MaxNumberofCpus == 1)" check.
> (3) Fixup the space around "||".
> (2,6) Simplify the three SafeInt multiplication into the ones suggested
> by Laszlo.
> (4) Get rid of the mixed sizeof(mCpuHotEjectData->QemuSelectorMap[0]) and
> sizeof(UINT64) in favour of UINT64 everywhere. I was planning to use
> the first, but describing the alignment needed is easier in terms of
> the
> second.
> Also, as Laszlo's comments on v8-patch-9 mention, we don't really need
> this alignment for correctness reasons. This patch retains it, so we
> don't pay access penalty for unaligned access.
> (5) Change alignment from UINT64 to UINT64-1.
> (7) Use the more idiomatic ALIGN_POINTER instead of ALIGN_VALUE.
> (8) RETURN_ERROR -> ASSERT_RETURN_ERROR.
>
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf| 4 ++
> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 77
> ++
> 2 files changed, 81 insertions(+)
Reviewed-by: Laszlo Ersek
Thanks!
Laszlo
>
> 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..5c025bc717c3 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -11,10 +11,13 @@
> #include
> #include
> #include
> +#include
> #include
> +#include
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -171,6 +174,77 @@ 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 Size;
> + UINT32 Idx;
> + UINT32 MaxNumberOfCpus;
> + RETURN_STATUS PcdStatus;
> +
> + MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> + if (MaxNumberOfCpus == 1) {
> +return;
> + }
> +
> + //
> + // We allocate CPU_HOT_EJECT_DATA and CPU_HOT_EJECT_DATA->QemuSelectorMap[]
> + // in a single allocation, and explicitly align the QemuSelectorMap[]
> (which
> + // is a UINT64 array) at its natural boundary.
> + // Accordingly, allocate:
> + // sizeof(*mCpuHotEjectData) + (MaxNumberOfCpus * sizeof(UINT64))
> + // and, add sizeof(UINT64) - 1 to use as padding if needed.
> + //
> +
> + if (RETURN_ERROR (SafeUintnMult (MaxNumberOfCpus, sizeof (UINT64), &Size))
> ||
> + RETURN_ERROR (SafeUintnAdd (Size, sizeof (*mCpuHotEjectData), &Size))
> ||
> + RETURN_ERROR (SafeUintnAdd (Size, sizeof (UINT64) - 1, &Size))) {
> +DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_EJECT_DATA\n", __FUNCTION__));
> +goto Fatal;
> + }
> +
> + mCpuHotEjectData = AllocatePool (Size);
> + if (mCpuHotEjectData == NULL) {
> +ASSERT (mCpuHotEjectData != NULL);
> +goto Fatal;
> + }
> +
> + mCpuHotEjectData->Handler = NULL;
> + mCpuHotEjectData->ArrayLength = MaxNumberOfCpus;
> +
> + mCpuHotEjectData->QemuSelectorMap = ALIGN_POINTER (mCpuHotEjectData + 1,
> +sizeof (UINT64));
> + //
> + // We use mCpuHotEjectData->QemuSelectorMap to map
> + // ProcessorNum -> QemuSelector. Initialize t
[edk2-devel] [PATCH v9 06/10] OvmfPkg/SmmCpuFeaturesLib: init CPU ejection state
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
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 comments from v8:
(1) Remove line before the "if (MaxNumberofCpus == 1)" check.
(3) Fixup the space around "||".
(2,6) Simplify the three SafeInt multiplication into the ones suggested
by Laszlo.
(4) Get rid of the mixed sizeof(mCpuHotEjectData->QemuSelectorMap[0]) and
sizeof(UINT64) in favour of UINT64 everywhere. I was planning to use
the first, but describing the alignment needed is easier in terms of the
second.
Also, as Laszlo's comments on v8-patch-9 mention, we don't really need
this alignment for correctness reasons. This patch retains it, so we
don't pay access penalty for unaligned access.
(5) Change alignment from UINT64 to UINT64-1.
(7) Use the more idiomatic ALIGN_POINTER instead of ALIGN_VALUE.
(8) RETURN_ERROR -> ASSERT_RETURN_ERROR.
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf| 4 ++
.../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 77 ++
2 files changed, 81 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..5c025bc717c3 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -11,10 +11,13 @@
#include
#include
#include
+#include
#include
+#include
#include
#include
#include
+#include
#include
#include
#include
@@ -171,6 +174,77 @@ 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 Size;
+ UINT32 Idx;
+ UINT32 MaxNumberOfCpus;
+ RETURN_STATUS PcdStatus;
+
+ MaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+ if (MaxNumberOfCpus == 1) {
+return;
+ }
+
+ //
+ // We allocate CPU_HOT_EJECT_DATA and CPU_HOT_EJECT_DATA->QemuSelectorMap[]
+ // in a single allocation, and explicitly align the QemuSelectorMap[] (which
+ // is a UINT64 array) at its natural boundary.
+ // Accordingly, allocate:
+ // sizeof(*mCpuHotEjectData) + (MaxNumberOfCpus * sizeof(UINT64))
+ // and, add sizeof(UINT64) - 1 to use as padding if needed.
+ //
+
+ if (RETURN_ERROR (SafeUintnMult (MaxNumberOfCpus, sizeof (UINT64), &Size)) ||
+ RETURN_ERROR (SafeUintnAdd (Size, sizeof (*mCpuHotEjectData), &Size)) ||
+ RETURN_ERROR (SafeUintnAdd (Size, sizeof (UINT64) - 1, &Size))) {
+DEBUG ((DEBUG_ERROR, "%a: invalid CPU_HOT_EJECT_DATA\n", __FUNCTION__));
+goto Fatal;
+ }
+
+ mCpuHotEjectData = AllocatePool (Size);
+ if (mCpuHotEjectData == NULL) {
+ASSERT (mCpuHotEjectData != NULL);
+goto Fatal;
+ }
+
+ mCpuHotEjectData->Handler = NULL;
+ mCpuHotEjectData->ArrayLength = MaxNumberOfCpus;
+
+ 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);
+ ASSERT_RETURN_ERROR (PcdStatu
