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).
> 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 (#65572): https://edk2.groups.io/g/devel/message/65572
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]
-=-=-=-=-=-=-=-=-=-=-=-
