On 9/24/20 10:03 AM, Laszlo Ersek wrote:
On 09/24/20 15:30, Tom Lendacky wrote:
On 9/24/20 1:22 AM, Laszlo Ersek wrote:
On 09/23/20 20:04, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lenda...@amd.com>
The AP reset vector stack allocation is only required if running as an
SEV-ES guest. Since the reset vector allocation is below 1MB in memory,
eliminate the requirement for bare-metal systems and non SEV-ES guests
to allocate the extra stack area, which can be large if the
PcdCpuMaxLogicalProcessorNumber value is large, and also remove the
CPU_STACK_ALIGNMENT alignment.
Fixes: 7b7508ad784d ("UefiCpuPkg: Allow AP booting under SEV-ES")
Cc: Garrett Kirkendall <garrett.kirkend...@amd.com>
Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 36 +++++++++-----------
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 07426274f639..a9708c6479d2 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1141,20 +1141,6 @@ RestoreWakeupBuffer(
);
}
-/**
- Calculate the size of the reset stack.
-
- @return Total amount of memory required for stacks
-**/
-STATIC
-UINTN
-GetApResetStackSize (
- VOID
- )
-{
- return AP_RESET_STACK_SIZE *
PcdGet32(PcdCpuMaxLogicalProcessorNumber);
-}
-
/**
Calculate the size of the reset vector.
@@ -1170,11 +1156,23 @@ GetApResetVectorSize (
{
UINTN Size;
- Size = ALIGN_VALUE (AddressMap->RendezvousFunnelSize +
- AddressMap->SwitchToRealSize +
- sizeof (MP_CPU_EXCHANGE_INFO),
- CPU_STACK_ALIGNMENT);
- Size += GetApResetStackSize ();
+ Size = AddressMap->RendezvousFunnelSize +
+ AddressMap->SwitchToRealSize +
+ sizeof (MP_CPU_EXCHANGE_INFO);
+
+ //
+ // The AP reset stack is only used by SEV-ES guests. Do not add to
the
+ // allocation if SEV-ES is not enabled.
+ //
+ if (PcdGetBool (PcdSevEsIsEnabled)) {
+ //
+ // Stack location is based on APIC ID, so use the total number of
+ // processors for calculating the total stack area.
+ //
+ Size += AP_RESET_STACK_SIZE *
PcdGet32(PcdCpuMaxLogicalProcessorNumber);
+
+ Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
+ }
return Size;
}
- I don't remember if it's required that the APIC ID space be densely
populated. For example, if we have a topology with 7 possible (=
maximum) logical CPUs, I'm unsure if a spec forbids any of those CPUs
from having APIC ID 7. That could cause a problem in
MpInitLibSevEsAPReset(), I assume.
Anyway: that's a different topic. This patch looks OK to me because it
only spells out the existent assumption wrt. APIC IDs vs.
PcdCpuMaxLogicalProcessorNumber, plus it does solve the problem it wants
to solve.
- I was a bit surprised at first upon seeing the reordering of alignment
vs. addition. But AP_RESET_STACK_SIZE is a whole multiple of
CPU_STACK_ALIGNMENT. So adding AP_RESET_STACK_SIZE to Size n times as
first step, does not change the congruence class of Size modulo
CPU_STACK_ALIGNMENT. Therefore ALIGN_VALUE() will increment Size by the
same value, regardless of whether it's done before or after the
AP_RESET_STACK_SIZE additions.
Ah, yes, I did change that order. I could submit one more version that
puts the ALIGN_VALUE back to the original position and fix the PcdGet32
space if that is desired (but, as you determined, it doesn't change the
resulting value).
Given that the stack grows down, one could plausibly claim that the
variant seen in this patch (= align at last) is *more* intuitive.
I'm OK with this version merged (with the whitespace fixed up).
Any concern from the maintainers on this? Are you waiting on me for
anything? Just want to check...
Thanks,
Tom
I'm surprised that PatchCheck.py didn't pick up on the
spacing with PcdGet32.
Or maybe ECC should warn about it in CI?...
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65849): https://edk2.groups.io/g/devel/message/65849
Mute This Topic: https://groups.io/mt/77041010/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
