From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (NAM10-MW2-obe.outbound.protection.outlook.com [40.107.94.89]) by mx.groups.io with SMTP id smtpd.web09.10255.1621005785320087779 for ; Fri, 14 May 2021 08:23:05 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@amd.com header.s=selector1 header.b=FzRNi8Sy; 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.94.89, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=SUW2xq0SHRLEZwxFx1UD8CLX71/P/QTho5pAhcoBcI0j2wsVaNHzp5au+9k3Eh4IO+3NlTXBrBSdumcHZSXPGF1d2afpaSVOo0h1EbftvYDm5VEHJZ5UvIEs3q3CxFIe+282Wrb3DO6AX/43AfbLY5xVnChpGmNqAKdfsUlrElsoQMbGQgw8vsiBrxM9XGhhpoW3PzCuvvAYXydVuh8nhqsHa3imPSUd4ER8JCKBVyz2/OBO7gROwZ+fW1eRdETWJhhKdr6qjkoZ9Jsv7sC/A63ztVv0owEKjXHNgqjmMLAwpBqBVcby/7p+Ap8jOBMzR5pxny2Xh3tAwWOSi4jc5w== 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=U27w6Rd/e3P3T2coxTVrwDIic6/d9rZP0p3ZIFlgNqY=; b=H5FkTVhPrES44Z7nxxQpVESEV2sKHf/T3EtCB4gDA9Yrj8nRxWn8XwY6XL7RzFn7EEIufjyi4M3fL1QJUn0AVc8CvP7sCB0r6hHmzva4rtAq6Nh4Z17xaf76hANPHi8pPzhT58SsBgKO76Ph2fyAfUojKtdMmjqPvv0/ZjPZ72oJBa3p3nKMsKiSogii8CgLcekTOfDQluiwiSK3OcXXpsdAl9fgNMef3oGu5WyXMqROLqmD/eu9sWow++TBKCzDzhO8Fm8A4Ii2NKEhYKBs8NxI8NwSvYyLhV/9ZspWCDBOwadDccSv/4p6ACwdrkjmMo4UOhshdD8Z6wLN+1zDIA== 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=U27w6Rd/e3P3T2coxTVrwDIic6/d9rZP0p3ZIFlgNqY=; b=FzRNi8Sy+eWVQbd3rIFHPJs8STxNSbroOWSzGRgBdYMmtuxjm7dgwfrdqEWLDJgG0gVM8Pj4Epl83kTcVc/2Ha0zIwqL/6YCKkCKL6KePad9oePjm7Ayu3ZlEo1irhmKJ197Wgq0YqB5s3cHoIjd1NvAmobxejpjGTWhpnOv2Kw= Authentication-Results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=amd.com; Received: from DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) by DM5PR1201MB0124.namprd12.prod.outlook.com (2603:10b6:4:58::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4129.25; Fri, 14 May 2021 15:23:03 +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.025; Fri, 14 May 2021 15:23:03 +0000 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Allocate a separate SEV-ES AP reset stack area To: =?UTF-8?Q?Marvin_H=c3=a4user?= , devel@edk2.groups.io CC: Brijesh Singh , Eric Dong , Ray Ni , Laszlo Ersek , Rahul Kumar References: <1162d1f4fe09048aaafba6f6ea3046bebd34fc2b.1620766224.git.thomas.lendacky@amd.com> From: "Lendacky, Thomas" Message-ID: Date: Fri, 14 May 2021 10:23:00 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 In-Reply-To: X-Originating-IP: [67.79.209.213] X-ClientProxiedBy: SN4PR0401CA0023.namprd04.prod.outlook.com (2603:10b6:803:21::33) 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 SN4PR0401CA0023.namprd04.prod.outlook.com (2603:10b6:803:21::33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4108.25 via Frontend Transport; Fri, 14 May 2021 15:23:02 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: abbaa32f-7e2f-4dac-dbff-08d916ec2a5c X-MS-TrafficTypeDiagnostic: DM5PR1201MB0124: 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: cVJx19HEvqCGmIRMV2BX5nok2NhTWZwmYOURczvI76JmyTs2bY4Ks045SAHpBcULo+ilp+9r/b/5MxU+Qd5HEeLovYMva2Q+cXbMz67hu5fHh6z8gnH0VjguXgB47CJ+X3lgF8amh9xgGLruJEC5KWfrgsSi48GbjyTYPqEEW5w/eMqACEFthZ68dar185QnTa0NlFP9+CZUlTSAuKDQ+cwI+z2jhSKVKSCCzaG0Za0UXunhh88xUAjQoSoTx3DF4z235H9GSiNQTrk85MJxTxCdMPPq0ZoMoqce7Af4YsbmgmYMLpuipiaC6RKeFeo+RdnSyJuoM4vL2S2CBmJAIvO37+SRJjo8kZnYUiZ6SV9lQ2W7XuoY3Gs6OoTuTubxk8hEvvG2xWSmNz7gEP8Jpf/o0wNYuo/JwR6hO/liYTGJUP+h8HbfWxPfEIC6Ee/j7QSnfQFaTdacEZzbkyo6PyfqBrEiF3CI05RJ4q7MURMR3ciAohwaeC71UnRPdSOLEtf09O/hx5oKgCePswNJBThRaLuAqQs1r50WBBdNXZ0Lpex5uG/3qRbzWP98KBn6OWCshTl42vqRVMYRsWvG0F/jhPyles2vQLkHW4TybfaE2R9NgDmuKZJibgi6qiisHJmJTzRaAtW6NfQ7e+1E4d8Vf618pZkWgajh0GF2awf3HaV5dOfHXek/5oLyoVraWZSIcVOtfhG9juX5KGg73ECnSWwVgqWKwj32FHk6kPfY+otB97FBXfizd5E/1ZiALp8NeWG/wi4RxZumReEq8w== 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)(376002)(366004)(39850400004)(346002)(396003)(136003)(31696002)(86362001)(36756003)(8936002)(38100700002)(66574015)(5660300002)(83380400001)(66476007)(66946007)(26005)(2616005)(956004)(186003)(2906002)(4326008)(966005)(316002)(54906003)(6512007)(8676002)(6486002)(16526019)(478600001)(66556008)(45080400002)(31686004)(53546011)(6506007)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?us-ascii?Q?OFbhA2g3MWiI88JuG+U44AQ5RyBCOOpi0LmJVx5yoq0HaqI0BRFqFeccYbzr?= =?us-ascii?Q?WUd1ADxDEfn+8yHGKe8VlSkRo49X7IY1XMQhyTw10134IjXCDqa8Cx7dH7cl?= =?us-ascii?Q?53O/j5nQe2/2RFxxWj3dc9VZMV27M2YH9W4Ljx8WkwowNbJuWZt4r/ieKunB?= =?us-ascii?Q?S3S0RH1EZDnsycSErtBY9tAyXomGrp87OD2ipKwNgvyxFoD89EgYUDktdhNG?= =?us-ascii?Q?pConRtfivtquoykuDxHsYK4U6smYhHF0BBxsERFWA2r2n9BNuXDWJ1HtnzEp?= =?us-ascii?Q?SMFGYorEIKzDrJTs2caJ92J1FJ8h8ckdB015DD//xWVzdD/66hAXtuHXZi5q?= =?us-ascii?Q?aH83bbOOW0yUocL69LdWcSYBR/zRd6jHpSit2sSHDLglKrPQ7vGWhDCHsm0x?= =?us-ascii?Q?EOFlLDOYyE0TtyMRqzVqOX/W9esGMPAiIxzfl+YjvCGlohECZHhtUqXflXVY?= =?us-ascii?Q?8nWlmEvMqtFgH6FO399oz7FhEjggbR4wfwKJFS0tDlrHimrwt6g8AbZO7jJ9?= =?us-ascii?Q?HVRmBukjUR0Oo1JU0ECHvSn/QhiEp6LnvCqze2lsLv1PIht9UAuX8obAhVRU?= =?us-ascii?Q?30na9BCtRt/3edarEK25c4igbZ4O593dIkCCYsRu0AGE5gNC44/ZTdjxy8PH?= =?us-ascii?Q?D7A1Z3QCJWmvgDQFRLQkWHmrLUo41PBYCgM/p6a1IC3IcI++D09WU57Oo0M5?= =?us-ascii?Q?YqOU9creczcJ0WIV6Hr01gJcwVkYLi6rK+3vivuGrbEsIF7KrvHes9KwUrGo?= =?us-ascii?Q?hE+B8lkXwqjO1eWBr5o9dyC63gFHJqE+a3amqGIhnd09HTvKIAJL4tWH8XOr?= =?us-ascii?Q?C1RrcxE1XbL45pptzkAz5+z2V+A0/Vz+jq8pqkYM6vRfczxX7CxfBS1uoiCr?= =?us-ascii?Q?WGNmsiP8gKSkXwnCEhLP9rMWSOpLCz3tLA9m2Imbcz0CnDgUvbYqCj+yYupp?= =?us-ascii?Q?c7fW1ZuXBKv3IJfIl5eskgTw3ZgYSw+AwBqIlctmzD3zzvLUgSbHeq5lPhC4?= =?us-ascii?Q?JO7Cs5LvJURsk5SheAXJXCyJskgMVAaSuPMx3KZstIuUk1NVFU9H9uF9xqAe?= =?us-ascii?Q?0T+0UYYf5Vc9JntWRtEuUA5i283oKrA8axRduBdLBlEyWIDgjgV3CkTDUOdk?= =?us-ascii?Q?vPdIBFUvclbU+lLwuERn8Uj2tocJMi0125qZsu7KWqeXmKNicJC6525WZtDR?= =?us-ascii?Q?tOfn1sWkAj1d+G8KOZE3W5wcyUFhSWbGue+2O9sX6POio4vsp3N9s5inag9+?= =?us-ascii?Q?uG4PvFV6J9wVC4kkNKByvQO9pbfU+XpiarsqPhxmoZqx8Hgi7QsUgP0s3MT6?= =?us-ascii?Q?X8s0isdjPwSCUDJetV3Iy5BU?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: abbaa32f-7e2f-4dac-dbff-08d916ec2a5c X-MS-Exchange-CrossTenant-AuthSource: DM5PR12MB1355.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2021 15:23:02.9836 (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: gbvnLjCbXfMIQTXNZv+gxhaLi59kJHonMlmN5ov3P/c+lZF1KgILw+/mWi58RSbJ7dt5Jz8KsmwDvT0GTADfIg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR1201MB0124 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 5/14/21 10:04 AM, Marvin H=C3=A4user wrote: > Good day Thomas, Hi Marvin, >=20 > Thank you very much for the quick patch. Comments inline. >=20 > Best regards, > Marvin >=20 > On 11.05.21 22:50, Lendacky, Thomas wrote: >> BZ: >> https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fbugz= illa.tianocore.org%2Fshow_bug.cgi%3Fid%3D3324&data=3D04%7C01%7Cthomas.l= endacky%40amd.com%7C25d0b0bdc5d14a8bc4ff08d916e99c10%7C3dd8961fe4884e608e11= a82d994e183d%7C0%7C0%7C637566014883375137%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM= C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata= =3D8%2BywcjFoZ00AymlBdxt%2BMCP0JClc64soY6pwcfz08zo%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 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 >> --- >> =C2=A0 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 12 ++++- >> =C2=A0 UefiCpuPkg/Library/MpInitLib/MpLib.c=C2=A0=C2=A0=C2=A0 | 48 +++++= ++++++++------- >> =C2=A0 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 14 ++++-- >> =C2=A0 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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 *mReservedApLoopFunc =3D NULL; >> =C2=A0 UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 mReservedTopOfApStack; >> =C2=A0 volatile UINT32=C2=A0 mNumberToFinish =3D 0; >> =C2=A0 +// >> +// Begin wakeup buffer allocation below 0x88000 >> +// >=20 > 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://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fgithu= b.com%2Ftianocore%2Fedk2%2Fcommit%2Fe4ff6349bf9ee4f3f392141374901ea4994e043= e&data=3D04%7C01%7Cthomas.lendacky%40amd.com%7C25d0b0bdc5d14a8bc4ff08d9= 16e99c10%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637566014883375137%7C= Unknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL= CJXVCI6Mn0%3D%7C1000&sdata=3DNHR7dQjJbXQK5j4zc0ni0Q%2FZtOQp7szwDEzpHY6V= 9Dc%3D&reserved=3D0 >=20 I think a separate patch would be best... and since you understand it so well now, maybe something you could submit :) >=20 >> +STATIC EFI_PHYSICAL_ADDRESS mWakeupBuffer =3D 0x88000; >=20 > Hmm, I wonder whether a global variable is the best solution here. With a= n > explanation from the commit above, a top-down allocator makes good sense > for this scenario. However, a "GetWakeupBuffer(Size, MaxAddress)" functio= n > might be more self-descriptive. What do you think? Given the previous comments to not introduce any regressions in the non-SEV-ES path, it is probably not a good idea to change this as part of this patch. A separate distinct patch would be best. But understand that GetWakeupBuffer() has a different implementation in PEI and DXE. The only common thing is that GetWakeupBuffer() knows to be under 1MB. PEI doesn't have the 0x88000 limitation that DXE does, so I don't think the MaxAddress parameter helps here. >=20 >> + >> =C2=A0 /** >> =C2=A0=C2=A0=C2=A0 Enable Debug Agent to support source debugging on AP = function. >> =C2=A0 @@ -102,7 +107,7 @@ GetWakeupBuffer ( >> =C2=A0=C2=A0=C2=A0 // LagacyBios driver depends on CPU Arch protocol whi= ch guarantees >> below >> =C2=A0=C2=A0=C2=A0 // allocation runs earlier than LegacyBios driver. >> =C2=A0=C2=A0=C2=A0 // >> -=C2=A0 StartAddress =3D 0x88000; >> +=C2=A0 StartAddress =3D mWakeupBuffer; >> =C2=A0=C2=A0=C2=A0 Status =3D gBS->AllocatePages ( >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 AllocateMaxAddress, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MemoryType, >> @@ -112,6 +117,11 @@ GetWakeupBuffer ( >> =C2=A0=C2=A0=C2=A0 ASSERT_EFI_ERROR (Status); >> =C2=A0=C2=A0=C2=A0 if (EFI_ERROR (Status)) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 StartAddress =3D (EFI_PHYSICAL_ADDRESS) -= 1; >> +=C2=A0 } else { >> +=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0 // Next wakeup buffer allocation must be below this = allocation >> +=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0 mWakeupBuffer =3D StartAddress; >> =C2=A0=C2=A0=C2=A0 } >> =C2=A0 =C2=A0=C2=A0=C2=A0 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..a76dae437606 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >> @@ -1164,20 +1164,6 @@ GetApResetVectorSize ( >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= AddressMap->SwitchToRealSize + >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= sizeof (MP_CPU_EXCHANGE_INFO); >> =C2=A0 -=C2=A0 // >> -=C2=A0 // The AP reset stack is only used by SEV-ES guests. Do not add = to the >> -=C2=A0 // allocation if SEV-ES is not enabled. >> -=C2=A0 // >> -=C2=A0 if (PcdGetBool (PcdSevEsIsEnabled)) { >> -=C2=A0=C2=A0=C2=A0 // >> -=C2=A0=C2=A0=C2=A0 // Stack location is based on APIC ID, so use the to= tal number of >> -=C2=A0=C2=A0=C2=A0 // processors for calculating the total stack area. >> -=C2=A0=C2=A0=C2=A0 // >> -=C2=A0=C2=A0=C2=A0 Size +=3D AP_RESET_STACK_SIZE * PcdGet32 >> (PcdCpuMaxLogicalProcessorNumber); >> - >> -=C2=A0=C2=A0=C2=A0 Size =3D ALIGN_VALUE (Size, CPU_STACK_ALIGNMENT); >> -=C2=A0 } >> - >> =C2=A0=C2=A0=C2=A0 return Size; >> =C2=A0 } >> =C2=A0 @@ -1207,9 +1193,39 @@ AllocateResetVector ( >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 >> CpuMpData->AddressMap.ModeTransitionOffset >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= ); >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> -=C2=A0=C2=A0=C2=A0 // The reset stack starts at the end of the buffer. >> +=C2=A0=C2=A0=C2=A0 // The AP reset stack is only used by SEV-ES guests.= Do not >> allocate it >> +=C2=A0=C2=A0=C2=A0 // if SEV-ES is not enabled. >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> -=C2=A0=C2=A0=C2=A0 CpuMpData->SevEsAPResetStackStart =3D CpuMpData->Wak= eupBuffer + >> ApResetVectorSize; >> +=C2=A0=C2=A0=C2=A0 if (PcdGetBool (PcdSevEsIsEnabled)) { >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 UINTN=C2=A0 ApResetStackSize; >=20 > 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? Yup, noted in other comments. So the variable will be moved. >=20 >> + >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Stack location is based on ProcessorN= umber, so use the total >> number >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // of processors for calculating the tot= al stack area. >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ApResetStackSize =3D AP_RESET_STACK_SIZE= * >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 PcdGet32 (PcdCpuMaxLogicalProcessorNumber); >> + >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Invoke GetWakeupBuffer a second time = to allocate the stack area >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // below 1MB. The returned buffer will b= e page aligned and sized and >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // below the previously allocated buffer= . >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CpuMpData->SevEsAPResetStackStart =3D Ge= tWakeupBuffer >> (ApResetStackSize); >> + >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Check to be sure that the "allocate b= elow" behavior hasn't >> changed. >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // This will also catch a failed allocat= ion, as "-1" is returned on >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // failure. >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (CpuMpData->SevEsAPResetStackStart >= =3D CpuMpData->WakeupBuffer) { >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG ((DEBUG_ERROR, >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "SEV-ES AP reset= stack is not below wakeup buffer\n")); >> + >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ASSERT (FALSE); >=20 > Should the ASSERT not only catch the broken "allocate below" behaviour, > i.e. not trigger on failed allocation? I think it's best to trigger on a failed allocation as well rather than continuing and allowing a page fault or some other problem to occur. Thanks, Tom >=20 >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 CpuDeadLoop (); >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >> +=C2=A0=C2=A0=C2=A0 } >> =C2=A0=C2=A0=C2=A0 } >> =C2=A0=C2=A0=C2=A0 BackupAndPrepareWakeupBuffer (CpuMpData); >> =C2=A0 } >> 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 @@ >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 +STATIC UINT64 mWakeupBuffer =3D BASE_1MB; >> + >> =C2=A0 /** >> =C2=A0=C2=A0=C2=A0 S3 SMM Init Done notification function. >> =C2=A0 @@ -220,11 +222,11 @@ GetWakeupBuffer ( >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Need memory un= der 1MB to be collected here >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 WakeupBufferEnd = =3D Hob.ResourceDescriptor->PhysicalStart + >> Hob.ResourceDescriptor->ResourceLength; >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (WakeupBufferEnd > BASE_1= MB) { >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (WakeupBufferEnd > mWakeu= pBuffer) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Wakeup buffer= should be under 1MB >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Wakeup buffer= should be under 1MB and under the previous one >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 WakeupBufferEnd = =3D BASE_1MB; >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 WakeupBufferEnd = =3D mWakeupBuffer; >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 while (WakeupBuff= erEnd > WakeupBufferSize) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> @@ -244,6 +246,12 @@ GetWakeupBuffer ( >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DEBUG= ((DEBUG_INFO, "WakeupBufferStart =3D %x, >> WakeupBufferSize =3D %x\n", >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 WakeupBufferStart, WakeupBuff= erSize)); >> + >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Next wakeup b= uffer allocation must be below this allocation >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mWakeupBuffer = =3D WakeupBufferStart; >> + >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 retur= n (UINTN)WakeupBufferStart; >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >=20