[edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area
Lendacky, Thomas
thomas.lendacky at amd.com
Thu May 20 14:21:59 UTC 2021
On 5/18/21 12:17 PM, Laszlo Ersek wrote:
> On 05/17/21 17:03, Lendacky, Thomas wrote:
>> On 5/16/21 11:22 PM, Laszlo Ersek wrote:
>>> On 05/14/21 22:28, 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%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503778789%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=194iu8I6ucUBr6bNR0fIBa%2FgQhtUQQpJdN55gzOHlmY%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 so that
>>>> when PcdSevEsIsEnabled is true, they will track the previous reset buffer
>>>> allocation in order to ensure that the new buffer allocation is below the
>>>> previous allocation. When PcdSevEsIsEnabled is false, the original logic
>>>> is followed.
>>>>
>>>> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b
>>>
>>> Is this really a *bugfix*?
>>>
>>> I called what's being fixed "a gap in a generic protection mechanism",
>>> in <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324%23c9&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503778789%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QFwYOCIEZb8Uc6zX60a7p6QWIkPdzTInggQQv3dRVlQ%3D&reserved=0>.
>>>
>>> I don't know if that makes this patch a feature addition (or feature
>>> completion -- the feature being "more extensive page protections"), or a
>>> bugfix.
>>>
>>> The distinction matters because of the soft feature freeze:
>>>
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning&data=04%7C01%7Cthomas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503788787%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bZJ%2BK1Y%2Fs4CXXy%2Fz3IKVnWWCUSz3Noo3cPsWR%2Fby5H0%3D&reserved=0
>>>
>>> We still have approximately 2 hours before the SFF starts. So if we can
>>> *finish* reviewing this in 2 hours, then it can be merged during the
>>> SFF, even if we call it a feature. But I'd like Marvin to take a look as
>>> well, plus I'd like at least one of Eric and Ray to check.
>>>
>>> ... I'm tempted not to call it a bugfix, because the lack of this patch
>>> does not break SEV-ES usage, as far as I understand.
>>>
>>>> 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>
>>>> Cc: Marvin Häuser <mhaeuser at posteo.de>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky at amd.com>
>>>>
>>>> ---
>>>>
>>>> Changes since v1:
>>>> - Renamed the wakeup buffer variables to be unique in the PEI and DXE
>>>> libraries and to make it obvious they are SEV-ES specific.
>>>> - Use PcdGetBool (PcdSevEsIsEnabled) to make the changes regression-free
>>>> so that the new support is only use for SEV-ES guests.
>>>> ---
>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 +++++++-
>>>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 49 +++++++++++++-------
>>>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 19 +++++++-
>>>> 3 files changed, 69 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> index 7839c249760e..93fc63bf93e3 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
>>>> @@ -29,6 +29,11 @@ VOID *mReservedApLoopFunc = NULL;
>>>> UINTN mReservedTopOfApStack;
>>>> volatile UINT32 mNumberToFinish = 0;
>>>>
>>>> +//
>>>> +// Begin wakeup buffer allocation below 0x88000
>>>> +//
>>>> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer = 0x88000;
>>>> +
>>>> /**
>>>> Enable Debug Agent to support source debugging on AP function.
>>>>
>>>> @@ -102,7 +107,14 @@ GetWakeupBuffer (
>>>> // LagacyBios driver depends on CPU Arch protocol which guarantees below
>>>> // allocation runs earlier than LegacyBios driver.
>>>> //
>>>> - StartAddress = 0x88000;
>>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // SEV-ES Wakeup buffer should be under 0x88000 and under any previous one
>>>> + //
>>>> + StartAddress = mSevEsDxeWakeupBuffer;
>>>> + } else {
>>>> + StartAddress = 0x88000;
>>>> + }
>>>> Status = gBS->AllocatePages (
>>>> AllocateMaxAddress,
>>>> MemoryType,
>>>> @@ -112,6 +124,11 @@ GetWakeupBuffer (
>>>> ASSERT_EFI_ERROR (Status);
>>>> if (EFI_ERROR (Status)) {
>>>> StartAddress = (EFI_PHYSICAL_ADDRESS) -1;
>>>> + } else if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // Next SEV-ES wakeup buffer allocation must be below this allocation
>>>> + //
>>>> + mSevEsDxeWakeupBuffer = StartAddress;
>>>> }
>>>>
>>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> index dc2a54aa31e8..b9a06747edbf 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>>> @@ -1164,20 +1164,6 @@ GetApResetVectorSize (
>>>> AddressMap->SwitchToRealSize +
>>>> sizeof (MP_CPU_EXCHANGE_INFO);
>>>>
>>>> - //
>>>> - // The AP reset stack is only used by SEV-ES guests. Do not add to the
>>>> - // allocation if SEV-ES is not enabled.
>>>> - //
>>>> - if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> - //
>>>> - // Stack location is based on APIC ID, so use the total number of
>>>> - // processors for calculating the total stack area.
>>>> - //
>>>> - Size += AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>>>> -
>>>> - Size = ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT);
>>>> - }
>>>> -
>>>> return Size;
>>>> }
>>>>
>>>> @@ -1192,6 +1178,7 @@ AllocateResetVector (
>>>> )
>>>> {
>>>> UINTN ApResetVectorSize;
>>>> + UINTN ApResetStackSize;
>>>>
>>>> if (CpuMpData->WakeupBuffer == (UINTN) -1) {
>>>> ApResetVectorSize = GetApResetVectorSize (&CpuMpData->AddressMap);
>>>> @@ -1207,9 +1194,39 @@ AllocateResetVector (
>>>> CpuMpData->AddressMap.ModeTransitionOffset
>>>> );
>>>> //
>>>> - // The reset stack starts at the end of the buffer.
>>>> + // The AP reset stack is only used by SEV-ES guests. Do not allocate it
>>>> + // if SEV-ES is not enabled.
>>>> //
>>>> - CpuMpData->SevEsAPResetStackStart = CpuMpData->WakeupBuffer + ApResetVectorSize;
>>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // Stack location is based on ProcessorNumber, so use the total number
>>>
>>> sneaky documenation fix :) I vaguely remember discussing earlier that
>>> the APIC ID reference was incorrect. OK.
>>
>> Yeah, I should have made mention of that in the commit message.
>>
>>>
>>>> + // of processors for calculating the total stack area.
>>>> + //
>>>> + ApResetStackSize = (AP_RESET_STACK_SIZE *
>>>> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
>>>> +
>>>> + //
>>>> + // Invoke GetWakeupBuffer a second time to allocate the stack area
>>>> + // below 1MB. The returned buffer will be page aligned and sized and
>>>> + // below the previously allocated buffer.
>>>> + //
>>>> + CpuMpData->SevEsAPResetStackStart = GetWakeupBuffer (ApResetStackSize);
>>>> +
>>>> + //
>>>> + // Check to be sure that the "allocate below" behavior hasn't changed.
>>>> + // This will also catch a failed allocation, as "-1" is returned on
>>>> + // failure.
>>>> + //
>>>> + if (CpuMpData->SevEsAPResetStackStart >= CpuMpData->WakeupBuffer) {
>>>> + DEBUG ((
>>>> + DEBUG_ERROR,
>>>> + "SEV-ES AP reset stack is not below wakeup buffer\n"
>>>> + ));
>>>> +
>>>> + ASSERT (FALSE);
>>>> + CpuDeadLoop ();
>>>> + }
>>>> + }
>>>> }
>>>> BackupAndPrepareWakeupBuffer (CpuMpData);
>>>> }
>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> index 3989bd6a7a9f..90015c650c68 100644
>>>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>>>> @@ -11,6 +11,8 @@
>>>> #include <Guid/S3SmmInitDone.h>
>>>> #include <Ppi/ShadowMicrocode.h>
>>>>
>>>> +STATIC UINT64 mSevEsPeiWakeupBuffer = BASE_1MB;
>>>> +
>>>> /**
>>>> S3 SMM Init Done notification function.
>>>>
>>>> @@ -220,7 +222,13 @@ GetWakeupBuffer (
>>>> // Need memory under 1MB to be collected here
>>>> //
>>>> WakeupBufferEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
>>>> - if (WakeupBufferEnd > BASE_1MB) {
>>>> + if (PcdGetBool (PcdSevEsIsEnabled) &&
>>>> + WakeupBufferEnd > mSevEsPeiWakeupBuffer) {
>>>> + //
>>>> + // SEV-ES Wakeup buffer should be under 1MB and under any previous one
>>>> + //
>>>> + WakeupBufferEnd = mSevEsPeiWakeupBuffer;
>>>> + } else if (WakeupBufferEnd > BASE_1MB) {
>>>> //
>>>> // Wakeup buffer should be under 1MB
>>>> //
>>>> @@ -244,6 +252,15 @@ GetWakeupBuffer (
>>>> }
>>>> DEBUG ((DEBUG_INFO, "WakeupBufferStart = %x, WakeupBufferSize = %x\n",
>>>> WakeupBufferStart, WakeupBufferSize));
>>>> +
>>>> + if (PcdGetBool (PcdSevEsIsEnabled)) {
>>>> + //
>>>> + // Next SEV-ES wakeup buffer allocation must be below this
>>>> + // allocation
>>>> + //
>>>> + mSevEsPeiWakeupBuffer = WakeupBufferStart;
>>>> + }
>>>> +
>>>> return (UINTN)WakeupBufferStart;
>>>> }
>>>> }
>>>>
>>>
>>> The code in the patch seems sound to me, but, now that I've managed to
>>> take a bit more careful look, I think the patch is incomplete.
>>>
>>> Note the BackupAndPrepareWakeupBuffer() function call -- visible in the
>>> context --, at the end of the AllocateResetVector() function! Before, we
>>> had a single area allocated for the reset vector, which was
>>> appropriately sized for SEV-ES stacks too, in case SEV-ES was enabled.
>>>
>>> That was because MpInitLibInitialize() called GetApResetVectorSize()
>>> too, and stored the result to the "BackupBufferSize" field. Thus, the
>>> BackupAndPrepareWakeupBuffer() function, and its counterpart
>>> RestoreWakeupBuffer(), would include the SEV-ES AP stacks area in the
>>> backup/restore operations.
>>
>> The restore is not performed for an SEV-ES guest (see FreeResetVector()),
>> because the memory is still needed.
>
> I apologize for missing this. I'm not too familiar with the SEV-ES
> specifics in UefiCpuPkg.
>
> One question: given that FreeResetVector() does not call
> RestoreWakeupBuffer() when SEV-ES is enabled, would it make sense for
> AllocateResetVector() to not call BackupAndPrepareWakeupBuffer() either,
> in case SEV-ES is enabled? Because, if we never restore, do we really
> need the backup? I wonder if such a patch could be prepended to this one
> (in order to form a 2-patch series).
>
> (Well, BackupAndPrepareWakeupBuffer() performs two things, backup and
> preparation -- I guess we certainly need the preparation of the wake up
> buffer, but do we need to back up the original contents of the area?
> Including the backup buffer allocation.)
We don't need to, but it doesn't hurt. I wanted to avoid sprinkling too
many SEV-ES specific checks throughout the code.
>
>>
>>>
>>> But now, with SEV-ES enabled, we'll have a separate, discontiguous area
>>> -- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart
>>> RestoreWakeupBuffer() take that into account.
>>>
>>> Therefore I think, while this patch is regression-free for the SEV-ES
>>> *disabled* case, it may corrupt memory (through not restoring the AP
>>> stack area's original contents) with SEV-ES enabled.
>>
>> This is the current behavior for SEV-ES. The wakeup buffer memory is
>> marked as reserved, at least in the DXE phase.
>>
>>>
>>> I think we need to turn "ApResetStackSize" into an explicit field. It
>>> should have a defined value only when SEV-ES is enabled. And for that
>>> case, we seem to need a *separate backup buffer* too.
>>>
>>> FWIW, I'd really like to re-classify this BZ as a Feature Request (see
>>> the Product field on BZ#3324), and I'd really like to delay the patch
>>> until after edk2-stable202105. The patch is not necessary for using
>>> SEV-ES, but it has a chance to break SEV-ES.
>>
>> I'm ok with delaying this if you want.
>
> Here's what I'd like to do:
>
> - Reach an agreement with Marvin about the ASSERT(). I'm fine if we drop
> it, and fine if we keep it.
I can drop it since there's already a debug message issued at that point.
>
> - Eric or Ray to check the patch as well, because (as I said above) I
> didn't follow the SEV-ES patches for UefiCpuPkg (that series was just
> huge, apologies), and so now I don't have memories to reach back to.
>
> - Figure out if we need to conditionalize the
> BackupAndPrepareWakeupBuffer() call (or a part of that function anyway).
>
> We can and should continue discussing these things during the feature
> freeze; I'd like us to merge the patch after the tag.
>
> Sorry again that I'm missing bits and pieces; I'm this close |<->| to
> email bankruptcy.
I know that feeling, stay solvent!
Thanks,
Tom
>
> Thanks
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75420): https://edk2.groups.io/g/devel/message/75420
Mute This Topic: https://groups.io/mt/82833645/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