From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.675.1621358248801260238 for ; Tue, 18 May 2021 10:17:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=J3Zg4Gue; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1621358248; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=FJYHPpwYlQ55oO984+IhHghVJL8qX67sLlcBv/uZ3YE=; b=J3Zg4GueNRUiBo9CaSyjPpRGEROez5zgp1bq39ISqAei9v6xs9SAiK8ZatvBIb6nVfEEYL 7xRnYcmaow8+SyFH/eD9SMsTDs1/ciflqi6K/7GYk8xlqlLxRBR5+rzVn2pFfI9Gzt/hi3 R5Gz6hLc+IimWTYo6gNxtBSeUEWnaxQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-333-w-Z_wvyrMnahHW58syLNig-1; Tue, 18 May 2021 13:17:24 -0400 X-MC-Unique: w-Z_wvyrMnahHW58syLNig-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6E2678049CA; Tue, 18 May 2021 17:17:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-30.ams2.redhat.com [10.36.112.30]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B26335D6AB; Tue, 18 May 2021 17:17:20 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] 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 , Rahul Kumar , =?UTF-8?Q?Marvin_H=c3=a4user?= References: <3cae2ac836884b131725866264e0a0e1897052de.1621024125.git.thomas.lendacky@amd.com> <5394e010-6088-18e8-9a90-65eb55bbfac2@redhat.com> <7d3d835a-4354-108d-17b7-8679eb8c67d1@amd.com> From: "Laszlo Ersek" Message-ID: <61a02049-7280-1a25-c718-c663acfcc56e@redhat.com> Date: Tue, 18 May 2021 19:17:19 +0200 MIME-Version: 1.0 In-Reply-To: <7d3d835a-4354-108d-17b7-8679eb8c67d1@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 . >> >> 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 >>> Cc: Ray Ni >>> Cc: Laszlo Ersek >>> Cc: Rahul Kumar >>> Cc: Marvin Häuser >>> Signed-off-by: Tom Lendacky >>> >>> --- >>> >>> 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 >>> #include >>> >>> +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