UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-)
When mInternalCr3 is used, get paging level type by a variable
which is set when mInternalCr3 is generated.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3015
Change-Id: I1295d671a6c01c0a077e4b79fa83d500d49ef29c
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/SmmCpuMemoryManagement.c | 41 +++++++++++++++++++++++++++++++++--------
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index ebfc46ad45..3264c72af2 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -32,7 +32,12 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
{Page1G, SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},
};
-UINTN mInternalGr3;
+UINTN mInternalCr3;
+
+//
+// Variables from Protected Mode SMI Entry Template
+//
+extern BOOLEAN gSmmFeatureEnable5LevelPaging;
/**
Set the internal page table base address.
@@ -46,7 +51,7 @@ SetPageTableBase (
IN UINTN Cr3
)
{
- mInternalGr3 = Cr3;
+ mInternalCr3 = Cr3;
}
/**
@@ -59,12 +64,34 @@ GetPageTableBase (
VOID
)
{
- if (mInternalGr3 != 0) {
- return mInternalGr3;
+ if (mInternalCr3 != 0) {
+ return mInternalCr3;
}
return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
}
+/**
+ Return if the page table base 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 != 0) {
+ return gSmmFeatureEnable5LevelPaging;
+ }
+
+ Cr4.UintN = AsmReadCr4 ();
+ return (BOOLEAN) (Cr4.Bits.LA57 == 1);
+}
+
/**
Return length according to page attributes.
@@ -131,7 +158,6 @@ GetPageTableEntry (
UINT64 *L3PageTable;
UINT64 *L4PageTable;
UINT64 *L5PageTable;
- IA32_CR4 Cr4;
BOOLEAN Enable5LevelPaging;
Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK;
@@ -140,8 +166,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();
if (sizeof(UINTN) == sizeof(UINT64)) {
if (Enable5LevelPaging) {
@@ -252,7 +277,7 @@ ConvertPageEntryAttribute (
if ((Attributes & EFI_MEMORY_RO) != 0) {
if (IsSet) {
NewPageEntry &= ~(UINT64)IA32_PG_RW;
- if (mInternalGr3 != 0) {
+ if (mInternalCr3 != 0) {
// Environment setup
// ReadOnly page need set Dirty bit for shadow stack
NewPageEntry |= IA32_PG_D;
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66554): https://edk2.groups.io/g/devel/message/66554
Mute This Topic: https://groups.io/mt/77745600/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
The overall logic looks good to me and I agree to add an internal function like the patch. Some minor comments: 1. Can you please separate the patch to two patches? One is to correct the Cr3 typo, the other is to fix the bug. 2. Can you please add comments in Is5LevelPageTableBase() to explain why directly checking CR4 doesn't work sometimes? And when (For example, when the function is called from entrypoint and the CR3/CR4 haven't been written)? 3. "When mInternalCr3 is used, get paging level type by a variable which is set when mInternalCr3 is generated." This commit message isn't so meaningful. It just translates the C logic to plain text. 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. 4. I cannot find below variable. Did you miss some code change in the patch? // // Variables from Protected Mode SMI Entry Template // extern BOOLEAN gSmmFeatureEnable5LevelPaging; -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66617): https://edk2.groups.io/g/devel/message/66617 Mute This Topic: https://groups.io/mt/77745600/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Ray, Thank you for the review. 1. Can you please separate the patch to two patches? One is to correct the Cr3 typo, the other is to fix the bug. Sure. I will update the patch. 1. Can you please add comments in Is5LevelPageTableBase() to explain why directly checking CR4 doesn't work sometimes? And when (For example, when the function is called from entrypoint and the CR3/CR4 haven't been written)? Sure. I will update the patch. 1. "When mInternalCr3 is used, get paging level type by a variable which is set when mInternalCr3 is generated." This commit message isn't so meaningful. It just translates the C logic to plain text. 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. Sure. I will update the commit message. 1. I cannot find below variable. Did you miss some code change in the patch? // // Variables from Protected Mode SMI Entry Template // extern BOOLEAN gSmmFeatureEnable5LevelPaging; gSmmFeatureEnable5LevelPaging is in platform smiEntry.nasm. Yes, It is not a good variable here. Do you think it is better to use extern “BOOLEAN m5LevelPagingNeeded;” in file Edk2/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c ? Thank you. BR Sheng Wei From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray Sent: 2020年10月27日 9:27 To: Sheng, W <w.sheng@intel.com>; devel@edk2.groups.io Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 The overall logic looks good to me and I agree to add an internal function like the patch. Some minor comments: 1. Can you please separate the patch to two patches? One is to correct the Cr3 typo, the other is to fix the bug. 2. Can you please add comments in Is5LevelPageTableBase() to explain why directly checking CR4 doesn't work sometimes? And when (For example, when the function is called from entrypoint and the CR3/CR4 haven't been written)? 3. "When mInternalCr3 is used, get paging level type by a variable which is set when mInternalCr3 is generated." This commit message isn't so meaningful. It just translates the C logic to plain text. 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. 4. I cannot find below variable. Did you miss some code change in the patch? // // Variables from Protected Mode SMI Entry Template // extern BOOLEAN gSmmFeatureEnable5LevelPaging; -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66641): https://edk2.groups.io/g/devel/message/66641 Mute This Topic: https://groups.io/mt/77745600/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
No. You cannot extern a variable defined only for x64. You may meet build failure in IA32 build. From: Sheng, W <w.sheng@intel.com> Sent: Tuesday, October 27, 2020 3:54 PM To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com> Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 Hi Ray, Thank you for the review. 1. Can you please separate the patch to two patches? One is to correct the Cr3 typo, the other is to fix the bug. Sure. I will update the patch. 1. Can you please add comments in Is5LevelPageTableBase() to explain why directly checking CR4 doesn't work sometimes? And when (For example, when the function is called from entrypoint and the CR3/CR4 haven't been written)? Sure. I will update the patch. 1. "When mInternalCr3 is used, get paging level type by a variable which is set when mInternalCr3 is generated." This commit message isn't so meaningful. It just translates the C logic to plain text. 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. Sure. I will update the commit message. 1. I cannot find below variable. Did you miss some code change in the patch? // // Variables from Protected Mode SMI Entry Template // extern BOOLEAN gSmmFeatureEnable5LevelPaging; gSmmFeatureEnable5LevelPaging is in platform smiEntry.nasm. Yes, It is not a good variable here. Do you think it is better to use extern “BOOLEAN m5LevelPagingNeeded;” in file Edk2/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c ? Thank you. BR Sheng Wei From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Ni, Ray Sent: 2020年10月27日 9:27 To: Sheng, W <w.sheng@intel.com<mailto:w.sheng@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 The overall logic looks good to me and I agree to add an internal function like the patch. Some minor comments: 1. Can you please separate the patch to two patches? One is to correct the Cr3 typo, the other is to fix the bug. 2. Can you please add comments in Is5LevelPageTableBase() to explain why directly checking CR4 doesn't work sometimes? And when (For example, when the function is called from entrypoint and the CR3/CR4 haven't been written)? 3. "When mInternalCr3 is used, get paging level type by a variable which is set when mInternalCr3 is generated." This commit message isn't so meaningful. It just translates the C logic to plain text. 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. 4. I cannot find below variable. Did you miss some code change in the patch? // // Variables from Protected Mode SMI Entry Template // extern BOOLEAN gSmmFeatureEnable5LevelPaging; -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66645): https://edk2.groups.io/g/devel/message/66645 Mute This Topic: https://groups.io/mt/77745600/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Sheng W Just a reminder, please make sure your update can work on pure EDKII platform both IA32 and X64. I recommend you validate the OVMF with SMM both IA32 and X64 to ensure there is no impact to the existing platform. Thank you Yao Jiewen From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray Sent: Tuesday, October 27, 2020 5:44 PM To: Sheng, W <w.sheng@intel.com>; devel@edk2.groups.io Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 No. You cannot extern a variable defined only for x64. You may meet build failure in IA32 build. From: Sheng, W <w.sheng@intel.com<mailto:w.sheng@intel.com>> Sent: Tuesday, October 27, 2020 3:54 PM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>> Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 Hi Ray, Thank you for the review. 1. Can you please separate the patch to two patches? One is to correct the Cr3 typo, the other is to fix the bug. Sure. I will update the patch. 1. Can you please add comments in Is5LevelPageTableBase() to explain why directly checking CR4 doesn't work sometimes? And when (For example, when the function is called from entrypoint and the CR3/CR4 haven't been written)? Sure. I will update the patch. 1. "When mInternalCr3 is used, get paging level type by a variable which is set when mInternalCr3 is generated." This commit message isn't so meaningful. It just translates the C logic to plain text. 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. Sure. I will update the commit message. 1. I cannot find below variable. Did you miss some code change in the patch? // // Variables from Protected Mode SMI Entry Template // extern BOOLEAN gSmmFeatureEnable5LevelPaging; gSmmFeatureEnable5LevelPaging is in platform smiEntry.nasm. Yes, It is not a good variable here. Do you think it is better to use extern “BOOLEAN m5LevelPagingNeeded;” in file Edk2/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c ? Thank you. BR Sheng Wei From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Ni, Ray Sent: 2020年10月27日 9:27 To: Sheng, W <w.sheng@intel.com<mailto:w.sheng@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 The overall logic looks good to me and I agree to add an internal function like the patch. Some minor comments: 1. Can you please separate the patch to two patches? One is to correct the Cr3 typo, the other is to fix the bug. 2. Can you please add comments in Is5LevelPageTableBase() to explain why directly checking CR4 doesn't work sometimes? And when (For example, when the function is called from entrypoint and the CR3/CR4 haven't been written)? 3. "When mInternalCr3 is used, get paging level type by a variable which is set when mInternalCr3 is generated." This commit message isn't so meaningful. It just translates the C logic to plain text. 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. 4. I cannot find below variable. Did you miss some code change in the patch? // // Variables from Protected Mode SMI Entry Template // extern BOOLEAN gSmmFeatureEnable5LevelPaging; -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66646): https://edk2.groups.io/g/devel/message/66646 Mute This Topic: https://groups.io/mt/77745600/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/27/20 11:00, Yao, Jiewen wrote: > Hi Sheng W > Just a reminder, please make sure your update can work on pure EDKII platform both IA32 and X64. > > I recommend you validate the OVMF with SMM both IA32 and X64 to ensure there is no impact to the existing platform. I have nothing to add: Ray and Jiewen have covered all the points I had in mind (and more). Only sending this notice to confirm that those points are indeed important, to me as well. Thanks! Laszlo > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray > Sent: Tuesday, October 27, 2020 5:44 PM > To: Sheng, W <w.sheng@intel.com>; devel@edk2.groups.io > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 > > No. You cannot extern a variable defined only for x64. You may meet build failure in IA32 build. > > From: Sheng, W <w.sheng@intel.com<mailto:w.sheng@intel.com>> > Sent: Tuesday, October 27, 2020 3:54 PM > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>> > Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 > > Hi Ray, > Thank you for the review. > > 1. Can you please separate the patch to two patches? One is to correct the Cr3 typo, the other is to fix the bug. > > Sure. I will update the patch. > > 1. Can you please add comments in Is5LevelPageTableBase() to explain why directly checking CR4 doesn't work sometimes? And when (For example, when the function is called from entrypoint and the CR3/CR4 haven't been written)? > > Sure. I will update the patch. > > 1. "When mInternalCr3 is used, get paging level type by a variable which is set when mInternalCr3 is generated." This commit message isn't so meaningful. It just translates the C logic to plain text. 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. > > Sure. I will update the commit message. > > 1. I cannot find below variable. Did you miss some code change in the patch? // // Variables from Protected Mode SMI Entry Template // extern BOOLEAN gSmmFeatureEnable5LevelPaging; > > gSmmFeatureEnable5LevelPaging is in platform smiEntry.nasm. > > Yes, It is not a good variable here. > > Do you think it is better to use extern “BOOLEAN m5LevelPagingNeeded;” in file Edk2/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c ? > > Thank you. > > BR > > Sheng Wei > > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Ni, Ray > Sent: 2020年10月27日 9:27 > To: Sheng, W <w.sheng@intel.com<mailto:w.sheng@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Adjust level paging type for Internal CR3 > > > The overall logic looks good to me and I agree to add an internal function like the patch. > > Some minor comments: > > 1. Can you please separate the patch to two patches? One is to correct the Cr3 typo, the other is to fix the bug. > 2. Can you please add comments in Is5LevelPageTableBase() to explain why directly checking CR4 doesn't work sometimes? And when (For example, when the function is called from entrypoint and the CR3/CR4 haven't been written)? > 3. "When mInternalCr3 is used, get paging level type by a variable which is set when mInternalCr3 is generated." This commit message isn't so meaningful. It just translates the C logic to plain text. 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. > 4. I cannot find below variable. Did you miss some code change in the patch? // // Variables from Protected Mode SMI Entry Template // extern BOOLEAN gSmmFeatureEnable5LevelPaging; > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66659): https://edk2.groups.io/g/devel/message/66659 Mute This Topic: https://groups.io/mt/77745600/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.