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

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

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