* [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area @ 2021-05-11 20:50 Lendacky, Thomas 2021-05-14 9:14 ` [edk2-devel] " Laszlo Ersek 2021-05-14 15:04 ` Marvin Häuser 0 siblings, 2 replies; 10+ messages in thread From: Lendacky, Thomas @ 2021-05-11 20:50 UTC (permalink / raw) To: devel; +Cc: Brijesh Singh, Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area 2021-05-11 20:50 [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area Lendacky, Thomas @ 2021-05-14 9:14 ` Laszlo Ersek 2021-05-14 13:33 ` Lendacky, Thomas 2021-05-14 15:04 ` Marvin Häuser 1 sibling, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2021-05-14 9:14 UTC (permalink / raw) To: devel, thomas.lendacky; +Cc: Brijesh Singh, Eric Dong, Ray Ni, Rahul Kumar 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area 2021-05-14 9:14 ` [edk2-devel] " Laszlo Ersek @ 2021-05-14 13:33 ` Lendacky, Thomas 2021-05-14 14:54 ` Lendacky, Thomas 0 siblings, 1 reply; 10+ messages in thread From: Lendacky, Thomas @ 2021-05-14 13:33 UTC (permalink / raw) To: Laszlo Ersek, devel; +Cc: Brijesh Singh, Eric Dong, Ray Ni, Rahul Kumar 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area 2021-05-14 13:33 ` Lendacky, Thomas @ 2021-05-14 14:54 ` Lendacky, Thomas 0 siblings, 0 replies; 10+ messages in thread From: Lendacky, Thomas @ 2021-05-14 14:54 UTC (permalink / raw) To: Laszlo Ersek, devel; +Cc: Brijesh Singh, Eric Dong, Ray Ni, Rahul Kumar 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 > >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area 2021-05-11 20:50 [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area Lendacky, Thomas 2021-05-14 9:14 ` [edk2-devel] " Laszlo Ersek @ 2021-05-14 15:04 ` Marvin Häuser 2021-05-14 15:23 ` Lendacky, Thomas 1 sibling, 1 reply; 10+ messages in thread From: Marvin Häuser @ 2021-05-14 15:04 UTC (permalink / raw) To: devel, thomas.lendacky Cc: Brijesh Singh, Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar 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; > } > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area 2021-05-14 15:04 ` Marvin Häuser @ 2021-05-14 15:23 ` Lendacky, Thomas 2021-05-14 15:44 ` Marvin Häuser 0 siblings, 1 reply; 10+ messages in thread From: Lendacky, Thomas @ 2021-05-14 15:23 UTC (permalink / raw) To: Marvin Häuser, devel Cc: Brijesh Singh, Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C25d0b0bdc5d14a8bc4ff08d916e99c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637566014883375137%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NHR7dQjJbXQK5j4zc0ni0Q%2FZtOQp7szwDEzpHY6V9Dc%3D&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; >> } >> } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area 2021-05-14 15:23 ` Lendacky, Thomas @ 2021-05-14 15:44 ` Marvin Häuser 2021-05-16 1:17 ` Laszlo Ersek 0 siblings, 1 reply; 10+ messages in thread From: Marvin Häuser @ 2021-05-14 15:44 UTC (permalink / raw) To: devel, thomas.lendacky Cc: Brijesh Singh, Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar 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&data=04%7C01%7Cthomas.lendacky%40amd.com%7C25d0b0bdc5d14a8bc4ff08d916e99c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637566014883375137%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NHR7dQjJbXQK5j4zc0ni0Q%2FZtOQp7szwDEzpHY6V9Dc%3D&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; >>> } >>> } > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area 2021-05-14 15:44 ` Marvin Häuser @ 2021-05-16 1:17 ` Laszlo Ersek 2021-05-16 22:15 ` Marvin Häuser 0 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2021-05-16 1:17 UTC (permalink / raw) To: Marvin Häuser, devel, thomas.lendacky Cc: Brijesh Singh, Eric Dong, Ray Ni, Rahul Kumar 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area 2021-05-16 1:17 ` Laszlo Ersek @ 2021-05-16 22:15 ` Marvin Häuser 2021-05-18 15:29 ` Laszlo Ersek 0 siblings, 1 reply; 10+ messages in thread From: Marvin Häuser @ 2021-05-16 22:15 UTC (permalink / raw) To: devel, lersek, thomas.lendacky Cc: Brijesh Singh, Eric Dong, Ray Ni, Rahul Kumar 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 > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area 2021-05-16 22:15 ` Marvin Häuser @ 2021-05-18 15:29 ` Laszlo Ersek 0 siblings, 0 replies; 10+ messages in thread From: Laszlo Ersek @ 2021-05-18 15:29 UTC (permalink / raw) To: Marvin Häuser, devel, thomas.lendacky Cc: Brijesh Singh, Eric Dong, Ray Ni, Rahul Kumar 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-18 15:29 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-05-11 20:50 [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area Lendacky, Thomas 2021-05-14 9:14 ` [edk2-devel] " Laszlo Ersek 2021-05-14 13:33 ` Lendacky, Thomas 2021-05-14 14:54 ` Lendacky, Thomas 2021-05-14 15:04 ` Marvin Häuser 2021-05-14 15:23 ` Lendacky, Thomas 2021-05-14 15:44 ` Marvin Häuser 2021-05-16 1:17 ` Laszlo Ersek 2021-05-16 22:15 ` Marvin Häuser 2021-05-18 15:29 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox