* [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm
@ 2022-08-10 1:45 duntan
2022-08-10 1:45 ` [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag duntan
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: duntan @ 2022-08-10 1:45 UTC (permalink / raw)
To: devel
Add a new IsShadowStack flag in PiSmmCpuDxeSmm.
Remove mInternalCr3 in PiSmmCpuDxeSmm.
Dun Tan (2):
UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag
UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 in PiSmmCpuDxeSmm
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 30 +++++-------------------------
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 26 +++++++++-----------------
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 73 +++++++++++++++++++++++--------------------------------------------------
4 files changed, 98 insertions(+), 144 deletions(-)
--
2.31.1.windows.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag 2022-08-10 1:45 [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm duntan @ 2022-08-10 1:45 ` duntan 2022-08-10 3:51 ` [edk2-devel] " Ni, Ray 2022-08-10 1:45 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 in PiSmmCpuDxeSmm duntan 2022-08-10 3:20 ` [edk2-devel] [PATCH 0/2] " Sean 2 siblings, 1 reply; 8+ messages in thread From: duntan @ 2022-08-10 1:45 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar Add a new IsShadowStack flag to identify whether current memory is shadow stack. The dirty bit in page table entry for this memory will be set if IsShadowStack is TRUE, instead of depending on mInternalCr3. Signed-off-by: Dun Tan <dun.tan@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c index 1f7cc15727..b369c0c435 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c @@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { }; UINTN mInternalCr3; +UINTN IsShadowStack = FALSE; /** Set the internal page table base address. @@ -249,7 +250,7 @@ ConvertPageEntryAttribute ( if ((Attributes & EFI_MEMORY_RO) != 0) { if (IsSet) { NewPageEntry &= ~(UINT64)IA32_PG_RW; - if (mInternalCr3 != 0) { + if (IsShadowStack) { // Environment setup // ReadOnly page need set Dirty bit for shadow stack NewPageEntry |= IA32_PG_D; @@ -734,10 +735,11 @@ SetShadowStack ( EFI_STATUS Status; SetPageTableBase (Cr3); - - Status = SmmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO); + IsShadowStack = TRUE; + Status = SmmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO); SetPageTableBase (0); + IsShadowStack = FALSE; return Status; } -- 2.31.1.windows.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag 2022-08-10 1:45 ` [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag duntan @ 2022-08-10 3:51 ` Ni, Ray 2022-08-10 4:02 ` duntan 0 siblings, 1 reply; 8+ messages in thread From: Ni, Ray @ 2022-08-10 3:51 UTC (permalink / raw) To: devel@edk2.groups.io, Tan, Dun; +Cc: Dong, Eric, Kumar, Rahul R Dun, Can you please update the commit message to explain it's a code refactoring and doesn't change any functionality? Also explain why such refactoring is needed. IsShadowStack: the name doesn't follow EDKII coding style. You need to use "mIsShadowStack". Thanks, Ray > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan > Sent: Wednesday, August 10, 2022 9:46 AM > To: devel@edk2.groups.io > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, > Rahul R <rahul.r.kumar@intel.com> > Subject: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a > new IsShadowStack flag > > Add a new IsShadowStack flag to identify whether current memory is > shadow stack. The dirty bit in page table entry for this memory will > be set if IsShadowStack is TRUE, instead of depending on mInternalCr3. > > Signed-off-by: Dun Tan <dun.tan@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 8 > +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git > a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index 1f7cc15727..b369c0c435 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { > }; > > UINTN mInternalCr3; > +UINTN IsShadowStack = FALSE; > > /** > Set the internal page table base address. > @@ -249,7 +250,7 @@ ConvertPageEntryAttribute ( > if ((Attributes & EFI_MEMORY_RO) != 0) { > if (IsSet) { > NewPageEntry &= ~(UINT64)IA32_PG_RW; > - if (mInternalCr3 != 0) { > + if (IsShadowStack) { > // Environment setup > // ReadOnly page need set Dirty bit for shadow stack > NewPageEntry |= IA32_PG_D; > @@ -734,10 +735,11 @@ SetShadowStack ( > EFI_STATUS Status; > > SetPageTableBase (Cr3); > - > - Status = SmmSetMemoryAttributes (BaseAddress, Length, > EFI_MEMORY_RO); > + IsShadowStack = TRUE; > + Status = SmmSetMemoryAttributes (BaseAddress, Length, > EFI_MEMORY_RO); > > SetPageTableBase (0); > + IsShadowStack = FALSE; > > return Status; > } > -- > 2.31.1.windows.1 > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag 2022-08-10 3:51 ` [edk2-devel] " Ni, Ray @ 2022-08-10 4:02 ` duntan 0 siblings, 0 replies; 8+ messages in thread From: duntan @ 2022-08-10 4:02 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R Ok, I'll send the V2 patch set soon. Thanks, Dun -----Original Message----- From: Ni, Ray <ray.ni@intel.com> Sent: Wednesday, August 10, 2022 11:52 AM To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com> Subject: RE: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag Dun, Can you please update the commit message to explain it's a code refactoring and doesn't change any functionality? Also explain why such refactoring is needed. IsShadowStack: the name doesn't follow EDKII coding style. You need to use "mIsShadowStack". Thanks, Ray > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan > Sent: Wednesday, August 10, 2022 9:46 AM > To: devel@edk2.groups.io > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; > Kumar, Rahul R <rahul.r.kumar@intel.com> > Subject: [edk2-devel] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new > IsShadowStack flag > > Add a new IsShadowStack flag to identify whether current memory is > shadow stack. The dirty bit in page table entry for this memory will > be set if IsShadowStack is TRUE, instead of depending on mInternalCr3. > > Signed-off-by: Dun Tan <dun.tan@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 8 > +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git > a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index 1f7cc15727..b369c0c435 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { }; > > UINTN mInternalCr3; > +UINTN IsShadowStack = FALSE; > > /** > Set the internal page table base address. > @@ -249,7 +250,7 @@ ConvertPageEntryAttribute ( > if ((Attributes & EFI_MEMORY_RO) != 0) { > if (IsSet) { > NewPageEntry &= ~(UINT64)IA32_PG_RW; > - if (mInternalCr3 != 0) { > + if (IsShadowStack) { > // Environment setup > // ReadOnly page need set Dirty bit for shadow stack > NewPageEntry |= IA32_PG_D; > @@ -734,10 +735,11 @@ SetShadowStack ( > EFI_STATUS Status; > > SetPageTableBase (Cr3); > - > - Status = SmmSetMemoryAttributes (BaseAddress, Length, > EFI_MEMORY_RO); > + IsShadowStack = TRUE; > + Status = SmmSetMemoryAttributes (BaseAddress, Length, > EFI_MEMORY_RO); > > SetPageTableBase (0); > + IsShadowStack = FALSE; > > return Status; > } > -- > 2.31.1.windows.1 > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 in PiSmmCpuDxeSmm 2022-08-10 1:45 [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm duntan 2022-08-10 1:45 ` [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag duntan @ 2022-08-10 1:45 ` duntan 2022-08-10 5:20 ` Ni, Ray 2022-08-10 3:20 ` [edk2-devel] [PATCH 0/2] " Sean 2 siblings, 1 reply; 8+ messages in thread From: duntan @ 2022-08-10 1:45 UTC (permalink / raw) To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar Remove mInternalCr3 in PiSmmCpuDxe pagetable related code. Currently, mInternalCr3 is used to pass address of pagetable which is different from Cr3 register. Now remove it and pass the page table base address from the root function to simplify the code logic. Change-Id: I8f58158b94a01cf829f1b4fb2b8c763dcdda0662 Signed-off-by: Dun Tan <dun.tan@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Signed-off-by: Dun Tan <dun.tan@intel.com> --- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 30 +++++------------------------- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 26 +++++++++----------------- UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------- UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 73 +++++++++++++++++++++++-------------------------------------------------- 4 files changed, 94 insertions(+), 142 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c index 8ec8790c05..97058a2810 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c @@ -28,26 +28,6 @@ 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. @@ -297,10 +277,10 @@ SetPageTableAttributes ( DEBUG ((DEBUG_INFO, "Start...\n")); PageTableSplitted = FALSE; - GetPageTable (&PageTableBase, NULL); - L3PageTable = (UINT64 *)PageTableBase; + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; + L3PageTable = (UINT64 *)PageTableBase; - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); + SmmSetMemoryAttributesEx (PageTableBase, FALSE, (EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); PageTableSplitted = (PageTableSplitted || IsSplitted); for (Index3 = 0; Index3 < 4; Index3++) { @@ -309,7 +289,7 @@ SetPageTableAttributes ( continue; } - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); + SmmSetMemoryAttributesEx (PageTableBase, FALSE, (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); PageTableSplitted = (PageTableSplitted || IsSplitted); for (Index2 = 0; Index2 < SIZE_4KB/sizeof (UINT64); Index2++) { @@ -323,7 +303,7 @@ SetPageTableAttributes ( continue; } - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); + SmmSetMemoryAttributesEx (PageTableBase, FALSE, (EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); PageTableSplitted = (PageTableSplitted || IsSplitted); } } diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h index dfeceec2aa..ef8bf5947d 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -264,7 +264,7 @@ extern UINTN mMaxNumberOfCpus; extern UINTN mNumberOfCpus; extern EFI_SMM_CPU_PROTOCOL mSmmCpu; extern EFI_MM_MP_PROTOCOL mSmmMp; -extern UINTN mInternalCr3; +extern BOOLEAN m5LevelPagingNeeded; /// /// The mode of the CPU at the time an SMI occurs @@ -682,7 +682,6 @@ SmmBlockingStartupThisAp ( **/ EFI_STATUS -EFIAPI SmmSetMemoryAttributes ( IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length, @@ -712,7 +711,6 @@ SmmSetMemoryAttributes ( **/ EFI_STATUS -EFIAPI SmmClearMemoryAttributes ( IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length, @@ -957,22 +955,12 @@ SetPageTableAttributes ( 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 - ); - /** This function sets the attributes for the memory region specified by BaseAddress and Length from their current attributes to the attributes specified by Attributes. + @param[in] PageTableBase The page table base. + @param[in] EnablePML5Paging If PML5 paging is enabled. @param[in] BaseAddress The physical address that is the start address of a memory region. @param[in] Length The size in bytes of the memory region. @param[in] Attributes The bit mask of attributes to set for the memory region. @@ -993,8 +981,9 @@ GetPageTable ( **/ EFI_STATUS -EFIAPI SmmSetMemoryAttributesEx ( + IN UINTN PageTableBase, + IN BOOLEAN EnablePML5Paging, IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length, IN UINT64 Attributes, @@ -1005,6 +994,8 @@ SmmSetMemoryAttributesEx ( This function clears the attributes for the memory region specified by BaseAddress and Length from their current attributes to the attributes specified by Attributes. + @param[in] PageTableBase The page table base. + @param[in] EnablePML5Paging If PML5 paging is enabled. @param[in] BaseAddress The physical address that is the start address of a memory region. @param[in] Length The size in bytes of the memory region. @param[in] Attributes The bit mask of attributes to clear for the memory region. @@ -1025,8 +1016,9 @@ SmmSetMemoryAttributesEx ( **/ EFI_STATUS -EFIAPI SmmClearMemoryAttributesEx ( + IN UINTN PageTableBase, + IN BOOLEAN EnablePML5Paging, IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length, IN UINT64 Attributes, diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c index b369c0c435..ce513c476f 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c @@ -32,24 +32,8 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { { Page1G, SIZE_1GB, PAGING_1G_ADDRESS_MASK_64 }, }; -UINTN mInternalCr3; UINTN IsShadowStack = FALSE; -/** - Set the internal page table base address. - If it is non zero, further MemoryAttribute modification will be on this page table. - If it is zero, further MemoryAttribute modification will be on real page table. - - @param Cr3 page table base. -**/ -VOID -SetPageTableBase ( - IN UINTN Cr3 - ) -{ - mInternalCr3 = Cr3; -} - /** Return length according to page attributes. @@ -99,31 +83,31 @@ PageAttributeToMask ( /** Return page table entry to match the address. - @param[in] Address The address to be checked. - @param[out] PageAttributes The page attribute of the page entry. + @param[in] PageTableBase The page table base. + @param[in] Enable5LevelPaging If PML5 paging is enabled. + @param[in] Address The address to be checked. + @param[out] PageAttributes The page attribute of the page entry. @return The page entry. **/ VOID * GetPageTableEntry ( + IN UINTN PageTableBase, + IN BOOLEAN Enable5LevelPaging, IN PHYSICAL_ADDRESS Address, OUT PAGE_ATTRIBUTE *PageAttribute ) { - UINTN Index1; - UINTN Index2; - UINTN Index3; - UINTN Index4; - UINTN Index5; - UINT64 *L1PageTable; - UINT64 *L2PageTable; - UINT64 *L3PageTable; - UINT64 *L4PageTable; - UINT64 *L5PageTable; - UINTN PageTableBase; - BOOLEAN Enable5LevelPaging; - - GetPageTable (&PageTableBase, &Enable5LevelPaging); + UINTN Index1; + UINTN Index2; + UINTN Index3; + UINTN Index4; + UINTN Index5; + UINT64 *L1PageTable; + UINT64 *L2PageTable; + UINT64 *L3PageTable; + UINT64 *L4PageTable; + UINT64 *L5PageTable; Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK; Index4 = ((UINTN)RShiftU64 (Address, 39)) & PAGING_PAE_INDEX_MASK; @@ -399,6 +383,8 @@ SplitPage ( Caller should make sure BaseAddress and Length is at page boundary. + @param[in] PageTableBase The page table base. + @param[in] EnablePML5Paging If PML5 paging is enabled. @param[in] BaseAddress The physical address that is the start address of a memory region. @param[in] Length The size in bytes of the memory region. @param[in] Attributes The bit mask of attributes to modify for the memory region. @@ -420,8 +406,9 @@ SplitPage ( range specified by BaseAddress and Length. **/ RETURN_STATUS -EFIAPI ConvertMemoryPageAttributes ( + IN UINTN PageTableBase, + IN BOOLEAN EnablePML5Paging, IN PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length, IN UINT64 Attributes, @@ -475,7 +462,7 @@ ConvertMemoryPageAttributes ( // Below logic is to check 2M/4K page to make sure we do not waste memory. // while (Length != 0) { - PageEntry = GetPageTableEntry (BaseAddress, &PageAttribute); + PageEntry = GetPageTableEntry (PageTableBase, EnablePML5Paging, BaseAddress, &PageAttribute); if (PageEntry == NULL) { return RETURN_UNSUPPORTED; } @@ -558,6 +545,8 @@ FlushTlbForAll ( This function sets the attributes for the memory region specified by BaseAddress and Length from their current attributes to the attributes specified by Attributes. + @param[in] PageTableBase The page table base. + @param[in] EnablePML5Paging If PML5 paging is enabled. @param[in] BaseAddress The physical address that is the start address of a memory region. @param[in] Length The size in bytes of the memory region. @param[in] Attributes The bit mask of attributes to set for the memory region. @@ -578,8 +567,9 @@ FlushTlbForAll ( **/ EFI_STATUS -EFIAPI SmmSetMemoryAttributesEx ( + IN UINTN PageTableBase, + IN BOOLEAN EnablePML5Paging, IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length, IN UINT64 Attributes, @@ -589,7 +579,7 @@ SmmSetMemoryAttributesEx ( EFI_STATUS Status; BOOLEAN IsModified; - Status = ConvertMemoryPageAttributes (BaseAddress, Length, Attributes, TRUE, IsSplitted, &IsModified); + Status = ConvertMemoryPageAttributes (PageTableBase, EnablePML5Paging, BaseAddress, Length, Attributes, TRUE, IsSplitted, &IsModified); if (!EFI_ERROR (Status)) { if (IsModified) { // @@ -606,6 +596,8 @@ SmmSetMemoryAttributesEx ( This function clears the attributes for the memory region specified by BaseAddress and Length from their current attributes to the attributes specified by Attributes. + @param[in] PageTableBase The page table base. + @param[in] EnablePML5Paging If PML5 paging is enabled. @param[in] BaseAddress The physical address that is the start address of a memory region. @param[in] Length The size in bytes of the memory region. @param[in] Attributes The bit mask of attributes to clear for the memory region. @@ -626,8 +618,9 @@ SmmSetMemoryAttributesEx ( **/ EFI_STATUS -EFIAPI SmmClearMemoryAttributesEx ( + IN UINTN PageTableBase, + IN BOOLEAN EnablePML5Paging, IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length, IN UINT64 Attributes, @@ -637,7 +630,7 @@ SmmClearMemoryAttributesEx ( EFI_STATUS Status; BOOLEAN IsModified; - Status = ConvertMemoryPageAttributes (BaseAddress, Length, Attributes, FALSE, IsSplitted, &IsModified); + Status = ConvertMemoryPageAttributes (PageTableBase, EnablePML5Paging, BaseAddress, Length, Attributes, FALSE, IsSplitted, &IsModified); if (!EFI_ERROR (Status)) { if (IsModified) { // @@ -673,14 +666,20 @@ SmmClearMemoryAttributesEx ( **/ EFI_STATUS -EFIAPI SmmSetMemoryAttributes ( IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length, IN UINT64 Attributes ) { - return SmmSetMemoryAttributesEx (BaseAddress, Length, Attributes, NULL); + IA32_CR4 Cr4; + UINTN PageTableBase; + BOOLEAN Enable5LevelPaging; + + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; + Cr4.UintN = AsmReadCr4 (); + Enable5LevelPaging = (BOOLEAN)(Cr4.Bits.LA57 == 1); + return SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, BaseAddress, Length, Attributes, NULL); } /** @@ -706,14 +705,20 @@ SmmSetMemoryAttributes ( **/ EFI_STATUS -EFIAPI SmmClearMemoryAttributes ( IN EFI_PHYSICAL_ADDRESS BaseAddress, IN UINT64 Length, IN UINT64 Attributes ) { - return SmmClearMemoryAttributesEx (BaseAddress, Length, Attributes, NULL); + IA32_CR4 Cr4; + UINTN PageTableBase; + BOOLEAN Enable5LevelPaging; + + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; + Cr4.UintN = AsmReadCr4 (); + Enable5LevelPaging = (BOOLEAN)(Cr4.Bits.LA57 == 1); + return SmmClearMemoryAttributesEx (PageTableBase, Enable5LevelPaging, BaseAddress, Length, Attributes, NULL); } /** @@ -736,7 +741,7 @@ SetShadowStack ( SetPageTableBase (Cr3); IsShadowStack = TRUE; - Status = SmmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO); + Status = SmmSetMemoryAttributesEx (Cr3, m5LevelPagingNeeded, BaseAddress, Length, EFI_MEMORY_RO, NULL); SetPageTableBase (0); IsShadowStack = FALSE; @@ -762,12 +767,7 @@ SetNotPresentPage ( { EFI_STATUS Status; - SetPageTableBase (Cr3); - - Status = SmmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RP); - - SetPageTableBase (0); - + Status = SmmSetMemoryAttributesEx (Cr3, m5LevelPagingNeeded, BaseAddress, Length, EFI_MEMORY_RP, NULL); return Status; } @@ -1560,6 +1560,9 @@ EdkiiSmmGetMemoryAttributes ( UINT64 MemAttr; PAGE_ATTRIBUTE PageAttr; INT64 Size; + UINTN PageTableBase; + BOOLEAN EnablePML5Paging; + IA32_CR4 Cr4; if ((Length < SIZE_4KB) || (Attributes == NULL)) { return EFI_INVALID_PARAMETER; @@ -1568,8 +1571,12 @@ EdkiiSmmGetMemoryAttributes ( Size = (INT64)Length; MemAttr = (UINT64)-1; + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; + Cr4.UintN = AsmReadCr4 (); + EnablePML5Paging = (BOOLEAN)(Cr4.Bits.LA57 == 1); + do { - PageEntry = GetPageTableEntry (BaseAddress, &PageAttr); + PageEntry = GetPageTableEntry (PageTableBase, EnablePML5Paging, BaseAddress, &PageAttr); if ((PageEntry == NULL) || (PageAttr == PageNone)) { return EFI_UNSUPPORTED; } diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c index 538394f239..6e920b32af 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c @@ -113,36 +113,6 @@ 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. @@ -1195,20 +1165,21 @@ SetPageTableAttributes ( VOID ) { - UINTN Index2; - UINTN Index3; - UINTN Index4; - UINTN Index5; - UINT64 *L1PageTable; - UINT64 *L2PageTable; - UINT64 *L3PageTable; - UINT64 *L4PageTable; - UINT64 *L5PageTable; - UINTN PageTableBase; - BOOLEAN IsSplitted; - BOOLEAN PageTableSplitted; - BOOLEAN CetEnabled; - BOOLEAN Enable5LevelPaging; + UINTN Index2; + UINTN Index3; + UINTN Index4; + UINTN Index5; + UINT64 *L1PageTable; + UINT64 *L2PageTable; + UINT64 *L3PageTable; + UINT64 *L4PageTable; + UINT64 *L5PageTable; + UINTN PageTableBase; + BOOLEAN IsSplitted; + BOOLEAN PageTableSplitted; + BOOLEAN CetEnabled; + BOOLEAN Enable5LevelPaging; + IA32_CR4 Cr4; // // Don't mark page table memory as read-only if @@ -1258,11 +1229,13 @@ SetPageTableAttributes ( PageTableSplitted = FALSE; L5PageTable = NULL; - GetPageTable (&PageTableBase, &Enable5LevelPaging); + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; + Cr4.UintN = AsmReadCr4 (); + Enable5LevelPaging = (BOOLEAN)(Cr4.Bits.LA57 == 1); if (Enable5LevelPaging) { L5PageTable = (UINT64 *)PageTableBase; - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, (EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); PageTableSplitted = (PageTableSplitted || IsSplitted); } @@ -1276,7 +1249,7 @@ SetPageTableAttributes ( L4PageTable = (UINT64 *)PageTableBase; } - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, (EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); PageTableSplitted = (PageTableSplitted || IsSplitted); for (Index4 = 0; Index4 < SIZE_4KB/sizeof (UINT64); Index4++) { @@ -1285,7 +1258,7 @@ SetPageTableAttributes ( continue; } - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, (EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); PageTableSplitted = (PageTableSplitted || IsSplitted); for (Index3 = 0; Index3 < SIZE_4KB/sizeof (UINT64); Index3++) { @@ -1299,7 +1272,7 @@ SetPageTableAttributes ( continue; } - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); PageTableSplitted = (PageTableSplitted || IsSplitted); for (Index2 = 0; Index2 < SIZE_4KB/sizeof (UINT64); Index2++) { @@ -1313,7 +1286,7 @@ SetPageTableAttributes ( continue; } - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, (EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable, SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); PageTableSplitted = (PageTableSplitted || IsSplitted); } } -- 2.31.1.windows.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 in PiSmmCpuDxeSmm 2022-08-10 1:45 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 in PiSmmCpuDxeSmm duntan @ 2022-08-10 5:20 ` Ni, Ray 0 siblings, 0 replies; 8+ messages in thread From: Ni, Ray @ 2022-08-10 5:20 UTC (permalink / raw) To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R Reviewed-by: Ray Ni <ray.ni@intel.com> > -----Original Message----- > From: Tan, Dun <dun.tan@intel.com> > Sent: Wednesday, August 10, 2022 9:46 AM > To: devel@edk2.groups.io > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, > Rahul R <rahul.r.kumar@intel.com> > Subject: [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 > in PiSmmCpuDxeSmm > > Remove mInternalCr3 in PiSmmCpuDxe pagetable related code. Currently, > mInternalCr3 is used to pass address of pagetable which is different > from Cr3 register. Now remove it and pass the page table base address > from the root function to simplify the code logic. > > Change-Id: I8f58158b94a01cf829f1b4fb2b8c763dcdda0662 > Signed-off-by: Dun Tan <dun.tan@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Rahul Kumar <rahul1.kumar@intel.com> > Signed-off-by: Dun Tan <dun.tan@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 30 +++++-------------- > ----------- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 26 > +++++++++----------------- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 107 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > ------------------------------------------------ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 73 > +++++++++++++++++++++++-------------------------------------------------- > 4 files changed, 94 insertions(+), 142 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > index 8ec8790c05..97058a2810 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c > @@ -28,26 +28,6 @@ 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. > > @@ -297,10 +277,10 @@ SetPageTableAttributes ( > DEBUG ((DEBUG_INFO, "Start...\n")); > PageTableSplitted = FALSE; > > - GetPageTable (&PageTableBase, NULL); > - L3PageTable = (UINT64 *)PageTableBase; > + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; > + L3PageTable = (UINT64 *)PageTableBase; > > - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)PageTableBase, > SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); > + SmmSetMemoryAttributesEx (PageTableBase, FALSE, > (EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > PageTableSplitted = (PageTableSplitted || IsSplitted); > > for (Index3 = 0; Index3 < 4; Index3++) { > @@ -309,7 +289,7 @@ SetPageTableAttributes ( > continue; > } > > - SmmSetMemoryAttributesEx > ((EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > + SmmSetMemoryAttributesEx (PageTableBase, FALSE, > (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > PageTableSplitted = (PageTableSplitted || IsSplitted); > > for (Index2 = 0; Index2 < SIZE_4KB/sizeof (UINT64); Index2++) { > @@ -323,7 +303,7 @@ SetPageTableAttributes ( > continue; > } > > - SmmSetMemoryAttributesEx > ((EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > + SmmSetMemoryAttributesEx (PageTableBase, FALSE, > (EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > PageTableSplitted = (PageTableSplitted || IsSplitted); > } > } > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index dfeceec2aa..ef8bf5947d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -264,7 +264,7 @@ extern UINTN mMaxNumberOfCpus; > extern UINTN mNumberOfCpus; > extern EFI_SMM_CPU_PROTOCOL mSmmCpu; > extern EFI_MM_MP_PROTOCOL mSmmMp; > -extern UINTN mInternalCr3; > +extern BOOLEAN m5LevelPagingNeeded; > > /// > /// The mode of the CPU at the time an SMI occurs > @@ -682,7 +682,6 @@ SmmBlockingStartupThisAp ( > > **/ > EFI_STATUS > -EFIAPI > SmmSetMemoryAttributes ( > IN EFI_PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > @@ -712,7 +711,6 @@ SmmSetMemoryAttributes ( > > **/ > EFI_STATUS > -EFIAPI > SmmClearMemoryAttributes ( > IN EFI_PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > @@ -957,22 +955,12 @@ SetPageTableAttributes ( > 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 > - ); > - > /** > This function sets the attributes for the memory region specified by > BaseAddress and > Length from their current attributes to the attributes specified by > Attributes. > > + @param[in] PageTableBase The page table base. > + @param[in] EnablePML5Paging If PML5 paging is enabled. > @param[in] BaseAddress The physical address that is the start address > of a memory region. > @param[in] Length The size in bytes of the memory region. > @param[in] Attributes The bit mask of attributes to set for the memory > region. > @@ -993,8 +981,9 @@ GetPageTable ( > > **/ > EFI_STATUS > -EFIAPI > SmmSetMemoryAttributesEx ( > + IN UINTN PageTableBase, > + IN BOOLEAN EnablePML5Paging, > IN EFI_PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > IN UINT64 Attributes, > @@ -1005,6 +994,8 @@ SmmSetMemoryAttributesEx ( > This function clears the attributes for the memory region specified by > BaseAddress and > Length from their current attributes to the attributes specified by > Attributes. > > + @param[in] PageTableBase The page table base. > + @param[in] EnablePML5Paging If PML5 paging is enabled. > @param[in] BaseAddress The physical address that is the start address > of a memory region. > @param[in] Length The size in bytes of the memory region. > @param[in] Attributes The bit mask of attributes to clear for the > memory region. > @@ -1025,8 +1016,9 @@ SmmSetMemoryAttributesEx ( > > **/ > EFI_STATUS > -EFIAPI > SmmClearMemoryAttributesEx ( > + IN UINTN PageTableBase, > + IN BOOLEAN EnablePML5Paging, > IN EFI_PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > IN UINT64 Attributes, > diff --git > a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index b369c0c435..ce513c476f 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -32,24 +32,8 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = { > { Page1G, SIZE_1GB, PAGING_1G_ADDRESS_MASK_64 }, > }; > > -UINTN mInternalCr3; > UINTN IsShadowStack = FALSE; > > -/** > - Set the internal page table base address. > - If it is non zero, further MemoryAttribute modification will be on this page > table. > - If it is zero, further MemoryAttribute modification will be on real page table. > - > - @param Cr3 page table base. > -**/ > -VOID > -SetPageTableBase ( > - IN UINTN Cr3 > - ) > -{ > - mInternalCr3 = Cr3; > -} > - > /** > Return length according to page attributes. > > @@ -99,31 +83,31 @@ PageAttributeToMask ( > /** > Return page table entry to match the address. > > - @param[in] Address The address to be checked. > - @param[out] PageAttributes The page attribute of the page entry. > + @param[in] PageTableBase The page table base. > + @param[in] Enable5LevelPaging If PML5 paging is enabled. > + @param[in] Address The address to be checked. > + @param[out] PageAttributes The page attribute of the page entry. > > @return The page entry. > **/ > VOID * > GetPageTableEntry ( > + IN UINTN PageTableBase, > + IN BOOLEAN Enable5LevelPaging, > IN PHYSICAL_ADDRESS Address, > OUT PAGE_ATTRIBUTE *PageAttribute > ) > { > - UINTN Index1; > - UINTN Index2; > - UINTN Index3; > - UINTN Index4; > - UINTN Index5; > - UINT64 *L1PageTable; > - UINT64 *L2PageTable; > - UINT64 *L3PageTable; > - UINT64 *L4PageTable; > - UINT64 *L5PageTable; > - UINTN PageTableBase; > - BOOLEAN Enable5LevelPaging; > - > - GetPageTable (&PageTableBase, &Enable5LevelPaging); > + UINTN Index1; > + UINTN Index2; > + UINTN Index3; > + UINTN Index4; > + UINTN Index5; > + UINT64 *L1PageTable; > + UINT64 *L2PageTable; > + UINT64 *L3PageTable; > + UINT64 *L4PageTable; > + UINT64 *L5PageTable; > > Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK; > Index4 = ((UINTN)RShiftU64 (Address, 39)) & PAGING_PAE_INDEX_MASK; > @@ -399,6 +383,8 @@ SplitPage ( > > Caller should make sure BaseAddress and Length is at page boundary. > > + @param[in] PageTableBase The page table base. > + @param[in] EnablePML5Paging If PML5 paging is enabled. > @param[in] BaseAddress The physical address that is the start address > of a memory region. > @param[in] Length The size in bytes of the memory region. > @param[in] Attributes The bit mask of attributes to modify for the > memory region. > @@ -420,8 +406,9 @@ SplitPage ( > range specified by BaseAddress and Length. > **/ > RETURN_STATUS > -EFIAPI > ConvertMemoryPageAttributes ( > + IN UINTN PageTableBase, > + IN BOOLEAN EnablePML5Paging, > IN PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > IN UINT64 Attributes, > @@ -475,7 +462,7 @@ ConvertMemoryPageAttributes ( > // Below logic is to check 2M/4K page to make sure we do not waste > memory. > // > while (Length != 0) { > - PageEntry = GetPageTableEntry (BaseAddress, &PageAttribute); > + PageEntry = GetPageTableEntry (PageTableBase, EnablePML5Paging, > BaseAddress, &PageAttribute); > if (PageEntry == NULL) { > return RETURN_UNSUPPORTED; > } > @@ -558,6 +545,8 @@ FlushTlbForAll ( > This function sets the attributes for the memory region specified by > BaseAddress and > Length from their current attributes to the attributes specified by > Attributes. > > + @param[in] PageTableBase The page table base. > + @param[in] EnablePML5Paging If PML5 paging is enabled. > @param[in] BaseAddress The physical address that is the start address > of a memory region. > @param[in] Length The size in bytes of the memory region. > @param[in] Attributes The bit mask of attributes to set for the memory > region. > @@ -578,8 +567,9 @@ FlushTlbForAll ( > > **/ > EFI_STATUS > -EFIAPI > SmmSetMemoryAttributesEx ( > + IN UINTN PageTableBase, > + IN BOOLEAN EnablePML5Paging, > IN EFI_PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > IN UINT64 Attributes, > @@ -589,7 +579,7 @@ SmmSetMemoryAttributesEx ( > EFI_STATUS Status; > BOOLEAN IsModified; > > - Status = ConvertMemoryPageAttributes (BaseAddress, Length, Attributes, > TRUE, IsSplitted, &IsModified); > + Status = ConvertMemoryPageAttributes (PageTableBase, > EnablePML5Paging, BaseAddress, Length, Attributes, TRUE, IsSplitted, > &IsModified); > if (!EFI_ERROR (Status)) { > if (IsModified) { > // > @@ -606,6 +596,8 @@ SmmSetMemoryAttributesEx ( > This function clears the attributes for the memory region specified by > BaseAddress and > Length from their current attributes to the attributes specified by > Attributes. > > + @param[in] PageTableBase The page table base. > + @param[in] EnablePML5Paging If PML5 paging is enabled. > @param[in] BaseAddress The physical address that is the start address > of a memory region. > @param[in] Length The size in bytes of the memory region. > @param[in] Attributes The bit mask of attributes to clear for the > memory region. > @@ -626,8 +618,9 @@ SmmSetMemoryAttributesEx ( > > **/ > EFI_STATUS > -EFIAPI > SmmClearMemoryAttributesEx ( > + IN UINTN PageTableBase, > + IN BOOLEAN EnablePML5Paging, > IN EFI_PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > IN UINT64 Attributes, > @@ -637,7 +630,7 @@ SmmClearMemoryAttributesEx ( > EFI_STATUS Status; > BOOLEAN IsModified; > > - Status = ConvertMemoryPageAttributes (BaseAddress, Length, Attributes, > FALSE, IsSplitted, &IsModified); > + Status = ConvertMemoryPageAttributes (PageTableBase, > EnablePML5Paging, BaseAddress, Length, Attributes, FALSE, IsSplitted, > &IsModified); > if (!EFI_ERROR (Status)) { > if (IsModified) { > // > @@ -673,14 +666,20 @@ SmmClearMemoryAttributesEx ( > > **/ > EFI_STATUS > -EFIAPI > SmmSetMemoryAttributes ( > IN EFI_PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > IN UINT64 Attributes > ) > { > - return SmmSetMemoryAttributesEx (BaseAddress, Length, Attributes, > NULL); > + IA32_CR4 Cr4; > + UINTN PageTableBase; > + BOOLEAN Enable5LevelPaging; > + > + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; > + Cr4.UintN = AsmReadCr4 (); > + Enable5LevelPaging = (BOOLEAN)(Cr4.Bits.LA57 == 1); > + return SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, > BaseAddress, Length, Attributes, NULL); > } > > /** > @@ -706,14 +705,20 @@ SmmSetMemoryAttributes ( > > **/ > EFI_STATUS > -EFIAPI > SmmClearMemoryAttributes ( > IN EFI_PHYSICAL_ADDRESS BaseAddress, > IN UINT64 Length, > IN UINT64 Attributes > ) > { > - return SmmClearMemoryAttributesEx (BaseAddress, Length, Attributes, > NULL); > + IA32_CR4 Cr4; > + UINTN PageTableBase; > + BOOLEAN Enable5LevelPaging; > + > + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; > + Cr4.UintN = AsmReadCr4 (); > + Enable5LevelPaging = (BOOLEAN)(Cr4.Bits.LA57 == 1); > + return SmmClearMemoryAttributesEx (PageTableBase, > Enable5LevelPaging, BaseAddress, Length, Attributes, NULL); > } > > /** > @@ -736,7 +741,7 @@ SetShadowStack ( > > SetPageTableBase (Cr3); > IsShadowStack = TRUE; > - Status = SmmSetMemoryAttributes (BaseAddress, Length, > EFI_MEMORY_RO); > + Status = SmmSetMemoryAttributesEx (Cr3, m5LevelPagingNeeded, > BaseAddress, Length, EFI_MEMORY_RO, NULL); > > SetPageTableBase (0); > IsShadowStack = FALSE; > @@ -762,12 +767,7 @@ SetNotPresentPage ( > { > EFI_STATUS Status; > > - SetPageTableBase (Cr3); > - > - Status = SmmSetMemoryAttributes (BaseAddress, Length, > EFI_MEMORY_RP); > - > - SetPageTableBase (0); > - > + Status = SmmSetMemoryAttributesEx (Cr3, m5LevelPagingNeeded, > BaseAddress, Length, EFI_MEMORY_RP, NULL); > return Status; > } > > @@ -1560,6 +1560,9 @@ EdkiiSmmGetMemoryAttributes ( > UINT64 MemAttr; > PAGE_ATTRIBUTE PageAttr; > INT64 Size; > + UINTN PageTableBase; > + BOOLEAN EnablePML5Paging; > + IA32_CR4 Cr4; > > if ((Length < SIZE_4KB) || (Attributes == NULL)) { > return EFI_INVALID_PARAMETER; > @@ -1568,8 +1571,12 @@ EdkiiSmmGetMemoryAttributes ( > Size = (INT64)Length; > MemAttr = (UINT64)-1; > > + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; > + Cr4.UintN = AsmReadCr4 (); > + EnablePML5Paging = (BOOLEAN)(Cr4.Bits.LA57 == 1); > + > do { > - PageEntry = GetPageTableEntry (BaseAddress, &PageAttr); > + PageEntry = GetPageTableEntry (PageTableBase, EnablePML5Paging, > BaseAddress, &PageAttr); > if ((PageEntry == NULL) || (PageAttr == PageNone)) { > return EFI_UNSUPPORTED; > } > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > index 538394f239..6e920b32af 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c > @@ -113,36 +113,6 @@ 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. > > @@ -1195,20 +1165,21 @@ SetPageTableAttributes ( > VOID > ) > { > - UINTN Index2; > - UINTN Index3; > - UINTN Index4; > - UINTN Index5; > - UINT64 *L1PageTable; > - UINT64 *L2PageTable; > - UINT64 *L3PageTable; > - UINT64 *L4PageTable; > - UINT64 *L5PageTable; > - UINTN PageTableBase; > - BOOLEAN IsSplitted; > - BOOLEAN PageTableSplitted; > - BOOLEAN CetEnabled; > - BOOLEAN Enable5LevelPaging; > + UINTN Index2; > + UINTN Index3; > + UINTN Index4; > + UINTN Index5; > + UINT64 *L1PageTable; > + UINT64 *L2PageTable; > + UINT64 *L3PageTable; > + UINT64 *L4PageTable; > + UINT64 *L5PageTable; > + UINTN PageTableBase; > + BOOLEAN IsSplitted; > + BOOLEAN PageTableSplitted; > + BOOLEAN CetEnabled; > + BOOLEAN Enable5LevelPaging; > + IA32_CR4 Cr4; > > // > // Don't mark page table memory as read-only if > @@ -1258,11 +1229,13 @@ SetPageTableAttributes ( > PageTableSplitted = FALSE; > L5PageTable = NULL; > > - GetPageTable (&PageTableBase, &Enable5LevelPaging); > + PageTableBase = AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64; > + Cr4.UintN = AsmReadCr4 (); > + Enable5LevelPaging = (BOOLEAN)(Cr4.Bits.LA57 == 1); > > if (Enable5LevelPaging) { > L5PageTable = (UINT64 *)PageTableBase; > - SmmSetMemoryAttributesEx ((EFI_PHYSICAL_ADDRESS)PageTableBase, > SIZE_4KB, EFI_MEMORY_RO, &IsSplitted); > + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, > (EFI_PHYSICAL_ADDRESS)PageTableBase, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > PageTableSplitted = (PageTableSplitted || IsSplitted); > } > > @@ -1276,7 +1249,7 @@ SetPageTableAttributes ( > L4PageTable = (UINT64 *)PageTableBase; > } > > - SmmSetMemoryAttributesEx > ((EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, > (EFI_PHYSICAL_ADDRESS)(UINTN)L4PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > PageTableSplitted = (PageTableSplitted || IsSplitted); > > for (Index4 = 0; Index4 < SIZE_4KB/sizeof (UINT64); Index4++) { > @@ -1285,7 +1258,7 @@ SetPageTableAttributes ( > continue; > } > > - SmmSetMemoryAttributesEx > ((EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, > (EFI_PHYSICAL_ADDRESS)(UINTN)L3PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > PageTableSplitted = (PageTableSplitted || IsSplitted); > > for (Index3 = 0; Index3 < SIZE_4KB/sizeof (UINT64); Index3++) { > @@ -1299,7 +1272,7 @@ SetPageTableAttributes ( > continue; > } > > - SmmSetMemoryAttributesEx > ((EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, > (EFI_PHYSICAL_ADDRESS)(UINTN)L2PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > PageTableSplitted = (PageTableSplitted || IsSplitted); > > for (Index2 = 0; Index2 < SIZE_4KB/sizeof (UINT64); Index2++) { > @@ -1313,7 +1286,7 @@ SetPageTableAttributes ( > continue; > } > > - SmmSetMemoryAttributesEx > ((EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > + SmmSetMemoryAttributesEx (PageTableBase, Enable5LevelPaging, > (EFI_PHYSICAL_ADDRESS)(UINTN)L1PageTable, SIZE_4KB, EFI_MEMORY_RO, > &IsSplitted); > PageTableSplitted = (PageTableSplitted || IsSplitted); > } > } > -- > 2.31.1.windows.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm 2022-08-10 1:45 [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm duntan 2022-08-10 1:45 ` [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag duntan 2022-08-10 1:45 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 in PiSmmCpuDxeSmm duntan @ 2022-08-10 3:20 ` Sean 2022-08-10 4:03 ` duntan 2 siblings, 1 reply; 8+ messages in thread From: Sean @ 2022-08-10 3:20 UTC (permalink / raw) To: devel@edk2.groups.io, dun.tan@intel.com [-- Attachment #1: Type: text/plain, Size: 1202 bytes --] Is there any documentation to why this change is important and what exactly was broken? Thanks Sean ________________________________ From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of duntan <dun.tan@intel.com> Sent: Tuesday, August 9, 2022 6:45:30 PM To: devel@edk2.groups.io <devel@edk2.groups.io> Subject: [edk2-devel] [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm Add a new IsShadowStack flag in PiSmmCpuDxeSmm. Remove mInternalCr3 in PiSmmCpuDxeSmm. Dun Tan (2): UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 in PiSmmCpuDxeSmm UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 30 +++++------------------------- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 26 +++++++++----------------- UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------- UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 73 +++++++++++++++++++++++-------------------------------------------------- 4 files changed, 98 insertions(+), 144 deletions(-) -- 2.31.1.windows.1 [-- Attachment #2: Type: text/html, Size: 2194 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm 2022-08-10 3:20 ` [edk2-devel] [PATCH 0/2] " Sean @ 2022-08-10 4:03 ` duntan 0 siblings, 0 replies; 8+ messages in thread From: duntan @ 2022-08-10 4:03 UTC (permalink / raw) To: Sean Brogan, devel@edk2.groups.io [-- Attachment #1: Type: text/plain, Size: 2317 bytes --] Hi Sean, The patch set is a code refactoring and doesn't change any functionality. The reason for this code refactoring is because: 1. In PiSmmCpuDxe driver entrypoint, this driver initializes smm page table which is different from Cr3 register. Currently, mInternalCr3 is used to pass address of page table which is different from Cr3 register. Now remove it and pass the page table base address from the root function to simplify the code logic. 2. Besides, current code logic will regard a RO range as shadow stack and set the dirty bit in corresponding page table entry if mInternalCr3 is not 0. This assumption may be confusing. A new mIsShadowStack flag will be created to identify if it is a shadow stack or not. Thanks, Dun From: Sean Brogan <spbrogan@outlook.com> Sent: Wednesday, August 10, 2022 11:21 AM To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com> Subject: Re: [edk2-devel] [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm Is there any documentation to why this change is important and what exactly was broken? Thanks Sean ________________________________ From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> on behalf of duntan <dun.tan@intel.com<mailto:dun.tan@intel.com>> Sent: Tuesday, August 9, 2022 6:45:30 PM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> Subject: [edk2-devel] [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm Add a new IsShadowStack flag in PiSmmCpuDxeSmm. Remove mInternalCr3 in PiSmmCpuDxeSmm. Dun Tan (2): UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 in PiSmmCpuDxeSmm UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 30 +++++------------------------- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 26 +++++++++----------------- UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------- UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 73 +++++++++++++++++++++++-------------------------------------------------- 4 files changed, 98 insertions(+), 144 deletions(-) -- 2.31.1.windows.1 [-- Attachment #2: Type: text/html, Size: 5816 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-08-10 5:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-10 1:45 [PATCH 0/2] Remove mInternalCr3 in PiSmmCpuDxeSmm duntan 2022-08-10 1:45 ` [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Add a new IsShadowStack flag duntan 2022-08-10 3:51 ` [edk2-devel] " Ni, Ray 2022-08-10 4:02 ` duntan 2022-08-10 1:45 ` [PATCH 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove mInternalCr3 in PiSmmCpuDxeSmm duntan 2022-08-10 5:20 ` Ni, Ray 2022-08-10 3:20 ` [edk2-devel] [PATCH 0/2] " Sean 2022-08-10 4:03 ` duntan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox