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

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

[Lendacky, Thomas]

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;
+
 /**
   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",
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;
 }
 
@@ -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;
+
+      //
+      // 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..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.
 
@@ -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) {
           //
@@ -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;
         }
       }
-- 
2.31.0

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

[Laszlo Ersek]

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

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

[Lendacky, Thomas]

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
> 

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

[Lendacky, Thomas]

On 5/14/21 8:33 AM, Tom Lendacky wrote:
> 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
>>>

...

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

Actually, WakeupBufferEnd is like a helper variable here, so probably best
to just do:

  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
    //    
    WakeupBufferEnd = BASE_1MB;
  }     

which makes for a nice diff:

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

Thanks,
Tom

> 
>>

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

[Marvin Häuser]

Good day Thomas,

Thank you very much for the quick patch. Comments inline.

Best regards,
Marvin

On 11.05.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
> +//

This definitely is not an issue of your patch at all, but I find the 
comments of the behaviour very lacking. Might this be a good opportunity 
to briefly document it? It took me quite a bit of git blame to fully 
figure it out, especially due to the non-descriptive commit message. The 
constant is explained very well in the description: 
https://github.com/tianocore/edk2/commit/e4ff6349bf9ee4f3f392141374901ea4994e043e

> +STATIC EFI_PHYSICAL_ADDRESS mWakeupBuffer = 0x88000;

Hmm, I wonder whether a global variable is the best solution here. With 
an explanation from the commit above, a top-down allocator makes good 
sense for this scenario. However, a "GetWakeupBuffer(Size, MaxAddress)" 
function might be more self-descriptive. What do you think?

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

Personally, I do not mind this at all, but I think the code style 
prohibits declaring variables in inner scopes. Though preferably I would 
like to see this, nowadays, arbitrary restriction lifted rather than the 
patch be changed. Do the package maintainers have comments on this?

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

Should the ASSERT not only catch the broken "allocate below" behaviour, 
i.e. not trigger on failed allocation?

> +        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.
>   
> @@ -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) {
>             //
> @@ -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;
>           }
>         }

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

[Lendacky, Thomas]

On 5/14/21 10:04 AM, Marvin Häuser wrote:
> Good day Thomas,

Hi Marvin,

> 
> Thank you very much for the quick patch. Comments inline.
> 
> Best regards,
> Marvin
> 
> On 11.05.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%7C25d0b0bdc5d14a8bc4ff08d916e99c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637566014883375137%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8%2BywcjFoZ00AymlBdxt%2BMCP0JClc64soY6pwcfz08zo%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
>> +//
> 
> This definitely is not an issue of your patch at all, but I find the
> comments of the behaviour very lacking. Might this be a good opportunity
> to briefly document it? It took me quite a bit of git blame to fully
> figure it out, especially due to the non-descriptive commit message. The
> constant is explained very well in the description:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fcommit%2Fe4ff6349bf9ee4f3f392141374901ea4994e043e&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C25d0b0bdc5d14a8bc4ff08d916e99c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637566014883375137%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NHR7dQjJbXQK5j4zc0ni0Q%2FZtOQp7szwDEzpHY6V9Dc%3D&amp;reserved=0
> 

I think a separate patch would be best... and since you understand it so
well now, maybe something you could submit :)

> 
>> +STATIC EFI_PHYSICAL_ADDRESS mWakeupBuffer = 0x88000;
> 
> Hmm, I wonder whether a global variable is the best solution here. With an
> explanation from the commit above, a top-down allocator makes good sense
> for this scenario. However, a "GetWakeupBuffer(Size, MaxAddress)" function
> might be more self-descriptive. What do you think?

Given the previous comments to not introduce any regressions in the
non-SEV-ES path, it is probably not a good idea to change this as part of
this patch. A separate distinct patch would be best.

But understand that GetWakeupBuffer() has a different implementation in
PEI and DXE. The only common thing is that GetWakeupBuffer() knows to be
under 1MB. PEI doesn't have the 0x88000 limitation that DXE does, so I
don't think the MaxAddress parameter helps here.

> 
>> +
>>   /**
>>     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",
>> 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;
>>   }
>>   @@ -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;
> 
> Personally, I do not mind this at all, but I think the code style
> prohibits declaring variables in inner scopes. Though preferably I would
> like to see this, nowadays, arbitrary restriction lifted rather than the
> patch be changed. Do the package maintainers have comments on this?

Yup, noted in other comments. So the variable will be moved.

> 
>> +
>> +      //
>> +      // 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);
> 
> Should the ASSERT not only catch the broken "allocate below" behaviour,
> i.e. not trigger on failed allocation?

I think it's best to trigger on a failed allocation as well rather than
continuing and allowing a page fault or some other problem to occur.

Thanks,
Tom

> 
>> +        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.
>>   @@ -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) {
>>             //
>> @@ -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;
>>           }
>>         }
> 

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

[Marvin Häuser]

On 14.05.21 17:23, Lendacky, Thomas wrote:
> On 5/14/21 10:04 AM, Marvin Häuser wrote:
>> Good day Thomas,
> Hi Marvin,
>
>> Thank you very much for the quick patch. Comments inline.
>>
>> Best regards,
>> Marvin
>>
>> On 11.05.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%7C25d0b0bdc5d14a8bc4ff08d916e99c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637566014883375137%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8%2BywcjFoZ00AymlBdxt%2BMCP0JClc64soY6pwcfz08zo%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
>>> +//
>> This definitely is not an issue of your patch at all, but I find the
>> comments of the behaviour very lacking. Might this be a good opportunity
>> to briefly document it? It took me quite a bit of git blame to fully
>> figure it out, especially due to the non-descriptive commit message. The
>> constant is explained very well in the description:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fcommit%2Fe4ff6349bf9ee4f3f392141374901ea4994e043e&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C25d0b0bdc5d14a8bc4ff08d916e99c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637566014883375137%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NHR7dQjJbXQK5j4zc0ni0Q%2FZtOQp7szwDEzpHY6V9Dc%3D&amp;reserved=0
>>
> I think a separate patch would be best... and since you understand it so
> well now, maybe something you could submit :)

:)

>>> +STATIC EFI_PHYSICAL_ADDRESS mWakeupBuffer = 0x88000;
>> Hmm, I wonder whether a global variable is the best solution here. With an
>> explanation from the commit above, a top-down allocator makes good sense
>> for this scenario. However, a "GetWakeupBuffer(Size, MaxAddress)" function
>> might be more self-descriptive. What do you think?
> Given the previous comments to not introduce any regressions in the
> non-SEV-ES path, it is probably not a good idea to change this as part of
> this patch. A separate distinct patch would be best.
>
> But understand that GetWakeupBuffer() has a different implementation in
> PEI and DXE. The only common thing is that GetWakeupBuffer() knows to be
> under 1MB. PEI doesn't have the 0x88000 limitation that DXE does, so I
> don't think the MaxAddress parameter helps here.

For now you are right, yes. There currently is an open ticket which 
would likely be resolved by aligning the PEI behaviour with DXE, which 
would resolve this issue as well. But also better to push to a separate 
thread, sorry.

>>> +
>>>    /**
>>>      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",
>>> 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;
>>>    }
>>>    @@ -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;
>> Personally, I do not mind this at all, but I think the code style
>> prohibits declaring variables in inner scopes. Though preferably I would
>> like to see this, nowadays, arbitrary restriction lifted rather than the
>> patch be changed. Do the package maintainers have comments on this?
> Yup, noted in other comments. So the variable will be moved.

Thx

>>> +
>>> +      //
>>> +      // 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);
>> Should the ASSERT not only catch the broken "allocate below" behaviour,
>> i.e. not trigger on failed allocation?
> I think it's best to trigger on a failed allocation as well rather than
> continuing and allowing a page fault or some other problem to occur.

Well, it should handle the error in a safe way, i.e. the deadloop below. 
To not ASSERT on plausible conditions is a common design guideline in 
most low-level projects including Linux kernel.

Best regards,
Marvin

> Thanks,
> Tom
>
>>> +        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.
>>>    @@ -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) {
>>>              //
>>> @@ -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;
>>>            }
>>>          }
>
> 
>
>

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

[Laszlo Ersek]

On 05/14/21 17:44, Marvin Häuser wrote:
> On 14.05.21 17:23, Lendacky, Thomas wrote:
>> On 5/14/21 10:04 AM, Marvin Häuser wrote:

>>>> +      // 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);
>>> Should the ASSERT not only catch the broken "allocate below" behaviour,
>>> i.e. not trigger on failed allocation?
>> I think it's best to trigger on a failed allocation as well rather than
>> continuing and allowing a page fault or some other problem to occur.
> 
> Well, it should handle the error in a safe way, i.e. the deadloop below.
> To not ASSERT on plausible conditions is a common design guideline in
> most low-level projects including Linux kernel.
> 
> Best regards,
> Marvin
> 
>> Thanks,
>> Tom
>>
>>>> +        CpuDeadLoop ();

"DEBUG + ASSERT(FALSE) + CpuDeadLoop()" is a pattern in edk2.

In RELEASE builds, it will lead to a CpuDeadLoop(). That's the main goal
-- don't continue execution if the condition controlling the whole block
fired.

In DEBUG and NOOPT builds, the pattern will lead to a debug message
(usually at the "error" level), followed by an assertion failure. The
error message of the assertion failure is irrelevant ("FALSE"). The
point of adding ASSERT ahead of CpuDeadLoop() is that the way ASSERT
hangs execution is customizable, via "PcdDebugPropertyMask", unlike
CpuDeadLoop(). In many cases, ASSERT() uses CpuDeadLoop() itself, so the
effect is the same -- the explicit CpuDeadLoop is not reached. In other
configs, ASSERT() can raise a debug exception (CpuBreakpoint()).

The required part of the pattern is CpuDeadLoop(); the DEBUG message
makes it more debugging-friendly, and the ASSERT(), with the tweakable
"hang method", makes it even more debugging-friendly.

Thanks
Laszlo

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

[Marvin Häuser]

Am 16.05.2021 um 03:17 schrieb Laszlo Ersek:
> On 05/14/21 17:44, Marvin Häuser wrote:
>> On 14.05.21 17:23, Lendacky, Thomas wrote:
>>> On 5/14/21 10:04 AM, Marvin Häuser wrote:
> 
>>>>> +      // 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);
>>>> Should the ASSERT not only catch the broken "allocate below" behaviour,
>>>> i.e. not trigger on failed allocation?
>>> I think it's best to trigger on a failed allocation as well rather than
>>> continuing and allowing a page fault or some other problem to occur.
>>
>> Well, it should handle the error in a safe way, i.e. the deadloop below.
>> To not ASSERT on plausible conditions is a common design guideline in
>> most low-level projects including Linux kernel.
>>
>> Best regards,
>> Marvin
>>
>>> Thanks,
>>> Tom
>>>
>>>>> +        CpuDeadLoop ();
> 
> "DEBUG + ASSERT(FALSE) + CpuDeadLoop()" is a pattern in edk2.
> 
> In RELEASE builds, it will lead to a CpuDeadLoop(). That's the main goal
> -- don't continue execution if the condition controlling the whole block
> fired.
> 
> In DEBUG and NOOPT builds, the pattern will lead to a debug message
> (usually at the "error" level), followed by an assertion failure. The
> error message of the assertion failure is irrelevant ("FALSE"). The
> point of adding ASSERT ahead of CpuDeadLoop() is that the way ASSERT
> hangs execution is customizable, via "PcdDebugPropertyMask", unlike
> CpuDeadLoop(). In many cases, ASSERT() uses CpuDeadLoop() itself, so the
> effect is the same -- the explicit CpuDeadLoop is not reached. In other
> configs, ASSERT() can raise a debug exception (CpuBreakpoint()).

I absolutely do not *expect* Tom to change this, it was just a slight 
remark (as many places have this anyway). I'll still try to explain why 
I made that remark, but for whom it is of no interest, I do not expect 
it to be read. I'm fine with the patch as-is myself. Thank you a lot, Tom!



I know it, unfortunately, is a pattern in EDK II - taking this pattern 
too far is what caused the 8-revision patch regarding untrusted inputs 
we submitted previously. :)

There are many concerns about unconventional ASSERTs, though I must 
admit none but one (and that one barely) really apply here, which is why 
I have trouble explaining why I believe it should be changed. Here are 
some reasons outside the context of this patch:

1) Consistency between DEBUG and RELEASE builds: I think one can justify 
to have a breakpoint on a condition that may realistically occur. But a 
deadloop can give a wrong impression about how production code works. 
E.g. it also is a common pattern in EDK II to ASSERT on memory 
allocation failure but *not* have a proper check after, so DEBUG builds 
will nicely error or deadloop, while RELEASE goes ahead and causes a CPU 
exception or memory corruption depending on the context. Thus, 
real-world error handling cannot really be tested. This does not apply 
because there *is* a RELEASE deadloop.

2) Static analysis: Some static analysers use ASSERT information for 
their own analysis, and try to give hints about unsafe or unreachable 
code based on own annotations. This kind of applies, but only when 
substituting EDK II ASSERT with properly recognisable ASSERTs (e.g. 
__builtin_unreachable).

2) Dynamic analysis: ASSERTs can be useful when fuzzing for example. 
Enabled Sanitizers will only catch unsafe behaviour, but maybe you have 
some extra code in place to sanity-check the results further. An ASSERT 
yields an error dump (usually followed by the worker dying). However, as 
allocation failures are perfectly expected, this can cause a dramatic 
about of False Positives and testing interruption. This does not apply 
because deadloop'd code cannot really be fuzz-tested anyway.

ASSERTs really are designed as unbreakable conditions, i.e. 1) 
preconditions 2) invariants 3) postconditions. No allocator in early 
kernel-space or lower can really guarantee allocation success, thus it 
cannot be a postcondition of any such function. And while it might make 
debugging look a bit easier (but you will see from the backtrace anyway 
where you halted), it messes with all tools that assume proper usage.

Also, I just realised, you can of course see it from the address value 
when debugging, but you cannot see it from the ASSERT or DEBUG message 
*which* of the two logical error conditions failed (i.e. broken 
allocator or OOM). Changing the ASSERT would fix that. :)

Best regards,
Marvin



> 
> The required part of the pattern is CpuDeadLoop(); the DEBUG message
> makes it more debugging-friendly, and the ASSERT(), with the tweakable
> "hang method", makes it even more debugging-friendly.
> 
> Thanks
> Laszlo
> 
> 
> 
> 
> 
> 

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

[Laszlo Ersek]

On 05/17/21 00:15, Marvin Häuser wrote:
> Am 16.05.2021 um 03:17 schrieb Laszlo Ersek:
>> On 05/14/21 17:44, Marvin Häuser wrote:
>>> On 14.05.21 17:23, Lendacky, Thomas wrote:
>>>> On 5/14/21 10:04 AM, Marvin Häuser wrote:
>>
>>>>>> +      // 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);
>>>>> Should the ASSERT not only catch the broken "allocate below"
>>>>> behaviour,
>>>>> i.e. not trigger on failed allocation?
>>>> I think it's best to trigger on a failed allocation as well rather than
>>>> continuing and allowing a page fault or some other problem to occur.
>>>
>>> Well, it should handle the error in a safe way, i.e. the deadloop below.
>>> To not ASSERT on plausible conditions is a common design guideline in
>>> most low-level projects including Linux kernel.
>>>
>>> Best regards,
>>> Marvin
>>>
>>>> Thanks,
>>>> Tom
>>>>
>>>>>> +        CpuDeadLoop ();
>>
>> "DEBUG + ASSERT(FALSE) + CpuDeadLoop()" is a pattern in edk2.
>>
>> In RELEASE builds, it will lead to a CpuDeadLoop(). That's the main goal
>> -- don't continue execution if the condition controlling the whole block
>> fired.
>>
>> In DEBUG and NOOPT builds, the pattern will lead to a debug message
>> (usually at the "error" level), followed by an assertion failure. The
>> error message of the assertion failure is irrelevant ("FALSE"). The
>> point of adding ASSERT ahead of CpuDeadLoop() is that the way ASSERT
>> hangs execution is customizable, via "PcdDebugPropertyMask", unlike
>> CpuDeadLoop(). In many cases, ASSERT() uses CpuDeadLoop() itself, so the
>> effect is the same -- the explicit CpuDeadLoop is not reached. In other
>> configs, ASSERT() can raise a debug exception (CpuBreakpoint()).
> 
> I absolutely do not *expect* Tom to change this, it was just a slight
> remark (as many places have this anyway). I'll still try to explain why
> I made that remark, but for whom it is of no interest, I do not expect
> it to be read. I'm fine with the patch as-is myself. Thank you a lot, Tom!
> 
> 
> 
> I know it, unfortunately, is a pattern in EDK II - taking this pattern
> too far is what caused the 8-revision patch regarding untrusted inputs
> we submitted previously. :)
> 
> There are many concerns about unconventional ASSERTs, though I must
> admit none but one (and that one barely) really apply here, which is why
> I have trouble explaining why I believe it should be changed. Here are
> some reasons outside the context of this patch:
> 
> 1) Consistency between DEBUG and RELEASE builds: I think one can justify
> to have a breakpoint on a condition that may realistically occur. But a
> deadloop can give a wrong impression about how production code works.
> E.g. it also is a common pattern in EDK II to ASSERT on memory
> allocation failure but *not* have a proper check after, so DEBUG builds
> will nicely error or deadloop, while RELEASE goes ahead and causes a CPU
> exception or memory corruption depending on the context. Thus,
> real-world error handling cannot really be tested. This does not apply
> because there *is* a RELEASE deadloop.
> 
> 2) Static analysis: Some static analysers use ASSERT information for
> their own analysis, and try to give hints about unsafe or unreachable
> code based on own annotations. This kind of applies, but only when
> substituting EDK II ASSERT with properly recognisable ASSERTs (e.g.
> __builtin_unreachable).
> 
> 2) Dynamic analysis: ASSERTs can be useful when fuzzing for example.
> Enabled Sanitizers will only catch unsafe behaviour, but maybe you have
> some extra code in place to sanity-check the results further. An ASSERT
> yields an error dump (usually followed by the worker dying). However, as
> allocation failures are perfectly expected, this can cause a dramatic
> about of False Positives and testing interruption. This does not apply
> because deadloop'd code cannot really be fuzz-tested anyway.
> 
> ASSERTs really are designed as unbreakable conditions, i.e. 1)
> preconditions 2) invariants 3) postconditions. No allocator in early
> kernel-space or lower can really guarantee allocation success, thus it
> cannot be a postcondition of any such function. And while it might make
> debugging look a bit easier (but you will see from the backtrace anyway
> where you halted), it messes with all tools that assume proper usage.
> 
> Also, I just realised, you can of course see it from the address value
> when debugging, but you cannot see it from the ASSERT or DEBUG message
> *which* of the two logical error conditions failed (i.e. broken
> allocator or OOM). Changing the ASSERT would fix that. :)

I'm OK if the ASSERT() is dropped; from my perspective it's really just
a small convenience / developer/debugging aid. We still have the debug
message and the explicit deadloop.

Thanks
Laszlo