From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.9966.1621004685736924516 for ; Fri, 14 May 2021 08:04:46 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@posteo.de header.s=2017 header.b=aOu+ETpf; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout01.posteo.de (Postfix) with ESMTPS id E965D240027 for ; Fri, 14 May 2021 17:04:43 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1621004683; bh=+zdXAjRQ/7PINlhJt6ktMUZm/QtE70++Qg/7sTofFf8=; h=Subject:To:Cc:From:Date:From; b=aOu+ETpfsH1sBz7rVZY1oqV2d/NEfboNq4HYb4PtiM4AAU/0WUVy7bdLo9J0tBTYA N+2Om9bPcWKZMYGWxo8EYOxtp4MUzLINtO2tEqTKNPNiLTx9OfmriJp54hy9sdGCMO Fp+zXfUB6ZoZSBVoGn6Rf97YbPnwAQ9gBLhB2MSDupA6Tz2yaoqQtddGhmINydemFY 84ytCELc/RdH6Mwrde5kBsBrae8nsaBQ0T5eZgRDfioCR+uQ7TFyinXHSV1KKhmxfB KRDnuQ3e4UhhuAvuCw1lYpCagKoSiHPmiTifQOwUCzXGn1VDznoCRWJXrvqdvCeSG7 Hj0f9cs9EzLlw== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4FhWyy3NDdz9rxF; Fri, 14 May 2021 17:04:42 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Brijesh Singh , Eric Dong , Ray Ni , Laszlo Ersek , Rahul Kumar References: <1162d1f4fe09048aaafba6f6ea3046bebd34fc2b.1620766224.git.thomas.lendacky@amd.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: Date: Fri, 14 May 2021 15:04:42 +0000 MIME-Version: 1.0 In-Reply-To: <1162d1f4fe09048aaafba6f6ea3046bebd34fc2b.1620766224.git.thomas.lendacky@amd.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB Good day Thomas, Thank you very much for the quick patch. Comments inline. Best regards, Marvin On 11.05.21 22:50, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3324 > > 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 > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Signed-off-by: Tom Lendacky > --- > 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://github.com/tianocore/edk2/commit/e4ff6349bf9ee4f3f392141374901ea4994e043e > +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? > + > /** > 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? > + > + // > + // 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? > + 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 > #include > > +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; > } > }