From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id DAEFAAC0AB4 for ; Fri, 17 Nov 2023 07:01:59 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=bnwJQQcW9YEuMK4Q8hKxRV8VtAa2ygeBOoldBYIfTF0=; c=relaxed/simple; d=groups.io; h=ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:From:To:CC:Subject:Thread-Topic:Thread-Index:Date:Message-ID:References:In-Reply-To:Accept-Language:msip_labels:MIME-Version:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type; s=20140610; t=1700204518; v=1; b=ovg8Nr0GoJlbuQ2ahcCK29bvnigJPbjf4w+mny0J1vOHtPNd9jDHWoY2EwzA7I8JXRnMgAuu fyKfgC5H9M1yn8rxRCSnPx4hR3kXIn3IoLaX0LN+sDGYM+41TltDmKmJo6k07BRNPyvHpyPdLU2 494xJZs0WryPgwAj3uA0Viww= X-Received: by 127.0.0.2 with SMTP id NwEYYY7687511xVvIHxSj6nI; Thu, 16 Nov 2023 23:01:58 -0800 X-Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web10.6873.1700204516063189813 for ; Thu, 16 Nov 2023 23:01:57 -0800 X-IronPort-AV: E=McAfee;i="6600,9927,10896"; a="394099741" X-IronPort-AV: E=Sophos;i="6.04,206,1695711600"; d="scan'208,217";a="394099741" X-Received: from fmviesa001.fm.intel.com ([10.60.135.141]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Nov 2023 23:01:55 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.04,206,1695711600"; d="scan'208,217";a="13820430" X-Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by fmviesa001.fm.intel.com with ESMTP/TLS/AES256-GCM-SHA384; 16 Nov 2023 23:01:54 -0800 X-Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) by ORSMSX603.amr.corp.intel.com (10.22.229.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.34; Thu, 16 Nov 2023 23:01:53 -0800 X-Received: from orsedg603.ED.cps.intel.com (10.7.248.4) by orsmsx610.amr.corp.intel.com (10.22.229.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.34 via Frontend Transport; Thu, 16 Nov 2023 23:01:53 -0800 X-Received: from NAM12-BN8-obe.outbound.protection.outlook.com (104.47.55.168) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.34; Thu, 16 Nov 2023 23:01:52 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VCEy3RezIoTODGwtQ8+6DpnwJAhsicl159QiXH+CGJzbDvpA92DfTRF2/zeKKiO1a1Uy284liXaMme70oZU6zCc+Wt1JCEQcLxYCHjtNWDwUEWAL7eqTccTbooJRjpoJ9JMpyUINUFXwMi3ThyGBjMLjCQJXvba0DDDtNGthJckts8KLxSGkMGEAzYdGNOwVWw/+PZgB50POTkFlzpAuxa8wsbqvd9WhT33L97nZOwiq+aSLR72+EN+hW6b1dBs1WpCgyS5NBBZI4VPAE7ywyTzn8Q+vJQIEUSIZ5ecTvd7uhoHzvo7vlNCcoLWvRlxtODf9nhuodK6qql18RWGlCw== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=H2NA1qugMO4JBMP16m4bZrNrLBwDQdqgnn3KmapG4cU=; b=AOY3NPe3g+ogkYcoETRRXrcdk6l9IBPqinb5sZC6MTC2b8TnxbRlGBeUS6sV4gYrrSx6fB/cRi+vCUJn2ImDI3LxQSSEFafP/wIbUKf9BtLsM/eprs0wehnSlGQzuA6oqyWbqiSLn8dYyU/xxuNPQ8g/bAl02u4z8lb4xgU4qVUSm89caJB0xxQrGIJGiw06k+5ernOsoJf3NEOCPFcXbcm+QQJXnycZl1Z+pfqKNIuoxHkf3zNzU1B3PWawlNpnDGth39RDGT3QC2aAB9O5HQtHKxBWkpJZM9/wmWb3B0HB1gCD3T/Clw1u1zO/3FuvhUDqBHltOxFrhkqMKr6rqQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com (2603:10b6:208:470::14) by CYXPR11MB8711.namprd11.prod.outlook.com (2603:10b6:930:d7::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7002.21; Fri, 17 Nov 2023 07:01:50 +0000 X-Received: from MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::b614:1f5e:8b0c:9858]) by MN6PR11MB8244.namprd11.prod.outlook.com ([fe80::b614:1f5e:8b0c:9858%4]) with mapi id 15.20.7002.021; Fri, 17 Nov 2023 07:01:50 +0000 From: "Ni, Ray" To: Laszlo Ersek , "devel@edk2.groups.io" , "Xie, Yuanhao" CC: "Dong, Eric" , "Kumar, Rahul R" , Gerd Hoffmann Subject: Re: [edk2-devel] [Patch V4] UefiCpuPkg/MpInitLib: Enable execute disable bit. Thread-Topic: [edk2-devel] [Patch V4] UefiCpuPkg/MpInitLib: Enable execute disable bit. Thread-Index: AQHaFfainWK8ZrU0qEC+LRp09RWmWrB4ixeAgAWJHao= Date: Fri, 17 Nov 2023 07:01:50 +0000 Message-ID: References: <20231113055948.1773-1-yuanhao.xie@intel.com> <79bd9a7b-8b84-798c-c7ce-b7292dab1c7d@redhat.com> In-Reply-To: <79bd9a7b-8b84-798c-c7ce-b7292dab1c7d@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: MN6PR11MB8244:EE_|CYXPR11MB8711:EE_ x-ms-office365-filtering-correlation-id: 4afd1177-38f0-445e-03e9-08dbe73b126f x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: aNQHzJKgawk6giK4+9BBuuhFIWqhwsDiZM33/fYRrNebnprcveqYNWuOLGx5TJk4N9B0iBNrfG6BLqchWpofukRv9txHQVUswXV6j4GZzJxEvg+EsE/H3+bzxkVBqKPq1RaQ46LxtYAA/e014757Y2oyqzLoe03sb7ZaaukvS9+OgMsi8VKMI3Lmv8gjgMSo1JaxN6uTbO+6xEhVTOB1EQRpZ8RqhKDzTdguL0vYYTsQGeVh7pfXZqdcX5SghzK7HRx29AzzipKEW/2ujpPIV8I52OcwzQR8YBm/+yfOyCJ2OQKg3oTxXz5yeLXB+tosTYn0Otwiei75mwfP6znfUyJvFFrMbOqPGTmpdZHpmOgYBRmKplhYoVe17WplqDvwTMQKpMEZYIFo9p0hyDztTXOnjyrDKaiVwQyJ6GUt3j+K7qKtLj0jhcZEYT3Z3Pl5BqTyV0YjEPzalQvhgV3qYLeqOX8X4V/npcyPzSTt/sg5d1OdT4TATpt61Kt50otYQF4MtdWJgRNsJZah20Y67bHAT1xd5tbhkk/A3iaNTasj1CnpgHCign7txx6zFpEy/xGwAdhzb0DbnTHGkaXDBgUqEr2Vussos0Lgy4dJEs2QG155reD+dMeC+4U/16X4BSs89dn6eSWbC7/tVkHm7g== x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?RMERtgDcB5pytuZBjNlM1OLR+6kaYA7L/3JP+vf5jlC9S8tXybc8i21N8m?= =?iso-8859-1?Q?6vOOyv0OYOk7vGdq3CcRAfQ9Bv9npqhRbx0PVRqzVrpggUVSzbpu3Wys/S?= =?iso-8859-1?Q?T0e4zconM85PLhkO/vfrv5KB3q7e9rFH6Ion6vd4udrN1n31bBDGU0m3/O?= =?iso-8859-1?Q?mxwC8x8O3DKbctx2JTg0WKXGi9Ixd2DM9GcYXhmA0rm9OZfxHWrH0nVk6P?= =?iso-8859-1?Q?07XCkqAGExET7QQ+rp5aTg5ZAwYsTIWpDMPmTdPfgJfEKMNx9FpJXRYu8M?= =?iso-8859-1?Q?LMTXjt3ygS2mkNnyFoESwVxnib/e0A6QzISqRwKY2f1Y2b8j6o7cYRZObO?= =?iso-8859-1?Q?1apYsveQQvgqic6vBAd0qM26NU42fUwmMYf7/NvFNS6g8+NYqvjCWYkm9L?= =?iso-8859-1?Q?MQa42bfa+V1AHTm+KvfCkjThxdXI0KmGR7ZH20nVlmblJPWHfWMIe6WnxT?= =?iso-8859-1?Q?pA9cQpbkGF3ko9lYsVJaOfgh6kNwZtmVf6zEY1/dhr+BYzWG22KX889Vzk?= =?iso-8859-1?Q?t4z4habYsh9IU3hs6HnKKfxSsoWMdH9/QAadi7iCGPt9PAWt3teIsgFTCW?= =?iso-8859-1?Q?T01gf0nrQHhlDekQpxIgY7NcngAr9gkBysEDVXJeRTiC9jzGY4tdGLVfSW?= =?iso-8859-1?Q?JTMFg2l/zZBjhEN2m6zAgPKmFMSvvg9FKb12GE/XBbGdENMFqZ4IUIqqqf?= =?iso-8859-1?Q?YTZvpcGKm2MXXa2UGQCYRvWFSPNDD99lMG260lT+bYMx8MIifZ6ktkm/uj?= =?iso-8859-1?Q?ZBICKW5cAsUgVWB9u3TwLxwhM+FqDGfvPwspfaYL9SP+w6vYEwaqpalcK2?= =?iso-8859-1?Q?iv1ZIiXquE3hhqrRXXC5svz5U4nDvVTpfJUMVakohjH2KpIRF+Qi+ILHAJ?= =?iso-8859-1?Q?waMJQFcofShC+nCIM2Mf08G3lHdtr3l+1xvx0fl6No3AI9vPgXmo9S1rKn?= =?iso-8859-1?Q?haq/6TYpTLwiBHZeTrEt5J15UsyTfvQPEIFfdi/xkesm3sNIjtB1oBuPz5?= =?iso-8859-1?Q?onRQoC5ZCn9MLycgNODxjSvQjT7v+1PFQmnpzOxaG+7HElLz63pS1vqKBG?= =?iso-8859-1?Q?O/UmA85AGqEZr0oTuc5f5voeiuBxkPxgFlwY8GqEY2z8X3wirzmvRZX2NW?= =?iso-8859-1?Q?E786GqhUntLFUsTFde5zVdNO3ViG+y77y2IjYM6tANCSsHJCf06D9fdFIv?= =?iso-8859-1?Q?PuFX+znBLKlHL+IAgEmo+4kJyrRZ/a/GmEb18AyYr+6cmH3JbgxBdqG+iP?= =?iso-8859-1?Q?sNoX/Ry4LjeZkJ652BK0uwhVXX6W9FhZARkOe9rW475yUzcb8MP9VC9RF7?= =?iso-8859-1?Q?RLLFqnAOS92bIDv8iRuG4zYKzAfajMuFXWvtHBbLydq3iZJGtIKPS7g3Eb?= =?iso-8859-1?Q?UWPX21o9kOnleWftg5+3quTyjpE+xSgFxvYevLA8kZ0vXKbcAoiig5H3N1?= =?iso-8859-1?Q?O9Xw+PZ6LQc2V2f71MjXWSUNyroiTMOIXKWDUBPr+W2Ocu4tF65V16oPMp?= =?iso-8859-1?Q?DnSz1N9Z9g4dOgtPrZVg4Q/+TgTDGcqMfsrxmw0DXeeaGnRn3oMF1p1WgC?= =?iso-8859-1?Q?8qASCvhOoO4wnTdrJzCuHbBptrgyuEOWawAFhGTOiOfJjzNsOIXzJs/8kS?= =?iso-8859-1?Q?UyvzzUBCK2BSI=3D?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MN6PR11MB8244.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 4afd1177-38f0-445e-03e9-08dbe73b126f X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Nov 2023 07:01:50.0838 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: YU8L6Uex/LLxcDNIBEaDsR4gnq+JQqT0ePr1SzGztuaok8JvyFfUkvvn/OV3v4ErH9mXa/KcnjEAg0MqculJkg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYXPR11MB8711 X-OriginatorOrg: intel.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,ray.ni@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 8Uep84IKIq1TiR9JLlnnKWPkx7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN6PR11MB8244F1E59C7570A68D4020F48CB7AMN6PR11MB8244namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ovg8Nr0G; arc=reject ("signature check failed: fail, {[1] = sig:microsoft.com:reject}"); dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=intel.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --_000_MN6PR11MB8244F1E59C7570A68D4020F48CB7AMN6PR11MB8244namp_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Laszlo, I agree that the BSP's XD status is saved in both CpuMpData and ExchangeInf= o. But, thinking from another perspective, ExchangeInfo is "only" used by the = assembly code. That's why the BSP code needs to save the XD status in CpuMpData and = ExchangeInfo. If we remove the XD status field in ExchangeInfo, then the assembly code ha= s to understand the structure layout of CpuMpData, which is what I prefer to avoid. If you compare all fields in ExchangeInfo and CpuMpData, following fields a= re already duplicated: * CpuMpData.CpuInfoHob <-> MpExchangeInfo.CpuInfo * InitFlag * SevEsIsEnabled * SevSnpIsEnabled * GhcbBase So, I prefer to keep the current change proposed in Yuanhao's patch. Thanks, Ray ________________________________ From: Laszlo Ersek Sent: Tuesday, November 14, 2023 2:02 AM To: devel@edk2.groups.io ; Xie, Yuanhao Cc: Dong, Eric ; Ni, Ray ; Kumar, Ra= hul R ; Gerd Hoffmann Subject: Re: [edk2-devel] [Patch V4] UefiCpuPkg/MpInitLib: Enable execute d= isable bit. On 11/13/23 06:59, Yuanhao Xie wrote: > From: Yuanhao Xie > > This patch synchronizes the No-Execute bit in the IA32_EFER > register for the APs before the RestoreVolatileRegisters operation. > > The commit 964a4f0, titled "Eliminate the second INIT-SIPI-SIPI > sequence," replaces the second INIT-SIPI-SIPI sequence with the BSP > calling the SwitchApContext function to initiate a specialized start-up > signal, waking up APs in the DXE instead of using INIT-SIPI-SIPI. > > Due to this change, the logic for "Enable execute disable bit" in > MpFuncs.nasm is no longer executed. However, to ensure the proper setup > of the page table, it is necessary to synchronize the IA32_EFER.NXE for > APs before executing RestoreVolatileRegisters . > > Based on SDM: > If IA32_EFER.NXE is set to 1, it signifies execute-disable, meaning > instruction fetches are not allowed from the 4-KByte page controlled by > this entry. Conversely, if it is set to 0, it is reserved. > > Signed-off-by: Yuanhao Xie > Cc: Eric Dong > Cc: Ray Ni > Cc: Rahul Kumar > Cc: Gerd Hoffmann > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 +++++++++++--- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 + > 2 files changed, 12 insertions(+), 3 deletions(-) Good problem description! > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/Mp= InitLib/MpLib.c > index 9a6ec5db5c..f29e66a14f 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -910,9 +910,16 @@ DxeApEntryPoint ( > CPU_MP_DATA *CpuMpData > ) > { > - UINTN ProcessorNumber; > + UINTN ProcessorNumber; > + MSR_IA32_EFER_REGISTER EferMsr; > > GetProcessorNumber (CpuMpData, &ProcessorNumber); > + if (CpuMpData->EnableExecuteDisableForSwitchContext) { > + EferMsr.Uint64 =3D AsmReadMsr64 (MSR_IA32_EFER); > + EferMsr.Bits.NXE =3D 1; > + AsmWriteMsr64 (MSR_IA32_EFER, EferMsr.Uint64); > + } > + > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FA= LSE); > InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); > PlaceAPInMwaitLoopOrRunLoop ( > @@ -2188,8 +2195,9 @@ MpInitLibInitialize ( > if (MpHandOff->WaitLoopExecutionMode =3D=3D sizeof (VOID *)) { > ASSERT (CpuMpData->ApLoopMode !=3D ApInHltLoop); > > - CpuMpData->FinishedCount =3D 0; > - CpuMpData->InitFlag =3D ApInitDone; > + CpuMpData->FinishedCount =3D 0; > + CpuMpData->InitFlag =3D ApInitDone; > + CpuMpData->EnableExecuteDisableForSwitchContext =3D IsBspExecuteDi= sableEnabled (); > SaveCpuMpData (CpuMpData); > // > // In scenarios where both the PEI and DXE phases run in the same > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/Mp= InitLib/MpLib.h > index 763db4963d..af296f6ac0 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -270,6 +270,7 @@ struct _CPU_MP_DATA { > UINT64 TotalTime; > EFI_EVENT WaitEvent; > UINTN **FailedCpuList; > + BOOLEAN EnableExecuteDisableForSwitchContext; > > AP_INIT_STATE InitFlag; > BOOLEAN SwitchBspFlag; Functionally this patch seems fine. (I cannot test it very usefully, because this code path is not active in OVMF.) However, there's one thing I think is less than ideal: after this patch, we'll have MP_CPU_EXCHANGE_INFO . EnableExecuteDisable but also MP_CPU_EXCHANGE_INFO . CpuMpData -> EnableExecuteDisableForSwitchContext Furthermore, we'll have two invocations of IsBspExecuteDisableEnabled(): - once for the original (HLT loop + INIT-SIPI-SIPI) method, in WakeUpAP() -> FillExchangeInfoData(), - and another time for the new method, in MpInitLibInitialize(). I feel that we should centralize this to one spot. Note that in commit 629c1dacc9bd ("UefiCpuPkg: ApWakeupFunction directly use CpuMpData.", 2023-07-11), you changed the prototype of ApWakeupFunction(), such that it would take CpuMpData directly, rather than MP_CPU_EXCHANGE_INFO. This was done so that in the next commit (964a4f032dcd, "UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI sequence.", 2023-07-11), you could invoke ApWakeupFunction() on *both* paths, old and new: - old (INIT-SIPI-SIPI): from the assembly language startup code, - new: from DxeApEntryPoint(). Therefore, it seems that the *old* field "MP_CPU_EXCHANGE_INFO.EnableExecuteDisable" is now superfluous. You have effectively pushed it down to "CpuMpData", so that it's available in DxeApEntryPoint(). But CpuMpData is similarly available in the assembly language startup code (that's why you could implement commit 629c1dacc9bd). So I think this patch is good, but it should be followed with a further patch: can you please rebase the *old* startup code to the new CpuMpData field "EnableExecuteDisableForSwitchContext", and then eliminate "MP_CPU_EXCHANGE_INFO.EnableExecuteDisable"? Where we do mov edi, esi add edi, MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable) cmp byte [edi], 0 jz SkipEnableExecuteDisable on IA32, and mov esi, MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable) cmp byte [ebx + esi], 0 jz SkipEnableExecuteDisableBit on X64, can we dereference the CpuMpData field (pointer) instead, and grab the new field "EnableExecuteDisableForSwitchContext"? Effectively MP_CPU_EXCHANGE_INFO seems to contain information that is needed *only* by the INIT-SIPI-SIPI startup path, and CpuMpData holds information that is needed by both startup paths. Given that we now have XD status in the latter, we should drop it from the former. (Of course, once we start using "EnableExecuteDisableForSwitchContext" on both startup paths, then the *name* will be wrong -- it will no longer be for "SwitchContext" only!) ... Yet further, this seems to indicate that calling IsBspExecuteDisableEnabled() upon every invocation of WakeUpAP() (via FillExchangeInfoData()) is superfluous, on the "old" startup path. We should call IsBspExecuteDisableEnabled() only once, namely in MpInitLibInitialize(), *regardless* of "WaitLoopExecutionMode", for filling in the new field. Is that right? Thanks, Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111336): https://edk2.groups.io/g/devel/message/111336 Mute This Topic: https://groups.io/mt/102556608/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --_000_MN6PR11MB8244F1E59C7570A68D4020F48CB7AMN6PR11MB8244namp_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
Laszlo,
I agree that the BSP's XD status is saved in both CpuMpData and ExchangeInf= o.
But, thinking from another perspective, ExchangeInfo is "only" us= ed by the assembly
code. That's why the BSP code needs to save the XD status in CpuMpData and = ExchangeInfo.

If we remove the XD status field in ExchangeInfo, then the assembly code ha= s to understand
the structure layout of CpuMpData, which is what I prefer to avoid.

If you compare all fields in ExchangeInfo and CpuMpData, following fields a= re already duplicated:
* CpuMpData.CpuInfoHob <-> MpExchangeInfo.CpuInfo
* InitFlag
* SevEsIsEnabled
* SevSnpIsEnabled
* GhcbBase


So, I prefer to keep the current change proposed in Yuanhao's patch.

Thanks,
Ray

From: Laszlo= Ersek <lersek@redhat.com>
Sent: Tuesday, November 14, 2023 2:02 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Xie, Yua= nhao <yuanhao.xie@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@= intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffman= n <kraxel@redhat.com>
Subject: Re: [edk2-devel] [Patch V4] UefiCpuPkg/MpInitLib: Enab= le execute disable bit.
 
On 11/13/23 06:59, Yuanhao Xie wrote:=
> From: Yuanhao Xie <yuanhao.xie@intel.com>
>
> This patch synchronizes the No-Execute bit in the IA32_EFER
> register for the APs before the RestoreVolatileRegisters operation. >
> The commit 964a4f0, titled "Eliminate the second INIT-SIPI-SIPI > sequence," replaces the second INIT-SIPI-SIPI sequence with the B= SP
> calling the SwitchApContext function to initiate a specialized start-u= p
> signal, waking up APs in the DXE instead of using INIT-SIPI-SIPI.
>
> Due to this change, the logic for "Enable execute disable bit&quo= t; in
> MpFuncs.nasm is no longer executed. However, to ensure the proper setu= p
> of the page table, it is necessary to synchronize the IA32_EFER.NXE fo= r
> APs before executing RestoreVolatileRegisters .
>
> Based on SDM:
> If IA32_EFER.NXE is set to 1, it signifies execute-disable, meaning > instruction fetches are not allowed from the 4-KByte page controlled b= y
> this entry. Conversely, if it is set to 0, it is reserved.
>
> Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 +++++++++++---
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  1 +
>  2 files changed, 12 insertions(+), 3 deletions(-)

Good problem description!

>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library= /MpInitLib/MpLib.c
> index 9a6ec5db5c..f29e66a14f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -910,9 +910,16 @@ DxeApEntryPoint (
>    CPU_MP_DATA  *CpuMpData
>    )
>  {
> -  UINTN  ProcessorNumber;
> +  UINTN         &nb= sp;         ProcessorNumber;
> +  MSR_IA32_EFER_REGISTER  EferMsr;
>
>    GetProcessorNumber (CpuMpData, &ProcessorNumber)= ;
> +  if (CpuMpData->EnableExecuteDisableForSwitchContext) {
> +    EferMsr.Uint64   =3D AsmReadMsr64 (MSR_I= A32_EFER);
> +    EferMsr.Bits.NXE =3D 1;
> +    AsmWriteMsr64 (MSR_IA32_EFER, EferMsr.Uint64);
> +  }
> +
>    RestoreVolatileRegisters (&CpuMpData->CpuData= [0].VolatileRegisters, FALSE);
>    InterlockedIncrement ((UINT32 *)&CpuMpData->F= inishedCount);
>    PlaceAPInMwaitLoopOrRunLoop (
> @@ -2188,8 +2195,9 @@ MpInitLibInitialize (
>      if (MpHandOff->WaitLoopExecutionMode = =3D=3D sizeof (VOID *)) {
>        ASSERT (CpuMpData->ApLoop= Mode !=3D ApInHltLoop);
>
> -      CpuMpData->FinishedCount =3D 0;
> -      CpuMpData->InitFlag  &nbs= p;   =3D ApInitDone;
> +      CpuMpData->FinishedCount  = ;            &n= bsp;         =3D 0;
> +      CpuMpData->InitFlag  &nbs= p;            &= nbsp;           &nbs= p; =3D ApInitDone;
> +      CpuMpData->EnableExecuteDisableForS= witchContext =3D IsBspExecuteDisableEnabled ();
>        SaveCpuMpData (CpuMpData); >        //
>        // In scenarios where both t= he PEI and DXE phases run in the same
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library= /MpInitLib/MpLib.h
> index 763db4963d..af296f6ac0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -270,6 +270,7 @@ struct _CPU_MP_DATA {
>    UINT64       &nbs= p;            &= nbsp;      TotalTime;
>    EFI_EVENT       &= nbsp;           &nbs= p;    WaitEvent;
>    UINTN        = ;            &n= bsp;       **FailedCpuList;
> +  BOOLEAN         &= nbsp;           &nbs= p;    EnableExecuteDisableForSwitchContext;
>
>    AP_INIT_STATE      &nb= sp;            = InitFlag;
>    BOOLEAN       &nb= sp;            =       SwitchBspFlag;

Functionally this patch seems fine.

(I cannot test it very usefully, because this code path is not active in OVMF.)

However, there's one thing I think is less than ideal: after this patch, we'll have

  MP_CPU_EXCHANGE_INFO . EnableExecuteDisable

but also

  MP_CPU_EXCHANGE_INFO . CpuMpData -> EnableExecuteDisableForSwitch= Context

Furthermore, we'll have two invocations of IsBspExecuteDisableEnabled():
- once for the original (HLT loop + INIT-SIPI-SIPI) method, in
  WakeUpAP() -> FillExchangeInfoData(),

- and another time for the new method, in MpInitLibInitialize().

I feel that we should centralize this to one spot.

Note that in commit 629c1dacc9bd ("UefiCpuPkg: ApWakeupFunction direct= ly
use CpuMpData.", 2023-07-11), you changed the prototype of
ApWakeupFunction(), such that it would take CpuMpData directly, rather
than MP_CPU_EXCHANGE_INFO. This was done so that in the next commit
(964a4f032dcd, "UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI
sequence.", 2023-07-11), you could invoke ApWakeupFunction() on *both*=
paths, old and new:

- old (INIT-SIPI-SIPI): from the assembly language startup code,

- new: from DxeApEntryPoint().

Therefore, it seems that the *old* field
"MP_CPU_EXCHANGE_INFO.EnableExecuteDisable" is now superfluous. Y= ou have
effectively pushed it down to "CpuMpData", so that it's available= in
DxeApEntryPoint().

But CpuMpData is similarly available in the assembly language startup
code (that's why you could implement commit 629c1dacc9bd).

So I think this patch is good, but it should be followed with a further
patch: can you please rebase the *old* startup code to the new CpuMpData field "EnableExecuteDisableForSwitchContext", and then eliminate<= br> "MP_CPU_EXCHANGE_INFO.EnableExecuteDisable"?

Where we do

     mov        = ; edi, esi
     add        = ; edi, MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable)
     cmp        = ; byte [edi], 0
     jz        =   SkipEnableExecuteDisable

on IA32, and

     mov        esi,= MP_CPU_EXCHANGE_INFO_FIELD (EnableExecuteDisable)
     cmp        byte= [ebx + esi], 0
     jz        = SkipEnableExecuteDisableBit

on X64, can we dereference the CpuMpData field (pointer) instead, and
grab the new field "EnableExecuteDisableForSwitchContext"?

Effectively MP_CPU_EXCHANGE_INFO seems to contain information that is
needed *only* by the INIT-SIPI-SIPI startup path, and CpuMpData holds
information that is needed by both startup paths. Given that we now have XD status in the latter, we should drop it from the former.

(Of course, once we start using "EnableExecuteDisableForSwitchContext&= quot;
on both startup paths, then the *name* will be wrong -- it will no
longer be for "SwitchContext" only!)

... Yet further, this seems to indicate that calling
IsBspExecuteDisableEnabled() upon every invocation of WakeUpAP() (via
FillExchangeInfoData()) is superfluous, on the "old" startup path= . We
should call IsBspExecuteDisableEnabled() only once, namely in
MpInitLibInitialize(), *regardless* of "WaitLoopExecutionMode", f= or
filling in the new field. Is that right?

Thanks,
Laszlo

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#111336) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--_000_MN6PR11MB8244F1E59C7570A68D4020F48CB7AMN6PR11MB8244namp_--