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.d...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Rahul Kumar <rahul1.ku...@intel.com> > Cc: Marvin Häuser <mhaeu...@posteo.de> > Signed-off-by: Tom Lendacky <thomas.lenda...@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; > } > } >
Acked-by: Laszlo Ersek <ler...@redhat.com> Should I forget to merge this after the stable tag, please remind me. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75415): https://edk2.groups.io/g/devel/message/75415 Mute This Topic: https://groups.io/mt/82833645/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
