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.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.)
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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-