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

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

On 10/19/20 17:02, Tom Lendacky wrote:
> On 10/2/20 11:42 AM, Tom Lendacky wrote:
>> 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.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).
>>
>> Any concern from the maintainers on this? Are you waiting on me for
>> anything? Just want to check...
> 
> Another ping to see what the status of this patch is...

... I guess we might as well break the process for a noble purpose,
every once in a while, so I went ahead and merged this... with just my R-b.

- [lersek@redhat.com: supply missing space character after "PcdGet32"]
- commit 93edd1887e34
- https://github.com/tianocore/edk2/pull/1031

Thanks
Laszlo


>>>> 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
>>>
>