[edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3
Sheng Wei
w.sheng at intel.com
Tue Nov 3 04:41:26 UTC 2020
Hi Laszlon, Ray,
Thank you for review.
I have just updated the comment in the code and the commit message, remove the Change-Id item.
I have added more detail about using mInternalCr3 in commit message.
Could you help to review and merge the patch?
https://edk2.groups.io/g/devel/message/66904?p=,,,20,0,0,0::Created,,posterid%3A2558558,20,2,0,78000108
https://edk2.groups.io/g/devel/message/66905?p=,,,20,0,0,0::Created,,posterid%3A2558558,20,2,0,78000109
https://edk2.groups.io/g/devel/message/66906?p=,,,20,0,0,0::Created,,posterid%3A2558558,20,2,0,78000110
Thank you
BR
Sheng Wei
> -----Original Message-----
> From: Laszlo Ersek <lersek at redhat.com>
> Sent: 2020年11月3日 2:05
> To: Sheng, W <w.sheng at intel.com>; devel at edk2.groups.io
> 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: [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://bugzilla.tianocore.org/show_bug.cgi?id=3015
> >
> > 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://edk2.groups.io/g/devel/message/66617
>
> 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://bugzilla.tianocore.org/show_bug.cgi?id=1521>.
>
> - 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://bugzilla.tianocore.org/show_bug.cgi?id=1946>.
>
> 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>
> > Cc: Eric Dong <eric.dong at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar at intel.com>
> > Cc: Jiewen Yao <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 (#66907): https://edk2.groups.io/g/devel/message/66907
Mute This Topic: https://groups.io/mt/77974726/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 --------------
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/20201103/f3fbb1ea/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-UefiCpuPkg-PiSmmCpuDxeSmm-Return-level-paging-type-f.patch
Type: application/octet-stream
Size: 5229 bytes
Desc: 0002-UefiCpuPkg-PiSmmCpuDxeSmm-Return-level-paging-type-f.patch
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20201103/f3fbb1ea/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0000-cover-letter.patch
Type: application/octet-stream
Size: 1860 bytes
Desc: 0000-cover-letter.patch
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20201103/f3fbb1ea/attachment-0002.obj>
More information about the edk2-devel-archive
mailing list