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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 5/14/21 4:14 AM, Laszlo Ersek wrote:
> On 05/11/21 22:50, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=04%7C01%7Cthomas.lendacky%40amd.com%7C7c28b41e27cc4b9a8b9508d916b8a955%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637565804655837525%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Ixw2PdtFLryJs9KHplJ8bvomtaqjBJF8KuDdWO5ERdw%3D&reserved=0
>>
>> 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.

Ok, will do here and in the PEI file.

> 
>> +
>>  /**
>>    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;
>   }
> 

Will do.

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

Will do.

> 
>> +
>> +      //
>> +      // 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" ...)

Ok.

> 
>> +
>> +      //
>> +      // 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.)
> 

Heh, I was trying to figure this one out and seeing multiple forms. I'll
update it.

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

Yup.

> 
>> @@ -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)?

Will do.

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

Will do.

Thanks,
Tom

> 
> Thanks!
> Laszlo
>