From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (NAM11-DM6-obe.outbound.protection.outlook.com [40.107.223.74]) by mx.groups.io with SMTP id smtpd.web12.8974.1621520524676187162 for ; Thu, 20 May 2021 07:22:04 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@amd.com header.s=selector1 header.b=G6oIyNta; 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.223.74, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=R/6iVSbV2gBA1Kitq6qI9u9WoqR6RdjbjI6TCIhIpFXOyjqRgXUG7fU61SmGwIXgNmX3oDVW7jKwY98/y4ELAbEmUgXyZDFZ2q60vVMKuWigZ5EWU7wGmKSTwWQwhvw/ISjFjl14gfdwZoZmU9IiDtxciEeB2OrsxFs4snrJjPiAubzIkd7NBBITjC96juuCstZU7KbhLwF18SHHroV3xA5MDT9ovTneBYHc9Cqzzda7qwpJUPxPWj3eCALnH2vxIlzrz46+dq9XmFlrgpyjritvpZWVvu/TySZ84prFr0zuM9pVmaCbBw5fXX5YdNQ0p4bWNsARa2+GkDDeK4BpLQ== 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=uaR5DKld0VX5hqmxPVD1x8/x5FvX2aiar2g4CWw3K84=; b=lm98v8YxHkSHFrA0UrsMTxAjqRIwJ99gJEq/JHXkvPjt6FqsndBNbyoRSOGnD6V4YTzEDWTMtxGK4z50dxeA984uEn34Rx4Z33R78ij2HOUaxydqepcRNxp+06uPBsFrvGLdAUSf0NPGQVb2sjqdm9C1UrJQQi3kYdOK/XDzTcJX12CanLyFZmt7kfbZpb0A/oj6BPNiD11O6lLD98Kha2po9qYHbQs0nPHikAKkJC9Per8+1407jFvekt7PWHa/UsidxdTfZNy/LXIylHVLb0gdK8cs2nAT2hPmXYUJKYHgd6GQK59V1C4SYQPRicg3du4Tovc0YZLM0CLoZ65vjQ== 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=uaR5DKld0VX5hqmxPVD1x8/x5FvX2aiar2g4CWw3K84=; b=G6oIyNta0lcv3DmfoKKxyp6M9BWVSmngnE5PIfeIQAo6q+IaPnQc2iB9g201xZzkxhgu/k1EpgbipD1eGbmDDH64m7zVWf0NVcttATz31IljrHo1QhNdJaJju/++m6L2cENhKsdgVsfQc+O1EsDZQz9BEIZJSC2u5fglx4z9WUE= 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 DM5PR12MB1772.namprd12.prod.outlook.com (2603:10b6:3:107::10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4150.23; Thu, 20 May 2021 14:22:02 +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.033; Thu, 20 May 2021 14:22:02 +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> <7d3d835a-4354-108d-17b7-8679eb8c67d1@amd.com> <61a02049-7280-1a25-c718-c663acfcc56e@redhat.com> From: "Lendacky, Thomas" Message-ID: Date: Thu, 20 May 2021 09:21:59 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 In-Reply-To: <61a02049-7280-1a25-c718-c663acfcc56e@redhat.com> X-Originating-IP: [67.79.209.213] X-ClientProxiedBy: SN7PR04CA0158.namprd04.prod.outlook.com (2603:10b6:806:125::13) 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 SN7PR04CA0158.namprd04.prod.outlook.com (2603:10b6:806:125::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4129.32 via Frontend Transport; Thu, 20 May 2021 14:22:01 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 479696e3-3047-4a7f-4d83-08d91b9aa2d0 X-MS-TrafficTypeDiagnostic: DM5PR12MB1772: 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: iA7JhYHzyj4naXRj5Ol7m0OMoh+m6y+Jq2FDSBJLD/pzA5pWgobuyBVkXtl1JyDAkU2Ewd9jJpbMD0dy7rUy/cCFtCL7j/Tfh6vd8pz05eF611ILoaU6+t7RFEvD3+XKSfpc/crGC+ldYlKCHLFBhRulcT8TJbj8DXtFjbLpuy22gOEE+UBEZMHGgkyKTY/Q5SZfNWDvp56vyr1RDyfV17wZf81J8r50o5nGVImVTXQETqVwPVRbtUv+US3VVvj928gN/nOb7tfW6RVZLwau5V4G8lhtZr5XWX6wIUeTA+9SSpM+icyAY67nlJiSOxopILnuA/FRCvQMEM1reH2ZMyaHfcHqHdPA5fo98zYSreh+TO9KyIGo2//b9pQAAsin0W/q3f32JXc/Vc86710RoerTqr77fQv27jMUibQzXmuA5KkGjNU49R0rVepKLHPr3ojhnlf4VsJrE9EY82z6jAF8bIEjfKHzTABBf358jyrDH76n9a5ZsXEiGQg9YGId5HjbytVoVkqqsFFfyHGJ9jo+WpKeHZGYq9OAuXh6KiRtwEdSAnPnJx0Hy3REpZrG826AFSVtL2PHS+Ubw5FqNHJZtv4ArldGmo3xF0hTrQMDO5XPxmZdDMkZ5PGzE1ZXfQoupdvU6O3hKwhR6LQQWpAiaGKpB6ZeeouMQdoHbbC5RqU7nyH/yDfv9e0PZ7GAHsLYbS8xdcz2DM6ehMShcBKTnwrD2tmX+NnjQTmMywVtVKranyyvEWtTFUaogybtSoiHI1MEwyyCTVKDci4/WhibUNO/iGwmYFVuUX2HcMA= 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)(16526019)(66556008)(186003)(86362001)(6512007)(83380400001)(66476007)(66574015)(31696002)(53546011)(6506007)(66946007)(966005)(498600001)(8676002)(956004)(54906003)(2616005)(38100700002)(31686004)(45080400002)(36756003)(8936002)(6486002)(5660300002)(4326008)(30864003)(2906002)(26005)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?us-ascii?Q?Ocv+ADvn0obMhhxsWL6Sm5topMLlZ4HaPw/vLRwaUHmUxrv5C2VqrkM+p8Kf?= =?us-ascii?Q?EINuPg3+pduj9aC4LCkTahiqbepGQrgvQgWTz9k8+5XV+xyw9wzJJYkDzdqt?= =?us-ascii?Q?JXWTZZllqSn0dv6vnza54jmCHh+kSKGxrMicJD/oY1Hi3vW8nlkWm61xCGD7?= =?us-ascii?Q?6a6C8iNjoJAj21pVvcavrZGRK/tlpEq2ZN5BubryaZ0D1JAhy9GwglEnouuG?= =?us-ascii?Q?TcGzHJE3tKDaicPWZOqM+9bSsBZjv0+WF8JP297TQti99kWox79iHybSUV4l?= =?us-ascii?Q?M0+8PRS/otAyNI6CJQLlFPHyS5oZgLfRpq1yHgvCL9urvUpJsMEbdJXw/Tfp?= =?us-ascii?Q?fRxVAAqKdOC9geWTL077AcssWPkNBIcKwgsjRMcvzm1S4LzIKYwrWaR2oCdT?= =?us-ascii?Q?KqCp622mZMqFR2y5cWkANs2ydnlKxaztsD7nCB+32J++s8//86CWDlE5SUPr?= =?us-ascii?Q?bTviPetkRPehYbv0ODDtQe4TkKySN3CjqIpecSaXZGpBBTj6dBelLfVt5zZo?= =?us-ascii?Q?RvvM9YbMvHM+h0dSoRA2pilIYYmD1OqXjjeUaKBqwtOrdVSE0zjj9Nel/TQv?= =?us-ascii?Q?0uOStN5XuBnDNvxtrYJ5iVUj9EQIBaJNtgri1gIeYwfk+USJroB8Ep/BTcMb?= =?us-ascii?Q?6+MeuuoT5KfI72cLwOEe95tx5cXRHa288JUtJ0s0DVWuYm4gxmb9UQ2H2l5W?= =?us-ascii?Q?XAIkDBgv6bs4bBuDlTGHNuu4cNXnM3Ngv4Lqn1mvnxZs6mN8qjY3UDVTTxGH?= =?us-ascii?Q?EipI2YqWzfTneH0GCf7OqnegQiHd0Fudisutde9C2htKkMPBCN2gI0tsZ0Vj?= =?us-ascii?Q?FZ5PMcVLaDHcfa/4sfqWIdrumi8JFsTatw1Mh30hbh98IWw6J+9IahG0/0KF?= =?us-ascii?Q?0cap2CcgtT5qyQe2DrvKVxoN2cVFmoisW3NFmdZ9xNHX+rbH0Wv3uLLFgnMI?= =?us-ascii?Q?VJRqYK6DUa2p2dv6a4YN0Pi0oYLRuqgeP1nzZR+ieQjJAKz/Hwt+Pk7d3peC?= =?us-ascii?Q?oA0f4AX0EQM0mfobVvvSVcZAvX25uL/xWBrdfEFOwWZxY38OZBWfnl01dq5Q?= =?us-ascii?Q?Dxqg6UI7gx31G0dlQj5HBt/NmNGypRXjGh+0S423CAJSFKqIWM8FgRPOBwnR?= =?us-ascii?Q?t/usSdb5urCDc4TlJ1gqjlz3YCdNOxRwoOjblMzdbuvx/EkOCM4aUeNiLmGJ?= =?us-ascii?Q?S5Zgnvj6Cuyfi2xOBBA93B/PpAvJt66pRD72fcvxou2JKvA6gM1+wNK2urYx?= =?us-ascii?Q?vyWZCRUS3aeVtZerEdqCiCUyV/FyWei2boKVxw3PHdaM0dJZMnRW2BeZD+Sa?= =?us-ascii?Q?3wSPgi0735PsjVIqUtsZeonq?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 479696e3-3047-4a7f-4d83-08d91b9aa2d0 X-MS-Exchange-CrossTenant-AuthSource: DM5PR12MB1355.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 May 2021 14:22:02.2398 (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: IU57Im/f9qhXLWB8Ugi6uHG06QxMn+6you8e+PhDSJmtTHp8KgFXWOMBXHTE2MIHSHrXL4P2Ey/Il/bgd5fngQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1772 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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=3Dhttps%3A%2F%= 2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=3D04%7C01%7Cth= omas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a20d05a%7C3dd8961fe4884e= 608e11a82d994e183d%7C0%7C0%7C637569550503778789%7CUnknown%7CTWFpbGZsb3d8eyJ= WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&= sdata=3D194iu8I6ucUBr6bNR0fIBa%2FgQhtUQQpJdN55gzOHlmY%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 avoi= d >>>> 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 buf= fer >>>> allocation in order to ensure that the new buffer allocation is below = the >>>> previous allocation. When PcdSevEsIsEnabled is false, the original log= ic >>>> 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=3Dhttps%3A%2F%2Fgit= hub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FEDK-II-Release-Planning&= amp;data=3D04%7C01%7Cthomas.lendacky%40amd.com%7Cec6b583f740d45dfb96408d91a= 20d05a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637569550503788787%7CUn= known%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ= XVCI6Mn0%3D%7C1000&sdata=3DbZJ%2BK1Y%2Fs4CXXy%2Fz3IKVnWWCUSz3Noo3cPsWR%= 2Fby5H0%3D&reserved=3D0 >>> >>> 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 a= s >>> 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=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-fr= ee >>>> 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/Libr= ary/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 = below >>>> // allocation runs earlier than LegacyBios driver. >>>> // >>>> - StartAddress =3D 0x88000; >>>> + if (PcdGetBool (PcdSevEsIsEnabled)) { >>>> + // >>>> + // SEV-ES Wakeup buffer should be under 0x88000 and under any pre= vious 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 alloca= tion >>>> + // >>>> + 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= /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); >>>> =20 >>>> - // >>>> - // 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 +=3D AP_RESET_STACK_SIZE * PcdGet32 (PcdCpuMaxLogicalProcess= orNumber); >>>> - >>>> - 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->AddressMa= p); >>>> @@ -1207,9 +1194,39 @@ AllocateResetVector ( >>>> CpuMpData->AddressMap.ModeTransit= ionOffset >>>> ); >>>> // >>>> - // The reset stack starts at the end of the buffer. >>>> + // The AP reset stack is only used by SEV-ES guests. Do not alloc= ate it >>>> + // if SEV-ES is not enabled. >>>> // >>>> - CpuMpData->SevEsAPResetStackStart =3D CpuMpData->WakeupBuffer + A= pResetVectorSize; >>>> + 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 =3D (AP_RESET_STACK_SIZE * >>>> + PcdGet32 (PcdCpuMaxLogicalProcessorNumber))= ; >>>> + >>>> + // >>>> + // Invoke GetWakeupBuffer a second time to allocate the stack a= rea >>>> + // below 1MB. The returned buffer will be page aligned and size= d and >>>> + // below the previously allocated buffer. >>>> + // >>>> + CpuMpData->SevEsAPResetStackStart =3D GetWakeupBuffer (ApResetS= tackSize); >>>> + >>>> + // >>>> + // Check to be sure that the "allocate below" behavior hasn't c= hanged. >>>> + // This will also catch a failed allocation, as "-1" is returne= d on >>>> + // failure. >>>> + // >>>> + if (CpuMpData->SevEsAPResetStackStart >=3D CpuMpData->WakeupBuf= fer) { >>>> + 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/Libr= ary/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 + H= ob.ResourceDescriptor->ResourceLength; >>>> - if (WakeupBufferEnd > BASE_1MB) { >>>> + if (PcdGetBool (PcdSevEsIsEnabled) && >>>> + WakeupBufferEnd > mSevEsPeiWakeupBuffer) { >>>> + // >>>> + // SEV-ES Wakeup buffer should be under 1MB and under any p= revious 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, WakeupBuffer= Size =3D %x\n", >>>> WakeupBufferStart, WakeupBufferSize)); >>>> + >>>> + if (PcdGetBool (PcdSevEsIsEnabled)) { >>>> + // >>>> + // Next SEV-ES wakeup buffer allocation must be below thi= s >>>> + // allocation >>>> + // >>>> + mSevEsPeiWakeupBuffer =3D 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, w= e >>> 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. >=20 > I apologize for missing this. I'm not too familiar with the SEV-ES > specifics in UefiCpuPkg. >=20 > 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). >=20 > (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. >=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. >>> >>> 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. >=20 > Here's what I'd like to do: >=20 > - 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. >=20 > - 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. >=20 > - Figure out if we need to conditionalize the > BackupAndPrepareWakeupBuffer() call (or a part of that function anyway). >=20 > We can and should continue discussing these things during the feature > freeze; I'd like us to merge the patch after the tag. >=20 > Sorry again that I'm missing bits and pieces; I'm this close |<->| to > email bankruptcy. I know that feeling, stay solvent! Thanks, Tom >=20 > Thanks > Laszlo >=20