From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (NAM11-BN8-obe.outbound.protection.outlook.com [40.107.236.66]) by mx.groups.io with SMTP id smtpd.web10.20064.1621263790642782147 for ; Mon, 17 May 2021 08:03:11 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@amd.com header.s=selector1 header.b=kheBhYcJ; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: amd.com, ip: 40.107.236.66, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=QZuFP1lpHlRiU6NMP0rsQ/3RLPMkU6oPOGSck1LW7e8xIWLkalT9X/fYgb0h0cgbRruHsBn3BlWfYVA1qk+++1azr0MmP8CwmNiKBwgDRBCHnYiaPJ9fPJptUpAymMma6vxa8JSlaK1IdP14mOmCjrUiVixWTcsMQvfwvFbyTAF/NDmAfhpvXFwvXtwE+yswXtlXEGyEILZnDMsUaikpGPp5XPZVkBZanr1Qn0B68afhK3tKBUR7uX7VBAjEpHzQdJ4Ua7ubsM5XOvl69bEgLvIQKlkmqcJojAlSFVo7Tt6H2kxiRjsBwS8X6825ovoD/zEknyrECK1Yg/PttTXgKw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=cd8Kc82Odqry9crqn/Nza7gnK98dQw2U/4eL3rxhsJA=; b=bE2fLHzBkS1FgCxZwaspysXThkm89PntanfOcFoM+hGtWCEo3s1rEOuyTtpqOWmMpu/j2SyuZIkr3lNUHZVunDmmKhnAgDPL71vW8Y+lO2pKi/66H0LmiINcq0iCS/0dZCCmj2tas7HYAgt+F4mu9pjGq6E2vM6BjF3wMZN8x8ImhA6ofMb5s/CTYtajjTl5mVOs7JynsgK2+EwkwBo7vewVXpoq/NCpA3VBuP77S/nfdf86RA7ZSRq8a9RGZ0J61+pMfwfLRZyq1BBPfaDxm5tBD6y+AgjyYgq0l1M0i0muI/h+6y73Fa/veK+puti/xrX3C8L4PFLRgTyikow+1A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=cd8Kc82Odqry9crqn/Nza7gnK98dQw2U/4eL3rxhsJA=; b=kheBhYcJgzxWLNpnyX3szd6wfi09A1gS7VzNz7EOTOk3s429VVasW+U3+WoadIZBy26WlK5lMY2DP+iGD7b/otj0NAIoQMxRMFb0Ek41zGwrW5pFFa6cLM0yD4SpdoTzIIUWc/b07HYS6vFK88eMCJVWw7X1VyfiybIdQtjQgCY= Authentication-Results: posteo.de; dkim=none (message not signed) header.d=none;posteo.de; dmarc=none action=none header.from=amd.com; Received: from DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) by DM5PR1201MB0121.namprd12.prod.outlook.com (2603:10b6:4:56::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4129.25; Mon, 17 May 2021 15:03:08 +0000 Received: from DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::b914:4704:ad6f:aba9]) by DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::b914:4704:ad6f:aba9%12]) with mapi id 15.20.4129.031; Mon, 17 May 2021 15:03:08 +0000 Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area To: Laszlo Ersek , devel@edk2.groups.io 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> From: "Lendacky, Thomas" Message-ID: <7d3d835a-4354-108d-17b7-8679eb8c67d1@amd.com> Date: Mon, 17 May 2021 10:03:06 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 In-Reply-To: <5394e010-6088-18e8-9a90-65eb55bbfac2@redhat.com> X-Originating-IP: [67.79.209.213] X-ClientProxiedBy: SA9PR13CA0153.namprd13.prod.outlook.com (2603:10b6:806:28::8) To DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) Return-Path: thomas.lendacky@amd.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from office-linux.texastahm.com (67.79.209.213) by SA9PR13CA0153.namprd13.prod.outlook.com (2603:10b6:806:28::8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4150.11 via Frontend Transport; Mon, 17 May 2021 15:03:07 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 05c761bc-f59c-4caa-7d37-08d91944e16c X-MS-TrafficTypeDiagnostic: DM5PR1201MB0121: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 6OCWq3bB8MHHEUDjrEopDufqFZtHKbBgs95IxiGz728LVlSfz3VSckG1Ik3y6glR0+AWvo8eFlLgFLyuXbnfNyM345adQ25XBfFG2wtCXOjCBWO03DLZK8yln7DZH5LZFVPU2buqLwJw48lS2C5H0gYPhfNMr3RXZaD88q9SzuR04Oq+zrjQTgPlM7PZ1us4swtNHxfiPtLY6wDxgc2qBJOe5Fqa4lG0IBfAmXwkceSx2m6x2uMEOQPnzWlxDgTDP6CmU69g0aq7X1Aqapp8MScDGKxBMNCiKcckQa5kskzL9vtc7niYtT8kH3LO0cngVcFGYnbcnA77blQbgQhb0ATgKA5aGZ18/k6BPxgKetRWX0iLlys+gw8Y21du2fXLzuKnZ0WwG7a1RG5hekfTavxOFHDLWj/+quG8DnufRvcUcgGKfO8FEvznNUlN1u9UponlXzHoJvlLlmVrhUxA8XGhyMZWFSfXk1mAverE1nPrYMbIALYMVgDFBMmN0X5AIIaDXptVwvWXth+SyGmmP1bRupxViU9qxJ/kYuVhqEoZjf/ib2OdsMUwNlcveKnctP1/Xwi2onNxrsWWnEn8sNsUd8GggboumEAoRBHnVpzZuvVCjsJKCNAx19RGTkHG48/uhSHWJgCPU8grJPn19hvwP9diWw6N0foRdMq542cm7/nIUVvzVK8rDWV0KPsBoz6hUnMMsQpNNgGN+/3bWP/6bXk2c7cz5GkG+6J53ypjX5lk1CKZqbSU2EI13l1tvp8NtKjRR9syT0LfrffSpA== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM5PR12MB1355.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(6029001)(4636009)(366004)(346002)(396003)(376002)(39850400004)(136003)(26005)(6486002)(31686004)(45080400002)(966005)(956004)(16526019)(6506007)(31696002)(6512007)(186003)(4326008)(53546011)(8936002)(66574015)(8676002)(2906002)(83380400001)(54906003)(38100700002)(5660300002)(316002)(66556008)(30864003)(86362001)(478600001)(2616005)(36756003)(66946007)(66476007)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?us-ascii?Q?rfjVUE0dE79V4BBQB1h5APZkDIZVwXnFdK6UwQIa8dSqdV/rru9Q0rNsxJ5K?= =?us-ascii?Q?E/CMFyhIGr0haj+6B8eQPVFIu/enLXZDj4XbErQXF1AZrK9sYKZb0QNEnKtO?= =?us-ascii?Q?d480SvA7EsS07wcYGcrtHnjNQ5RY0Y8QgrFUbfw/C1qo7aEKQoXTYuEx+Vut?= =?us-ascii?Q?Ng8dw2PJdVsFiN6LL1XL7Xrn0wrTyWJSfJIAuGRFWFtGPk8nZ9YQDk1Z7glu?= =?us-ascii?Q?6bwxfL02MN3S4snZKZYAespRyefY76Yj3v9Rg6sJRXtngCvI/8vGhl9XNVom?= =?us-ascii?Q?V/OZRQ8lH440PJ6Fxn4SmiQ4m6cE/ZYtorFZI9UOQH0H/CpPowyyUN/55QKz?= =?us-ascii?Q?Us5Esmiwz4F3nUmnajbHTJhTnTnSGa4foPXZtsbyFAdgLA77HQYRQbHS26PI?= =?us-ascii?Q?PHsOjzY93UwEaq11I1IL/x4KhsDI6QookzV3OYy4LpYPJGBlF3nlDB0HlseF?= =?us-ascii?Q?Qk3aS2a+yUd7xDMjAQH5Br5AF5wOoXprOqH2QgTxbDmXsbfjgFIw5kDYf85h?= =?us-ascii?Q?Dm5KGLcCF/B3RzlywySZ3iuBf2HYwKTr/6X0CiugY9qMAF9wJURutKQ21p4e?= =?us-ascii?Q?Mp1sciGZk6Qh58DWaSWRxbdF6mSUgc4Q8omR73oho0qDuJrZc9NAJ1UGCgut?= =?us-ascii?Q?qtRX75wWZKw5q6Ncv1is4KlK3C/VFyTGDZloymaILbhfgwCi6bqAWnzLT3I3?= =?us-ascii?Q?b+XPN8UVQKNM0hLktExwr53dVvGvGWv15bpXLqfnlO1y1Qp6zP2sC0NSKvAg?= =?us-ascii?Q?qmyQqJ5NgtX5UpJzA9KuXOY+vYGUDsR5M4DtXnbkKsgR7oQvUD/3yxfmwFnq?= =?us-ascii?Q?ol3bH2pYkxRbu2PWJTDvwgddWr560ruHm1QEfXSnSP32WAtJcE/iGaOvoAMk?= =?us-ascii?Q?NwO4hzcxUS1QkYeO19ktRPZXB4MSGe+/O+T2Wfqr19MyO2tIvWlqGR9NKf4+?= =?us-ascii?Q?iRd4fytLnfYyBPoOOQvbn/UMZ8kWG5QhSrLY2EphQPaHxXmQqwngWK2g/bBr?= =?us-ascii?Q?dnkTc8CcHkfVPByYz0b7DBv2HCEG40T5C+wmduerlkabDvD4+LHC1vwsfiQF?= =?us-ascii?Q?72hBavB7FmX7DTV3bl48bMdrkrLIN70fIYocWYv3NH/9pdIpM92hF0G7eA8R?= =?us-ascii?Q?eXLJBeDL86TYJydsqnw+qjZb3Nz96niWQHIhsa0fsZ/w/eeAXFBaPPz/xsdF?= =?us-ascii?Q?kdsehqMBqrKQ+Il856levPJUPNzoj/+IVm9096P6U4xRuggKdH91uVBqgFJ7?= =?us-ascii?Q?V3J5XmaqefucGIwqBBfkEPmdGucAoOnhHuhGMIvT3FSh3x8c2jbhSvtU/egC?= =?us-ascii?Q?OUtUXfSjHpa6/i4lYVd2qIuz?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 05c761bc-f59c-4caa-7d37-08d91944e16c X-MS-Exchange-CrossTenant-AuthSource: DM5PR12MB1355.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2021 15:03:08.1849 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: keRQO6P2s4XBr25MkrsVwUj6Tu5cR7W+/tEbswf/w42RbCUMW314ceFGI1eSr+Alw8RVO9DR9AB+KjZB7kLb+g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR1201MB0121 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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=3Dhttps%3A%2F%2F= bugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=3D04%7C01%7Cthom= as.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb6417%7C3dd8961fe4884e60= 8e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnknown%7CTWFpbGZsb3d8eyJWI= joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sd= ata=3DN9MNXaBazq2tiVRHWSPVXRdlcZ97JOf24mc7p0m5Tqw%3D&reserved=3D0 >> >> 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 ti= me >> 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 buffe= r >> allocation in order to ensure that the new buffer allocation is below th= e >> previous allocation. When PcdSevEsIsEnabled is false, the original logic >> is followed. >> >> Fixes: 7b7508ad784d16a5208c8d12dff43aef6df0835b >=20 > Is this really a *bugfix*? >=20 > I called what's being fixed "a gap in a generic protection mechanism", > in . >=20 > 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. >=20 > The distinction matters because of the soft feature freeze: >=20 > https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fgithu= b.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning&am= p;data=3D04%7C01%7Cthomas.lendacky%40amd.com%7C8142daa079b04c0b3b5508d918eb= 6417%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637568221542784370%7CUnkn= own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV= CI6Mn0%3D%7C1000&sdata=3D1n8z7KFAlm3Vb7fPOFpYQFlZ5lQFOF%2FdLtujjqhns9s%= 3D&reserved=3D0 >=20 > 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. >=20 > ... 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. >=20 >> Cc: Eric Dong >> Cc: Ray Ni >> Cc: Laszlo Ersek >> Cc: Rahul Kumar >> Cc: Marvin H=C3=A4user >> 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/Librar= y/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 =3D NULL; >> UINTN mReservedTopOfApStack; >> volatile UINT32 mNumberToFinish =3D 0; >> =20 >> +// >> +// Begin wakeup buffer allocation below 0x88000 >> +// >> +STATIC EFI_PHYSICAL_ADDRESS mSevEsDxeWakeupBuffer =3D 0x88000; >> + >> /** >> Enable Debug Agent to support source debugging on AP function. >> =20 >> @@ -102,7 +107,14 @@ GetWakeupBuffer ( >> // LagacyBios driver depends on CPU Arch protocol which guarantees be= low >> // allocation runs earlier than LegacyBios driver. >> // >> - StartAddress =3D 0x88000; >> + if (PcdGetBool (PcdSevEsIsEnabled)) { >> + // >> + // SEV-ES Wakeup buffer should be under 0x88000 and under any previ= ous one >> + // >> + StartAddress =3D mSevEsDxeWakeupBuffer; >> + } else { >> + StartAddress =3D 0x88000; >> + } >> Status =3D gBS->AllocatePages ( >> AllocateMaxAddress, >> MemoryType, >> @@ -112,6 +124,11 @@ GetWakeupBuffer ( >> ASSERT_EFI_ERROR (Status); >> if (EFI_ERROR (Status)) { >> StartAddress =3D (EFI_PHYSICAL_ADDRESS) -1; >> + } else if (PcdGetBool (PcdSevEsIsEnabled)) { >> + // >> + // Next SEV-ES wakeup buffer allocation must be below this allocati= on >> + // >> + mSevEsDxeWakeupBuffer =3D StartAddress; >> } >> =20 >> DEBUG ((DEBUG_INFO, "WakeupBufferStart =3D %x, WakeupBufferSize =3D %= x\n", >> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/M= pInitLib/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); >> =20 >> - // >> - // The AP reset stack is only used by SEV-ES guests. Do not add to th= e >> - // 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 +=3D AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcessor= Number); >> - >> - Size =3D ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT); >> - } >> - >> return Size; >> } >> =20 >> @@ -1192,6 +1178,7 @@ AllocateResetVector ( >> ) >> { >> UINTN ApResetVectorSize; >> + UINTN ApResetStackSize; >> =20 >> if (CpuMpData->WakeupBuffer =3D=3D (UINTN) -1) { >> ApResetVectorSize =3D GetApResetVectorSize (&CpuMpData->AddressMap)= ; >> @@ -1207,9 +1194,39 @@ AllocateResetVector ( >> CpuMpData->AddressMap.ModeTransitio= nOffset >> ); >> // >> - // The reset stack starts at the end of the buffer. >> + // The AP reset stack is only used by SEV-ES guests. Do not allocat= e it >> + // if SEV-ES is not enabled. >> // >> - CpuMpData->SevEsAPResetStackStart =3D CpuMpData->WakeupBuffer + ApR= esetVectorSize; >> + if (PcdGetBool (PcdSevEsIsEnabled)) { >> + // >> + // Stack location is based on ProcessorNumber, so use the total n= umber >=20 > 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. >=20 >> + // of processors for calculating the total stack area. >> + // >> + ApResetStackSize =3D (AP_RESET_STACK_SIZE * >> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); >> + >> + // >> + // Invoke GetWakeupBuffer a second time to allocate the stack are= a >> + // below 1MB. The returned buffer will be page aligned and sized = and >> + // below the previously allocated buffer. >> + // >> + CpuMpData->SevEsAPResetStackStart =3D GetWakeupBuffer (ApResetSta= ckSize); >> + >> + // >> + // Check to be sure that the "allocate below" behavior hasn't cha= nged. >> + // This will also catch a failed allocation, as "-1" is returned = on >> + // failure. >> + // >> + if (CpuMpData->SevEsAPResetStackStart >=3D CpuMpData->WakeupBuffe= r) { >> + 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/Librar= y/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 >> =20 >> +STATIC UINT64 mSevEsPeiWakeupBuffer =3D BASE_1MB; >> + >> /** >> S3 SMM Init Done notification function. >> =20 >> @@ -220,7 +222,13 @@ GetWakeupBuffer ( >> // Need memory under 1MB to be collected here >> // >> WakeupBufferEnd =3D 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 pre= vious one >> + // >> + WakeupBufferEnd =3D mSevEsPeiWakeupBuffer; >> + } else if (WakeupBufferEnd > BASE_1MB) { >> // >> // Wakeup buffer should be under 1MB >> // >> @@ -244,6 +252,15 @@ GetWakeupBuffer ( >> } >> DEBUG ((DEBUG_INFO, "WakeupBufferStart =3D %x, WakeupBufferSi= ze =3D %x\n", >> WakeupBufferStart, WakeupBufferSize)); >> + >> + if (PcdGetBool (PcdSevEsIsEnabled)) { >> + // >> + // Next SEV-ES wakeup buffer allocation must be below this >> + // allocation >> + // >> + mSevEsPeiWakeupBuffer =3D WakeupBufferStart; >> + } >> + >> return (UINTN)WakeupBufferStart; >> } >> } >> >=20 > 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. >=20 > 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. >=20 > 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. >=20 > But now, with SEV-ES enabled, we'll have a separate, discontiguous area > -- and neither BackupAndPrepareWakeupBuffer(), nor its counterpart > RestoreWakeupBuffer() take that into account. >=20 > 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. >=20 > 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. >=20 > 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. Thanks, Tom >=20 > Thanks > Laszlo >=20