[edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area
Lendacky, Thomas
thomas.lendacky at amd.com
Fri May 14 15:23:00 UTC 2021
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 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
>> +//
>
> 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/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