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

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

On 09/22/20 21:59, 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.
>
> Cc: Garrett Kirkendall <garrett.kirkendall@amd.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 07426274f639..39af2f9fba7d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1152,7 +1152,15 @@ GetApResetStackSize (
>    VOID
>    )
>  {
> -  return AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber);
> +  //
> +  // The AP reset stack is only used by SEV-ES guests. Don't add to the
> +  // allocation if not necessary.
> +  //
> +  if (PcdGetBool (PcdSevEsIsEnabled) == TRUE) {
> +    return AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber);
> +  } else {
> +    return 0;
> +  }
>  }
>
>  /**
>

Looks like this is a fix for commit 7b7508ad784d ("UefiCpuPkg: Allow AP
booting under SEV-ES", 2020-08-17).

In retrospect, that commit changed "ApResetVectorSize" -- which is
passed to GetWakeupBuffer() -- from value [a]

  RendezvousFunnelSize + sizeof (MP_CPU_EXCHANGE_INFO)

to value [b]

  ALIGN_VALUE ((RendezvousFunnelSize +
                SwitchToRealSize +
                sizeof (MP_CPU_EXCHANGE_INFO)),
               CPU_STACK_ALIGNMENT) +
  AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber)

The currently proposed patch does not entirely restore
"ApResetVectorSize" to the value it used to carry before commit
7b7508ad784d. Namely, while the patch eliminates the last addend, two
other changes from commit 7b7508ad784d remain in place:

- the addition of "SwitchToRealSize",
- the alignment up to CPU_STACK_ALIGNMENT.

As far as I understand, "SwitchToRealSize" is never zero
(AsmGetAddressMap() populates it with a difference of build-time
constants). I think it's not useful when SEV-ES is inactive.

Furthermore, the alignment for stack purposes is useless if we won't
have AP stacks (i.e., again when SEV-ES is inactive).

(1) Therefore I'd propose:

- folding GetApResetStackSize() into GetApResetVectorSize(),

- modifying GetApResetVectorSize() such that it return the original sum
  [a] if SEV-ES is inactive, and the larger sum [b] if SEV-ES is active.

Hmmm. OK, maybe "SwitchToRealSize" should remain in place. It's a small
addition, and it reflects a code portion that is permanent. However, I
do think the alignment is both useless and confusing. If we won't
allocate an array of stacks, the alignment really makes no sense.


(2) A style comment: PcdGetBool()'s return value should not be compared
with TRUE or FALSE; just use PcdGetBool() as the whole controlling
expression for the "if".


(3) Even better... can you modify GetApResetVectorSize() to take
&CpuMpData rather than &CpuMpData->AddressMap, and then check
CpuMpData->SevEsIsEnabled?

Hmmm, wait, that's not really simple, as we call GetApResetVectorSize()
from MpInitLibInitialize() too, way before we set
CpuMpData->SevEsIsEnabled from the PCD.

So I guess we should pass a dedicated BOOLEAN parameter to
GetApResetVectorSize(), called "SevEsIsEnabled". At the call site in
MpInitLibInitialize(), we should pass in the PCD's value. At the call
site in AllocateResetVector(), we should pass in
CpuMpData->SevEsIsEnabled.

The reason I'm suggesting (3) is that I don't feel comfortable with
checking dynamic PCDs outside of entry point functions / initialization
functions.

Thanks,
Laszlo