[edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area
Lendacky, Thomas
thomas.lendacky at amd.com
Fri May 14 13:33:36 UTC 2021
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 at intel.com>
>> Cc: Ray Ni <ray.ni at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar at intel.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky at 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75121): https://edk2.groups.io/g/devel/message/75121
Mute This Topic: https://groups.io/mt/82757192/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list