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

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

[Lendacky, Thomas]

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;
 }
-- 
2.28.0

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

[Laszlo Ersek]

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.

- We should insert a space character after "PcdGet32", before merging
this patch. I think Ray or Eric could do this without a repost.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

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

[Lendacky, Thomas]

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). I'm surprised that PatchCheck.py didn't pick up on the 
spacing with PcdGet32.

Thanks,
Tom

> 
> - We should insert a space character after "PcdGet32", before merging
> this patch. I think Ray or Eric could do this without a repost.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 

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

[Laszlo Ersek]

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

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

[Lendacky, Thomas]

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

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
> 

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

[Lendacky, Thomas]

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

Thanks,
Tom

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

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

[Laszlo Ersek]

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

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

[Brian J. Johnson]

On 9/24/20 1:22 AM, Laszlo Ersek wrote:
> - 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.

FWIW, there are many bare metal processors with non-contiguous APIC IDs. 
  Intel puts the socket ID in the upper bits of the APIC ID.  So if the 
socket doesn't have a power-of-two number of cores, there is always a 
gap.  Plus, the BSP doesn't always have APIC ID 0 -- it depends on which 
physical cores passed the manufacturing tests for that particular part. 
That has broken various kernels, BIOSes, and other software at times. 
So it's best not to make assumptions.

I don't know if that applies to VMs, though.  The standards may be 
different (if there are any standards at all in that area.)

-- 
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

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

[Lendacky, Thomas]

On 9/25/20 11:09 AM, Brian J. Johnson wrote:
> On 9/24/20 1:22 AM, Laszlo Ersek wrote:
>> - 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.
> 
> FWIW, there are many bare metal processors with non-contiguous APIC IDs. 
>   Intel puts the socket ID in the upper bits of the APIC ID.  So if the 
> socket doesn't have a power-of-two number of cores, there is always a 
> gap.  Plus, the BSP doesn't always have APIC ID 0 -- it depends on which 
> physical cores passed the manufacturing tests for that particular part. 
> That has broken various kernels, BIOSes, and other software at times. So 
> it's best not to make assumptions.
> 
> I don't know if that applies to VMs, though.  The standards may be 
> different (if there are any standards at all in that area.)

Yes, I'm working on a patch to use GetProcessorNumber() instead of using 
the APIC ID.

Thanks,
Tom

>