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%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=N9MNXaBazq2tiVRHWSPVXRdlcZ97JOf24mc7p0m5Tqw%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%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=BiE2cBZRVRT3anwklHEKBHdhg8v2KjV%2FiiPwtx%2Fpon4%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%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1n8z7KFAlm3Vb7fPOFpYQFlZ5lQFOF%2FdLtujjqhns9s%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.d...@intel.com> >>> Cc: Ray Ni <ray...@intel.com> >>> Cc: Laszlo Ersek <ler...@redhat.com> >>> Cc: Rahul Kumar <rahul1.ku...@intel.com> >>> Cc: Marvin Häuser <mhaeu...@posteo.de> >>> Signed-off-by: Tom Lendacky <thomas.lenda...@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.) > >> >> 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. - 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. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75279): https://edk2.groups.io/g/devel/message/75279 Mute This Topic: https://groups.io/mt/82833645/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
