[EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3
Sheng Wei
w.sheng at intel.com
Thu Nov 5 04:16:39 UTC 2020
Hi Laszlo, Bret, Ray,
Thank you for the review comments.
I have updated the patches.
https://edk2.groups.io/g/devel/message/67025?p=,,,20,0,0,0::relevance,,posterid%3A2558558,20,2,0,78044934
https://edk2.groups.io/g/devel/message/67027?p=,,,20,0,0,0::relevance,,posterid%3A2558558,20,2,0,78044936
https://edk2.groups.io/g/devel/message/67026?p=,,,20,0,0,0::relevance,,posterid%3A2558558,20,2,0,78044935
I refine the coding style and update the comment / commit message.
Could you help to review and merge the patch again?
There is some description of the issue and its background.
If mInternalCr3 is non zero, it will use the page table from mInternalCr3.
And it will use mInternalIs5LevelPaging to reflect the paging type.
If mInternalCr3 is zero, it will use the page table from CR3, and reflect
the paging type by CR4 LA57 bit.
This patch is a bug fix when enable CET feature with 5 level paging.
The page table of SMM shadows shack memory is generated in PiCpuSmmEntry().
This page table is not set to CR3 in PiCpuSmmEntry(), it is only for
SMI entry. When CET feature is enabled, we need to set some attributes for
SMM shadows shack memory in PiCpuSmmEntry().
We set this page table to mInternalCr3 for update the memory attribute.
The CR4 LA57 bit does not reflect the paging type of mInternalCr3,
so we need to use a virable (mInternalIs5LevelPaging) to reflect if
mInternalCr3 is 5 level paging or 4 level paging.
We also use the same function to update the memory attribue with the
page table in CR3, use CR4 LA57 bit to reflect the paging type of CR3.
So we need to use GetPageTableBase() and Is5LevelPageTableBase() to return
the page table and its paging type for GetPageTableEntry() when set the
memory attribute.
These patches are only used to fix the bug in current code. Not feature enabling.
Thank you.
BR
Sheng Wei
From: Bret Barkelew <Bret.Barkelew at microsoft.com>
Sent: 2020年11月3日 2:32
To: devel at edk2.groups.io; lersek at redhat.com; Sheng, W <w.sheng at intel.com>
Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>
Subject: RE: [EXTERNAL] Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3
I second the request for explanation.
- Bret
From: Laszlo Ersek via groups.io<mailto:lersek=redhat.com at groups.io>
Sent: Monday, November 2, 2020 10:05 AM
To: Sheng Wei<mailto:w.sheng at intel.com>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Cc: Dong, Eric<mailto:eric.dong at intel.com>; Ni, Ray<mailto:ray.ni at intel.com>; Rahul Kumar<mailto:rahul1.kumar at intel.com>; Yao, Jiewen<mailto:jiewen.yao at intel.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: 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=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3015&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=J5khqRr2cyZ%2FFpOXfX4V3juer6n4wvDxTRiQos%2BGJww%3D&reserved=0
>
> 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=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F66617&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=1jszRyYlHbQY5Kl%2F1uDuILrUFyIqFqbGbGmR2KDx5%2BE%3D&reserved=0
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
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1521&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=0SJTtV%2BOacJuIIrwErqO1z3HVQY3MakyWsyGRwSPOMc%3D&reserved=0>.
- 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
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1946&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gbfshsEySndGIWKsCkbs%2F0%2BV2tA4bPflmH0Y3xcsEVI%3D&reserved=0>.
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 <w.sheng at intel.com<mailto:w.sheng at intel.com>>
> Cc: Eric Dong <eric.dong at intel.com<mailto:eric.dong at intel.com>>
> Cc: Ray Ni <ray.ni at intel.com<mailto:ray.ni at intel.com>>
> Cc: Laszlo Ersek <lersek at redhat.com<mailto:lersek at redhat.com>>
> Cc: Rahul Kumar <rahul1.kumar at intel.com<mailto:rahul1.kumar at intel.com>>
> Cc: Jiewen Yao <jiewen.yao at intel.com<mailto:jiewen.yao at intel.com>>
> ---
> 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/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 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 Attributes.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/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[] = {
> };
>
> UINTN mInternalCr3;
> +BOOLEAN mInternalIs5LevelPaging = 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 = 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 CR3.
> + // So, return the page level type from mInternalIs5LevelPaging instead of the CR4 LA57 bit.
(3) Invalid comment style. Missing empty "//" lines before and after.
> + if (mInternalCr3 != 0) {
> + return mInternalIs5LevelPaging;
> + }
> +
> + Cr4.UintN = AsmReadCr4 ();
> + return (BOOLEAN) (Cr4.Bits.LA57 == 1);
> +}
> +
> /**
> Return length according to page attributes.
>
> @@ -131,7 +169,6 @@ GetPageTableEntry (
> UINT64 *L3PageTable;
> UINT64 *L4PageTable;
> UINT64 *L5PageTable;
> - IA32_CR4 Cr4;
> BOOLEAN Enable5LevelPaging;
>
> Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK;
> @@ -140,8 +177,7 @@ GetPageTableEntry (
> Index2 = ((UINTN)Address >> 21) & PAGING_PAE_INDEX_MASK;
> Index1 = ((UINTN)Address >> 12) & PAGING_PAE_INDEX_MASK;
>
> - Cr4.UintN = AsmReadCr4 ();
> - Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> + Enable5LevelPaging = Is5LevelPageTableBase();
(4) Missing space character before the opening parenthesis.
>
> if (sizeof(UINTN) == sizeof(UINT64)) {
> if (Enable5LevelPaging) {
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/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 = Pml4Entry;
>
> + SetPageTableType(m5LevelPagingNeeded);
(5) Missing space character before the opening parenthesis.
> +
> if (m5LevelPagingNeeded) {
> //
> // Fill PML5 entry
>
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67028): https://edk2.groups.io/g/devel/message/67028
Mute This Topic: https://groups.io/mt/78045102/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20201105/527207ab/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0000-cover-letter.patch
Type: application/octet-stream
Size: 2045 bytes
Desc: 0000-cover-letter.patch
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20201105/527207ab/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-UefiCpuPkg-PiSmmCpuDxeSmm-Correct-the-Cr3-typo.patch
Type: application/octet-stream
Size: 1916 bytes
Desc: 0001-UefiCpuPkg-PiSmmCpuDxeSmm-Correct-the-Cr3-typo.patch
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20201105/527207ab/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-UefiCpuPkg-PiSmmCpuDxeSmm-Return-level-paging-type-f.patch
Type: application/octet-stream
Size: 5437 bytes
Desc: 0002-UefiCpuPkg-PiSmmCpuDxeSmm-Return-level-paging-type-f.patch
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20201105/527207ab/attachment-0002.obj>
More information about the edk2-devel-archive
mailing list