[edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area
Marvin Häuser
mhaeuser at posteo.de
Fri May 14 15:44:25 UTC 2021
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 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.
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;
>>> }
>>> }
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75129): https://edk2.groups.io/g/devel/message/75129
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