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.d...@intel.com> >> Cc: Ray Ni <ray...@intel.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Rahul Kumar <rahul1.ku...@intel.com> >> Signed-off-by: Tom Lendacky <thomas.lenda...@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; >> } >> } > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75128): https://edk2.groups.io/g/devel/message/75128 Mute This Topic: https://groups.io/mt/82757192/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
