Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area

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

On 05/11/21 22:50, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3324
> 
> The SEV-ES stacks currently share a page with the reset code and data.
> Separate the SEV-ES stacks from the reset vector code and data to avoid
> possible stack overflows from overwriting the code and/or data.
> 
> When SEV-ES is enabled, invoke the GetWakeupBuffer() routine a second time
> to allocate a new area, below the reset vector and data.
> 
> Both the PEI and DXE versions of GetWakeupBuffer() are changed to track
> the previous reset buffer allocation in order to ensure that the new
> buffer allocation is below the previous allocation.
> 
> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 12 ++++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 48 +++++++++++++-------
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 14 ++++--
>  3 files changed, 54 insertions(+), 20 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 7839c249760e..fdfa0755d37a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -29,6 +29,11 @@ VOID             *mReservedApLoopFunc = NULL;
>  UINTN            mReservedTopOfApStack;
>  volatile UINT32  mNumberToFinish = 0;
>  
> +//
> +// Begin wakeup buffer allocation below 0x88000
> +//
> +STATIC EFI_PHYSICAL_ADDRESS mWakeupBuffer = 0x88000;

(1) Please rename this to mSevEsDxeWakeupBuffer. The code is correct
as-is, but regarding code editors that can jump to tags, it helps if we
eliminate duplicate identifiers.

> +
>  /**
>    Enable Debug Agent to support source debugging on AP function.
>  
> @@ -102,7 +107,7 @@ GetWakeupBuffer (
>    // LagacyBios driver depends on CPU Arch protocol which guarantees below
>    // allocation runs earlier than LegacyBios driver.
>    //
> -  StartAddress = 0x88000;
> +  StartAddress = mWakeupBuffer;
>    Status = gBS->AllocatePages (
>                    AllocateMaxAddress,
>                    MemoryType,
> @@ -112,6 +117,11 @@ GetWakeupBuffer (
>    ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
>      StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
> +  } else {
> +    //
> +    // Next wakeup buffer allocation must be below this allocation
> +    //
> +    mWakeupBuffer = StartAddress;
>    }
>  
>    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",

(2) Please make these changes conditional on "PcdSevEsIsEnabled". If the
PCD is false, the behavior of the function shouldn't change at all --
not just the caller-observable behavior, but the internal behavior either.

For the SEV-ES disabled case, it's much easier to see that the change is
regression-free if we don't just rely on GetWakeupBuffer() not being
called for a second time, but explicitly make the patch so that it does
nothing here if the PCD is false.

Something like:

  if (PcdGetBool (PcdSevEsIsEnabled)) {
    StartAddress = mSevEsDxeWakeupBuffer;
  } else {
    StartAddress = 0x88000;
  }

  ...

  if (EFI_ERROR (Status)) {
    StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
  } else if PcdGetBool (PcdSevEsIsEnabled) {
    mSevEsDxeWakeupBuffer = StartAddress;
  }


> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index dc2a54aa31e8..a76dae437606 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
>             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;
>  }
>  

This change is clearly regression-free in case the PCD is false. OK.

> @@ -1207,9 +1193,39 @@ AllocateResetVector (
>                                      CpuMpData->AddressMap.ModeTransitionOffset
>                                      );
>      //
> -    // The reset stack starts at the end of the buffer.
> +    // The AP reset stack is only used by SEV-ES guests. Do not allocate it
> +    // if SEV-ES is not enabled.
>      //
> -    CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
> +    if (PcdGetBool (PcdSevEsIsEnabled)) {
> +      UINTN  ApResetStackSize;

(3) While I very much like inner-block-scoped variables, and use them
heavily in ArmVirtPkg and OvmfPkg, unfortunately they are not tolerated
in the rest of edk2. Please move this variable to the top of the
function, and if necessary, put "SevEs" in its name.

> +
> +      //
> +      // Stack location is based on ProcessorNumber, so use the total number
> +      // of processors for calculating the total stack area.
> +      //
> +      ApResetStackSize = AP_RESET_STACK_SIZE *
> +                           PcdGet32 (PcdCpuMaxLogicalProcessorNumber);

(4) A bit more idiomatic way to break this up:

      ApResetStackSize = (AP_RESET_STACK_SIZE *
                          PcdGet32 (PcdCpuMaxLogicalProcessorNumber));

(indented similarly to the controlling expressions of "if", "while" ...)

> +
> +      //
> +      // Invoke GetWakeupBuffer a second time to allocate the stack area
> +      // below 1MB. The returned buffer will be page aligned and sized and
> +      // below the previously allocated buffer.
> +      //
> +      CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
> +
> +      //
> +      // Check to be sure that the "allocate below" behavior hasn't changed.
> +      // This will also catch a failed allocation, as "-1" is returned on
> +      // failure.
> +      //
> +      if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
> +        DEBUG ((DEBUG_ERROR,
> +          "SEV-ES AP reset stack is not below wakeup buffer\n"));

(5) Indentation:

        DEBUG ((
          DEBUG_ERROR,
          "SEV-ES AP reset stack is not below wakeup buffer\n"
          ));

(The condensed style you used is superior IMO, and I encourage it in
ArmVirtPkg and OvmfPkg, but it's not universally accepted in the rest of
edk2.)


> +
> +        ASSERT (FALSE);
> +        CpuDeadLoop ();
> +      }
> +    }
>    }
>    BackupAndPrepareWakeupBuffer (CpuMpData);
>  }
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 3989bd6a7a9f..4d09e89b4128 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -11,6 +11,8 @@
>  #include <Guid/S3SmmInitDone.h>
>  #include <Ppi/ShadowMicrocode.h>
>  
> +STATIC UINT64 mWakeupBuffer = BASE_1MB;
> +
>  /**
>    S3 SMM Init Done notification function.
>  

(6) Please rename this to mSevEsPeiWakeupBuffer. (Same argument as in (1).)

> @@ -220,11 +222,11 @@ GetWakeupBuffer (
>          // Need memory under 1MB to be collected here
>          //
>          WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
> -        if (WakeupBufferEnd > BASE_1MB) {
> +        if (WakeupBufferEnd > mWakeupBuffer) {
>            //
> -          // Wakeup buffer should be under 1MB
> +          // Wakeup buffer should be under 1MB and under the previous one
>            //
> -          WakeupBufferEnd = BASE_1MB;
> +          WakeupBufferEnd = mWakeupBuffer;
>          }
>          while (WakeupBufferEnd > WakeupBufferSize) {
>            //

(7) Can we use a WakeupBufferLimit helper variable here, and set it like
"StartAddress" under (2)?

> @@ -244,6 +246,12 @@ GetWakeupBuffer (
>            }
>            DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>                                 WakeupBufferStart, WakeupBufferSize));
> +
> +          //
> +          // Next wakeup buffer allocation must be below this allocation
> +          //
> +          mWakeupBuffer = WakeupBufferStart;
> +
>            return (UINTN)WakeupBufferStart;
>          }
>        }
> 

(8) And IMO I think we should adjust mSevEsPeiWakeupBuffer here
conditionally on the PCD as well.

Thanks!
Laszlo