[edk2-devel] [PATCH v6 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Reflect page table depth with page table address
Sheng Wei
w.sheng at intel.com
Wed Nov 11 05:49:25 UTC 2020
Hi Laszlo,
Thank you for the review comments
I will update the patch.
BR
Sheng Wei
> -----Original Message-----
> From: Laszlo Ersek <lersek at redhat.com>
> Sent: 2020年11月11日 3:25
> To: devel at edk2.groups.io; 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: [edk2-devel] [PATCH v6 2/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Reflect page table depth with page table address
>
> On 11/10/20 03:24, Sheng Wei wrote:
> > When trying to get page table base, if mInternalCr3 is zero, it will
> > use the page table from CR3, and reflect the page table depth by CR4 LA57
> bit.
> > If mInternalCr3 is non zero, it will use the page table from
> > mInternalCr3 and reflect the page table depth of mInternalCr3 at same time.
> > In the case of X64, we use m5LevelPagingNeeded to reflect the depth of
> > the page table. And in the case of IA32, it will not the page table
> > depth information.
> >
> > This patch is a bug fix when enable CET feature with 5 level paging.
> > The SMM page tables are allocated / initialized in PiCpuSmmEntry().
> > When CET is enabled, PiCpuSmmEntry() must further modify the attribute
> > of shadow stack pages. This page table is not set to CR3 in PiCpuSmmEntry().
> > So the page table base address is set to mInternalCr3 for modifty the
> > page table attribute. It could not use CR4 LA57 bit to reflect the
> > page table depth for mInternalCr3.
> > So we create a architecture-specific implementation GetPageTable()
> > with
> > 2 output parameters. One parameter is used to output the page table
> > address. Another parameter is used to reflect if it is 5 level paging
> > or not.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015
> >
> > 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/Ia32/PageTbl.c | 24 ++++++++++++-
> > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 12 ++++---
> > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 27 +++---
> ---------
> > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 40
> +++++++++++++++++++---
> > 4 files changed, 70 insertions(+), 33 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > index 2483f2ea84..f5d64392bd 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c
> > @@ -10,6 +10,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > #include "PiSmmCpuDxeSmm.h"
> >
> > +extern UINTN mInternalCr3;
> > +
>
> (1) This extern declaration (and its counterpart in the X64 source file) both
> belong in the "PiSmmCpuDxeSmm.h" header.
>
> > /**
> > Disable CET.
> > **/
> > @@ -28,6 +30,26 @@ EnableCet (
> > VOID
> > );
> >
> > +/**
> > + Get page table base address and the depth of the page table.
> > +
> > + @param[out] Base Page table base address.
> > + @param[out] FiveLevels TRUE means 5 level paging. FALSE means 4 level
> paging.
> > +**/
> > +VOID
> > +GetPageTable (
> > + OUT UINTN *Base,
> > + OUT BOOLEAN *FiveLevels OPTIONAL
> > + )
> > +{
> > + *Base = ((mInternalCr3 == 0) ?
> > + (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64) :
> > + mInternalCr3);
> > + if (FiveLevels != NULL) {
> > + *FiveLevels = FALSE;
> > + }
> > +}
> > +
> > /**
> > Create PageTable for SMM use.
> >
> > @@ -268,7 +290,7 @@ SetPageTableAttributes (
> > DEBUG ((DEBUG_INFO, "Start...\n"));
> > PageTableSplitted = FALSE;
> >
> > - L3PageTable = (UINT64 *)GetPageTableBase ();
> > + GetPageTable ((UINTN *)&L3PageTable, NULL);
> >
> > SmmSetMemoryAttributesEx
> ((EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable, SIZE_4KB, EFI_MEMORY_RO,
> &IsSplitted);
> > PageTableSplitted = (PageTableSplitted || IsSplitted);
>
> (2) Please introduce a helper variable instead;
>
> UINTN PageTableBase;
>
> and write
>
> GetPageTable (&PageTableBase, NULL);
> L3PageTable = (UINT64 *)PageTableBase;
>
> This is much cleaner.
>
> There is a big conceptual difference between converting an integer to a pointer,
> and reinterpreting the bit pattern of an integer as a pointer.
>
> (3) Furthermore, if you look at the same context, you will see that we have the
> expression a bit later:
>
> (EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable
>
> and now you can simplify that to
>
> (EFI_PHYSICAL_ADDRESS)PageTableBase
>
>
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > index 7fb3a2d9e4..59bc764140 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> > @@ -942,13 +942,15 @@ SetPageTableAttributes (
> > );
> >
> > /**
> > - Return page table base.
> > + Get page table base address and the depth of the page table.
> >
> > - @return page table base.
> > + @param[out] Base Page table base address.
> > + @param[out] FiveLevels TRUE means 5 level paging. FALSE means 4 level
> paging.
> > **/
> > -UINTN
> > -GetPageTableBase (
> > - VOID
> > +VOID
> > +GetPageTable (
> > + OUT UINTN *Base,
> > + OUT BOOLEAN *FiveLevels OPTIONAL
> > );
> >
> > /**
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > index d67f036aea..fe71b77295 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> > @@ -49,22 +49,6 @@ SetPageTableBase (
> > mInternalCr3 = Cr3;
> > }
> >
> > -/**
> > - Return page table base.
> > -
> > - @return page table base.
> > -**/
> > -UINTN
> > -GetPageTableBase (
> > - VOID
> > - )
> > -{
> > - if (mInternalCr3 != 0) {
> > - return mInternalCr3;
> > - }
> > - return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64); -}
> > -
> > /**
> > Return length according to page attributes.
> >
> > @@ -131,8 +115,8 @@ GetPageTableEntry (
> > UINT64 *L3PageTable;
> > UINT64 *L4PageTable;
> > UINT64 *L5PageTable;
> > - IA32_CR4 Cr4;
> > BOOLEAN Enable5LevelPaging;
> > + UINT64 *PageTableBase = NULL;
>
> (4) Initialization of local variables (that are not static) is forbidden by the edk2
> coding style.
>
> (5) The type of this variable should be UINTN, not (UINT64*).
>
> >
> > Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK;
> > Index4 = ((UINTN)RShiftU64 (Address, 39)) & PAGING_PAE_INDEX_MASK;
> > @@ -140,12 +124,11 @@ 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);
> > + GetPageTable ((UINTN *)&PageTableBase, &Enable5LevelPaging);
>
> (6) Please adjust the first argument according to (5) -- drop the
> (UINTN*) cast.
>
> >
> > if (sizeof(UINTN) == sizeof(UINT64)) {
> > if (Enable5LevelPaging) {
> > - L5PageTable = (UINT64 *)GetPageTableBase ();
> > + L5PageTable = PageTableBase;
> > if (L5PageTable[Index5] == 0) {
> > *PageAttribute = PageNone;
> > return NULL;
> > @@ -153,7 +136,7 @@ GetPageTableEntry (
> >
> > L4PageTable = (UINT64 *)(UINTN)(L5PageTable[Index5] &
> ~mAddressEncMask & PAGING_4K_ADDRESS_MASK_64);
> > } else {
> > - L4PageTable = (UINT64 *)GetPageTableBase ();
> > + L4PageTable = PageTableBase;
> > }
> > if (L4PageTable[Index4] == 0) {
> > *PageAttribute = PageNone;
> > @@ -162,7 +145,7 @@ GetPageTableEntry (
> >
> > L3PageTable = (UINT64 *)(UINTN)(L4PageTable[Index4] &
> ~mAddressEncMask & PAGING_4K_ADDRESS_MASK_64);
> > } else {
> > - L3PageTable = (UINT64 *)GetPageTableBase ();
> > + L3PageTable = PageTableBase;
> > }
> > if (L3PageTable[Index3] == 0) {
> > *PageAttribute = PageNone;
>
> (7) And so all three of these should be (UINT64 *)PageTableBase.
>
>
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > index 810985df20..51469d3b88 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> > @@ -19,6 +19,8 @@ BOOLEAN
> mCpuSmmRestrictedMemoryAccess;
> > BOOLEAN m5LevelPagingNeeded;
> > X86_ASSEMBLY_PATCH_LABEL gPatch5LevelPagingNeeded;
> >
> > +extern UINTN mInternalCr3;
>
> (8) Belongs in "PiSmmCpuDxeSmm.h".
>
> > +
> > /**
> > Disable CET.
> > **/
> > @@ -104,6 +106,35 @@ Is5LevelPagingNeeded (
> > }
> > }
> >
> > +/**
> > + Get page table base address and the depth of the page table.
> > +
> > + @param[out] Base Page table base address.
> > + @param[out] FiveLevels TRUE means 5 level paging. FALSE means 4 level
> paging.
> > +**/
> > +VOID
> > +GetPageTable (
> > + OUT UINTN *Base,
> > + OUT BOOLEAN *FiveLevels OPTIONAL
> > + )
> > +{
> > + IA32_CR4 Cr4;
> > +
> > + if (mInternalCr3 == 0) {
> > + *Base = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64;
> > + if (FiveLevels != NULL) {
> > + Cr4.UintN = AsmReadCr4 ();
> > + *FiveLevels = (BOOLEAN)(Cr4.Bits.LA57 == 1);
> > + }
> > + return;
> > + }
> > +
> > + *Base = mInternalCr3;
> > + if (FiveLevels != NULL) {
> > + *FiveLevels = m5LevelPagingNeeded;
> > + }
> > +}
> > +
> > /**
> > Set sub-entries number in entry.
> >
> > @@ -1114,11 +1145,10 @@ SetPageTableAttributes (
> > BOOLEAN IsSplitted;
> > BOOLEAN PageTableSplitted;
> > BOOLEAN CetEnabled;
> > - IA32_CR4 Cr4;
> > BOOLEAN Enable5LevelPaging;
> > + UINT64 *PageTableBase = NULL;
>
> (9) Initialization of local variables (that are not static) is forbidden by the edk2
> coding style.
>
> (10) The type of this variable should be UINTN, not (UINT64*).
>
>
> >
> > - Cr4.UintN = AsmReadCr4 ();
> > - Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> > + GetPageTable ((UINTN *)&PageTableBase, &Enable5LevelPaging);
>
> (11) Please drop the (UINTN*) cast.
>
> >
> > //
> > // Don't mark page table memory as read-only if @@ -1164,7 +1194,7
> > @@ SetPageTableAttributes (
> > PageTableSplitted = FALSE;
> > L5PageTable = NULL;
> > if (Enable5LevelPaging) {
> > - L5PageTable = (UINT64 *)GetPageTableBase ();
> > + L5PageTable = PageTableBase;
> > SmmSetMemoryAttributesEx
> ((EFI_PHYSICAL_ADDRESS)(UINTN)L5PageTable, SIZE_4KB, EFI_MEMORY_RO,
> &IsSplitted);
> > PageTableSplitted = (PageTableSplitted || IsSplitted);
> > }
> > @@ -1176,7 +1206,7 @@ SetPageTableAttributes (
> > continue;
> > }
> > } else {
> > - L4PageTable = (UINT64 *)GetPageTableBase ();
> > + L4PageTable = PageTableBase;
> > }
> > SmmSetMemoryAttributesEx
> ((EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO,
> &IsSplitted);
> > PageTableSplitted = (PageTableSplitted || IsSplitted);
> >
>
> (12) Please add the necessary (UINT64*)PageTableBase casts (two of them).
>
> (13) You can simplify
>
> (EFI_PHYSICAL_ADDRESS)(UINTN)L5PageTable
>
> to
>
> (EFI_PHYSICAL_ADDRESS)PageTableBase
>
> Thanks
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67275): https://edk2.groups.io/g/devel/message/67275
Mute This Topic: https://groups.io/mt/78152032/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list