Re: [PATCH v2 1/1] UefiCpuPkg/MpInitLib: Reduce reset vector memory pressure

["Laszlo Ersek" <lersek@redhat.com>]

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.lendacky@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.kirkendall@amd.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@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