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

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

On 05/14/21 22:28, 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 so that
> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
> allocation in order to ensure that the new buffer allocation is below the
> previous allocation. When PcdSevEsIsEnabled is false, the original logic
> is followed.
> 
> 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>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
> ---
> 
> Changes since v1:
> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
>   libraries and to make it obvious they are SEV-ES specific.
> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
>   so that the new support is only use for SEV-ES guests.
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 49 +++++++++++++-------
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
>  3 files changed, 69 insertions(+), 18 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 7839c249760e..93fc63bf93e3 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 mSevEsDxeWakeupBuffer = 0x88000;
> +
>  /**
>    Enable Debug Agent to support source debugging on AP function.
>  
> @@ -102,7 +107,14 @@ GetWakeupBuffer (
>    // LagacyBios driver depends on CPU Arch protocol which guarantees below
>    // allocation runs earlier than LegacyBios driver.
>    //
> -  StartAddress = 0x88000;
> +  if (PcdGetBool (PcdSevEsIsEnabled)) {
> +    //
> +    // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
> +    //
> +    StartAddress = mSevEsDxeWakeupBuffer;
> +  } else {
> +    StartAddress = 0x88000;
> +  }
>    Status = gBS->AllocatePages (
>                    AllocateMaxAddress,
>                    MemoryType,
> @@ -112,6 +124,11 @@ GetWakeupBuffer (
>    ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
>      StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
> +  } else if (PcdGetBool (PcdSevEsIsEnabled)) {
> +    //
> +    // Next SEV-ES wakeup buffer allocation must be below this allocation
> +    //
> +    mSevEsDxeWakeupBuffer = StartAddress;
>    }
>  
>    DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index dc2a54aa31e8..b9a06747edbf 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;
>  }
>  
> @@ -1192,6 +1178,7 @@ AllocateResetVector (
>    )
>  {
>    UINTN           ApResetVectorSize;
> +  UINTN           ApResetStackSize;
>  
>    if (CpuMpData->WakeupBuffer == (UINTN) -1) {
>      ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
> @@ -1207,9 +1194,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)) {
> +      //
> +      // 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));
> +
> +      //
> +      // 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"
> +          ));
> +
> +        ASSERT (FALSE);
> +        CpuDeadLoop ();
> +      }
> +    }
>    }
>    BackupAndPrepareWakeupBuffer (CpuMpData);
>  }
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 3989bd6a7a9f..90015c650c68 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 mSevEsPeiWakeupBuffer = BASE_1MB;
> +
>  /**
>    S3 SMM Init Done notification function.
>  
> @@ -220,7 +222,13 @@ GetWakeupBuffer (
>          // Need memory under 1MB to be collected here
>          //
>          WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
> -        if (WakeupBufferEnd > BASE_1MB) {
> +        if (PcdGetBool (PcdSevEsIsEnabled) &&
> +            WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
> +          //
> +          // SEV-ES Wakeup buffer should be under 1MB and under any previous one
> +          //
> +          WakeupBufferEnd = mSevEsPeiWakeupBuffer;
> +        } else if (WakeupBufferEnd > BASE_1MB) {
>            //
>            // Wakeup buffer should be under 1MB
>            //
> @@ -244,6 +252,15 @@ GetWakeupBuffer (
>            }
>            DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>                                 WakeupBufferStart, WakeupBufferSize));
> +
> +          if (PcdGetBool (PcdSevEsIsEnabled)) {
> +            //
> +            // Next SEV-ES wakeup buffer allocation must be below this
> +            // allocation
> +            //
> +            mSevEsPeiWakeupBuffer = WakeupBufferStart;
> +          }
> +
>            return (UINTN)WakeupBufferStart;
>          }
>        }
> 

Merged in commit dbc22a178546 via
<https://github.com/tianocore/edk2/pull/1674>.

Thanks
Laszlo