From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (NAM11-CO1-obe.outbound.protection.outlook.com [40.107.220.93]) by mx.groups.io with SMTP id smtpd.web10.3003.1604341908931099767 for ; Mon, 02 Nov 2020 10:31:49 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=AJw9iQNc; spf=pass (domain: microsoft.com, ip: 40.107.220.93, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=RzYhZ85xguH0gdWcbj7LiUftHKOYQ/V2Ob3XsBMbbvBfX8ymuVfH2x84pDalDQo672bfLZWXTtAUeXXsNw/ME0n3b3zt7auHC2aGcfbOizVXJWOVBkeXKrm/xAU4oL+grY9tOArO8E0PseBxGeLjdb6jFYjygP1zrd9W5DZWBl7GZ79F0xpjlxzpnuJ4xNMzmiWWHrJUYwCFPUTuNLN+KxUhuPSDS6+N9th/ZtbIyPK6G3OsSJmdeaEo7fOZ6Hf7L14BdBjcgWLWVnKNapRbvGLpQ5aH6gns5zFFeLyCZGPAmmGAziGmkYs9wjqNZKwlGn3llH4AoVA0aua1uJhqcA== 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=r6YZMbCJtFCE9fazExuYnorKYnVXuFE0Uzeo+WNhrwI=; b=iQoth+hnrdUFPWSkOF3Tf3mDUMuzlqQVcAS+fJ5lLcVTdi5LFo1h9nncF8eUIy9IAXrAZIrHjLfQPcyLTNKYG1uykLuksKYIiQ4HxuvpzDuBE9bUOvZ81ZFW2QaiKKYyZCZKvO2Q51wGwjtJuPkuVmndlFXGY/AZ56sd2j9MEo2TH4cP10uTwexhdSxQGbpt4TrBvYryg3pwloC/4hP2i2pnibysNnWC2RcInY5cjyZCg0YL39Flo8Zh5K40mqjzx48LOf37jlmTZmOIuu0XYzEs/FfL4eqPaEVhYlChf4bitg9X89Bm2YWc2Sw5PnySqefHAWR4ieGVQ0Xj1frRPg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=r6YZMbCJtFCE9fazExuYnorKYnVXuFE0Uzeo+WNhrwI=; b=AJw9iQNcKaWHufcWHPFC/94Z8mJ0u6Au5BrZhV9Kca7pa/bh46P4t8JP8nX1lgpDRkWN1OQlvyJWCZ1LkAYwiNV8i+uqX+c6PQfr8iMoBYZ2NtqPpvn+uRW7fA5j3byAnzKpXygT/MvYlfqk2tiD9OPhoBQCuPEnLPfMPB8UNxg= Received: from MW4PR21MB1857.namprd21.prod.outlook.com (2603:10b6:303:74::12) by MW2PR2101MB1050.namprd21.prod.outlook.com (2603:10b6:302:a::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3564.6; Mon, 2 Nov 2020 18:31:46 +0000 Received: from MW4PR21MB1857.namprd21.prod.outlook.com ([fe80::7584:fc3a:1487:8fda]) by MW4PR21MB1857.namprd21.prod.outlook.com ([fe80::7584:fc3a:1487:8fda%5]) with mapi id 15.20.3541.007; Mon, 2 Nov 2020 18:31:46 +0000 From: "Bret Barkelew" To: "devel@edk2.groups.io" , "lersek@redhat.com" , Sheng Wei CC: "Dong, Eric" , "Ni, Ray" , Rahul Kumar , "Yao, Jiewen" Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3 Thread-Topic: [EXTERNAL] Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3 Thread-Index: AQHWsUK92vOl3nscZEGC85YcaR25bqm1KkDt Date: Mon, 2 Nov 2020 18:31:46 +0000 Message-ID: References: <20201102045330.3540-1-w.sheng@intel.com> <20201102045330.3540-3-w.sheng@intel.com>,<7961ac60-2b3e-a43a-34b6-5562081d780a@redhat.com> In-Reply-To: <7961ac60-2b3e-a43a-34b6-5562081d780a@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=True;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2020-11-02T18:31:31.7946621Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Privileged authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=microsoft.com; x-originating-ip: [71.212.128.184] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: aceea71d-2ca1-4fc6-0f7b-08d87f5d8dff x-ms-traffictypediagnostic: MW2PR2101MB1050: x-ld-processed: 72f988bf-86f1-41af-91ab-2d7cd011db47,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:2958; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: GlfiKcHxWcDBtfdpZbFKRMuDWD9NgjwX5VDOcXxPqUO5o0psdu5C7/XPOwbpQapI8bsgN8S+N5X+uqrO0XzGgQv50IHcW9ticvFAiEGWp8QrSjhMc+X36EQBm3dZj2nGoUwM1rPPXZJ5skhlmkro+onGCrbE6XCIdwTE8vBfgc+ZPOCzX44QkHFpLbMopoN1iwNeOLBf2RxmS3mg5Fh/7zA0QVZnjGbf4X066oTyk4g8YOIc3ISgVpLdMj++Y/emqrlGDgeOlKHMOT01wZXajuVhPT1mqrkJtBR7w1pKAJJzHfgdh6ziVuYGwN3d1B4QX4ZZs+rpcZ9ofPY8ilVDxUo6ajv/XhWo2VrRnxiYoeR2nwhBoqd5tSubu4DbO35HRr+4u1nWA8LkC7c3qhT+QQ== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MW4PR21MB1857.namprd21.prod.outlook.com;PTR:;CAT:NONE;SFS:(6029001)(4636009)(376002)(136003)(346002)(39860400002)(366004)(396003)(82950400001)(186003)(10290500003)(71200400001)(86362001)(966005)(76116006)(478600001)(2906002)(5660300002)(55016002)(66446008)(64756008)(6506007)(166002)(316002)(26005)(53546011)(66946007)(4326008)(8936002)(66476007)(66556008)(82960400001)(54906003)(8990500004)(7696005)(33656002)(19627235002)(9686003)(30864003)(110136005)(8676002)(83380400001)(52536014);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: rMczzZDnpWf7rciysb8vdin1Tcbyxz7hNLeRwQ5C692l2lc+xxJZZrgZ+BFEFCcg3e7enNZWfos0NjEOuAwnhhQyQpog1MRhUsQR4ZqhqtSQwHZjmP08fGfYHJbRDdcly+2nudKA0djcpXAQkODFqP3ir0zVZdDD7OzjvCtxbVdMgOv8NshOj8Bnna312q89zljiZ8lcDATez8Ci4lBv4Na5v7d2qqUCq5X6zXKT3+Saxc6V5Krpv2G8qHhVb8cQf3NeiG3/YIBY+c5wjdpSNYy0O+zNb18L2cK6PA2BMRY6gTiGN9VkQwmu6smJ8NMT6sB5SYF74v5j9t0PgexQVgPiNra+n45nqZbP62S4591GxqGU73WMsJFIG5t6xC7Xeo4vErCdTbyy3zZTPxBa+DsdWRBwC7DnFqZgdpmQii947vDbEpQNahFCzczj+KpIRAtTrfSXddOYS3XHLcEwz6AbcPx4ZfGlze4MFCiNFLZPp74mtUlAh8kEuhDAfYTXlMmOsOEXiZqjKBbQDJrCbXKMUb0IwKOQ46y/z1yMA2rvQIa36rGd22CjSOOyUN2oNG837MB7EHlUsxIaU2vrlLFxM9XyPnV3wEftPoMOb17n6K7bI1M223XaC1wUi9Uc+OaTqpHKfEINMYwXCaiwtw== x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MW4PR21MB1857.namprd21.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: aceea71d-2ca1-4fc6-0f7b-08d87f5d8dff X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Nov 2020 18:31:46.1923 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ila04J/WCAwBsLT9YThAFCKPU6Kv8fSNB/+WMfvjnsPUvVnFXH/sFkPXRw7PV/YHpbmoPo/q3GZ1WGfq32xkCQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW2PR2101MB1050 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MW4PR21MB18577C47121CD5DA9335A8E1EF100MW4PR21MB1857namp_" --_000_MW4PR21MB18577C47121CD5DA9335A8E1EF100MW4PR21MB1857namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I second the request for explanation. - Bret From: Laszlo Ersek via groups.io Sent: Monday, November 2, 2020 10:05 AM To: Sheng Wei; devel@edk2.groups.io Cc: Dong, Eric; Ni, Ray; Rahul Kumar; Yao, Jiewen Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxe= Smm: Return level paging type for Internal CR3 On 11/02/20 05:53, Sheng Wei wrote: > When the functions called from entrypoint the page table is > set to mInternalCr3, mInternalIs5LevelPaging reflects > the page table type pointed by mInternalCr3. > > REF: https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2= Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3015&data=3D04%7C01%7CBre= t.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f= 141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8= eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&a= mp;sdata=3DJ5khqRr2cyZ%2FFpOXfX4V3juer6n4wvDxTRiQos%2BGJww%3D&reserved= =3D0 > > Change-Id: I9e44c6385a05930850f5ba60d10ed3e391b628bb (1) None of the patches should contain such a "Change-Id" line; they are meaningless for upstream edk2. They can be removed by the maintainer just before merging, of course. (2) However, I still have absolutely no idea what the problem is. Ray provided some great feedback here: https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.g= roups.io%2Fg%2Fdevel%2Fmessage%2F66617&data=3D04%7C01%7CBret.Barkelew%4= 0microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141af91ab2d7= cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w= LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3D1= jszRyYlHbQY5Kl%2F1uDuILrUFyIqFqbGbGmR2KDx5%2BE%3D&reserved=3D0 In particular: Better to explain the case when the functions called from entrypoint the page table is set to mInternalCr3, gSmmFeatureEnable5LevelPaging reflects the page table type pointed by mInternalCr3. And this has not been *explained*. The commit message should include a reminder *why* we sometimes use "mInternalCr3" (i.e., when it is nonzero), and why we use the CR3 register in other cases. This reminder is not related to the change in this patch, it should re-state the *original* use case for "mInternalCr3". And then in the next paragraph, the commit message should explain that, *IF* we use "mInternalCr3", rather than CR3, *then* accessing CR4 for 5-level paging is wrong -- because in that case, we should save the 5-level paging status similarly (I guess?) to how we save "mInternalCr3". So, on the surface, the change seems to make sense, but without knowing why we use "mInternalCr3" in the first place, I find it difficult to reason about this change. ... - According to git-blame, "mInternalCr3" comes from edk2 commit 3eb69b081c68 ("UefiCpuPkg/PiSmmCpu: Add Shadow Stack Support for X86 SMM.", 2019-02-28), and it is related to . - Also according to git-blame, the original "Enable5LevelPaging" assignment, based on "Cr4.Bits.LA57", comes from commit 4eee0cc7cc0d ("UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports", 2019-07-12), related to . Commit 3eb69b081c68 (the "CET ShadowStack" feature) *precedes* commit 4eee0cc7cc0d (the 5-level paging feature) in the git commit history. Therefore the bug was introduced commit 4eee0cc7cc0d -- the 5-level paging enablement did not take CET ShadowStack support in consideration. In other words, this patch fixes (or "completes") 5-level paging support for "CET ShadowStack". Is that correct? If so, then *WHY* is none of the expressions "CET ShadowStack" and "5-level paging" present in any of the subject lines? Why are there no references to commits 3eb69b081c68 and 4eee0cc7cc0d in the commit messages? Why does Bug 3015 not reference Bug 1521 and Bug 1946? After all, if I understand correctly, this patch covers a corner case in the intersection of two features. Why is that not documented clearly? I want to see a statement such as "if we are using a shadow stack, then we need to use a CR4 value that matches the shadow stack, for determining whether 5-level paging is enabled or not". Or *something* like this. > Signed-off-by: Sheng Wei > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > Cc: Jiewen Yao > --- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 ++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 42 +++++++++++++++= +++++-- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 ++ > 3 files changed, 51 insertions(+), 3 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiS= mmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 7fb3a2d9e4..3eb6af62a7 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -951,6 +951,16 @@ GetPageTableBase ( > VOID > ); > > +/** > + This function set the internal page table type to 5 level paging or 4= level paging. > + > + @param Is5LevelPaging TRUE means 5 level paging. FALSE means 4 level = paging. > +**/ > +VOID > +SetPageTableType ( > + IN BOOLEAN Is5LevelPaging > + ); > + > /** > This function sets the attributes for the memory region specified by = BaseAddress and > Length from their current attributes to the attributes specified by A= ttributes. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCp= uPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index d67f036aea..91c0fd6587 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] =3D { > }; > > UINTN mInternalCr3; > +BOOLEAN mInternalIs5LevelPaging =3D FALSE; > > /** > Set the internal page table base address. > @@ -65,6 +66,43 @@ GetPageTableBase ( > return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64); > } > > +/** > + This function set the internal page table type to 5 level paging or 4= level paging. > + > + @param Is5LevelPaging TRUE means 5 level paging. FALSE means 4 level = paging. > +**/ > +VOID > +SetPageTableType ( > + IN BOOLEAN Is5LevelPaging > + ) > +{ > + mInternalIs5LevelPaging =3D Is5LevelPaging; > +} > + > +/** > + Return if the page table is 5 level paging. > + > + @return TRUE The page table base is 5 level paging. > + @return FALSE The page table base is 4 level paging. > +**/ > +STATIC > +BOOLEAN > +Is5LevelPageTableBase ( > + VOID > + ) > +{ > + IA32_CR4 Cr4; > + > + // If mInternalCr3 is non zero, it will not use the page table from C= R3. > + // So, return the page level type from mInternalIs5LevelPaging instea= d of the CR4 LA57 bit. (3) Invalid comment style. Missing empty "//" lines before and after. > + if (mInternalCr3 !=3D 0) { > + return mInternalIs5LevelPaging; > + } > + > + Cr4.UintN =3D AsmReadCr4 (); > + return (BOOLEAN) (Cr4.Bits.LA57 =3D=3D 1); > +} > + > /** > Return length according to page attributes. > > @@ -131,7 +169,6 @@ GetPageTableEntry ( > UINT64 *L3PageTable; > UINT64 *L4PageTable; > UINT64 *L5PageTable; > - IA32_CR4 Cr4; > BOOLEAN Enable5LevelPaging; > > Index5 =3D ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK; > @@ -140,8 +177,7 @@ GetPageTableEntry ( > Index2 =3D ((UINTN)Address >> 21) & PAGING_PAE_INDEX_MASK; > Index1 =3D ((UINTN)Address >> 12) & PAGING_PAE_INDEX_MASK; > > - Cr4.UintN =3D AsmReadCr4 (); > - Enable5LevelPaging =3D (BOOLEAN) (Cr4.Bits.LA57 =3D=3D 1); > + Enable5LevelPaging =3D Is5LevelPageTableBase(); (4) Missing space character before the opening parenthesis. > > if (sizeof(UINTN) =3D=3D sizeof(UINT64)) { > if (Enable5LevelPaging) { > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmC= puDxeSmm/X64/PageTbl.c > index 810985df20..6f2f4adb7d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -387,6 +387,8 @@ SmmInitPageTable ( > SetSubEntriesNum (Pml4Entry, 3); > PTEntry =3D Pml4Entry; > > + SetPageTableType(m5LevelPagingNeeded); (5) Missing space character before the opening parenthesis. > + > if (m5LevelPagingNeeded) { > // > // Fill PML5 entry > Laszlo --_000_MW4PR21MB18577C47121CD5DA9335A8E1EF100MW4PR21MB1857namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

I second the request for explanation.

 

- Bret

 

From: Laszlo Ersek via groups.io=
Sent: Monday, November 2, 2020 10:05 AM
To: Sheng Wei; devel@edk2.groups.io
Cc: Dong, Eric; Ni, Ray; Rahul Kumar; Yao, Jiewen
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSm= mCpuDxeSmm: Return level paging type for Internal CR3

 

On 11/02/20 05:53, S= heng Wei wrote:
> When the functions called from entrypoint the page table is
> set to mInternalCr3, mInternalIs5LevelPaging reflects
> the page table type pointed by mInternalCr3.
>
> REF: https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fbugzil= la.tianocore.org%2Fshow_bug.cgi%3Fid%3D3015&amp;data=3D04%7C01%7CBret.B= arkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141= af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJ= WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&= amp;sdata=3DJ5khqRr2cyZ%2FFpOXfX4V3juer6n4wvDxTRiQos%2BGJww%3D&amp;rese= rved=3D0
>
> Change-Id: I9e44c6385a05930850f5ba60d10ed3e391b628bb

(1) None of the patches should contain such a "Change-Id" line; = they are
meaningless for upstream edk2.

They can be removed by the maintainer just before merging, of course.


(2) However, I still have absolutely no idea what the problem is.

Ray provided some great feedback here:

https://nam06.safelinks.protection.outlook.com/?url=3Dhttps= %3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F66617&amp;data=3D04%7C0= 1%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f9= 88bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpb= GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7= C1000&amp;sdata=3D1jszRyYlHbQY5Kl%2F1uDuILrUFyIqFqbGbGmR2KDx5%2BE%3D&am= p;amp;reserved=3D0

In particular:

    Better to explain the case when the functions called fr= om entrypoint
    the page table is set to mInternalCr3, gSmmFeatureEnabl= e5LevelPaging
    reflects the page table type pointed by mInternalCr3.
And this has not been *explained*.

The commit message should include a reminder *why* we sometimes use
"mInternalCr3" (i.e., when it is nonzero), and why we use the CR= 3
register in other cases. This reminder is not related to the change in
this patch, it should re-state the *original* use case for "mInternal= Cr3".

And then in the next paragraph, the commit message should explain that, *IF* we use "mInternalCr3", rather than CR3, *then* accessing CR= 4 for
5-level paging is wrong -- because in that case, we should save the
5-level paging status similarly (I guess?) to how we save "mInternalC= r3".

So, on the surface, the change seems to make sense, but without knowing why we use "mInternalCr3" in the first place, I find it difficul= t to
reason about this change.

...

- According to git-blame, "mInternalCr3" comes from edk2 commit<= br> 3eb69b081c68 ("UefiCpuPkg/PiSmmCpu: Add Shadow Stack Support for X86<= br> SMM.", 2019-02-28), and it is related to
<https://nam06.safelinks.protection.outlook.com/?url= =3Dhttps%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1521&amp= ;data=3D04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87= f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CU= nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC= JXVCI6Mn0%3D%7C1000&amp;sdata=3D0SJTtV%2BOacJuIIrwErqO1z3HVQY3MakyWsyGR= wSPOMc%3D&amp;reserved=3D0>.

- Also according to git-blame, the original "Enable5LevelPaging"=
assignment, based on "Cr4.Bits.LA57", comes from commit 4eee0cc7= cc0d
("UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports",=
2019-07-12), related to
<https://nam06.safelinks.protection.outlook.com/?ur= l=3Dhttps%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1946&amp= ;data=3D04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87= f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CU= nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC= JXVCI6Mn0%3D%7C1000&amp;sdata=3DgbfshsEySndGIWKsCkbs%2F0%2BV2tA4bPflmH0= Y3xcsEVI%3D&amp;reserved=3D0>.

Commit 3eb69b081c68 (the "CET ShadowStack" feature) *precedes* c= ommit
4eee0cc7cc0d (the 5-level paging feature) in the git commit history.

Therefore the bug was introduced commit 4eee0cc7cc0d -- the 5-level
paging enablement did not take CET ShadowStack support in consideration.
In other words, this patch fixes (or "completes") 5-level paging= support
for "CET ShadowStack".

Is that correct?

If so, then *WHY* is none of the expressions "CET ShadowStack" a= nd
"5-level paging" present in any of the subject lines? Why are th= ere no
references to commits 3eb69b081c68 and 4eee0cc7cc0d in the commit
messages? Why does Bug 3015 not reference Bug 1521 and Bug 1946?

After all, if I understand correctly, this patch covers a corner case in the intersection of two features. Why is that not documented clearly?

I want to see a statement such as "if we are using a shadow stack, th= en
we need to use a CR4 value that matches the shadow stack, for
determining whether 5-level paging is enabled or not".

Or *something* like this.


> Signed-off-by: Sheng Wei <w.sheng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h   &nb= sp;     | 10 ++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 42 +++++++= +++++++++++++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c    =         |  2 ++
>  3 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/= PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 7fb3a2d9e4..3eb6af62a7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -951,6 +951,16 @@ GetPageTableBase (
>    VOID
>    );
> 
> +/**
> +  This function set the internal page table type to 5 level pag= ing or 4 level paging.
> +
> +  @param Is5LevelPaging TRUE means 5 level paging. FALSE means = 4 level paging.
> +**/
> +VOID
> +SetPageTableType (
> +  IN BOOLEAN  Is5LevelPaging
> +  );
> +
>  /**
>    This function sets the attributes for the memory re= gion specified by BaseAddress and
>    Length from their current attributes to the attribu= tes specified by Attributes.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/Uef= iCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index d67f036aea..91c0fd6587 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] =3D { >  };
> 
>  UINTN  mInternalCr3;
> +BOOLEAN mInternalIs5LevelPaging =3D FALSE;
> 
>  /**
>    Set the internal page table base address.
> @@ -65,6 +66,43 @@ GetPageTableBase (
>    return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_= 64);
>  }
> 
> +/**
> +  This function set the internal page table type to 5 level pag= ing or 4 level paging.
> +
> +  @param Is5LevelPaging TRUE means 5 level paging. FALSE means = 4 level paging.
> +**/
> +VOID
> +SetPageTableType (
> +  IN BOOLEAN  Is5LevelPaging
> +  )
> +{
> +  mInternalIs5LevelPaging =3D Is5LevelPaging;
> +}
> +
> +/**
> +  Return if the page table is 5 level paging.
> +
> +  @return TRUE  The page table base is 5 level paging.
> +  @return FALSE The page table base is 4 level paging.
> +**/
> +STATIC
> +BOOLEAN
> +Is5LevelPageTableBase (
> +  VOID
> +  )
> +{
> +  IA32_CR4         = ;     Cr4;
> +
> +  // If mInternalCr3 is non zero, it will not use the page tabl= e from CR3.
> +  // So, return the page level type from mInternalIs5LevelPagin= g instead of the CR4 LA57 bit.

(3) Invalid comment style. Missing empty "//" lines before and a= fter.

> +  if (mInternalCr3 !=3D 0) {
> +    return mInternalIs5LevelPaging;
> +  }
> +
> +  Cr4.UintN =3D AsmReadCr4 ();
> +  return (BOOLEAN) (Cr4.Bits.LA57 =3D=3D 1);
> +}
> +
>  /**
>    Return length according to page attributes.
> 
> @@ -131,7 +169,6 @@ GetPageTableEntry (
>    UINT64       &nb= sp;        *L3PageTable;
>    UINT64       &nb= sp;        *L4PageTable;
>    UINT64       &nb= sp;        *L5PageTable;
> -  IA32_CR4         = ;     Cr4;
>    BOOLEAN       &n= bsp;       Enable5LevelPaging;
> 
>    Index5 =3D ((UINTN)RShiftU64 (Address, 48)) & P= AGING_PAE_INDEX_MASK;
> @@ -140,8 +177,7 @@ GetPageTableEntry (
>    Index2 =3D ((UINTN)Address >> 21) & PAGIN= G_PAE_INDEX_MASK;
>    Index1 =3D ((UINTN)Address >> 12) & PAGIN= G_PAE_INDEX_MASK;
> 
> -  Cr4.UintN =3D AsmReadCr4 ();
> -  Enable5LevelPaging =3D (BOOLEAN) (Cr4.Bits.LA57 =3D=3D 1); > +  Enable5LevelPaging =3D Is5LevelPageTableBase();

(4) Missing space character before the opening parenthesis.

> 
>    if (sizeof(UINTN) =3D=3D sizeof(UINT64)) {
>      if (Enable5LevelPaging) {
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiS= mmCpuDxeSmm/X64/PageTbl.c
> index 810985df20..6f2f4adb7d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -387,6 +387,8 @@ SmmInitPageTable (
>    SetSubEntriesNum (Pml4Entry, 3);
>    PTEntry =3D Pml4Entry;
> 
> +  SetPageTableType(m5LevelPagingNeeded);

(5) Missing space character before the opening parenthesis.

> +
>    if (m5LevelPagingNeeded) {
>      //
>      // Fill PML5 entry
>

Laszlo





 

--_000_MW4PR21MB18577C47121CD5DA9335A8E1EF100MW4PR21MB1857namp_--