* [Patch V4 00/21] Fix issues in CpuPageTableLib
@ 2023-03-23 7:40 duntan
2023-03-23 7:40 ` [Patch V4 01/21] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition duntan
` (21 more replies)
0 siblings, 22 replies; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel
In the V4 atch set
1. In the patch "UefiCpuPkg/MpInitLib: Add code to initialize MapMask", add code to set MapMask to MAX_UINT64 to fix the boot failure tested by Gred.
2. Add patches to enable PAE paing in CpuPageTable Lib and add random test for PAE paging
3. Add patch to reduce the number of random test since CI may timeout
4. Modify the patch to fix non-1:1 mapping issue
Dun Tan (19):
UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition
UefiCpuPkg/CpuPageTableLib: Add check for input Length
UefiCpuPkg/CpuPageTableLib:Initialize some LocalVariable at beginning
UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
UefiCpuPkg/CpuPageTableLib:Clear PageSize bit(Bit7) for non-leaf
UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry
UefiCpuPkg/MpInitLib: Add code to initialize MapMask
UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr
UefiCpuPkg/CpuPageTableLib: Add manual test to check Mask and Attr
UefiCpuPkg/CpuPageTableLib:Modify RandomBoolean() in RandomTest
UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr
UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test
UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter.
UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified
UefiCpuPkg/CpuPageTableLib: Add check for page table creation
UefiCpuPkg: Combine branch for non-present and leaf ParentEntry
UefiCpuPkg/CpuPageTableLib: Enable PAE paging
UefiCpuPkg/CpuPageTableLib: Add RandomTest for PAE paging
UefiCpuPkg/CpuPageTableLib: Reduce the number of random tests
Zhiguang Liu (2):
UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf
UefiCpuPkg: Modify UnitTest code since tested API is changed
UefiCpuPkg/Include/Library/CpuPageTableLib.h | 42 ++++++++++++++++++++++--------------------
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h | 135 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------------
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableParse.c | 25 +++++++++++++++++++++----
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 323 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------------
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c | 22 +++++++++++++++-------
UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c | 11 +++++------
8 files changed, 768 insertions(+), 327 deletions(-)
--
2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* [Patch V4 01/21] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 7:40 ` [Patch V4 02/21] UefiCpuPkg/CpuPageTableLib: Add check for input Length duntan
` (20 subsequent siblings)
21 siblings, 0 replies; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Remove unneeded 'if' condition in CpuPageTableLib code.
The deleted code is in the code branch for present non-leaf parent
entry. So the 'if' check for (ParentPagingEntry->Pnle.Bits.Present
== 0) is always FALSE.
Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 37713ec659..52535e5a8d 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -1,7 +1,7 @@
/** @file
This library implements CpuPageTableLib that are generic for IA32 family CPU.
- Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -375,15 +375,6 @@ PageTableLibMapInLevel (
// we need to change PDPTE[0].ReadWrite = 1 and let all PDE[0-255].ReadWrite = 0 in this step.
// when PDPTE[0].Nx = 1 but caller wants to map [0-2MB] as Nx = 0 (PDT[0].Nx = 0)
// we need to change PDPTE[0].Nx = 0 and let all PDE[0-255].Nx = 1 in this step.
- if ((ParentPagingEntry->Pnle.Bits.Present == 0) && (Mask->Bits.Present == 1) && (Attribute->Bits.Present == 1)) {
- if (Modify) {
- ParentPagingEntry->Pnle.Bits.Present = 1;
- }
-
- ChildAttribute.Bits.Present = 0;
- ChildMask.Bits.Present = 1;
- }
-
if ((ParentPagingEntry->Pnle.Bits.ReadWrite == 0) && (Mask->Bits.ReadWrite == 1) && (Attribute->Bits.ReadWrite == 1)) {
if (Modify) {
ParentPagingEntry->Pnle.Bits.ReadWrite = 1;
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 02/21] UefiCpuPkg/CpuPageTableLib: Add check for input Length
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
2023-03-23 7:40 ` [Patch V4 01/21] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 12:20 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 03/21] UefiCpuPkg/CpuPageTableLib:Initialize some LocalVariable at beginning duntan
` (19 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Add check for input Length in PageTableMap (). Return
RETURN_SUCCESS when input Length is 0.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Include/Library/CpuPageTableLib.h | 4 ++--
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 6 +++++-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index 2dc9b7d18e..5f44ece548 100644
--- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
+++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
@@ -1,7 +1,7 @@
/** @file
Public include file for PageTableLib library.
- Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -81,7 +81,7 @@ typedef enum {
@retval RETURN_BUFFER_TOO_SMALL The buffer is too small for page table creation/updating.
BufferSize is updated to indicate the expected buffer size.
Caller may still get RETURN_BUFFER_TOO_SMALL with the new BufferSize.
- @retval RETURN_SUCCESS PageTable is created/updated successfully.
+ @retval RETURN_SUCCESS PageTable is created/updated successfully or the input Length is 0.
**/
RETURN_STATUS
EFIAPI
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 52535e5a8d..218068a3e1 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -544,7 +544,7 @@ PageTableLibMapInLevel (
@retval RETURN_BUFFER_TOO_SMALL The buffer is too small for page table creation/updating.
BufferSize is updated to indicate the expected buffer size.
Caller may still get RETURN_BUFFER_TOO_SMALL with the new BufferSize.
- @retval RETURN_SUCCESS PageTable is created/updated successfully.
+ @retval RETURN_SUCCESS PageTable is created/updated successfully or the input Length is 0.
**/
RETURN_STATUS
EFIAPI
@@ -567,6 +567,10 @@ PageTableMap (
IA32_PAGE_LEVEL MaxLeafLevel;
IA32_MAP_ATTRIBUTE ParentAttribute;
+ if (Length == 0) {
+ return RETURN_SUCCESS;
+ }
+
if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) || (PagingMode >= PagingModeMax)) {
//
// 32bit paging is never supported.
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 03/21] UefiCpuPkg/CpuPageTableLib:Initialize some LocalVariable at beginning
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
2023-03-23 7:40 ` [Patch V4 01/21] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition duntan
2023-03-23 7:40 ` [Patch V4 02/21] UefiCpuPkg/CpuPageTableLib: Add check for input Length duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 12:26 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 04/21] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan
` (18 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Move some local variable initialization to the beginning of the
function. Also delete duplicated calculation for RegionLength.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 218068a3e1..127b65183f 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -258,6 +258,7 @@ PageTableLibMapInLevel (
UINTN BitStart;
UINTN Index;
IA32_PAGING_ENTRY *PagingEntry;
+ UINTN PagingEntryIndex;
IA32_PAGING_ENTRY *CurrentPagingEntry;
UINT64 RegionLength;
UINT64 SubLength;
@@ -288,6 +289,14 @@ PageTableLibMapInLevel (
LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
ParentAttribute = &LocalParentAttribute;
+ //
+ // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or 4K (1 << 12).
+ //
+ BitStart = 12 + (Level - 1) * 9;
+ PagingEntryIndex = (UINTN)BitFieldRead64 (LinearAddress + Offset, BitStart, BitStart + 9 - 1);
+ RegionLength = REGION_LENGTH (Level);
+ RegionMask = RegionLength - 1;
+
//
// ParentPagingEntry ONLY is deferenced for checking Present and MustBeOne bits
// when Modify is FALSE.
@@ -353,8 +362,7 @@ PageTableLibMapInLevel (
//
PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOneMask);
- RegionLength = REGION_LENGTH (Level);
- PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
+ PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
for (SubOffset = 0, Index = 0; Index < 512; Index++) {
PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
SubOffset += RegionLength;
@@ -425,15 +433,10 @@ PageTableLibMapInLevel (
}
//
- // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or 4K (1 << 12).
// RegionStart: points to the linear address that's aligned on RegionLength and lower than (LinearAddress + Offset).
//
- BitStart = 12 + (Level - 1) * 9;
- Index = (UINTN)BitFieldRead64 (LinearAddress + Offset, BitStart, BitStart + 9 - 1);
- RegionLength = LShiftU64 (1, BitStart);
- RegionMask = RegionLength - 1;
- RegionStart = (LinearAddress + Offset) & ~RegionMask;
-
+ Index = PagingEntryIndex;
+ RegionStart = (LinearAddress + Offset) & ~RegionMask;
ParentAttribute->Uint64 = PageTableLibGetPnleMapAttribute (&ParentPagingEntry->Pnle, ParentAttribute);
//
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 04/21] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (2 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 03/21] UefiCpuPkg/CpuPageTableLib:Initialize some LocalVariable at beginning duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 12:29 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 05/21] UefiCpuPkg/CpuPageTableLib:Clear PageSize bit(Bit7) for non-leaf duntan
` (17 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
In previous code logic, when splitting a leaf parent entry to
smaller granularity child page table, if the parent entry
Attribute&Mask(without PageTableBaseAddress field) is equal to the
input attribute&mask(without PageTableBaseAddress field), the split
process won't happen. This may lead to failure in non-1:1 mapping.
For example, there is a page table in which [0, 1G] is mapped(Lv4[0]
,Lv3[0,0], a non-leaf level4 entry and a leaf level3 entry). And we
want to remap [0, 2M] linear address range to [1G, 1G + 2M] with the
same attibute. The expected behaviour should be: split Lv3[0,0]
entry into 512 level2 entries and remap the first level2 entry to
cover [0, 2M]. But the split won't happen in previous code since
PageTableBaseAddress of input Attribute is not checked.
So, when checking if a leaf parent entry needs to be splitted, we
should also check if PageTableBaseAddress calculated by parent entry
is equal to the value caculated by input attribute.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 127b65183f..ad68792ca8 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -274,6 +274,8 @@ PageTableLibMapInLevel (
IA32_MAP_ATTRIBUTE ChildMask;
IA32_MAP_ATTRIBUTE CurrentMask;
IA32_MAP_ATTRIBUTE LocalParentAttribute;
+ UINT64 PhysicalAddrInEntry;
+ UINT64 PhysicalAddrInAttr;
ASSERT (Level != 0);
ASSERT ((Attribute != NULL) && (Mask != NULL));
@@ -341,7 +343,15 @@ PageTableLibMapInLevel (
// This function is called when the memory length is less than the region length of the parent level.
// No need to split the page when the attributes equal.
//
- return RETURN_SUCCESS;
+ if (Mask->Bits.PageTableBaseAddress == 0) {
+ return RETURN_SUCCESS;
+ }
+
+ PhysicalAddrInEntry = IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&PleBAttribute) + PagingEntryIndex * RegionLength;
+ PhysicalAddrInAttr = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) & (~RegionMask);
+ if (PhysicalAddrInEntry == PhysicalAddrInAttr) {
+ return RETURN_SUCCESS;
+ }
}
ASSERT (Buffer == NULL || *BufferSize >= SIZE_4KB);
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 05/21] UefiCpuPkg/CpuPageTableLib:Clear PageSize bit(Bit7) for non-leaf
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (3 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 04/21] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 12:29 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 06/21] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry duntan
` (16 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Clear PageSize bit(Bit7) for non-leaf entry in PageTableLibSetPnle.
This function is used to set non-leaf entry attributes so it should
make sure that the PageSize bit of the entry should be 0.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index ad68792ca8..cf0cfeca77 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -202,7 +202,8 @@ PageTableLibSetPnle (
Pnle->Bits.Nx = Attribute->Bits.Nx;
}
- Pnle->Bits.Accessed = 0;
+ Pnle->Bits.Accessed = 0;
+ Pnle->Bits.MustBeZero = 0;
//
// Set the attributes (WT, CD, A) to 0.
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 06/21] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (4 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 05/21] UefiCpuPkg/CpuPageTableLib:Clear PageSize bit(Bit7) for non-leaf duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 12:35 ` Ni, Ray
[not found] ` <174F0C7EF4E09C53.19937@groups.io>
2023-03-23 7:40 ` [Patch V4 07/21] UefiCpuPkg/MpInitLib: Add code to initialize MapMask duntan
` (15 subsequent siblings)
21 siblings, 2 replies; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
When splitting leaf parent entry to smaller granularity, create
child page table before modifing parent entry. In previous code
logic, when splitting a leaf parent entry, parent entry will
point to a null 4k memory before child page table is created in
this 4k memory. When the page table to be modified is the page
table in CR3, if the executed CpuPageTableLib code is in the
range mapped by the modified leaf parent entry, then issue will
happen.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index cf0cfeca77..76febdd42d 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -363,8 +363,13 @@ PageTableLibMapInLevel (
//
// Create 512 child-level entries that map to 2M/4K.
//
- ParentPagingEntry->Uintn = (UINTN)Buffer + *BufferSize;
- ZeroMem ((VOID *)ParentPagingEntry->Uintn, SIZE_4KB);
+ PagingEntry = (IA32_PAGING_ENTRY *)((UINTN)Buffer + *BufferSize);
+ ZeroMem (PagingEntry, SIZE_4KB);
+
+ for (SubOffset = 0, Index = 0; Index < 512; Index++) {
+ PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
+ SubOffset += RegionLength;
+ }
//
// Set NOP attributes
@@ -372,12 +377,7 @@ PageTableLibMapInLevel (
// will make the entire region read-only even the child entries set the RW bit.
//
PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOneMask);
-
- PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
- for (SubOffset = 0, Index = 0; Index < 512; Index++) {
- PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
- SubOffset += RegionLength;
- }
+ ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) | (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
}
} else {
//
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 07/21] UefiCpuPkg/MpInitLib: Add code to initialize MapMask
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (5 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 06/21] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 12:37 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr duntan
` (14 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
In function CreatePageTable(), add code to initialize MapMask to
MAX_UINT64. When creating new page table or map non-present range
to present, all attributes should be provided.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c b/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
index 7cf91ed9c4..f20068152b 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
+++ b/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
@@ -36,10 +36,7 @@ CreatePageTable (
MapAttribute.Uint64 = Address;
MapAttribute.Bits.Present = 1;
MapAttribute.Bits.ReadWrite = 1;
-
- MapMask.Bits.PageTableBaseAddress = 1;
- MapMask.Bits.Present = 1;
- MapMask.Bits.ReadWrite = 1;
+ MapMask.Uint64 = MAX_UINT64;
PageTable = 0;
PageTableBufferSize = 0;
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (6 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 07/21] UefiCpuPkg/MpInitLib: Add code to initialize MapMask duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 12:52 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 09/21] UefiCpuPkg/CpuPageTableLib: Add manual test to check " duntan
` (13 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
For different usage, check if the combination for Mask and
Attr is valid when creating or updating page table.
1.For non-present range
1.1Mask.Present is 0 but some other attributes is provided.
This case is invalid.
1.2Mask.Present is 1 and Attr.Present is 0. In this case,all
other attributes should not be provided.
1.3Mask.Present is 1 and Attr.Present is 1. In this case,all
attributes should be provided to intialize the attribute.
2.For present range
2.1Mask.Present is 1 and Attr.Present is 0.In this case, all
other attributes should not be provided.
All other usage for present range is permitted.
In the mentioned cases, 1.2 and 2.1 can be merged into 1 check.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Include/Library/CpuPageTableLib.h | 2 +-
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 78 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index 5f44ece548..6bda15b5bc 100644
--- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
+++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
@@ -76,7 +76,7 @@ typedef enum {
@param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
@retval RETURN_UNSUPPORTED PagingMode is not supported.
- @retval RETURN_INVALID_PARAMETER PageTable, BufferSize, Attribute or Mask is NULL.
+ @retval RETURN_INVALID_PARAMETER PageTable, BufferSize, Attribute or Mask is NULL or the combination of Attribute and Mask is invalid.
@retval RETURN_INVALID_PARAMETER *BufferSize is not multiple of 4KB.
@retval RETURN_BUFFER_TOO_SMALL The buffer is too small for page table creation/updating.
BufferSize is updated to indicate the expected buffer size.
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 76febdd42d..2ad22b333d 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -215,6 +215,43 @@ PageTableLibSetPnle (
Pnle->Bits.CacheDisabled = 0;
}
+/**
+ Check if the combination for Attribute and Mask is valid for non-present entry.
+ 1.Mask.Present is 0 but some other attributes is provided. This case should be invalid.
+ 2.Map non-present range to present. In this case, all attributes should be provided.
+
+ @param[in] Attribute The attribute of the linear address range.
+ @param[in] Mask The mask used for attribute to check.
+
+ @retval RETURN_INVALID_PARAMETER The combination for Attribute and Mask is invalid.
+ @retval RETURN_SUCCESS The combination for Attribute and Mask is valid.
+**/
+RETURN_STATUS
+IsAttributesAndMaskValidForNonPresentEntry (
+ IN IA32_MAP_ATTRIBUTE *Attribute,
+ IN IA32_MAP_ATTRIBUTE *Mask
+ )
+{
+ if ((Mask->Bits.Present == 1) && (Attribute->Bits.Present == 1)) {
+ //
+ // Creating new page table or remapping non-present range to present.
+ //
+ if ((Mask->Bits.ReadWrite == 0) || (Mask->Bits.UserSupervisor == 0) || (Mask->Bits.WriteThrough == 0) || (Mask->Bits.CacheDisabled == 0) ||
+ (Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || (Mask->Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
+ (Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey == 0) || (Mask->Bits.Nx == 0))
+ {
+ return RETURN_INVALID_PARAMETER;
+ }
+ } else if ((Mask->Bits.Present == 0) && (Mask->Uint64 > 1)) {
+ //
+ // Only change other attributes for non-present range is not permitted.
+ //
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ return RETURN_SUCCESS;
+}
+
/**
Update page table to map [LinearAddress, LinearAddress + Length) with specified attribute in the specified level.
@@ -237,6 +274,7 @@ PageTableLibSetPnle (
when a new physical base address is set.
@param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
+ @retval RETURN_INVALID_PARAMETER The combination of Attribute and Mask for non-present entry is invalid.
@retval RETURN_SUCCESS PageTable is created/updated successfully.
**/
RETURN_STATUS
@@ -260,6 +298,7 @@ PageTableLibMapInLevel (
UINTN Index;
IA32_PAGING_ENTRY *PagingEntry;
UINTN PagingEntryIndex;
+ UINTN PagingEntryIndexEnd;
IA32_PAGING_ENTRY *CurrentPagingEntry;
UINT64 RegionLength;
UINT64 SubLength;
@@ -306,6 +345,14 @@ PageTableLibMapInLevel (
//
if (ParentPagingEntry->Pce.Present == 0) {
+ //
+ // [LinearAddress, LinearAddress + Length] contains non-present range.
+ //
+ Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute, Mask);
+ if (RETURN_ERROR (Status)) {
+ return Status;
+ }
+
//
// The parent entry is CR3 or PML5E/PML4E/PDPTE/PDE.
// It does NOT point to an existing page directory.
@@ -380,6 +427,27 @@ PageTableLibMapInLevel (
ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) | (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
}
} else {
+ //
+ // If (LinearAddress + Length - 1) is not in the same ParentPagingEntry with (LinearAddress + Offset), then the remaining child PagingEntry
+ // starting from PagingEntryIndex of ParentPagingEntry is all covered by [LinearAddress + Offset, LinearAddress + Length - 1].
+ //
+ PagingEntryIndexEnd = (BitFieldRead64 (LinearAddress + Length - 1, BitStart + 9, 63) != BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ? 511 :
+ (UINTN)BitFieldRead64 (LinearAddress + Length - 1, BitStart, BitStart + 9 - 1);
+ PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
+ for (Index = PagingEntryIndex; Index <= PagingEntryIndexEnd; Index++) {
+ if (PagingEntry[Index].Pce.Present == 0) {
+ //
+ // [LinearAddress, LinearAddress + Length] contains non-present range.
+ //
+ Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute, Mask);
+ if (RETURN_ERROR (Status)) {
+ return Status;
+ }
+
+ break;
+ }
+ }
+
//
// It's a non-leaf entry
//
@@ -427,7 +495,6 @@ PageTableLibMapInLevel (
// Update child entries to use restrictive attribute inherited from parent.
// e.g.: Set PDE[0-255].ReadWrite = 0
//
- PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
for (Index = 0; Index < 512; Index++) {
if (PagingEntry[Index].Pce.Present == 0) {
continue;
@@ -553,7 +620,7 @@ PageTableLibMapInLevel (
@param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
@retval RETURN_UNSUPPORTED PagingMode is not supported.
- @retval RETURN_INVALID_PARAMETER PageTable, BufferSize, Attribute or Mask is NULL.
+ @retval RETURN_INVALID_PARAMETER PageTable, BufferSize, Attribute or Mask is NULL or the combination of Attribute and Mask is invalid.
@retval RETURN_INVALID_PARAMETER *BufferSize is not multiple of 4KB.
@retval RETURN_BUFFER_TOO_SMALL The buffer is too small for page table creation/updating.
BufferSize is updated to indicate the expected buffer size.
@@ -615,6 +682,14 @@ PageTableMap (
return RETURN_INVALID_PARAMETER;
}
+ //
+ // If to map [LinearAddress, LinearAddress + Length] as non-present,
+ // all attributes except Present should not be provided.
+ //
+ if ((Attribute->Bits.Present == 0) && (Mask->Bits.Present == 1) && (Mask->Uint64 > 1)) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
MaxLeafLevel = (IA32_PAGE_LEVEL)(UINT8)PagingMode;
MaxLevel = (IA32_PAGE_LEVEL)(UINT8)(PagingMode >> 8);
MaxLinearAddress = LShiftU64 (1, 12 + MaxLevel * 9);
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 09/21] UefiCpuPkg/CpuPageTableLib: Add manual test to check Mask and Attr
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (7 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 12:57 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 10/21] UefiCpuPkg/CpuPageTableLib:Modify RandomBoolean() in RandomTest duntan
` (12 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Add manual test case to check input Mask and Attribute. The check
steps are:
1.Create Page table to cover [0, 2G]. All fields of MapMask should
be set.
2.Update Page table to set [2G - 8K,2G] from present to non-present.
All fields of MapMask except present should not be set.
3.Still set [2G - 8K, 2G] as not present, this case is permitted.
But set [2G - 8K, 2G] as RW is not permitted.
4.Update Page table to set [2G - 8K, 2G] as present and RW. All
fields of MapMask should be set.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 127 insertions(+), 2 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
index 3014a03243..3e84e2ba11 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
@@ -1,7 +1,7 @@
/** @file
Unit tests of the CpuPageTableLib instance of the CpuPageTableLib class
- Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -697,6 +697,131 @@ TestCaseManualChangeNx (
return UNIT_TEST_PASSED;
}
+/**
+ Check if the input Mask and Attribute is as expected when creating new page table or
+ updating existing page table.
+
+ @param[in] Context [Optional] An optional parameter that enables:
+ 1) test-case reuse with varied parameters and
+ 2) test-case re-entry for Target tests that need a
+ reboot. This parameter is a VOID* and it is the
+ responsibility of the test author to ensure that the
+ contents are well understood by all test cases that may
+ consume it.
+
+ @retval UNIT_TEST_PASSED The Unit test has completed and the test
+ case was successful.
+ @retval UNIT_TEST_ERROR_TEST_FAILED A test case assertion has failed.
+**/
+UNIT_TEST_STATUS
+EFIAPI
+TestCaseToCheckMapMaskAndAttr (
+ IN UNIT_TEST_CONTEXT Context
+ )
+{
+ UINTN PageTable;
+ PAGING_MODE PagingMode;
+ VOID *Buffer;
+ UINTN PageTableBufferSize;
+ IA32_MAP_ATTRIBUTE MapAttribute;
+ IA32_MAP_ATTRIBUTE ExpectedMapAttribute;
+ IA32_MAP_ATTRIBUTE MapMask;
+ RETURN_STATUS Status;
+ IA32_MAP_ENTRY *Map;
+ UINTN MapCount;
+
+ PagingMode = Paging4Level;
+ PageTableBufferSize = 0;
+ PageTable = 0;
+ Buffer = NULL;
+ MapAttribute.Uint64 = 0;
+ MapAttribute.Bits.Present = 1;
+ MapMask.Uint64 = 0;
+ MapMask.Bits.Present = 1;
+ //
+ // Create Page table to cover [0, 2G]. All fields of MapMask should be set.
+ //
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+ MapMask.Uint64 = MAX_UINT64;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
+ Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
+
+ //
+ // Update Page table to set [2G - 8K, 2G] from present to non-present. All fields of MapMask except present should be set.
+ //
+ PageTableBufferSize = 0;
+ MapAttribute.Uint64 = SIZE_2GB - SIZE_8KB;
+ MapMask.Uint64 = 0;
+ MapMask.Bits.Present = 1;
+ MapMask.Bits.ReadWrite = 1;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+ MapMask.Bits.ReadWrite = 0;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
+ Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
+
+ //
+ // Still set [2G - 8K, 2G] as not present, this case is permitted. But set [2G - 8K, 2G] as RW is not permitted.
+ //
+ PageTableBufferSize = 0;
+ MapAttribute.Uint64 = 0;
+ MapMask.Uint64 = 0;
+ MapMask.Bits.Present = 1;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
+ MapAttribute.Bits.ReadWrite = 1;
+ MapMask.Bits.ReadWrite = 1;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+
+ //
+ // Update Page table to set [2G - 8K, 2G] as present and RW. All fields of MapMask should not be set.
+ //
+ PageTableBufferSize = 0;
+ MapAttribute.Uint64 = SIZE_2GB - SIZE_8KB;
+ MapAttribute.Bits.ReadWrite = 1;
+ MapAttribute.Bits.Present = 1;
+ MapMask.Uint64 = 0;
+ MapMask.Bits.ReadWrite = 1;
+ MapMask.Bits.Present = 1;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+ MapMask.Uint64 = MAX_UINT64;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
+
+ MapCount = 0;
+ Status = PageTableParse (PageTable, PagingMode, NULL, &MapCount);
+ UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
+ Map = AllocatePages (EFI_SIZE_TO_PAGES (MapCount* sizeof (IA32_MAP_ENTRY)));
+ Status = PageTableParse (PageTable, PagingMode, Map, &MapCount);
+ UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
+
+ //
+ // There should be two ranges [0, 2G-8k] with RW = 0 and [2G-8k, 2G] with RW = 1
+ //
+ UT_ASSERT_EQUAL (MapCount, 2);
+ UT_ASSERT_EQUAL (Map[0].LinearAddress, 0);
+ UT_ASSERT_EQUAL (Map[0].Length, SIZE_2GB - SIZE_8KB);
+ ExpectedMapAttribute.Uint64 = 0;
+ ExpectedMapAttribute.Bits.Present = 1;
+ UT_ASSERT_EQUAL (Map[0].Attribute.Uint64, ExpectedMapAttribute.Uint64);
+ UT_ASSERT_EQUAL (Map[1].LinearAddress, SIZE_2GB - SIZE_8KB);
+ UT_ASSERT_EQUAL (Map[1].Length, SIZE_8KB);
+ ExpectedMapAttribute.Uint64 = SIZE_2GB - SIZE_8KB;
+ ExpectedMapAttribute.Bits.Present = 1;
+ ExpectedMapAttribute.Bits.ReadWrite = 1;
+ UT_ASSERT_EQUAL (Map[1].Attribute.Uint64, ExpectedMapAttribute.Uint64);
+ return UNIT_TEST_PASSED;
+}
+
/**
Initialize the unit test framework, suite, and unit tests for the
sample unit tests and run the unit tests.
@@ -746,7 +871,7 @@ UefiTestMain (
AddTestCase (ManualTestCase, "Check if the parent entry has different ReadWrite attribute", "Manual Test Case5", TestCaseManualChangeReadWrite, NULL, NULL, NULL);
AddTestCase (ManualTestCase, "Check if the parent entry has different Nx attribute", "Manual Test Case6", TestCaseManualChangeNx, NULL, NULL, NULL);
AddTestCase (ManualTestCase, "Check if the needed size is expected", "Manual Test Case7", TestCaseManualSizeNotMatch, NULL, NULL, NULL);
-
+ AddTestCase (ManualTestCase, "Check MapMask when creating new page table or mapping not-present range", "Manual Test Case8", TestCaseToCheckMapMaskAndAttr, NULL, NULL, NULL);
//
// Populate the Random Test Cases.
//
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 10/21] UefiCpuPkg/CpuPageTableLib:Modify RandomBoolean() in RandomTest
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (8 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 09/21] UefiCpuPkg/CpuPageTableLib: Add manual test to check " duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 12:59 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 11/21] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr duntan
` (11 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Add an input parameter to control the probability of returning
true. Change RandomBoolean() in RandomTest from 50% chance
returning true to returning true with the percentage of input
Probability.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 43 +++++++++++++++++++++----------------------
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index 97a388ca1c..52eb9daa10 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -1,7 +1,7 @@
/** @file
Random test case for Unit tests of the CpuPageTableLib instance of the CpuPageTableLib class
- Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -81,22 +81,6 @@ LocalRandomBytes (
}
}
-/**
- Return a random boolean.
-
- @return boolean
-**/
-BOOLEAN
-RandomBoolean (
- VOID
- )
-{
- BOOLEAN Value;
-
- LocalRandomBytes ((UINT8 *)&Value, sizeof (BOOLEAN));
- return Value%2;
-}
-
/**
Return a 32bit random number.
@@ -139,6 +123,21 @@ Random64 (
return (UINT64)(Value % (Limit - Start + 1)) + Start;
}
+/**
+ Returns true with the percentage of input Probability.
+
+ @param[in] Probability The percentage to return true.
+
+ @return boolean
+**/
+BOOLEAN
+RandomBoolean (
+ UINT8 Probability
+ )
+{
+ return ((Probability > ((UINT8)Random64 (0, 100))) ? TRUE : FALSE);
+}
+
/**
Check if the Page table entry is valid
@@ -178,7 +177,7 @@ ValidateAndRandomeModifyPageTablePageTableEntry (
UT_ASSERT_EQUAL ((PagingEntry->Uint64 & mValidMaskLeaf[Level].Uint64), PagingEntry->Uint64);
}
- if ((RandomNumber < 100) && RandomBoolean ()) {
+ if ((RandomNumber < 100) && RandomBoolean (50)) {
RandomNumber++;
if (Level == 1) {
TempPhysicalBase = PagingEntry->Pte4K.Bits.PageTableBaseAddress;
@@ -211,7 +210,7 @@ ValidateAndRandomeModifyPageTablePageTableEntry (
UT_ASSERT_EQUAL ((PagingEntry->Uint64 & mValidMaskNoLeaf[Level].Uint64), PagingEntry->Uint64);
}
- if ((RandomNumber < 100) && RandomBoolean ()) {
+ if ((RandomNumber < 100) && RandomBoolean (50)) {
RandomNumber++;
TempPhysicalBase = PagingEntry->Pnle.Bits.PageTableBaseAddress;
@@ -299,7 +298,7 @@ GenerateSingleRandomMapEntry (
//
// use AlignedTable to avoid that a random number can be very hard to be 1G or 2M aligned
//
- if ((MapsIndex != 0) && (RandomBoolean ())) {
+ if ((MapsIndex != 0) && (RandomBoolean (50))) {
FormerLinearAddress = MapEntrys->Maps[Random32 (0, (UINT32)MapsIndex-1)].LinearAddress;
if (FormerLinearAddress < 2 * (UINT64)SIZE_1GB) {
FormerLinearAddressBottom = 0;
@@ -323,7 +322,7 @@ GenerateSingleRandomMapEntry (
//
MapEntrys->Maps[MapsIndex].Length = Random64 (0, MIN (MaxAddress - MapEntrys->Maps[MapsIndex].LinearAddress, 10 * (UINT64)SIZE_1GB)) & AlignedTable[Random32 (0, ARRAY_SIZE (AlignedTable) -1)];
- if ((MapsIndex != 0) && (RandomBoolean ())) {
+ if ((MapsIndex != 0) && (RandomBoolean (50))) {
MapEntrys->Maps[MapsIndex].Attribute.Uint64 = MapEntrys->Maps[Random32 (0, (UINT32)MapsIndex-1)].Attribute.Uint64;
MapEntrys->Maps[MapsIndex].Mask.Uint64 = MapEntrys->Maps[Random32 (0, (UINT32)MapsIndex-1)].Mask.Uint64;
} else {
@@ -344,7 +343,7 @@ GenerateSingleRandomMapEntry (
// Need to avoid such case when remove the Random option ONLY_ONE_ONE_MAPPING
//
MapEntrys->Maps[MapsIndex].Attribute.Bits.PageTableBaseAddress = (Random64 (0, (((UINT64)1)<<52) - 1) & AlignedTable[Random32 (0, ARRAY_SIZE (AlignedTable) -1)])>> 12;
- if (RandomBoolean ()) {
+ if (RandomBoolean (50)) {
MapEntrys->Maps[MapsIndex].Mask.Bits.PageTableBaseAddress = 0;
}
}
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 11/21] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (9 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 10/21] UefiCpuPkg/CpuPageTableLib:Modify RandomBoolean() in RandomTest duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 13:11 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 12/21] UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test duntan
` (10 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Zhiguang Liu
Modify RandomTest to check invalid input. When creating new page
table or updating exsiting page table:
1.If set [LinearAddress, LinearAddress+Length] to non-preset, all
other attributes should not be provided.
2.If [LinearAddress, LinearAddress+Length] contain non-present range,
the Returnstatus of PageTableMap() should be InvalidParameter when:
2.1Some of attributes are not provided when mapping non-present range
to present.
2.2Set any other attribute without setting the non-present range to
Present.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
---
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c | 6 +++++-
2 files changed, 131 insertions(+), 25 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index 52eb9daa10..b99e31f4c8 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -273,6 +273,27 @@ ValidateAndRandomeModifyPageTable (
return Status;
}
+/**
+ Remove the last MAP_ENTRY in MapEntrys.
+
+ @param MapEntrys Pointer to MapEntrys buffer
+**/
+VOID
+RemoveLastMapEntry (
+ IN OUT MAP_ENTRYS *MapEntrys
+ )
+{
+ UINTN MapsIndex;
+
+ if (MapEntrys->Count == 0) {
+ return;
+ }
+
+ MapsIndex = MapEntrys->Count - 1;
+ ZeroMem (&(MapEntrys->Maps[MapsIndex]), sizeof (MAP_ENTRY));
+ MapEntrys->Count = MapsIndex;
+}
+
/**
Generate single random map entry.
The map entry can be the input of function PageTableMap
@@ -327,7 +348,16 @@ GenerateSingleRandomMapEntry (
MapEntrys->Maps[MapsIndex].Mask.Uint64 = MapEntrys->Maps[Random32 (0, (UINT32)MapsIndex-1)].Mask.Uint64;
} else {
MapEntrys->Maps[MapsIndex].Attribute.Uint64 = Random64 (0, MAX_UINT64) & mSupportedBit.Uint64;
- MapEntrys->Maps[MapsIndex].Mask.Uint64 = Random64 (0, MAX_UINT64) & mSupportedBit.Uint64;
+ if (RandomBoolean (5)) {
+ //
+ // The probability to get random Mask should be small since all bits of a random number
+ // have a high probability of containing 0, which may be a invalid input.
+ //
+ MapEntrys->Maps[MapsIndex].Mask.Uint64 = Random64 (0, MAX_UINT64) & mSupportedBit.Uint64;
+ } else {
+ MapEntrys->Maps[MapsIndex].Mask.Uint64 = MAX_UINT64;
+ }
+
if (MapEntrys->Maps[MapsIndex].Mask.Bits.ProtectionKey != 0) {
MapEntrys->Maps[MapsIndex].Mask.Bits.ProtectionKey = 0xF;
}
@@ -337,15 +367,7 @@ GenerateSingleRandomMapEntry (
MapEntrys->Maps[MapsIndex].Attribute.Bits.PageTableBaseAddress = MapEntrys->Maps[MapsIndex].LinearAddress >> 12;
MapEntrys->Maps[MapsIndex].Mask.Bits.PageTableBaseAddress = 0xFFFFFFFFFF;
} else {
- //
- // Todo: If the mask bit for base address is zero, when dump the pagetable, every entry mapping to physical address zeor.
- // This means the map count will be a large number, and impossible to finish in proper time.
- // Need to avoid such case when remove the Random option ONLY_ONE_ONE_MAPPING
- //
MapEntrys->Maps[MapsIndex].Attribute.Bits.PageTableBaseAddress = (Random64 (0, (((UINT64)1)<<52) - 1) & AlignedTable[Random32 (0, ARRAY_SIZE (AlignedTable) -1)])>> 12;
- if (RandomBoolean (50)) {
- MapEntrys->Maps[MapsIndex].Mask.Bits.PageTableBaseAddress = 0;
- }
}
MapEntrys->Count += 1;
@@ -608,23 +630,62 @@ SingleMapEntryTest (
IN UINTN InitMapCount
)
{
- UINTN MapsIndex;
- RETURN_STATUS Status;
- UINTN PageTableBufferSize;
- VOID *Buffer;
- IA32_MAP_ENTRY *Map;
- UINTN MapCount;
- UINTN Index;
- UINTN KeyPointCount;
- UINTN NewKeyPointCount;
- UINT64 *KeyPointBuffer;
- UINTN Level;
- UINT64 Value;
- UNIT_TEST_STATUS TestStatus;
-
- MapsIndex = MapEntrys->Count;
+ UINTN MapsIndex;
+ RETURN_STATUS Status;
+ UINTN PageTableBufferSize;
+ VOID *Buffer;
+ IA32_MAP_ENTRY *Map;
+ UINTN MapCount;
+ UINTN Index;
+ UINTN KeyPointCount;
+ UINTN NewKeyPointCount;
+ UINT64 *KeyPointBuffer;
+ UINTN Level;
+ UINT64 Value;
+ UNIT_TEST_STATUS TestStatus;
+ IA32_MAP_ATTRIBUTE *Mask;
+ IA32_MAP_ATTRIBUTE *Attribute;
+ UINT64 PreviousAddress;
+ UINT64 RangeLimit;
+ BOOLEAN IsNotPresent;
+
+ MapsIndex = MapEntrys->Count;
+ MapCount = 0;
+ PreviousAddress = 0;
+ IsNotPresent = FALSE;
GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
+ RangeLimit = MapEntrys->Maps[MapsIndex].LinearAddress + MapEntrys->Maps[MapsIndex].Length;
+ Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
+
+ if (MapCount != 0) {
+ UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
+ Map = AllocatePages (EFI_SIZE_TO_PAGES (MapCount * sizeof (IA32_MAP_ENTRY)));
+ ASSERT (Map != NULL);
+ Status = PageTableParse (*PageTable, PagingMode, Map, &MapCount);
+ }
+
+ //
+ // Check if the generated MapEntrys->Maps[MapsIndex] contains not-present range.
+ //
+ if (MapEntrys->Maps[MapsIndex].Length > 0) {
+ for (Index = 0; Index < MapCount; Index++) {
+ if ((PreviousAddress < Map[Index].LinearAddress) &&
+ (MapEntrys->Maps[MapsIndex].LinearAddress < Map[Index].LinearAddress) && (RangeLimit > PreviousAddress))
+ {
+ //
+ // MapEntrys->Maps[MapsIndex] contains not-present range in exsiting page table.
+ //
+ break;
+ }
+
+ PreviousAddress = Map[Index].LinearAddress + Map[Index].Length;
+ }
+
+ if (PreviousAddress < RangeLimit) {
+ IsNotPresent = TRUE;
+ }
+ }
PageTableBufferSize = 0;
Status = PageTableMap (
@@ -637,6 +698,47 @@ SingleMapEntryTest (
&MapEntrys->Maps[MapsIndex].Attribute,
&MapEntrys->Maps[MapsIndex].Mask
);
+
+ Attribute = &MapEntrys->Maps[MapsIndex].Attribute;
+ Mask = &MapEntrys->Maps[MapsIndex].Mask;
+ //
+ // If set [LinearAddress, LinearAddress+Attribute] to not preset, all
+ // other attributes should not be provided.
+ //
+ if ((MapEntrys->Maps[MapsIndex].Length > 0) && (Attribute->Bits.Present == 0) && (Mask->Bits.Present == 1) && (Mask->Uint64 > 1)) {
+ RemoveLastMapEntry (MapEntrys);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+ return UNIT_TEST_PASSED;
+ }
+
+ //
+ // Return Status for non-present range also should be InvalidParameter when:
+ // 1. Some of attributes are not provided when mapping non-present range to present.
+ // 2. Set any other attribute without setting the non-present range to Present.
+ //
+ if (IsNotPresent) {
+ if ((Mask->Bits.Present == 1) && (Attribute->Bits.Present == 1)) {
+ //
+ // Creating new page table or remapping non-present range to present.
+ //
+ if ((Mask->Bits.ReadWrite == 0) || (Mask->Bits.UserSupervisor == 0) || (Mask->Bits.WriteThrough == 0) || (Mask->Bits.CacheDisabled == 0) ||
+ (Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || (Mask->Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
+ (Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey == 0) || (Mask->Bits.Nx == 0))
+ {
+ RemoveLastMapEntry (MapEntrys);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+ return UNIT_TEST_PASSED;
+ }
+ } else if ((Mask->Bits.Present == 0) && (Mask->Uint64 > 1)) {
+ //
+ // Only change other attributes for non-present range is not permitted.
+ //
+ RemoveLastMapEntry (MapEntrys);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+ return UNIT_TEST_PASSED;
+ }
+ }
+
if (PageTableBufferSize != 0) {
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
index 5bd70c0f65..10fdee2f94 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
@@ -1,7 +1,7 @@
/** @file
helper file for Unit tests of the CpuPageTableLib instance of the CpuPageTableLib class
- Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -171,6 +171,10 @@ IsPageTableValid (
UNIT_TEST_STATUS Status;
IA32_PAGING_ENTRY *PagingEntry;
+ if (PageTable == 0) {
+ return UNIT_TEST_PASSED;
+ }
+
if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) || (PagingMode >= PagingModeMax)) {
//
// 32bit paging is never supported.
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 12/21] UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (10 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 11/21] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 7:40 ` [Patch V4 13/21] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter duntan
` (9 subsequent siblings)
21 siblings, 0 replies; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Enable non-1:1 mapping in random test. In previous test, non-1:1
test will fail due to the non-1:1 mapping issue in CpuPageTableLib
and invalid Input Mask when creating new page table or mapping
not-present range. Now these issue have been fixed.
Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
index 3e84e2ba11..d37cae9fbd 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
@@ -9,10 +9,10 @@
#include "CpuPageTableLibUnitTest.h"
// ----------------------------------------------------------------------- PageMode--TestCount-TestRangeCount---RandomOptions
-static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level = { Paging4Level, 100, 20, ONLY_ONE_ONE_MAPPING|USE_RANDOM_ARRAY };
-static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level1GB = { Paging4Level1GB, 100, 20, ONLY_ONE_ONE_MAPPING|USE_RANDOM_ARRAY };
-static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging5Level = { Paging5Level, 100, 20, ONLY_ONE_ONE_MAPPING|USE_RANDOM_ARRAY };
-static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging5Level1GB = { Paging5Level1GB, 100, 20, ONLY_ONE_ONE_MAPPING|USE_RANDOM_ARRAY };
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level = { Paging4Level, 100, 20, USE_RANDOM_ARRAY };
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level1GB = { Paging4Level1GB, 100, 20, USE_RANDOM_ARRAY };
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging5Level = { Paging5Level, 100, 20, USE_RANDOM_ARRAY };
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging5Level1GB = { Paging5Level1GB, 100, 20, USE_RANDOM_ARRAY };
/**
Check if the input parameters are not supported.
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 13/21] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter.
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (11 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 12/21] UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 13:20 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 14/21] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified duntan
` (8 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Add OUTPUT IsModified parameter in PageTableMap() to indicate
if page table has been modified. With this parameter, caller
can know if need to call FlushTlb when the page table is in CR3.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Include/Library/CpuPageTableLib.h | 4 +++-
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 50 +++++++++++++++++++++++++++++++++++++++++++-------
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 72 ++++++++++++++++++++++++++++++++++++------------------------------------
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 6 ++++--
UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c | 6 ++++--
5 files changed, 90 insertions(+), 48 deletions(-)
diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index 6bda15b5bc..c94d82ea65 100644
--- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
+++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
@@ -74,6 +74,7 @@ typedef enum {
Page table entries that map the linear address range are reset to 0 before set to the new attribute
when a new physical base address is set.
@param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
+ @param[out] IsModified TRUE means page table is modified. FALSE means page table is not modified.
@retval RETURN_UNSUPPORTED PagingMode is not supported.
@retval RETURN_INVALID_PARAMETER PageTable, BufferSize, Attribute or Mask is NULL or the combination of Attribute and Mask is invalid.
@@ -93,7 +94,8 @@ PageTableMap (
IN UINT64 LinearAddress,
IN UINT64 Length,
IN IA32_MAP_ATTRIBUTE *Attribute,
- IN IA32_MAP_ATTRIBUTE *Mask
+ IN IA32_MAP_ATTRIBUTE *Mask,
+ OUT BOOLEAN *IsModified OPTIONAL
);
typedef struct {
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 2ad22b333d..797fc2f600 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -273,6 +273,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
Page table entries that map the linear address range are reset to 0 before set to the new attribute
when a new physical base address is set.
@param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
+ @param[out] IsModified TRUE means page table is modified. FALSE means page table is not modified.
@retval RETURN_INVALID_PARAMETER The combination of Attribute and Mask for non-present entry is invalid.
@retval RETURN_SUCCESS PageTable is created/updated successfully.
@@ -290,7 +291,8 @@ PageTableLibMapInLevel (
IN UINT64 Length,
IN UINT64 Offset,
IN IA32_MAP_ATTRIBUTE *Attribute,
- IN IA32_MAP_ATTRIBUTE *Mask
+ IN IA32_MAP_ATTRIBUTE *Mask,
+ OUT BOOLEAN *IsModified
)
{
RETURN_STATUS Status;
@@ -316,6 +318,8 @@ PageTableLibMapInLevel (
IA32_MAP_ATTRIBUTE LocalParentAttribute;
UINT64 PhysicalAddrInEntry;
UINT64 PhysicalAddrInAttr;
+ IA32_PAGING_ENTRY OriginalParentPagingEntry;
+ IA32_PAGING_ENTRY OriginalCurrentPagingEntry;
ASSERT (Level != 0);
ASSERT ((Attribute != NULL) && (Mask != NULL));
@@ -328,8 +332,9 @@ PageTableLibMapInLevel (
NopAttribute.Bits.ReadWrite = 1;
NopAttribute.Bits.UserSupervisor = 1;
- LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
- ParentAttribute = &LocalParentAttribute;
+ LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
+ ParentAttribute = &LocalParentAttribute;
+ OriginalParentPagingEntry.Uint64 = ParentPagingEntry->Uint64;
//
// RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or 4K (1 << 12).
@@ -563,7 +568,15 @@ PageTableLibMapInLevel (
ASSERT (CreateNew || (Mask->Bits.Nx == 0) || (Attribute->Bits.Nx == 1));
}
+ //
+ // Check if any leaf PagingEntry is modified.
+ //
+ OriginalCurrentPagingEntry.Uint64 = CurrentPagingEntry->Uint64;
PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, &CurrentMask);
+
+ if (OriginalCurrentPagingEntry.Uint64 != CurrentPagingEntry->Uint64) {
+ *IsModified = TRUE;
+ }
}
} else {
//
@@ -586,7 +599,8 @@ PageTableLibMapInLevel (
Length,
Offset,
Attribute,
- Mask
+ Mask,
+ IsModified
);
if (RETURN_ERROR (Status)) {
return Status;
@@ -598,6 +612,14 @@ PageTableLibMapInLevel (
Index++;
}
+ //
+ // Check if ParentPagingEntry entry is modified here is enough. Except the changes happen in leaf PagingEntry during
+ // the while loop, if there is any other change happens in page table, the ParentPagingEntry must has been modified.
+ //
+ if (OriginalParentPagingEntry.Uint64 != ParentPagingEntry->Uint64) {
+ *IsModified = TRUE;
+ }
+
return RETURN_SUCCESS;
}
@@ -618,6 +640,7 @@ PageTableLibMapInLevel (
Page table entries that map the linear address range are reset to 0 before set to the new attribute
when a new physical base address is set.
@param[in] Mask The mask used for attribute. The corresponding field in Attribute is ignored if that in Mask is 0.
+ @param[out] IsModified TRUE means page table is modified. FALSE means page table is not modified.
@retval RETURN_UNSUPPORTED PagingMode is not supported.
@retval RETURN_INVALID_PARAMETER PageTable, BufferSize, Attribute or Mask is NULL or the combination of Attribute and Mask is invalid.
@@ -637,7 +660,8 @@ PageTableMap (
IN UINT64 LinearAddress,
IN UINT64 Length,
IN IA32_MAP_ATTRIBUTE *Attribute,
- IN IA32_MAP_ATTRIBUTE *Mask
+ IN IA32_MAP_ATTRIBUTE *Mask,
+ OUT BOOLEAN *IsModified OPTIONAL
)
{
RETURN_STATUS Status;
@@ -647,6 +671,7 @@ PageTableMap (
IA32_PAGE_LEVEL MaxLevel;
IA32_PAGE_LEVEL MaxLeafLevel;
IA32_MAP_ATTRIBUTE ParentAttribute;
+ BOOLEAN LocalIsModified;
if (Length == 0) {
return RETURN_SUCCESS;
@@ -709,6 +734,10 @@ PageTableMap (
TopPagingEntry.Pce.Nx = 0;
}
+ if (IsModified != NULL) {
+ *IsModified = FALSE;
+ }
+
ParentAttribute.Uint64 = 0;
ParentAttribute.Bits.PageTableBaseAddress = 1;
ParentAttribute.Bits.Present = 1;
@@ -732,7 +761,8 @@ PageTableMap (
Length,
0,
Attribute,
- Mask
+ Mask,
+ &LocalIsModified
);
if (RETURN_ERROR (Status)) {
return Status;
@@ -749,6 +779,7 @@ PageTableMap (
return RETURN_INVALID_PARAMETER;
}
+ LocalIsModified = FALSE;
//
// Update the page table when the supplied buffer is sufficient.
//
@@ -764,8 +795,13 @@ PageTableMap (
Length,
0,
Attribute,
- Mask
+ Mask,
+ &LocalIsModified
);
+ if (IsModified != NULL) {
+ *IsModified = LocalIsModified;
+ }
+
if (!RETURN_ERROR (Status)) {
*PageTable = (UINTN)(TopPagingEntry.Uintn & IA32_PE_BASE_ADDRESS_MASK_40);
}
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
index d37cae9fbd..6343b56c2f 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
@@ -51,26 +51,26 @@ TestCaseForParameter (
//
// If the input linear address is not 4K align, it should return invalid parameter
//
- UT_ASSERT_EQUAL (PageTableMap (&PageTable, PagingMode, &Buffer, &PageTableBufferSize, 1, SIZE_4KB, &MapAttribute, &MapMask), RETURN_INVALID_PARAMETER);
+ UT_ASSERT_EQUAL (PageTableMap (&PageTable, PagingMode, &Buffer, &PageTableBufferSize, 1, SIZE_4KB, &MapAttribute, &MapMask, NULL), RETURN_INVALID_PARAMETER);
//
// If the input PageTableBufferSize is not 4K align, it should return invalid parameter
//
PageTableBufferSize = 10;
- UT_ASSERT_EQUAL (PageTableMap (&PageTable, PagingMode, &Buffer, &PageTableBufferSize, 0, SIZE_4KB, &MapAttribute, &MapMask), RETURN_INVALID_PARAMETER);
+ UT_ASSERT_EQUAL (PageTableMap (&PageTable, PagingMode, &Buffer, &PageTableBufferSize, 0, SIZE_4KB, &MapAttribute, &MapMask, NULL), RETURN_INVALID_PARAMETER);
//
// If the input PagingMode is Paging32bit, it should return invalid parameter
//
PageTableBufferSize = 0;
PagingMode = Paging32bit;
- UT_ASSERT_EQUAL (PageTableMap (&PageTable, PagingMode, &Buffer, &PageTableBufferSize, 1, SIZE_4KB, &MapAttribute, &MapMask), RETURN_UNSUPPORTED);
+ UT_ASSERT_EQUAL (PageTableMap (&PageTable, PagingMode, &Buffer, &PageTableBufferSize, 1, SIZE_4KB, &MapAttribute, &MapMask, NULL), RETURN_UNSUPPORTED);
//
// If the input MapMask is NULL, it should return invalid parameter
//
PagingMode = Paging5Level1GB;
- UT_ASSERT_EQUAL (PageTableMap (&PageTable, PagingMode, &Buffer, &PageTableBufferSize, 1, SIZE_4KB, &MapAttribute, NULL), RETURN_INVALID_PARAMETER);
+ UT_ASSERT_EQUAL (PageTableMap (&PageTable, PagingMode, &Buffer, &PageTableBufferSize, 1, SIZE_4KB, &MapAttribute, NULL, NULL), RETURN_INVALID_PARAMETER);
return UNIT_TEST_PASSED;
}
@@ -119,10 +119,10 @@ TestCaseWhichNoNeedExtraSize (
//
// Create page table to cover [0, 10M], it should have 5 PTE
//
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, (UINT64)SIZE_2MB * 5, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, (UINT64)SIZE_2MB * 5, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, (UINT64)SIZE_2MB * 5, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, (UINT64)SIZE_2MB * 5, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
TestStatus = IsPageTableValid (PageTable, PagingMode);
if (TestStatus != UNIT_TEST_PASSED) {
@@ -134,7 +134,7 @@ TestCaseWhichNoNeedExtraSize (
// We assume the fucntion doesn't need to change page table, return success and output BufferSize is 0
//
Buffer = NULL;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, (UINT64)SIZE_4KB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, (UINT64)SIZE_4KB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (PageTableBufferSize, 0);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
TestStatus = IsPageTableValid (PageTable, PagingMode);
@@ -148,7 +148,7 @@ TestCaseWhichNoNeedExtraSize (
//
MapMask.Bits.Nx = 0;
PageTableBufferSize = 0;
- Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, 0, (UINT64)SIZE_4KB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, 0, (UINT64)SIZE_4KB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
UT_ASSERT_EQUAL (PageTableBufferSize, 0);
TestStatus = IsPageTableValid (PageTable, PagingMode);
@@ -164,7 +164,7 @@ TestCaseWhichNoNeedExtraSize (
MapAttribute.Bits.Accessed = 1;
MapMask.Bits.Accessed = 1;
PageTableBufferSize = 0;
- Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, (UINT64)SIZE_2MB, (UINT64)SIZE_2MB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, (UINT64)SIZE_2MB, (UINT64)SIZE_2MB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
UT_ASSERT_EQUAL (PageTableBufferSize, 0);
TestStatus = IsPageTableValid (PageTable, PagingMode);
@@ -217,10 +217,10 @@ TestCase1Gmapto4K (
MapAttribute.Bits.Present = 1;
MapMask.Bits.Present = 1;
MapMask.Uint64 = MAX_UINT64;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
//
@@ -281,11 +281,11 @@ TestCaseManualChangeReadWrite (
//
// Create Page table to cover [0,2G], with ReadWrite = 1
//
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
BackupPageTableBufferSize = PageTableBufferSize;
Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
IsPageTableValid (PageTable, PagingMode);
@@ -331,7 +331,7 @@ TestCaseManualChangeReadWrite (
// Call library to change ReadWrite to 0 for [0,2M]
//
MapAttribute.Bits.ReadWrite = 0;
- Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, 0, SIZE_2MB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, 0, SIZE_2MB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
IsPageTableValid (PageTable, PagingMode);
MapCount = 0;
@@ -360,7 +360,7 @@ TestCaseManualChangeReadWrite (
//
MapAttribute.Bits.ReadWrite = 1;
PageTableBufferSize = 0;
- Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, 0, SIZE_2MB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, 0, SIZE_2MB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
IsPageTableValid (PageTable, PagingMode);
MapCount = 0;
@@ -434,10 +434,10 @@ TestCaseManualSizeNotMatch (
//
// Create Page table to cover [2M-4K, 4M], with ReadWrite = 1
//
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2MB - SIZE_4KB, SIZE_4KB + SIZE_2MB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2MB - SIZE_4KB, SIZE_4KB + SIZE_2MB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2MB - SIZE_4KB, SIZE_4KB + SIZE_2MB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2MB - SIZE_4KB, SIZE_4KB + SIZE_2MB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
IsPageTableValid (PageTable, PagingMode);
@@ -493,7 +493,7 @@ TestCaseManualSizeNotMatch (
MapAttribute.Bits.ReadWrite = 1;
PageTableBufferSize = 0;
MapAttribute.Bits.PageTableBaseAddress = (SIZE_2MB - SIZE_4KB) >> 12;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2MB - SIZE_4KB, SIZE_4KB * 2, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2MB - SIZE_4KB, SIZE_4KB * 2, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
return UNIT_TEST_PASSED;
}
@@ -540,10 +540,10 @@ TestCaseManualNotMergeEntry (
//
// Create Page table to cover [0,4M], and [4M, 1G] is not present
//
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_2MB * 2, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_2MB * 2, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_2MB * 2, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_2MB * 2, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
TestStatus = IsPageTableValid (PageTable, PagingMode);
if (TestStatus != UNIT_TEST_PASSED) {
@@ -555,7 +555,7 @@ TestCaseManualNotMergeEntry (
// It looks like the chioce is not bad, but sometime, we need to keep some small entry
//
PageTableBufferSize = 0;
- Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
TestStatus = IsPageTableValid (PageTable, PagingMode);
if (TestStatus != UNIT_TEST_PASSED) {
@@ -564,7 +564,7 @@ TestCaseManualNotMergeEntry (
MapAttribute.Bits.Accessed = 1;
PageTableBufferSize = 0;
- Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_2MB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_2MB, &MapAttribute, &MapMask, NULL);
//
// If it didn't use a big 1G entry to cover whole range, only change [0,2M] for some attribute won't need extra memory
//
@@ -619,10 +619,10 @@ TestCaseManualChangeNx (
//
// Create Page table to cover [0,2G], with Nx = 0
//
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB * 2, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB * 2, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB * 2, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB * 2, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
TestStatus = IsPageTableValid (PageTable, PagingMode);
if (TestStatus != UNIT_TEST_PASSED) {
@@ -666,7 +666,7 @@ TestCaseManualChangeNx (
//
// Call library to change Nx to 0 for [0,1G]
//
- Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, NULL, &PageTableBufferSize, (UINT64)0, (UINT64)SIZE_1GB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
TestStatus = IsPageTableValid (PageTable, PagingMode);
if (TestStatus != UNIT_TEST_PASSED) {
@@ -741,30 +741,30 @@ TestCaseToCheckMapMaskAndAttr (
//
// Create Page table to cover [0, 2G]. All fields of MapMask should be set.
//
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
MapMask.Uint64 = MAX_UINT64;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
//
// Update Page table to set [2G - 8K, 2G] from present to non-present. All fields of MapMask except present should be set.
//
PageTableBufferSize = 0;
- MapAttribute.Uint64 = SIZE_2GB - SIZE_8KB;
+ MapAttribute.Uint64 = 0;
MapMask.Uint64 = 0;
MapMask.Bits.Present = 1;
MapMask.Bits.ReadWrite = 1;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
MapMask.Bits.ReadWrite = 0;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
//
@@ -774,11 +774,11 @@ TestCaseToCheckMapMaskAndAttr (
MapAttribute.Uint64 = 0;
MapMask.Uint64 = 0;
MapMask.Bits.Present = 1;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
MapAttribute.Bits.ReadWrite = 1;
MapMask.Bits.ReadWrite = 1;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
//
@@ -791,10 +791,10 @@ TestCaseToCheckMapMaskAndAttr (
MapMask.Uint64 = 0;
MapMask.Bits.ReadWrite = 1;
MapMask.Bits.Present = 1;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
MapMask.Uint64 = MAX_UINT64;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
MapCount = 0;
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index b99e31f4c8..816fd7b446 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -696,7 +696,8 @@ SingleMapEntryTest (
MapEntrys->Maps[MapsIndex].LinearAddress,
MapEntrys->Maps[MapsIndex].Length,
&MapEntrys->Maps[MapsIndex].Attribute,
- &MapEntrys->Maps[MapsIndex].Mask
+ &MapEntrys->Maps[MapsIndex].Mask,
+ NULL
);
Attribute = &MapEntrys->Maps[MapsIndex].Attribute;
@@ -756,7 +757,8 @@ SingleMapEntryTest (
MapEntrys->Maps[MapsIndex].LinearAddress,
MapEntrys->Maps[MapsIndex].Length,
&MapEntrys->Maps[MapsIndex].Attribute,
- &MapEntrys->Maps[MapsIndex].Mask
+ &MapEntrys->Maps[MapsIndex].Mask,
+ NULL
);
}
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c b/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
index f20068152b..da8729e752 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
+++ b/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
@@ -57,7 +57,8 @@ CreatePageTable (
Address,
Length,
&MapAttribute,
- &MapMask
+ &MapMask,
+ NULL
);
ASSERT (Status == EFI_BUFFER_TOO_SMALL);
DEBUG ((DEBUG_INFO, "AP Page Table Buffer Size = %x\n", PageTableBufferSize));
@@ -72,7 +73,8 @@ CreatePageTable (
Address,
Length,
&MapAttribute,
- &MapMask
+ &MapMask,
+ NULL
);
ASSERT_EFI_ERROR (Status);
return PageTable;
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 14/21] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (12 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 13/21] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 13:23 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 15/21] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf duntan
` (7 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Zhiguang Liu
Modify RandomTest to check if parameter IsModified of
PageTableMap() correctlly indicates whether input page table
is modified or not.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
---
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 45 +++++++++++++++++++++++++++++++++------------
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index 816fd7b446..6f1485976a 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -636,6 +636,8 @@ SingleMapEntryTest (
VOID *Buffer;
IA32_MAP_ENTRY *Map;
UINTN MapCount;
+ IA32_MAP_ENTRY *Map2;
+ UINTN MapCount2;
UINTN Index;
UINTN KeyPointCount;
UINTN NewKeyPointCount;
@@ -648,11 +650,13 @@ SingleMapEntryTest (
UINT64 PreviousAddress;
UINT64 RangeLimit;
BOOLEAN IsNotPresent;
+ BOOLEAN IsModified;
MapsIndex = MapEntrys->Count;
MapCount = 0;
PreviousAddress = 0;
IsNotPresent = FALSE;
+ IsModified = FALSE;
GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
RangeLimit = MapEntrys->Maps[MapsIndex].LinearAddress + MapEntrys->Maps[MapsIndex].Length;
@@ -697,7 +701,7 @@ SingleMapEntryTest (
MapEntrys->Maps[MapsIndex].Length,
&MapEntrys->Maps[MapsIndex].Attribute,
&MapEntrys->Maps[MapsIndex].Mask,
- NULL
+ &IsModified
);
Attribute = &MapEntrys->Maps[MapsIndex].Attribute;
@@ -758,7 +762,7 @@ SingleMapEntryTest (
MapEntrys->Maps[MapsIndex].Length,
&MapEntrys->Maps[MapsIndex].Attribute,
&MapEntrys->Maps[MapsIndex].Mask,
- NULL
+ &IsModified
);
}
@@ -772,18 +776,31 @@ SingleMapEntryTest (
return TestStatus;
}
- MapCount = 0;
- Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
- if (MapCount != 0) {
+ MapCount2 = 0;
+ Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount2);
+ if (MapCount2 != 0) {
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
//
- // Allocate memory for Maps
+ // Allocate memory for Map2
// Note the memory is only used in this one Single MapEntry Test
//
- Map = AllocatePages (EFI_SIZE_TO_PAGES (MapCount * sizeof (IA32_MAP_ENTRY)));
- ASSERT (Map != NULL);
- Status = PageTableParse (*PageTable, PagingMode, Map, &MapCount);
+ Map2 = AllocatePages (EFI_SIZE_TO_PAGES (MapCount2 * sizeof (IA32_MAP_ENTRY)));
+ ASSERT (Map2 != NULL);
+ Status = PageTableParse (*PageTable, PagingMode, Map2, &MapCount2);
+ }
+
+ //
+ // Check if PageTable has been modified.
+ //
+ if (MapCount2 != MapCount) {
+ UT_ASSERT_EQUAL (IsModified, TRUE);
+ } else {
+ if (CompareMem (Map, Map2, MapCount2 * sizeof (IA32_MAP_ENTRY)) != 0) {
+ UT_ASSERT_EQUAL (IsModified, TRUE);
+ } else {
+ UT_ASSERT_EQUAL (IsModified, FALSE);
+ }
}
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
@@ -793,17 +810,17 @@ SingleMapEntryTest (
// Note the memory is only used in this one Single MapEntry Test
//
KeyPointCount = 0;
- GetKeyPointList (MapEntrys, Map, MapCount, NULL, &KeyPointCount);
+ GetKeyPointList (MapEntrys, Map2, MapCount2, NULL, &KeyPointCount);
KeyPointBuffer = AllocatePages (EFI_SIZE_TO_PAGES (KeyPointCount * sizeof (UINT64)));
ASSERT (KeyPointBuffer != NULL);
NewKeyPointCount = 0;
- GetKeyPointList (MapEntrys, Map, MapCount, KeyPointBuffer, &NewKeyPointCount);
+ GetKeyPointList (MapEntrys, Map2, MapCount2, KeyPointBuffer, &NewKeyPointCount);
//
// Compare all key point's attribute
//
for (Index = 0; Index < NewKeyPointCount; Index++) {
- if (!CompareEntrysforOnePoint (KeyPointBuffer[Index], MapEntrys, Map, MapCount, InitMap, InitMapCount)) {
+ if (!CompareEntrysforOnePoint (KeyPointBuffer[Index], MapEntrys, Map2, MapCount2, InitMap, InitMapCount)) {
DEBUG ((DEBUG_INFO, "Error happens at below key point\n"));
DEBUG ((DEBUG_INFO, "Index = %d KeyPointBuffer[Index] = 0x%lx\n", Index, KeyPointBuffer[Index]));
Value = GetEntryFromPageTable (*PageTable, PagingMode, KeyPointBuffer[Index], &Level);
@@ -817,6 +834,10 @@ SingleMapEntryTest (
FreePages (Map, EFI_SIZE_TO_PAGES (MapCount * sizeof (IA32_MAP_ENTRY)));
}
+ if (MapCount2 != 0) {
+ FreePages (Map2, EFI_SIZE_TO_PAGES (MapCount2 * sizeof (IA32_MAP_ENTRY)));
+ }
+
return UNIT_TEST_PASSED;
}
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 15/21] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (13 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 14/21] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 13:25 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 16/21] UefiCpuPkg: Modify UnitTest code since tested API is changed duntan
` (6 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar
From: Zhiguang Liu <zhiguang.liu@intel.com>
The definition of IA32_MAP_ATTRIBUTE has 64 bits, and one of the bit
field PageTableBaseAddress is from bit 12 to bit 52. This means if the
compiler treats the 64bits value as two UINT32 value, the field
PageTableBaseAddress spans two UINT32 value. That's why when building in
NOOPT mode in IA32, the below issue is noticed:
unresolved external symbol __allshl
This patch fix the build failure by seperate field PageTableBaseAddress
into two fields, make sure no field spans two UINT32 value.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: Ray Ni <ray.ni@intel.com>
---
UefiCpuPkg/Include/Library/CpuPageTableLib.h | 32 ++++++++++++++++----------------
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 22 +++++++++++-----------
3 files changed, 90 insertions(+), 89 deletions(-)
diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index c94d82ea65..5e545a35f6 100644
--- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
+++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
@@ -11,22 +11,22 @@
typedef union {
struct {
- UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in memory
- UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
- UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
- UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
- UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
- UINT64 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
- UINT64 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
- UINT64 Pat : 1; // PAT
-
- UINT64 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
- UINT64 Reserved1 : 3; // Ignored
-
- UINT64 PageTableBaseAddress : 40; // Page Table Base Address
- UINT64 Reserved2 : 7; // Ignored
- UINT64 ProtectionKey : 4; // Protection key
- UINT64 Nx : 1; // No Execute bit
+ UINT32 Present : 1; // 0 = Not present in memory, 1 = Present in memory
+ UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
+ UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
+ UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
+ UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
+ UINT32 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
+ UINT32 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
+ UINT32 Pat : 1; // PAT
+ UINT32 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
+ UINT32 Reserved1 : 3; // Ignored
+ UINT32 PageTableBaseAddressLow : 20; // Page Table Base Address Low
+
+ UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address High
+ UINT32 Reserved2 : 7; // Ignored
+ UINT32 ProtectionKey : 4; // Protection key
+ UINT32 Nx : 1; // No Execute bit
} Bits;
UINT64 Uint64;
} IA32_MAP_ATTRIBUTE;
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
index 8d856d7c7e..2c67ecb469 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
@@ -29,11 +29,12 @@ typedef enum {
} IA32_PAGE_LEVEL;
typedef struct {
- UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in memory
- UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
- UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
- UINT64 Reserved : 58;
- UINT64 Nx : 1; // No Execute bit
+ UINT32 Present : 1; // 0 = Not present in memory, 1 = Present in memory
+ UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
+ UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
+ UINT32 Reserved0 : 29;
+ UINT32 Reserved1 : 31;
+ UINT32 Nx : 1; // No Execute bit
} IA32_PAGE_COMMON_ENTRY;
///
@@ -41,20 +42,20 @@ typedef struct {
///
typedef union {
struct {
- UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in memory
- UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
- UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
- UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
- UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
- UINT64 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
- UINT64 Available0 : 1; // Ignored
- UINT64 MustBeZero : 1; // Must Be Zero
-
- UINT64 Available2 : 4; // Ignored
-
- UINT64 PageTableBaseAddress : 40; // Page Table Base Address
- UINT64 Available3 : 11; // Ignored
- UINT64 Nx : 1; // No Execute bit
+ UINT32 Present : 1; // 0 = Not present in memory, 1 = Present in memory
+ UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
+ UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
+ UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
+ UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
+ UINT32 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
+ UINT32 Available0 : 1; // Ignored
+ UINT32 MustBeZero : 1; // Must Be Zero
+ UINT32 Available2 : 4; // Ignored
+ UINT32 PageTableBaseAddressLow : 20; // Page Table Base Address Low
+
+ UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address High
+ UINT32 Available3 : 11; // Ignored
+ UINT32 Nx : 1; // No Execute bit
} Bits;
UINT64 Uint64;
} IA32_PAGE_NON_LEAF_ENTRY;
@@ -86,23 +87,23 @@ typedef IA32_PAGE_NON_LEAF_ENTRY IA32_PDE;
///
typedef union {
struct {
- UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in memory
- UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
- UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
- UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
- UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
- UINT64 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
- UINT64 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
- UINT64 MustBeOne : 1; // Page Size. Must Be One
-
- UINT64 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
- UINT64 Available1 : 3; // Ignored
- UINT64 Pat : 1; // PAT
-
- UINT64 PageTableBaseAddress : 39; // Page Table Base Address
- UINT64 Available3 : 7; // Ignored
- UINT64 ProtectionKey : 4; // Protection key
- UINT64 Nx : 1; // No Execute bit
+ UINT32 Present : 1; // 0 = Not present in memory, 1 = Present in memory
+ UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
+ UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
+ UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
+ UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
+ UINT32 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
+ UINT32 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
+ UINT32 MustBeOne : 1; // Page Size. Must Be One
+ UINT32 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
+ UINT32 Available1 : 3; // Ignored
+ UINT32 Pat : 1; // PAT
+ UINT32 PageTableBaseAddressLow : 19; // Page Table Base Address Low
+
+ UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address High
+ UINT32 Available3 : 7; // Ignored
+ UINT32 ProtectionKey : 4; // Protection key
+ UINT32 Nx : 1; // No Execute bit
} Bits;
UINT64 Uint64;
} IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE;
@@ -123,22 +124,22 @@ typedef IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE IA32_PDPTE_1G;
///
typedef union {
struct {
- UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in memory
- UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
- UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
- UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
- UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
- UINT64 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
- UINT64 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
- UINT64 Pat : 1; // PAT
-
- UINT64 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
- UINT64 Available1 : 3; // Ignored
-
- UINT64 PageTableBaseAddress : 40; // Page Table Base Address
- UINT64 Available3 : 7; // Ignored
- UINT64 ProtectionKey : 4; // Protection key
- UINT64 Nx : 1; // No Execute bit
+ UINT32 Present : 1; // 0 = Not present in memory, 1 = Present in memory
+ UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
+ UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
+ UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
+ UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
+ UINT32 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
+ UINT32 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
+ UINT32 Pat : 1; // PAT
+ UINT32 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
+ UINT32 Available1 : 3; // Ignored
+ UINT32 PageTableBaseAddressLow : 20; // Page Table Base Address Low
+
+ UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address High
+ UINT32 Available3 : 7; // Ignored
+ UINT32 ProtectionKey : 4; // Protection key
+ UINT32 Nx : 1; // No Execute bit
} Bits;
UINT64 Uint64;
} IA32_PTE_4K;
@@ -149,16 +150,16 @@ typedef union {
///
typedef union {
struct {
- UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in memory
- UINT64 MustBeZero : 2; // Must Be Zero
- UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
- UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
- UINT64 MustBeZero2 : 4; // Must Be Zero
-
- UINT64 Available : 3; // Ignored
-
- UINT64 PageTableBaseAddress : 40; // Page Table Base Address
- UINT64 MustBeZero3 : 12; // Must Be Zero
+ UINT32 Present : 1; // 0 = Not present in memory, 1 = Present in memory
+ UINT32 MustBeZero : 2; // Must Be Zero
+ UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through caching
+ UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
+ UINT32 MustBeZero2 : 4; // Must Be Zero
+ UINT32 Available : 3; // Ignored
+ UINT32 PageTableBaseAddressLow : 20; // Page Table Base Address Low
+
+ UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address High
+ UINT32 MustBeZero3 : 12; // Must Be Zero
} Bits;
UINT64 Uint64;
} IA32_PDPTE_PAE;
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 797fc2f600..982652b58b 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -26,7 +26,7 @@ PageTableLibSetPte4K (
IN IA32_MAP_ATTRIBUTE *Mask
)
{
- if (Mask->Bits.PageTableBaseAddress) {
+ if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
Pte4K->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);
}
@@ -93,7 +93,7 @@ PageTableLibSetPleB (
IN IA32_MAP_ATTRIBUTE *Mask
)
{
- if (Mask->Bits.PageTableBaseAddress) {
+ if (Mask->Bits.PageTableBaseAddressLow || Mask->Bits.PageTableBaseAddressHigh) {
PleB->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);
}
@@ -238,7 +238,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
//
if ((Mask->Bits.ReadWrite == 0) || (Mask->Bits.UserSupervisor == 0) || (Mask->Bits.WriteThrough == 0) || (Mask->Bits.CacheDisabled == 0) ||
(Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || (Mask->Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
- (Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey == 0) || (Mask->Bits.Nx == 0))
+ ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask->Bits.PageTableBaseAddressHigh == 0)) || (Mask->Bits.ProtectionKey == 0) || (Mask->Bits.Nx == 0))
{
return RETURN_INVALID_PARAMETER;
}
@@ -396,7 +396,7 @@ PageTableLibMapInLevel (
// This function is called when the memory length is less than the region length of the parent level.
// No need to split the page when the attributes equal.
//
- if (Mask->Bits.PageTableBaseAddress == 0) {
+ if ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask->Bits.PageTableBaseAddressHigh == 0)) {
return RETURN_SUCCESS;
}
@@ -696,7 +696,7 @@ PageTableMap (
return RETURN_INVALID_PARAMETER;
}
- if ((LinearAddress % SIZE_4KB != 0) || (Length % SIZE_4KB != 0)) {
+ if (((UINTN)LinearAddress % SIZE_4KB != 0) || ((UINTN)Length % SIZE_4KB != 0)) {
//
// LinearAddress and Length should be multiple of 4K.
//
@@ -738,12 +738,12 @@ PageTableMap (
*IsModified = FALSE;
}
- ParentAttribute.Uint64 = 0;
- ParentAttribute.Bits.PageTableBaseAddress = 1;
- ParentAttribute.Bits.Present = 1;
- ParentAttribute.Bits.ReadWrite = 1;
- ParentAttribute.Bits.UserSupervisor = 1;
- ParentAttribute.Bits.Nx = 0;
+ ParentAttribute.Uint64 = 0;
+ ParentAttribute.Bits.PageTableBaseAddressLow = 1;
+ ParentAttribute.Bits.Present = 1;
+ ParentAttribute.Bits.ReadWrite = 1;
+ ParentAttribute.Bits.UserSupervisor = 1;
+ ParentAttribute.Bits.Nx = 0;
//
// Query the required buffer size without modifying the page table.
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 16/21] UefiCpuPkg: Modify UnitTest code since tested API is changed
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (14 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 15/21] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 7:40 ` [Patch V4 17/21] UefiCpuPkg/CpuPageTableLib: Add check for page table creation duntan
` (5 subsequent siblings)
21 siblings, 0 replies; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar
From: Zhiguang Liu <zhiguang.liu@intel.com>
Last commit changed the CpuPageTableLib API PageTableMap, unit
test code should also be modified.
Cc: Eric Dong <eric.dong@intel.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 38 ++++++++++++++++++--------------------
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c | 4 ++--
3 files changed, 67 insertions(+), 59 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
index 6343b56c2f..e1efc84c82 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
@@ -422,15 +422,14 @@ TestCaseManualSizeNotMatch (
UINTN MapCount;
IA32_PAGING_ENTRY *PagingEntry;
- PagingMode = Paging4Level;
- PageTableBufferSize = 0;
- PageTable = 0;
- Buffer = NULL;
- MapAttribute.Uint64 = 0;
- MapMask.Uint64 = MAX_UINT64;
- MapAttribute.Bits.Present = 1;
- MapAttribute.Bits.ReadWrite = 1;
- MapAttribute.Bits.PageTableBaseAddress = (SIZE_2MB - SIZE_4KB) >> 12;
+ PagingMode = Paging4Level;
+ PageTableBufferSize = 0;
+ PageTable = 0;
+ Buffer = NULL;
+ MapMask.Uint64 = MAX_UINT64;
+ MapAttribute.Uint64 = (SIZE_2MB - SIZE_4KB);
+ MapAttribute.Bits.Present = 1;
+ MapAttribute.Bits.ReadWrite = 1;
//
// Create Page table to cover [2M-4K, 4M], with ReadWrite = 1
//
@@ -460,9 +459,9 @@ TestCaseManualSizeNotMatch (
// [2M-4K,2M], R/W = 0
// [2M ,4M], R/W = 1
//
- PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)PageTable; // Get 4 level entry
- PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)(PagingEntry->Pnle.Bits.PageTableBaseAddress << 12); // Get 3 level entry
- PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)(PagingEntry->Pnle.Bits.PageTableBaseAddress << 12); // Get 2 level entry
+ PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)PageTable; // Get 4 level entry
+ PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (PagingEntry); // Get 3 level entry
+ PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (PagingEntry); // Get 2 level entry
PagingEntry->Uint64 = PagingEntry->Uint64 & (~(UINT64)0x2);
MapCount = 0;
Status = PageTableParse (PageTable, PagingMode, NULL, &MapCount);
@@ -480,20 +479,19 @@ TestCaseManualSizeNotMatch (
UT_ASSERT_EQUAL (Map[1].LinearAddress, SIZE_2MB);
UT_ASSERT_EQUAL (Map[1].Length, SIZE_2MB);
- ExpectedMapAttribute.Uint64 = MapAttribute.Uint64;
- ExpectedMapAttribute.Bits.ReadWrite = 1;
- ExpectedMapAttribute.Bits.PageTableBaseAddress = SIZE_2MB >> 12;
+ ExpectedMapAttribute.Uint64 = MapAttribute.Uint64 + SIZE_4KB;
+ ExpectedMapAttribute.Bits.ReadWrite = 1;
UT_ASSERT_EQUAL (Map[1].Attribute.Uint64, ExpectedMapAttribute.Uint64);
//
// Set Page table [2M-4K, 2M+4K]'s ReadWrite = 1, [2M,2M+4K]'s ReadWrite is already 1
// Just need to set [2M-4K,2M], won't need extra size, so the status should be success
//
- MapAttribute.Bits.Present = 1;
- MapAttribute.Bits.ReadWrite = 1;
- PageTableBufferSize = 0;
- MapAttribute.Bits.PageTableBaseAddress = (SIZE_2MB - SIZE_4KB) >> 12;
- Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2MB - SIZE_4KB, SIZE_4KB * 2, &MapAttribute, &MapMask, NULL);
+ MapAttribute.Uint64 = SIZE_2MB - SIZE_4KB;
+ MapAttribute.Bits.Present = 1;
+ MapAttribute.Bits.ReadWrite = 1;
+ PageTableBufferSize = 0;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2MB - SIZE_4KB, SIZE_4KB * 2, &MapAttribute, &MapMask, NULL);
UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
return UNIT_TEST_PASSED;
}
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index 6f1485976a..18a5010c30 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -157,7 +157,8 @@ ValidateAndRandomeModifyPageTablePageTableEntry (
)
{
UINT64 Index;
- UINT64 TempPhysicalBase;
+ UINT32 PageTableBaseAddressLow;
+ UINT32 PageTableBaseAddressHigh;
IA32_PAGING_ENTRY *ChildPageEntry;
UNIT_TEST_STATUS Status;
@@ -180,17 +181,21 @@ ValidateAndRandomeModifyPageTablePageTableEntry (
if ((RandomNumber < 100) && RandomBoolean (50)) {
RandomNumber++;
if (Level == 1) {
- TempPhysicalBase = PagingEntry->Pte4K.Bits.PageTableBaseAddress;
+ PageTableBaseAddressLow = PagingEntry->Pte4K.Bits.PageTableBaseAddressLow;
+ PageTableBaseAddressHigh = PagingEntry->Pte4K.Bits.PageTableBaseAddressHigh;
} else {
- TempPhysicalBase = PagingEntry->PleB.Bits.PageTableBaseAddress;
+ PageTableBaseAddressLow = PagingEntry->PleB.Bits.PageTableBaseAddressLow;
+ PageTableBaseAddressHigh = PagingEntry->PleB.Bits.PageTableBaseAddressHigh;
}
PagingEntry->Uint64 = (Random64 (0, MAX_UINT64) & mValidMaskLeaf[Level].Uint64) | mValidMaskLeafFlag[Level].Uint64;
PagingEntry->Pte4K.Bits.Present = 1;
if (Level == 1) {
- PagingEntry->Pte4K.Bits.PageTableBaseAddress = TempPhysicalBase;
+ PagingEntry->Pte4K.Bits.PageTableBaseAddressLow = PageTableBaseAddressLow;
+ PagingEntry->Pte4K.Bits.PageTableBaseAddressHigh = PageTableBaseAddressHigh;
} else {
- PagingEntry->PleB.Bits.PageTableBaseAddress = TempPhysicalBase;
+ PagingEntry->PleB.Bits.PageTableBaseAddressLow = PageTableBaseAddressLow;
+ PagingEntry->PleB.Bits.PageTableBaseAddressHigh = PageTableBaseAddressHigh;
}
if ((PagingEntry->Uint64 & mValidMaskLeaf[Level].Uint64) != PagingEntry->Uint64) {
@@ -212,15 +217,17 @@ ValidateAndRandomeModifyPageTablePageTableEntry (
if ((RandomNumber < 100) && RandomBoolean (50)) {
RandomNumber++;
- TempPhysicalBase = PagingEntry->Pnle.Bits.PageTableBaseAddress;
+ PageTableBaseAddressLow = PagingEntry->PleB.Bits.PageTableBaseAddressLow;
+ PageTableBaseAddressHigh = PagingEntry->PleB.Bits.PageTableBaseAddressHigh;
- PagingEntry->Uint64 = Random64 (0, MAX_UINT64) & mValidMaskNoLeaf[Level].Uint64;
- PagingEntry->Pnle.Bits.Present = 1;
- PagingEntry->Pnle.Bits.PageTableBaseAddress = TempPhysicalBase;
+ PagingEntry->Uint64 = Random64 (0, MAX_UINT64) & mValidMaskNoLeaf[Level].Uint64;
+ PagingEntry->Pnle.Bits.Present = 1;
+ PagingEntry->PleB.Bits.PageTableBaseAddressLow = PageTableBaseAddressLow;
+ PagingEntry->PleB.Bits.PageTableBaseAddressHigh = PageTableBaseAddressHigh;
ASSERT ((PagingEntry->Uint64 & mValidMaskLeafFlag[Level].Uint64) != mValidMaskLeafFlag[Level].Uint64);
}
- ChildPageEntry = (IA32_PAGING_ENTRY *)(UINTN)((PagingEntry->Pnle.Bits.PageTableBaseAddress) << 12);
+ ChildPageEntry = (IA32_PAGING_ENTRY *)(UINTN)(IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&PagingEntry->Pnle));
for (Index = 0; Index < 512; Index++) {
Status = ValidateAndRandomeModifyPageTablePageTableEntry (&ChildPageEntry[Index], Level-1, MaxLeafLevel, Address + (Index<<(9*(Level-1) + 3)));
if (Status != UNIT_TEST_PASSED) {
@@ -364,10 +371,12 @@ GenerateSingleRandomMapEntry (
}
if (mRandomOption & ONLY_ONE_ONE_MAPPING) {
- MapEntrys->Maps[MapsIndex].Attribute.Bits.PageTableBaseAddress = MapEntrys->Maps[MapsIndex].LinearAddress >> 12;
- MapEntrys->Maps[MapsIndex].Mask.Bits.PageTableBaseAddress = 0xFFFFFFFFFF;
+ MapEntrys->Maps[MapsIndex].Attribute.Uint64 &= (~IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK);
+ MapEntrys->Maps[MapsIndex].Attribute.Uint64 |= MapEntrys->Maps[MapsIndex].LinearAddress;
+ MapEntrys->Maps[MapsIndex].Mask.Uint64 |= IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK;
} else {
- MapEntrys->Maps[MapsIndex].Attribute.Bits.PageTableBaseAddress = (Random64 (0, (((UINT64)1)<<52) - 1) & AlignedTable[Random32 (0, ARRAY_SIZE (AlignedTable) -1)])>> 12;
+ MapEntrys->Maps[MapsIndex].Attribute.Uint64 &= (~IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK);
+ MapEntrys->Maps[MapsIndex].Attribute.Uint64 |= (Random64 (0, (((UINT64)1)<<52) - 1) & AlignedTable[Random32 (0, ARRAY_SIZE (AlignedTable) -1)]);
}
MapEntrys->Count += 1;
@@ -414,8 +423,9 @@ CompareEntrysforOnePoint (
//
for (Index = 0; Index < MapCount; Index++) {
if ((Address >= Map[Index].LinearAddress) && (Address < (Map[Index].LinearAddress + Map[Index].Length))) {
- AttributeInMap.Uint64 = (Map[Index].Attribute.Uint64 & mSupportedBit.Uint64);
- AttributeInMap.Bits.PageTableBaseAddress = ((Address - Map[Index].LinearAddress) >> 12) + Map[Index].Attribute.Bits.PageTableBaseAddress;
+ AttributeInMap.Uint64 = (Map[Index].Attribute.Uint64 & mSupportedBit.Uint64);
+ AttributeInMap.Uint64 &= (~IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK);
+ AttributeInMap.Uint64 |= (Address - Map[Index].LinearAddress + IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&Map[Index].Attribute)) & IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK;
break;
}
}
@@ -425,8 +435,10 @@ CompareEntrysforOnePoint (
//
for (Index = 0; Index < InitMapCount; Index++) {
if ((Address >= InitMap[Index].LinearAddress) && (Address < (InitMap[Index].LinearAddress + InitMap[Index].Length))) {
- AttributeInInitMap.Uint64 = (InitMap[Index].Attribute.Uint64 & mSupportedBit.Uint64);
- AttributeInInitMap.Bits.PageTableBaseAddress = ((Address - InitMap[Index].LinearAddress) >> 12) + InitMap[Index].Attribute.Bits.PageTableBaseAddress;
+ AttributeInInitMap.Uint64 = (InitMap[Index].Attribute.Uint64 & mSupportedBit.Uint64);
+ AttributeInInitMap.Uint64 &= (~IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK);
+ AttributeInInitMap.Uint64 |= (Address - InitMap[Index].LinearAddress + IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&InitMap[Index].Attribute)) & IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK;
+
break;
}
}
@@ -443,8 +455,9 @@ CompareEntrysforOnePoint (
MaskInMapEntrys.Uint64 |= MapEntrys->Maps[Index].Mask.Uint64;
AttributeInMapEntrys.Uint64 &= (~MapEntrys->Maps[Index].Mask.Uint64);
AttributeInMapEntrys.Uint64 |= (MapEntrys->Maps[Index].Attribute.Uint64 & MapEntrys->Maps[Index].Mask.Uint64);
- if (MapEntrys->Maps[Index].Mask.Bits.PageTableBaseAddress != 0) {
- AttributeInMapEntrys.Bits.PageTableBaseAddress = ((Address - MapEntrys->Maps[Index].LinearAddress) >> 12) + MapEntrys->Maps[Index].Attribute.Bits.PageTableBaseAddress;
+ if (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&MapEntrys->Maps[Index].Mask) != 0) {
+ AttributeInMapEntrys.Uint64 &= (~IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK);
+ AttributeInMapEntrys.Uint64 |= (Address - MapEntrys->Maps[Index].LinearAddress + IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&MapEntrys->Maps[Index].Attribute)) & IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK;
}
}
}
@@ -458,8 +471,8 @@ CompareEntrysforOnePoint (
if ((AttributeInMap.Uint64 & MaskInMapEntrys.Uint64) != (AttributeInMapEntrys.Uint64 & MaskInMapEntrys.Uint64)) {
DEBUG ((DEBUG_INFO, "======detailed information begin=====\n"));
DEBUG ((DEBUG_INFO, "\nError: Detect different attribute on a point with linear address: 0x%lx\n", Address));
- DEBUG ((DEBUG_INFO, "By parsing page table, the point has Attribute 0x%lx, and map to physical address 0x%lx\n", IA32_MAP_ATTRIBUTE_ATTRIBUTES (&AttributeInMap) & MaskInMapEntrys.Uint64, AttributeInMap.Bits.PageTableBaseAddress));
- DEBUG ((DEBUG_INFO, "While according to inputs, the point should Attribute 0x%lx, and should map to physical address 0x%lx\n", IA32_MAP_ATTRIBUTE_ATTRIBUTES (&AttributeInMapEntrys) & MaskInMapEntrys.Uint64, AttributeInMapEntrys.Bits.PageTableBaseAddress));
+ DEBUG ((DEBUG_INFO, "By parsing page table, the point has Attribute 0x%lx, and map to physical address 0x%lx\n", IA32_MAP_ATTRIBUTE_ATTRIBUTES (&AttributeInMap) & MaskInMapEntrys.Uint64, IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&AttributeInMap)));
+ DEBUG ((DEBUG_INFO, "While according to inputs, the point should Attribute 0x%lx, and should map to physical address 0x%lx\n", IA32_MAP_ATTRIBUTE_ATTRIBUTES (&AttributeInMapEntrys) & MaskInMapEntrys.Uint64, IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&AttributeInMapEntrys)));
DEBUG ((DEBUG_INFO, "The total Mask is 0x%lx\n", MaskInMapEntrys.Uint64));
if (MapEntrys->InitCount != 0) {
@@ -728,7 +741,7 @@ SingleMapEntryTest (
//
if ((Mask->Bits.ReadWrite == 0) || (Mask->Bits.UserSupervisor == 0) || (Mask->Bits.WriteThrough == 0) || (Mask->Bits.CacheDisabled == 0) ||
(Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || (Mask->Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
- (Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey == 0) || (Mask->Bits.Nx == 0))
+ ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask->Bits.PageTableBaseAddressHigh == 0)) || (Mask->Bits.ProtectionKey == 0) || (Mask->Bits.Nx == 0))
{
RemoveLastMapEntry (MapEntrys);
UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
@@ -1013,21 +1026,18 @@ TestCaseforRandomTest (
UT_ASSERT_EQUAL (Random64 (100, 100), 100);
UT_ASSERT_TRUE ((Random32 (9, 10) >= 9) & (Random32 (9, 10) <= 10));
UT_ASSERT_TRUE ((Random64 (9, 10) >= 9) & (Random64 (9, 10) <= 10));
-
- mSupportedBit.Bits.Present = 1;
- mSupportedBit.Bits.ReadWrite = 1;
- mSupportedBit.Bits.UserSupervisor = 1;
- mSupportedBit.Bits.WriteThrough = 1;
- mSupportedBit.Bits.CacheDisabled = 1;
- mSupportedBit.Bits.Accessed = 1;
- mSupportedBit.Bits.Dirty = 1;
- mSupportedBit.Bits.Pat = 1;
- mSupportedBit.Bits.Global = 1;
- mSupportedBit.Bits.Reserved1 = 0;
- mSupportedBit.Bits.PageTableBaseAddress = 0;
- mSupportedBit.Bits.Reserved2 = 0;
- mSupportedBit.Bits.ProtectionKey = 0xF;
- mSupportedBit.Bits.Nx = 1;
+ mSupportedBit.Uint64 = 0;
+ mSupportedBit.Bits.Present = 1;
+ mSupportedBit.Bits.ReadWrite = 1;
+ mSupportedBit.Bits.UserSupervisor = 1;
+ mSupportedBit.Bits.WriteThrough = 1;
+ mSupportedBit.Bits.CacheDisabled = 1;
+ mSupportedBit.Bits.Accessed = 1;
+ mSupportedBit.Bits.Dirty = 1;
+ mSupportedBit.Bits.Pat = 1;
+ mSupportedBit.Bits.Global = 1;
+ mSupportedBit.Bits.ProtectionKey = 0xF;
+ mSupportedBit.Bits.Nx = 1;
mRandomOption = ((CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT *)Context)->RandomOption;
mNumberIndex = 0;
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
index 10fdee2f94..22f179c21f 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
@@ -140,7 +140,7 @@ IsPageTableEntryValid (
UT_ASSERT_EQUAL ((PagingEntry->Uint64 & mValidMaskNoLeaf[Level].Uint64), PagingEntry->Uint64);
}
- ChildPageEntry = (IA32_PAGING_ENTRY *)(UINTN)(((UINTN)(PagingEntry->Pnle.Bits.PageTableBaseAddress)) << 12);
+ ChildPageEntry = (IA32_PAGING_ENTRY *)(UINTN)(IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&PagingEntry->Pnle));
for (Index = 0; Index < 512; Index++) {
Status = IsPageTableEntryValid (&ChildPageEntry[Index], Level-1, MaxLeafLevel, Address + (Index<<(9*(Level-1) + 3)));
if (Status != UNIT_TEST_PASSED) {
@@ -233,7 +233,7 @@ GetEntryFromSubPageTable (
//
// Not a leaf
//
- ChildPageEntry = (IA32_PAGING_ENTRY *)(UINTN)(((UINTN)(PagingEntry->Pnle.Bits.PageTableBaseAddress)) << 12);
+ ChildPageEntry = (IA32_PAGING_ENTRY *)(UINTN)(IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&PagingEntry->Pnle));
*Level = *Level -1;
Index = Address >> (*Level * 9 + 3);
ASSERT (Index == (Index & ((1<< 9) - 1)));
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 17/21] UefiCpuPkg/CpuPageTableLib: Add check for page table creation
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (15 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 16/21] UefiCpuPkg: Modify UnitTest code since tested API is changed duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 13:29 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 18/21] UefiCpuPkg: Combine branch for non-present and leaf ParentEntry duntan
` (4 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Add code to compare ParentPagingEntry Attribute&Mask and input
Attribute&Mask to decide if new next level page table is needed
in non-present ParentPagingEntry condition. This can help avoid
unneccessary page table creation.
For example, there is a page table in which [0, 1G] is mapped(Lv4[0]
,Lv3[0,0], a non-leaf level4 entry and a leaf level3 entry).And we
only want to map [1G, 1G+2M] linear address still as non-present.
The expected behaviour should be nothing happens in the process.
However, previous code logic doesn't check if ParentPagingEntry
Attribute&Mask and input Attribute&Mask are the same in non-present
ParentPagingEntry condition. Then a new 4K memory is allocated for
Lv2 since 1G+2M is not 1G-aligned.
So when ParentPagingEntry is non-present, before allocate 4K memory
for next level paging, we also check if ParentPagingEntry Attribute&
Mask and input Attribute&Mask are the same.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 982652b58b..55a756ad90 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -358,6 +358,16 @@ PageTableLibMapInLevel (
return Status;
}
+ //
+ // Check the attribute in ParentPagingEntry is equal to attribute calculated by input Attribue and Mask.
+ //
+ PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute (&ParentPagingEntry->PleB, ParentAttribute);
+ if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
+ == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))
+ {
+ return RETURN_SUCCESS;
+ }
+
//
// The parent entry is CR3 or PML5E/PML4E/PDPTE/PDE.
// It does NOT point to an existing page directory.
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 18/21] UefiCpuPkg: Combine branch for non-present and leaf ParentEntry
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (16 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 17/21] UefiCpuPkg/CpuPageTableLib: Add check for page table creation duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 13:32 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 19/21] UefiCpuPkg/CpuPageTableLib: Enable PAE paging duntan
` (3 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Combine 'if' condition branch for non-present and leaf Parent
Entry in PageTableLibMapInLevel. Most steps of these two condition
are the same. This commit doesn't change any functionality.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 83 +++++++++++++++++++++++++++++++----------------------------------------------------
1 file changed, 31 insertions(+), 52 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 55a756ad90..773948349e 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -348,68 +348,45 @@ PageTableLibMapInLevel (
// ParentPagingEntry ONLY is deferenced for checking Present and MustBeOne bits
// when Modify is FALSE.
//
-
- if (ParentPagingEntry->Pce.Present == 0) {
- //
- // [LinearAddress, LinearAddress + Length] contains non-present range.
- //
- Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute, Mask);
- if (RETURN_ERROR (Status)) {
- return Status;
- }
-
+ if ((ParentPagingEntry->Pce.Present == 0) || IsPle (ParentPagingEntry, Level + 1)) {
//
- // Check the attribute in ParentPagingEntry is equal to attribute calculated by input Attribue and Mask.
- //
- PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute (&ParentPagingEntry->PleB, ParentAttribute);
- if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
- == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))
- {
- return RETURN_SUCCESS;
- }
-
- //
- // The parent entry is CR3 or PML5E/PML4E/PDPTE/PDE.
+ // When ParentPagingEntry is non-present, parent entry is CR3 or PML5E/PML4E/PDPTE/PDE.
// It does NOT point to an existing page directory.
+ // When ParentPagingEntry is present, parent entry is leaf PDPTE_1G or PDE_2M. Split to 2M or 4K pages.
+ // Note: it's impossible the parent entry is a PTE_4K.
//
- ASSERT (Buffer == NULL || *BufferSize >= SIZE_4KB);
- CreateNew = TRUE;
- *BufferSize -= SIZE_4KB;
+ OneOfPagingEntry.Pnle.Uint64 = 0;
+ PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute (&ParentPagingEntry->PleB, ParentAttribute);
- if (Modify) {
- ParentPagingEntry->Uintn = (UINTN)Buffer + *BufferSize;
- ZeroMem ((VOID *)ParentPagingEntry->Uintn, SIZE_4KB);
+ if (ParentPagingEntry->Pce.Present == 0) {
//
- // Set default attribute bits for PML5E/PML4E/PDPTE/PDE.
+ // [LinearAddress, LinearAddress + Length] contains non-present range.
//
- PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOneMask);
+ Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute, Mask);
+ if (RETURN_ERROR (Status)) {
+ return Status;
+ }
} else {
- //
- // Just make sure Present and MustBeZero (PageSize) bits are accurate.
- //
- OneOfPagingEntry.Pnle.Uint64 = 0;
+ PageTableLibSetPle (Level, &OneOfPagingEntry, 0, &PleBAttribute, &AllOneMask);
}
- } else if (IsPle (ParentPagingEntry, Level + 1)) {
- //
- // The parent entry is a PDPTE_1G or PDE_2M. Split to 2M or 4K pages.
- // Note: it's impossible the parent entry is a PTE_4K.
- //
+
//
- // Use NOP attributes as the attribute of grand-parents because CPU will consider
- // the actual attributes of grand-parents when determing the memory type.
+ // Check if the attribute, the physical address calculated by ParentPagingEntry is equal to
+ // the attribute, the physical address calculated by input Attribue and Mask.
//
- PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute (&ParentPagingEntry->PleB, ParentAttribute);
if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
== (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))
{
- //
- // This function is called when the memory length is less than the region length of the parent level.
- // No need to split the page when the attributes equal.
- //
if ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask->Bits.PageTableBaseAddressHigh == 0)) {
return RETURN_SUCCESS;
}
+ //
+ // Non-present entry won't reach there since:
+ // 1.When map non-present entry to present, the attribute must be different.
+ // 2.When still map non-present entry to non-present, PageTableBaseAddressLow and High in Mask must be 0.
+ //
+ ASSERT (ParentPagingEntry->Pce.Present == 1);
PhysicalAddrInEntry = IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&PleBAttribute) + PagingEntryIndex * RegionLength;
PhysicalAddrInAttr = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) & (~RegionMask);
if (PhysicalAddrInEntry == PhysicalAddrInAttr) {
@@ -420,17 +397,19 @@ PageTableLibMapInLevel (
ASSERT (Buffer == NULL || *BufferSize >= SIZE_4KB);
CreateNew = TRUE;
*BufferSize -= SIZE_4KB;
- PageTableLibSetPle (Level, &OneOfPagingEntry, 0, &PleBAttribute, &AllOneMask);
+
if (Modify) {
- //
- // Create 512 child-level entries that map to 2M/4K.
- //
PagingEntry = (IA32_PAGING_ENTRY *)((UINTN)Buffer + *BufferSize);
ZeroMem (PagingEntry, SIZE_4KB);
- for (SubOffset = 0, Index = 0; Index < 512; Index++) {
- PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
- SubOffset += RegionLength;
+ if (ParentPagingEntry->Pce.Present) {
+ //
+ // Create 512 child-level entries that map to 2M/4K.
+ //
+ for (SubOffset = 0, Index = 0; Index < 512; Index++) {
+ PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
+ SubOffset += RegionLength;
+ }
}
//
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 19/21] UefiCpuPkg/CpuPageTableLib: Enable PAE paging
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (17 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 18/21] UefiCpuPkg: Combine branch for non-present and leaf ParentEntry duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 13:38 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 20/21] UefiCpuPkg/CpuPageTableLib: Add RandomTest for " duntan
` (2 subsequent siblings)
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Modify CpuPageTableLib code to enable PAE paging.
In PageTableMap() API:
When creating new PAE page table, after creating page table,
set all MustBeZero fields of 4 PDPTE to 0. The MustBeZero
fields are treated as RW and other attributes by the common
map logic. So they might be set to 1.
When updating exsiting PAE page table, the special steps are:
1.Prepare 4K-aligned 32bytes memory in stack for 4 temp PDPTE.
2.Copy original 4 PDPTE to the 4 temp PDPTE and set the RW,
UserSupervisor to 1 and set Nx of 4 temp PDPTE to 0.
4.After updating the page table, set the MustBeZero fields of
4 temp PDPTE to 0.
5.Copy the temp PDPTE to original PDPTE.
In PageTableParse() API, also create 4 temp PDPTE in stack.
Copy original 4 PDPTE to the 4 temp PDPTE. Then set the RW,
UserSupervisor to 1 and set Nx of 4 temp PDPTE to 0. Finally
use the address of temp PDPTE as the page table address.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h | 10 +++++++---
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableParse.c | 25 +++++++++++++++++++++----
3 files changed, 76 insertions(+), 12 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
index 2c67ecb469..521d56c148 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
@@ -15,11 +15,14 @@
#include <Library/DebugLib.h>
#include <Library/CpuPageTableLib.h>
-#define IA32_PE_BASE_ADDRESS_MASK_40 0xFFFFFFFFFF000ull
-#define IA32_PE_BASE_ADDRESS_MASK_39 0xFFFFFFFFFE000ull
+#define IA32_PE_BASE_ADDRESS_MASK_40 0xFFFFFFFFFF000ull
+#define IA32_PE_BASE_ADDRESS_MASK_39 0xFFFFFFFFFE000ull
+#define IA32_PE_BASE_ADDRESS_MASK_PAE_PDPTE 0xFFFFFFFFFFFE0ull
#define REGION_LENGTH(l) LShiftU64 (1, (l) * 9 + 3)
+#define MAX_PAE_PDPTE_NUM 4
+
typedef enum {
Pte = 1,
Pde = 2,
@@ -60,7 +63,8 @@ typedef union {
UINT64 Uint64;
} IA32_PAGE_NON_LEAF_ENTRY;
-#define IA32_PNLE_PAGE_TABLE_BASE_ADDRESS(pa) ((pa)->Uint64 & IA32_PE_BASE_ADDRESS_MASK_40)
+#define IA32_PNLE_PAGE_TABLE_BASE_ADDRESS(pa) ((pa)->Uint64 & IA32_PE_BASE_ADDRESS_MASK_40)
+#define IA32_PAE_PDPTE_PAGE_TABLE_BASE_ADDRESS(pa) ((pa)->Uint64 & IA32_PE_BASE_ADDRESS_MASK_PAE_PDPTE)
///
/// Format of a PML5 Entry (PML5E) that References a PML4 Table
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 773948349e..8769e45f25 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -661,15 +661,17 @@ PageTableMap (
IA32_PAGE_LEVEL MaxLeafLevel;
IA32_MAP_ATTRIBUTE ParentAttribute;
BOOLEAN LocalIsModified;
+ UINTN Index;
+ IA32_PAGING_ENTRY *PagingEntry;
+ UINT8 BufferInStack[SIZE_4KB - 1 + MAX_PAE_PDPTE_NUM * sizeof (IA32_PAGING_ENTRY)];
if (Length == 0) {
return RETURN_SUCCESS;
}
- if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) || (PagingMode >= PagingModeMax)) {
+ if ((PagingMode == Paging32bit) || (PagingMode >= PagingModeMax)) {
//
// 32bit paging is never supported.
- // PAE paging will be supported later.
//
return RETURN_UNSUPPORTED;
}
@@ -706,17 +708,32 @@ PageTableMap (
MaxLeafLevel = (IA32_PAGE_LEVEL)(UINT8)PagingMode;
MaxLevel = (IA32_PAGE_LEVEL)(UINT8)(PagingMode >> 8);
- MaxLinearAddress = LShiftU64 (1, 12 + MaxLevel * 9);
+ MaxLinearAddress = (PagingMode == PagingPae) ? LShiftU64 (1, 32) : LShiftU64 (1, 12 + MaxLevel * 9);
if ((LinearAddress > MaxLinearAddress) || (Length > MaxLinearAddress - LinearAddress)) {
//
- // Maximum linear address is (1 << 48) or (1 << 57)
+ // Maximum linear address is (1 << 32), (1 << 48) or (1 << 57)
//
return RETURN_INVALID_PARAMETER;
}
TopPagingEntry.Uintn = *PageTable;
if (TopPagingEntry.Uintn != 0) {
+ if (PagingMode == PagingPae) {
+ //
+ // Create 4 temporary PDPTE at a 4k-aligned address.
+ // Copy the original PDPTE content and set ReadWrite, UserSupervisor to 1, set Nx to 0.
+ //
+ TopPagingEntry.Uintn = ALIGN_VALUE ((UINTN)BufferInStack, BASE_4KB);
+ PagingEntry = (IA32_PAGING_ENTRY *)(TopPagingEntry.Uintn);
+ CopyMem (PagingEntry, (VOID *)(*PageTable), MAX_PAE_PDPTE_NUM * sizeof (IA32_PAGING_ENTRY));
+ for (Index = 0; Index < MAX_PAE_PDPTE_NUM; Index++) {
+ PagingEntry[Index].Pnle.Bits.ReadWrite = 1;
+ PagingEntry[Index].Pnle.Bits.UserSupervisor = 1;
+ PagingEntry[Index].Pnle.Bits.Nx = 0;
+ }
+ }
+
TopPagingEntry.Pce.Present = 1;
TopPagingEntry.Pce.ReadWrite = 1;
TopPagingEntry.Pce.UserSupervisor = 1;
@@ -792,7 +809,33 @@ PageTableMap (
}
if (!RETURN_ERROR (Status)) {
- *PageTable = (UINTN)(TopPagingEntry.Uintn & IA32_PE_BASE_ADDRESS_MASK_40);
+ PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)(TopPagingEntry.Uintn & IA32_PE_BASE_ADDRESS_MASK_40);
+
+ if (PagingMode == PagingPae) {
+ //
+ // These MustBeZero fields are treated as RW and other attributes by the common map logic. So they might be set to 1.
+ //
+ for (Index = 0; Index < MAX_PAE_PDPTE_NUM; Index++) {
+ PagingEntry[Index].PdptePae.Bits.MustBeZero = 0;
+ PagingEntry[Index].PdptePae.Bits.MustBeZero2 = 0;
+ PagingEntry[Index].PdptePae.Bits.MustBeZero3 = 0;
+ }
+
+ if (*PageTable != 0) {
+ //
+ // Copy temp PDPTE to original PDPTE.
+ //
+ CopyMem ((VOID *)(*PageTable), PagingEntry, MAX_PAE_PDPTE_NUM * sizeof (IA32_PAGING_ENTRY));
+ }
+ }
+
+ if (*PageTable == 0) {
+ //
+ // Do not assign the *PageTable when it's an existing page table.
+ // If it's an existing PAE page table, PagingEntry is the temp buffer in stack.
+ //
+ *PageTable = (UINTN)PagingEntry;
+ }
}
return Status;
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableParse.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableParse.c
index 65490751ab..f6d7b9bb4c 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableParse.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableParse.c
@@ -158,6 +158,7 @@ VOID
PageTableLibParsePnle (
IN UINT64 PageTableBaseAddress,
IN UINTN Level,
+ IN UINTN MaxLevel,
IN UINT64 RegionStart,
IN IA32_MAP_ATTRIBUTE *ParentMapAttribute,
IN OUT IA32_MAP_ENTRY *Map,
@@ -171,13 +172,15 @@ PageTableLibParsePnle (
UINTN Index;
IA32_MAP_ATTRIBUTE MapAttribute;
UINT64 RegionLength;
+ UINTN PagingEntryNumber;
ASSERT (OneEntry != NULL);
- PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)PageTableBaseAddress;
- RegionLength = REGION_LENGTH (Level);
+ PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)PageTableBaseAddress;
+ RegionLength = REGION_LENGTH (Level);
+ PagingEntryNumber = ((MaxLevel == 3) && (Level == 3)) ? MAX_PAE_PDPTE_NUM : 512;
- for (Index = 0; Index < 512; Index++, RegionStart += RegionLength) {
+ for (Index = 0; Index < PagingEntryNumber; Index++, RegionStart += RegionLength) {
if (PagingEntry[Index].Pce.Present == 0) {
continue;
}
@@ -228,6 +231,7 @@ PageTableLibParsePnle (
PageTableLibParsePnle (
IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&PagingEntry[Index].Pnle),
Level - 1,
+ MaxLevel,
RegionStart,
&MapAttribute,
Map,
@@ -269,6 +273,8 @@ PageTableParse (
IA32_MAP_ENTRY *LastEntry;
IA32_MAP_ENTRY OneEntry;
UINTN MaxLevel;
+ UINTN Index;
+ IA32_PAGING_ENTRY BufferInStack[MAX_PAE_PDPTE_NUM];
if ((PagingMode == Paging32bit) || (PagingMode >= PagingModeMax)) {
//
@@ -290,6 +296,17 @@ PageTableParse (
return RETURN_SUCCESS;
}
+ if (PagingMode == PagingPae) {
+ CopyMem (BufferInStack, (VOID *)PageTable, sizeof (BufferInStack));
+ for (Index = 0; Index < MAX_PAE_PDPTE_NUM; Index++) {
+ BufferInStack[Index].Pnle.Bits.ReadWrite = 1;
+ BufferInStack[Index].Pnle.Bits.UserSupervisor = 1;
+ BufferInStack[Index].Pnle.Bits.Nx = 0;
+ }
+
+ PageTable = (UINTN)BufferInStack;
+ }
+
//
// Page table layout is as below:
//
@@ -319,7 +336,7 @@ PageTableParse (
MapCapacity = *MapCount;
*MapCount = 0;
LastEntry = NULL;
- PageTableLibParsePnle ((UINT64)PageTable, MaxLevel, 0, &NopAttribute, Map, MapCount, MapCapacity, &LastEntry, &OneEntry);
+ PageTableLibParsePnle ((UINT64)PageTable, MaxLevel, MaxLevel, 0, &NopAttribute, Map, MapCount, MapCapacity, &LastEntry, &OneEntry);
if (*MapCount > MapCapacity) {
return RETURN_BUFFER_TOO_SMALL;
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 20/21] UefiCpuPkg/CpuPageTableLib: Add RandomTest for PAE paging
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (18 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 19/21] UefiCpuPkg/CpuPageTableLib: Enable PAE paging duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 13:40 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 21/21] UefiCpuPkg/CpuPageTableLib: Reduce the number of random tests duntan
2023-03-23 9:13 ` [edk2-devel] [Patch V4 00/21] Fix issues in CpuPageTableLib Gerd Hoffmann
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Add RandomTest for PAE paging.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 2 ++
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 3 +--
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c | 12 ++++++++----
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
index e1efc84c82..8554eefa39 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
@@ -9,6 +9,7 @@
#include "CpuPageTableLibUnitTest.h"
// ----------------------------------------------------------------------- PageMode--TestCount-TestRangeCount---RandomOptions
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPagingPae = { PagingPae, 100, 20, USE_RANDOM_ARRAY };
static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level = { Paging4Level, 100, 20, USE_RANDOM_ARRAY };
static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level1GB = { Paging4Level1GB, 100, 20, USE_RANDOM_ARRAY };
static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging5Level = { Paging5Level, 100, 20, USE_RANDOM_ARRAY };
@@ -880,6 +881,7 @@ UefiTestMain (
goto EXIT;
}
+ AddTestCase (RandomTestCase, "Random Test for PagingPae", "Random Test Case1", TestCaseforRandomTest, NULL, NULL, &mTestContextPagingPae);
AddTestCase (RandomTestCase, "Random Test for Paging4Level", "Random Test Case1", TestCaseforRandomTest, NULL, NULL, &mTestContextPaging4Level);
AddTestCase (RandomTestCase, "Random Test for Paging4Level1G", "Random Test Case2", TestCaseforRandomTest, NULL, NULL, &mTestContextPaging4Level1GB);
AddTestCase (RandomTestCase, "Random Test for Paging5Level", "Random Test Case3", TestCaseforRandomTest, NULL, NULL, &mTestContextPaging5Level);
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index 18a5010c30..7e79b01823 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -258,10 +258,9 @@ ValidateAndRandomeModifyPageTable (
UNIT_TEST_STATUS Status;
IA32_PAGING_ENTRY *PagingEntry;
- if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) || (PagingMode >= PagingModeMax)) {
+ if ((PagingMode == Paging32bit) || (PagingMode >= PagingModeMax)) {
//
// 32bit paging is never supported.
- // PAE paging will be supported later.
//
return UNIT_TEST_ERROR_TEST_FAILED;
}
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
index 22f179c21f..67776255c2 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
@@ -175,10 +175,9 @@ IsPageTableValid (
return UNIT_TEST_PASSED;
}
- if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) || (PagingMode >= PagingModeMax)) {
+ if ((PagingMode == Paging32bit) || (PagingMode >= PagingModeMax)) {
//
// 32bit paging is never supported.
- // PAE paging will be supported later.
//
return UNIT_TEST_ERROR_TEST_FAILED;
}
@@ -187,7 +186,12 @@ IsPageTableValid (
MaxLevel = (UINT8)(PagingMode >> 8);
PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)PageTable;
- for (Index = 0; Index < 512; Index++) {
+ for (Index = 0; Index < ((PagingMode == PagingPae) ? 4 : 512); Index++) {
+ if (PagingMode == PagingPae) {
+ UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero, 0);
+ UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero2, 0);
+ }
+
Status = IsPageTableEntryValid (&PagingEntry[Index], MaxLevel, MaxLeafLevel, Index << (9 * MaxLevel + 3));
if (Status != UNIT_TEST_PASSED) {
return Status;
@@ -264,7 +268,7 @@ GetEntryFromPageTable (
UINT64 Index;
IA32_PAGING_ENTRY *PagingEntry;
- if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) || (PagingMode >= PagingModeMax)) {
+ if ((PagingMode == Paging32bit) || (PagingMode >= PagingModeMax)) {
//
// 32bit paging is never supported.
// PAE paging will be supported later.
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [Patch V4 21/21] UefiCpuPkg/CpuPageTableLib: Reduce the number of random tests
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (19 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 20/21] UefiCpuPkg/CpuPageTableLib: Add RandomTest for " duntan
@ 2023-03-23 7:40 ` duntan
2023-03-23 13:40 ` Ni, Ray
2023-03-23 9:13 ` [edk2-devel] [Patch V4 00/21] Fix issues in CpuPageTableLib Gerd Hoffmann
21 siblings, 1 reply; 44+ messages in thread
From: duntan @ 2023-03-23 7:40 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Reduce the number of random tests. In previous patch, non-1:1
mapping is enbaled and it may need more than an hour and a half
for the CI test, which may lead to CI timeout. Reduce the number
of random test count to pass the CI.
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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
index 8554eefa39..c60302eeac 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
@@ -9,11 +9,11 @@
#include "CpuPageTableLibUnitTest.h"
// ----------------------------------------------------------------------- PageMode--TestCount-TestRangeCount---RandomOptions
-static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPagingPae = { PagingPae, 100, 20, USE_RANDOM_ARRAY };
-static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level = { Paging4Level, 100, 20, USE_RANDOM_ARRAY };
-static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level1GB = { Paging4Level1GB, 100, 20, USE_RANDOM_ARRAY };
-static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging5Level = { Paging5Level, 100, 20, USE_RANDOM_ARRAY };
-static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging5Level1GB = { Paging5Level1GB, 100, 20, USE_RANDOM_ARRAY };
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPagingPae = { PagingPae, 30, 20, USE_RANDOM_ARRAY };
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level = { Paging4Level, 30, 20, USE_RANDOM_ARRAY };
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging4Level1GB = { Paging4Level1GB, 30, 20, USE_RANDOM_ARRAY };
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging5Level = { Paging5Level, 30, 20, USE_RANDOM_ARRAY };
+static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT mTestContextPaging5Level1GB = { Paging5Level1GB, 30, 20, USE_RANDOM_ARRAY };
/**
Check if the input parameters are not supported.
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [edk2-devel] [Patch V4 00/21] Fix issues in CpuPageTableLib
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
` (20 preceding siblings ...)
2023-03-23 7:40 ` [Patch V4 21/21] UefiCpuPkg/CpuPageTableLib: Reduce the number of random tests duntan
@ 2023-03-23 9:13 ` Gerd Hoffmann
2023-03-23 9:16 ` duntan
21 siblings, 1 reply; 44+ messages in thread
From: Gerd Hoffmann @ 2023-03-23 9:13 UTC (permalink / raw)
To: devel, dun.tan
On Thu, Mar 23, 2023 at 03:40:36PM +0800, duntan wrote:
> In the V4 atch set
> 1. In the patch "UefiCpuPkg/MpInitLib: Add code to initialize MapMask", add code to set MapMask to MAX_UINT64 to fix the boot failure tested by Gred.
> 2. Add patches to enable PAE paing in CpuPageTable Lib and add random test for PAE paging
> 3. Add patch to reduce the number of random test since CI may timeout
> 4. Modify the patch to fix non-1:1 mapping issue
Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [edk2-devel] [Patch V4 00/21] Fix issues in CpuPageTableLib
2023-03-23 9:13 ` [edk2-devel] [Patch V4 00/21] Fix issues in CpuPageTableLib Gerd Hoffmann
@ 2023-03-23 9:16 ` duntan
0 siblings, 0 replies; 44+ messages in thread
From: duntan @ 2023-03-23 9:16 UTC (permalink / raw)
To: Gerd Hoffmann, devel@edk2.groups.io
Thank you Gerd!
Thanks,
Dun
-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com>
Sent: Thursday, March 23, 2023 5:13 PM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
Subject: Re: [edk2-devel] [Patch V4 00/21] Fix issues in CpuPageTableLib
On Thu, Mar 23, 2023 at 03:40:36PM +0800, duntan wrote:
> In the V4 atch set
> 1. In the patch "UefiCpuPkg/MpInitLib: Add code to initialize MapMask", add code to set MapMask to MAX_UINT64 to fix the boot failure tested by Gred.
> 2. Add patches to enable PAE paing in CpuPageTable Lib and add random
> test for PAE paging 3. Add patch to reduce the number of random test
> since CI may timeout 4. Modify the patch to fix non-1:1 mapping issue
Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 02/21] UefiCpuPkg/CpuPageTableLib: Add check for input Length
2023-03-23 7:40 ` [Patch V4 02/21] UefiCpuPkg/CpuPageTableLib: Add check for input Length duntan
@ 2023-03-23 12:20 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 12:20 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V4 02/21] UefiCpuPkg/CpuPageTableLib: Add check for input
> Length
>
> Add check for input Length in PageTableMap (). Return
> RETURN_SUCCESS when input Length is 0.
>
> 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>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/Include/Library/CpuPageTableLib.h | 4 ++--
> UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 6 +++++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> index 2dc9b7d18e..5f44ece548 100644
> --- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> +++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> @@ -1,7 +1,7 @@
> /** @file
> Public include file for PageTableLib library.
>
> - Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -81,7 +81,7 @@ typedef enum {
> @retval RETURN_BUFFER_TOO_SMALL The buffer is too small for page
> table creation/updating.
> BufferSize is updated to indicate the expected buffer size.
> Caller may still get RETURN_BUFFER_TOO_SMALL with the
> new BufferSize.
> - @retval RETURN_SUCCESS PageTable is created/updated successfully.
> + @retval RETURN_SUCCESS PageTable is created/updated
> successfully or the input Length is 0.
> **/
> RETURN_STATUS
> EFIAPI
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 52535e5a8d..218068a3e1 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -544,7 +544,7 @@ PageTableLibMapInLevel (
> @retval RETURN_BUFFER_TOO_SMALL The buffer is too small for page
> table creation/updating.
> BufferSize is updated to indicate the expected buffer size.
> Caller may still get RETURN_BUFFER_TOO_SMALL with the
> new BufferSize.
> - @retval RETURN_SUCCESS PageTable is created/updated successfully.
> + @retval RETURN_SUCCESS PageTable is created/updated
> successfully or the input Length is 0.
> **/
> RETURN_STATUS
> EFIAPI
> @@ -567,6 +567,10 @@ PageTableMap (
> IA32_PAGE_LEVEL MaxLeafLevel;
> IA32_MAP_ATTRIBUTE ParentAttribute;
>
> + if (Length == 0) {
> + return RETURN_SUCCESS;
> + }
> +
> if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) ||
> (PagingMode >= PagingModeMax)) {
> //
> // 32bit paging is never supported.
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 03/21] UefiCpuPkg/CpuPageTableLib:Initialize some LocalVariable at beginning
2023-03-23 7:40 ` [Patch V4 03/21] UefiCpuPkg/CpuPageTableLib:Initialize some LocalVariable at beginning duntan
@ 2023-03-23 12:26 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 12:26 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Reviewed-by: Ray Ni <ray.ni@Intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V4 03/21] UefiCpuPkg/CpuPageTableLib:Initialize some
> LocalVariable at beginning
>
> Move some local variable initialization to the beginning of the
> function. Also delete duplicated calculation for RegionLength.
>
> 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>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 21
> ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 218068a3e1..127b65183f 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -258,6 +258,7 @@ PageTableLibMapInLevel (
> UINTN BitStart;
> UINTN Index;
> IA32_PAGING_ENTRY *PagingEntry;
> + UINTN PagingEntryIndex;
> IA32_PAGING_ENTRY *CurrentPagingEntry;
> UINT64 RegionLength;
> UINT64 SubLength;
> @@ -288,6 +289,14 @@ PageTableLibMapInLevel (
> LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
> ParentAttribute = &LocalParentAttribute;
>
> + //
> + // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21)
> or 4K (1 << 12).
> + //
> + BitStart = 12 + (Level - 1) * 9;
> + PagingEntryIndex = (UINTN)BitFieldRead64 (LinearAddress + Offset,
> BitStart, BitStart + 9 - 1);
> + RegionLength = REGION_LENGTH (Level);
> + RegionMask = RegionLength - 1;
> +
> //
> // ParentPagingEntry ONLY is deferenced for checking Present and
> MustBeOne bits
> // when Modify is FALSE.
> @@ -353,8 +362,7 @@ PageTableLibMapInLevel (
> //
> PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute,
> &AllOneMask);
>
> - RegionLength = REGION_LENGTH (Level);
> - PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> + PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> for (SubOffset = 0, Index = 0; Index < 512; Index++) {
> PagingEntry[Index].Uint64 = OneOfPagingEntry.Uint64 + SubOffset;
> SubOffset += RegionLength;
> @@ -425,15 +433,10 @@ PageTableLibMapInLevel (
> }
>
> //
> - // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or
> 4K (1 << 12).
> // RegionStart: points to the linear address that's aligned on RegionLength
> and lower than (LinearAddress + Offset).
> //
> - BitStart = 12 + (Level - 1) * 9;
> - Index = (UINTN)BitFieldRead64 (LinearAddress + Offset, BitStart,
> BitStart + 9 - 1);
> - RegionLength = LShiftU64 (1, BitStart);
> - RegionMask = RegionLength - 1;
> - RegionStart = (LinearAddress + Offset) & ~RegionMask;
> -
> + Index = PagingEntryIndex;
> + RegionStart = (LinearAddress + Offset) & ~RegionMask;
> ParentAttribute->Uint64 = PageTableLibGetPnleMapAttribute
> (&ParentPagingEntry->Pnle, ParentAttribute);
>
> //
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 04/21] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
2023-03-23 7:40 ` [Patch V4 04/21] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan
@ 2023-03-23 12:29 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 12:29 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
> + PhysicalAddrInEntry =
> IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&PleBAttribute) +
> PagingEntryIndex * RegionLength;
Overall logic looks good to me.
But above code might fail to build in IA32 build due to UINT32 * UINT64.
> + PhysicalAddrInAttr =
> (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset)
> & (~RegionMask);
> + if (PhysicalAddrInEntry == PhysicalAddrInAttr) {
> + return RETURN_SUCCESS;
> + }
> }
>
> ASSERT (Buffer == NULL || *BufferSize >= SIZE_4KB);
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 05/21] UefiCpuPkg/CpuPageTableLib:Clear PageSize bit(Bit7) for non-leaf
2023-03-23 7:40 ` [Patch V4 05/21] UefiCpuPkg/CpuPageTableLib:Clear PageSize bit(Bit7) for non-leaf duntan
@ 2023-03-23 12:29 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 12:29 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V4 05/21] UefiCpuPkg/CpuPageTableLib:Clear PageSize
> bit(Bit7) for non-leaf
>
> Clear PageSize bit(Bit7) for non-leaf entry in PageTableLibSetPnle.
> This function is used to set non-leaf entry attributes so it should
> make sure that the PageSize bit of the entry should be 0.
>
> 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>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index ad68792ca8..cf0cfeca77 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -202,7 +202,8 @@ PageTableLibSetPnle (
> Pnle->Bits.Nx = Attribute->Bits.Nx;
> }
>
> - Pnle->Bits.Accessed = 0;
> + Pnle->Bits.Accessed = 0;
> + Pnle->Bits.MustBeZero = 0;
>
> //
> // Set the attributes (WT, CD, A) to 0.
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 06/21] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry
2023-03-23 7:40 ` [Patch V4 06/21] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry duntan
@ 2023-03-23 12:35 ` Ni, Ray
[not found] ` <174F0C7EF4E09C53.19937@groups.io>
1 sibling, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 12:35 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
> + ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) |
> (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
In this branch, ParentPagingEntry is a leaf entry covering 2M or 1G physical address range.
Then, why not use IA32_PE_BASE_ADDRESS_MASK_39? Because for the two types leaf entry, address
only occupies 39 bits, not 40bits.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 07/21] UefiCpuPkg/MpInitLib: Add code to initialize MapMask
2023-03-23 7:40 ` [Patch V4 07/21] UefiCpuPkg/MpInitLib: Add code to initialize MapMask duntan
@ 2023-03-23 12:37 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 12:37 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V4 07/21] UefiCpuPkg/MpInitLib: Add code to initialize
> MapMask
>
> In function CreatePageTable(), add code to initialize MapMask to
> MAX_UINT64. When creating new page table or map non-present range
> to present, all attributes should be provided.
>
> 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>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
> b/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
> index 7cf91ed9c4..f20068152b 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
> @@ -36,10 +36,7 @@ CreatePageTable (
> MapAttribute.Uint64 = Address;
> MapAttribute.Bits.Present = 1;
> MapAttribute.Bits.ReadWrite = 1;
> -
> - MapMask.Bits.PageTableBaseAddress = 1;
> - MapMask.Bits.Present = 1;
> - MapMask.Bits.ReadWrite = 1;
> + MapMask.Uint64 = MAX_UINT64;
>
> PageTable = 0;
> PageTableBufferSize = 0;
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr
2023-03-23 7:40 ` [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr duntan
@ 2023-03-23 12:52 ` Ni, Ray
2023-03-24 1:55 ` duntan
0 siblings, 1 reply; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 12:52 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
The commit message is very detailed.
But the function header comments are not.
Can you please update the function header comments to add 4 lines of
@retval RETURN_INVALID_PARAMETER, each line describes one invalid case.
It's ok to have multiple lines starting with @retval RETURN_INVALID_PARAMETER>
> + // If (LinearAddress + Length - 1) is not in the same ParentPagingEntry
> with (LinearAddress + Offset), then the remaining child PagingEntry
> + // starting from PagingEntryIndex of ParentPagingEntry is all covered by
> [LinearAddress + Offset, LinearAddress + Length - 1].
> + //
> + PagingEntryIndexEnd = (BitFieldRead64 (LinearAddress + Length - 1,
> BitStart + 9, 63) != BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ?
> 511 :
> + (UINTN)BitFieldRead64 (LinearAddress + Length - 1, BitStart,
> BitStart + 9 - 1);
> + PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> + for (Index = PagingEntryIndex; Index <= PagingEntryIndexEnd; Index++) {
> + if (PagingEntry[Index].Pce.Present == 0) {
> + //
> + // [LinearAddress, LinearAddress + Length] contains non-present range.
> + //
> + Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute,
> Mask);
> + if (RETURN_ERROR (Status)) {
> + return Status;
> + }
> +
> + break;
Why break out of the loop when meeting a non-present entry?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 09/21] UefiCpuPkg/CpuPageTableLib: Add manual test to check Mask and Attr
2023-03-23 7:40 ` [Patch V4 09/21] UefiCpuPkg/CpuPageTableLib: Add manual test to check " duntan
@ 2023-03-23 12:57 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 12:57 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
> + PageTableBufferSize = 0;
> + PageTable = 0;
> + Buffer = NULL;
> + MapAttribute.Uint64 = 0;
> + MapAttribute.Bits.Present = 1;
> + MapMask.Uint64 = 0;
> + MapMask.Bits.Present = 1;
> + //
> + // Create Page table to cover [0, 2G]. All fields of MapMask should be set.
> + //
> + Status = PageTableMap (&PageTable, PagingMode, Buffer,
> &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
> + UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
> + MapMask.Uint64 = MAX_UINT64;
> + Status = PageTableMap (&PageTable, PagingMode, Buffer,
> &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
> + UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
> + Buffer = AllocatePages (EFI_SIZE_TO_PAGES (PageTableBufferSize));
> + Status = PageTableMap (&PageTable, PagingMode, Buffer,
> &PageTableBufferSize, 0, SIZE_2GB, &MapAttribute, &MapMask);
> + UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
> +
> + //
> + // Update Page table to set [2G - 8K, 2G] from present to non-present. All
> fields of MapMask except present should be set.
"should not be set"?
> +
> + //
> + // Update Page table to set [2G - 8K, 2G] as present and RW. All fields of
> MapMask should not be set.
"should be set"?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 10/21] UefiCpuPkg/CpuPageTableLib:Modify RandomBoolean() in RandomTest
2023-03-23 7:40 ` [Patch V4 10/21] UefiCpuPkg/CpuPageTableLib:Modify RandomBoolean() in RandomTest duntan
@ 2023-03-23 12:59 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 12:59 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V4 10/21] UefiCpuPkg/CpuPageTableLib:Modify
> RandomBoolean() in RandomTest
>
> Add an input parameter to control the probability of returning
> true. Change RandomBoolean() in RandomTest from 50% chance
> returning true to returning true with the percentage of input
> Probability.
>
> 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>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 43
> +++++++++++++++++++++----------------------
> 1 file changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> index 97a388ca1c..52eb9daa10 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> @@ -1,7 +1,7 @@
> /** @file
> Random test case for Unit tests of the CpuPageTableLib instance of the
> CpuPageTableLib class
>
> - Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
> @@ -81,22 +81,6 @@ LocalRandomBytes (
> }
> }
>
> -/**
> - Return a random boolean.
> -
> - @return boolean
> -**/
> -BOOLEAN
> -RandomBoolean (
> - VOID
> - )
> -{
> - BOOLEAN Value;
> -
> - LocalRandomBytes ((UINT8 *)&Value, sizeof (BOOLEAN));
> - return Value%2;
> -}
> -
> /**
> Return a 32bit random number.
>
> @@ -139,6 +123,21 @@ Random64 (
> return (UINT64)(Value % (Limit - Start + 1)) + Start;
> }
>
> +/**
> + Returns true with the percentage of input Probability.
> +
> + @param[in] Probability The percentage to return true.
> +
> + @return boolean
> +**/
> +BOOLEAN
> +RandomBoolean (
> + UINT8 Probability
> + )
> +{
> + return ((Probability > ((UINT8)Random64 (0, 100))) ? TRUE : FALSE);
> +}
> +
> /**
> Check if the Page table entry is valid
>
> @@ -178,7 +177,7 @@
> ValidateAndRandomeModifyPageTablePageTableEntry (
> UT_ASSERT_EQUAL ((PagingEntry->Uint64 &
> mValidMaskLeaf[Level].Uint64), PagingEntry->Uint64);
> }
>
> - if ((RandomNumber < 100) && RandomBoolean ()) {
> + if ((RandomNumber < 100) && RandomBoolean (50)) {
> RandomNumber++;
> if (Level == 1) {
> TempPhysicalBase = PagingEntry->Pte4K.Bits.PageTableBaseAddress;
> @@ -211,7 +210,7 @@
> ValidateAndRandomeModifyPageTablePageTableEntry (
> UT_ASSERT_EQUAL ((PagingEntry->Uint64 &
> mValidMaskNoLeaf[Level].Uint64), PagingEntry->Uint64);
> }
>
> - if ((RandomNumber < 100) && RandomBoolean ()) {
> + if ((RandomNumber < 100) && RandomBoolean (50)) {
> RandomNumber++;
> TempPhysicalBase = PagingEntry->Pnle.Bits.PageTableBaseAddress;
>
> @@ -299,7 +298,7 @@ GenerateSingleRandomMapEntry (
> //
> // use AlignedTable to avoid that a random number can be very hard to be
> 1G or 2M aligned
> //
> - if ((MapsIndex != 0) && (RandomBoolean ())) {
> + if ((MapsIndex != 0) && (RandomBoolean (50))) {
> FormerLinearAddress = MapEntrys->Maps[Random32 (0,
> (UINT32)MapsIndex-1)].LinearAddress;
> if (FormerLinearAddress < 2 * (UINT64)SIZE_1GB) {
> FormerLinearAddressBottom = 0;
> @@ -323,7 +322,7 @@ GenerateSingleRandomMapEntry (
> //
> MapEntrys->Maps[MapsIndex].Length = Random64 (0, MIN (MaxAddress -
> MapEntrys->Maps[MapsIndex].LinearAddress, 10 * (UINT64)SIZE_1GB)) &
> AlignedTable[Random32 (0, ARRAY_SIZE (AlignedTable) -1)];
>
> - if ((MapsIndex != 0) && (RandomBoolean ())) {
> + if ((MapsIndex != 0) && (RandomBoolean (50))) {
> MapEntrys->Maps[MapsIndex].Attribute.Uint64 = MapEntrys-
> >Maps[Random32 (0, (UINT32)MapsIndex-1)].Attribute.Uint64;
> MapEntrys->Maps[MapsIndex].Mask.Uint64 = MapEntrys-
> >Maps[Random32 (0, (UINT32)MapsIndex-1)].Mask.Uint64;
> } else {
> @@ -344,7 +343,7 @@ GenerateSingleRandomMapEntry (
> // Need to avoid such case when remove the Random option
> ONLY_ONE_ONE_MAPPING
> //
> MapEntrys->Maps[MapsIndex].Attribute.Bits.PageTableBaseAddress =
> (Random64 (0, (((UINT64)1)<<52) - 1) & AlignedTable[Random32 (0,
> ARRAY_SIZE (AlignedTable) -1)])>> 12;
> - if (RandomBoolean ()) {
> + if (RandomBoolean (50)) {
> MapEntrys->Maps[MapsIndex].Mask.Bits.PageTableBaseAddress = 0;
> }
> }
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 11/21] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr
2023-03-23 7:40 ` [Patch V4 11/21] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr duntan
@ 2023-03-23 13:11 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 13:11 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann, Liu, Zhiguang
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>;
> Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [Patch V4 11/21] UefiCpuPkg/CpuPageTableLib:Modify RandomTest
> to check Mask/Attr
>
> Modify RandomTest to check invalid input. When creating new page
> table or updating exsiting page table:
> 1.If set [LinearAddress, LinearAddress+Length] to non-preset, all
1. "non-present"
> + //
> + if (MapEntrys->Maps[MapsIndex].Length > 0) {
> + for (Index = 0; Index < MapCount; Index++) {
> + if ((PreviousAddress < Map[Index].LinearAddress) &&
> + (MapEntrys->Maps[MapsIndex].LinearAddress <
> Map[Index].LinearAddress) && (RangeLimit > PreviousAddress))
> + {
> + //
> + // MapEntrys->Maps[MapsIndex] contains not-present range in
> exsiting page table.
> + //
> + break;
> + }
> +
> + PreviousAddress = Map[Index].LinearAddress + Map[Index].Length;
> + }
> +
> + if (PreviousAddress < RangeLimit) {
2. can you use "if (Index < MapCount)"?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 13/21] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter.
2023-03-23 7:40 ` [Patch V4 13/21] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter duntan
@ 2023-03-23 13:20 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 13:20 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
> @@ -290,7 +291,8 @@ PageTableLibMapInLevel (
> IN UINT64 Length,
> IN UINT64 Offset,
> IN IA32_MAP_ATTRIBUTE *Attribute,
> - IN IA32_MAP_ATTRIBUTE *Mask
> + IN IA32_MAP_ATTRIBUTE *Mask,
> + OUT BOOLEAN *IsModified
> )
> {
> RETURN_STATUS Status;
> @@ -316,6 +318,8 @@ PageTableLibMapInLevel (
> IA32_MAP_ATTRIBUTE LocalParentAttribute;
> UINT64 PhysicalAddrInEntry;
> UINT64 PhysicalAddrInAttr;
> + IA32_PAGING_ENTRY OriginalParentPagingEntry;
> + IA32_PAGING_ENTRY OriginalCurrentPagingEntry;
>
> ASSERT (Level != 0);
> ASSERT ((Attribute != NULL) && (Mask != NULL));
> @@ -328,8 +332,9 @@ PageTableLibMapInLevel (
> NopAttribute.Bits.ReadWrite = 1;
> NopAttribute.Bits.UserSupervisor = 1;
>
> - LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
> - ParentAttribute = &LocalParentAttribute;
> + LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
> + ParentAttribute = &LocalParentAttribute;
> + OriginalParentPagingEntry.Uint64 = ParentPagingEntry->Uint64;
1. can you add a blank line before "OriginalParentPagingEntry" assignment?
This helps to tell that the new added line has nothing to do with the original two lines code
that changes "ParentAttribute".
>
> + if (IsModified != NULL) {
> + *IsModified = FALSE;
> + }
2. can you use code as below?
If (IsModified == NULL) {
IsModified = LocalIsModified;
}
Then all code can just change "*IsModified" without checking if it's NULL.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 14/21] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified
2023-03-23 7:40 ` [Patch V4 14/21] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified duntan
@ 2023-03-23 13:23 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 13:23 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann, Liu, Zhiguang
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>;
> Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [Patch V4 14/21] UefiCpuPkg/CpuPageTableLib: Modify RandomTest
> to check IsModified
>
> Modify RandomTest to check if parameter IsModified of
> PageTableMap() correctlly indicates whether input page table
> is modified or not.
>
> 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>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 45
> +++++++++++++++++++++++++++++++++------------
> 1 file changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> index 816fd7b446..6f1485976a 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> @@ -636,6 +636,8 @@ SingleMapEntryTest (
> VOID *Buffer;
> IA32_MAP_ENTRY *Map;
> UINTN MapCount;
> + IA32_MAP_ENTRY *Map2;
> + UINTN MapCount2;
> UINTN Index;
> UINTN KeyPointCount;
> UINTN NewKeyPointCount;
> @@ -648,11 +650,13 @@ SingleMapEntryTest (
> UINT64 PreviousAddress;
> UINT64 RangeLimit;
> BOOLEAN IsNotPresent;
> + BOOLEAN IsModified;
>
> MapsIndex = MapEntrys->Count;
> MapCount = 0;
> PreviousAddress = 0;
> IsNotPresent = FALSE;
> + IsModified = FALSE;
>
> GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
> RangeLimit = MapEntrys->Maps[MapsIndex].LinearAddress + MapEntrys-
> >Maps[MapsIndex].Length;
> @@ -697,7 +701,7 @@ SingleMapEntryTest (
> MapEntrys->Maps[MapsIndex].Length,
> &MapEntrys->Maps[MapsIndex].Attribute,
> &MapEntrys->Maps[MapsIndex].Mask,
> - NULL
> + &IsModified
> );
>
> Attribute = &MapEntrys->Maps[MapsIndex].Attribute;
> @@ -758,7 +762,7 @@ SingleMapEntryTest (
> MapEntrys->Maps[MapsIndex].Length,
> &MapEntrys->Maps[MapsIndex].Attribute,
> &MapEntrys->Maps[MapsIndex].Mask,
> - NULL
> + &IsModified
> );
> }
>
> @@ -772,18 +776,31 @@ SingleMapEntryTest (
> return TestStatus;
> }
>
> - MapCount = 0;
> - Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
> - if (MapCount != 0) {
> + MapCount2 = 0;
> + Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount2);
> + if (MapCount2 != 0) {
> UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
>
> //
> - // Allocate memory for Maps
> + // Allocate memory for Map2
> // Note the memory is only used in this one Single MapEntry Test
> //
> - Map = AllocatePages (EFI_SIZE_TO_PAGES (MapCount * sizeof
> (IA32_MAP_ENTRY)));
> - ASSERT (Map != NULL);
> - Status = PageTableParse (*PageTable, PagingMode, Map, &MapCount);
> + Map2 = AllocatePages (EFI_SIZE_TO_PAGES (MapCount2 * sizeof
> (IA32_MAP_ENTRY)));
> + ASSERT (Map2 != NULL);
> + Status = PageTableParse (*PageTable, PagingMode, Map2, &MapCount2);
> + }
> +
> + //
> + // Check if PageTable has been modified.
> + //
> + if (MapCount2 != MapCount) {
> + UT_ASSERT_EQUAL (IsModified, TRUE);
> + } else {
> + if (CompareMem (Map, Map2, MapCount2 * sizeof
> (IA32_MAP_ENTRY)) != 0) {
> + UT_ASSERT_EQUAL (IsModified, TRUE);
> + } else {
> + UT_ASSERT_EQUAL (IsModified, FALSE);
> + }
> }
>
> UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
> @@ -793,17 +810,17 @@ SingleMapEntryTest (
> // Note the memory is only used in this one Single MapEntry Test
> //
> KeyPointCount = 0;
> - GetKeyPointList (MapEntrys, Map, MapCount, NULL, &KeyPointCount);
> + GetKeyPointList (MapEntrys, Map2, MapCount2, NULL, &KeyPointCount);
> KeyPointBuffer = AllocatePages (EFI_SIZE_TO_PAGES (KeyPointCount *
> sizeof (UINT64)));
> ASSERT (KeyPointBuffer != NULL);
> NewKeyPointCount = 0;
> - GetKeyPointList (MapEntrys, Map, MapCount, KeyPointBuffer,
> &NewKeyPointCount);
> + GetKeyPointList (MapEntrys, Map2, MapCount2, KeyPointBuffer,
> &NewKeyPointCount);
>
> //
> // Compare all key point's attribute
> //
> for (Index = 0; Index < NewKeyPointCount; Index++) {
> - if (!CompareEntrysforOnePoint (KeyPointBuffer[Index], MapEntrys, Map,
> MapCount, InitMap, InitMapCount)) {
> + if (!CompareEntrysforOnePoint (KeyPointBuffer[Index], MapEntrys,
> Map2, MapCount2, InitMap, InitMapCount)) {
> DEBUG ((DEBUG_INFO, "Error happens at below key point\n"));
> DEBUG ((DEBUG_INFO, "Index = %d KeyPointBuffer[Index] = 0x%lx\n",
> Index, KeyPointBuffer[Index]));
> Value = GetEntryFromPageTable (*PageTable, PagingMode,
> KeyPointBuffer[Index], &Level);
> @@ -817,6 +834,10 @@ SingleMapEntryTest (
> FreePages (Map, EFI_SIZE_TO_PAGES (MapCount * sizeof
> (IA32_MAP_ENTRY)));
> }
>
> + if (MapCount2 != 0) {
> + FreePages (Map2, EFI_SIZE_TO_PAGES (MapCount2 * sizeof
> (IA32_MAP_ENTRY)));
> + }
> +
> return UNIT_TEST_PASSED;
> }
>
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 15/21] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf
2023-03-23 7:40 ` [Patch V4 15/21] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf duntan
@ 2023-03-23 13:25 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 13:25 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Liu, Zhiguang, Dong, Eric, Kumar, Rahul R
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: [Patch V4 15/21] UefiCpuPkg: Fix IA32 build failure in
> CpuPageTableLib.inf
>
> From: Zhiguang Liu <zhiguang.liu@intel.com>
>
> The definition of IA32_MAP_ATTRIBUTE has 64 bits, and one of the bit
> field PageTableBaseAddress is from bit 12 to bit 52. This means if the
> compiler treats the 64bits value as two UINT32 value, the field
> PageTableBaseAddress spans two UINT32 value. That's why when building in
> NOOPT mode in IA32, the below issue is noticed:
> unresolved external symbol __allshl
> This patch fix the build failure by seperate field PageTableBaseAddress
> into two fields, make sure no field spans two UINT32 value.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> ---
> UefiCpuPkg/Include/Library/CpuPageTableLib.h | 32
> ++++++++++++++++----------------
> UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h | 125
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++--------------------------------------------------------------
> UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 22
> +++++++++++-----------
> 3 files changed, 90 insertions(+), 89 deletions(-)
>
> diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> index c94d82ea65..5e545a35f6 100644
> --- a/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> +++ b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
> @@ -11,22 +11,22 @@
>
> typedef union {
> struct {
> - UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in
> memory
> - UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> - UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
> - UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through
> caching
> - UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> - UINT64 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
> - UINT64 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
> - UINT64 Pat : 1; // PAT
> -
> - UINT64 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
> - UINT64 Reserved1 : 3; // Ignored
> -
> - UINT64 PageTableBaseAddress : 40; // Page Table Base Address
> - UINT64 Reserved2 : 7; // Ignored
> - UINT64 ProtectionKey : 4; // Protection key
> - UINT64 Nx : 1; // No Execute bit
> + UINT32 Present : 1; // 0 = Not present in memory, 1 = Present
> in memory
> + UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> + UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
> + UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-
> Through caching
> + UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> + UINT32 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by
> CPU)
> + UINT32 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
> + UINT32 Pat : 1; // PAT
> + UINT32 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
> + UINT32 Reserved1 : 3; // Ignored
> + UINT32 PageTableBaseAddressLow : 20; // Page Table Base Address
> Low
> +
> + UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address
> High
> + UINT32 Reserved2 : 7; // Ignored
> + UINT32 ProtectionKey : 4; // Protection key
> + UINT32 Nx : 1; // No Execute bit
> } Bits;
> UINT64 Uint64;
> } IA32_MAP_ATTRIBUTE;
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
> index 8d856d7c7e..2c67ecb469 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h
> @@ -29,11 +29,12 @@ typedef enum {
> } IA32_PAGE_LEVEL;
>
> typedef struct {
> - UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in
> memory
> - UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> - UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
> - UINT64 Reserved : 58;
> - UINT64 Nx : 1; // No Execute bit
> + UINT32 Present : 1; // 0 = Not present in memory, 1 = Present in
> memory
> + UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> + UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
> + UINT32 Reserved0 : 29;
> + UINT32 Reserved1 : 31;
> + UINT32 Nx : 1; // No Execute bit
> } IA32_PAGE_COMMON_ENTRY;
>
> ///
> @@ -41,20 +42,20 @@ typedef struct {
> ///
> typedef union {
> struct {
> - UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in
> memory
> - UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> - UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
> - UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through
> caching
> - UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> - UINT64 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
> - UINT64 Available0 : 1; // Ignored
> - UINT64 MustBeZero : 1; // Must Be Zero
> -
> - UINT64 Available2 : 4; // Ignored
> -
> - UINT64 PageTableBaseAddress : 40; // Page Table Base Address
> - UINT64 Available3 : 11; // Ignored
> - UINT64 Nx : 1; // No Execute bit
> + UINT32 Present : 1; // 0 = Not present in memory, 1 = Present
> in memory
> + UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> + UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
> + UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-
> Through caching
> + UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> + UINT32 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by
> CPU)
> + UINT32 Available0 : 1; // Ignored
> + UINT32 MustBeZero : 1; // Must Be Zero
> + UINT32 Available2 : 4; // Ignored
> + UINT32 PageTableBaseAddressLow : 20; // Page Table Base Address
> Low
> +
> + UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address
> High
> + UINT32 Available3 : 11; // Ignored
> + UINT32 Nx : 1; // No Execute bit
> } Bits;
> UINT64 Uint64;
> } IA32_PAGE_NON_LEAF_ENTRY;
> @@ -86,23 +87,23 @@ typedef IA32_PAGE_NON_LEAF_ENTRY IA32_PDE;
> ///
> typedef union {
> struct {
> - UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in
> memory
> - UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> - UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
> - UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through
> caching
> - UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> - UINT64 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
> - UINT64 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
> - UINT64 MustBeOne : 1; // Page Size. Must Be One
> -
> - UINT64 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
> - UINT64 Available1 : 3; // Ignored
> - UINT64 Pat : 1; // PAT
> -
> - UINT64 PageTableBaseAddress : 39; // Page Table Base Address
> - UINT64 Available3 : 7; // Ignored
> - UINT64 ProtectionKey : 4; // Protection key
> - UINT64 Nx : 1; // No Execute bit
> + UINT32 Present : 1; // 0 = Not present in memory, 1 = Present
> in memory
> + UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> + UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
> + UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-
> Through caching
> + UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> + UINT32 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by
> CPU)
> + UINT32 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
> + UINT32 MustBeOne : 1; // Page Size. Must Be One
> + UINT32 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
> + UINT32 Available1 : 3; // Ignored
> + UINT32 Pat : 1; // PAT
> + UINT32 PageTableBaseAddressLow : 19; // Page Table Base Address
> Low
> +
> + UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address
> High
> + UINT32 Available3 : 7; // Ignored
> + UINT32 ProtectionKey : 4; // Protection key
> + UINT32 Nx : 1; // No Execute bit
> } Bits;
> UINT64 Uint64;
> } IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE;
> @@ -123,22 +124,22 @@ typedef IA32_PAGE_LEAF_ENTRY_BIG_PAGESIZE
> IA32_PDPTE_1G;
> ///
> typedef union {
> struct {
> - UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in
> memory
> - UINT64 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> - UINT64 UserSupervisor : 1; // 0 = Supervisor, 1=User
> - UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through
> caching
> - UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> - UINT64 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by CPU)
> - UINT64 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
> - UINT64 Pat : 1; // PAT
> -
> - UINT64 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
> - UINT64 Available1 : 3; // Ignored
> -
> - UINT64 PageTableBaseAddress : 40; // Page Table Base Address
> - UINT64 Available3 : 7; // Ignored
> - UINT64 ProtectionKey : 4; // Protection key
> - UINT64 Nx : 1; // No Execute bit
> + UINT32 Present : 1; // 0 = Not present in memory, 1 = Present
> in memory
> + UINT32 ReadWrite : 1; // 0 = Read-Only, 1= Read/Write
> + UINT32 UserSupervisor : 1; // 0 = Supervisor, 1=User
> + UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-
> Through caching
> + UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> + UINT32 Accessed : 1; // 0 = Not accessed, 1 = Accessed (set by
> CPU)
> + UINT32 Dirty : 1; // 0 = Not dirty, 1 = Dirty (set by CPU)
> + UINT32 Pat : 1; // PAT
> + UINT32 Global : 1; // 0 = Not global, 1 = Global (if CR4.PGE = 1)
> + UINT32 Available1 : 3; // Ignored
> + UINT32 PageTableBaseAddressLow : 20; // Page Table Base Address
> Low
> +
> + UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address
> High
> + UINT32 Available3 : 7; // Ignored
> + UINT32 ProtectionKey : 4; // Protection key
> + UINT32 Nx : 1; // No Execute bit
> } Bits;
> UINT64 Uint64;
> } IA32_PTE_4K;
> @@ -149,16 +150,16 @@ typedef union {
> ///
> typedef union {
> struct {
> - UINT64 Present : 1; // 0 = Not present in memory, 1 = Present in
> memory
> - UINT64 MustBeZero : 2; // Must Be Zero
> - UINT64 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-Through
> caching
> - UINT64 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> - UINT64 MustBeZero2 : 4; // Must Be Zero
> -
> - UINT64 Available : 3; // Ignored
> -
> - UINT64 PageTableBaseAddress : 40; // Page Table Base Address
> - UINT64 MustBeZero3 : 12; // Must Be Zero
> + UINT32 Present : 1; // 0 = Not present in memory, 1 = Present
> in memory
> + UINT32 MustBeZero : 2; // Must Be Zero
> + UINT32 WriteThrough : 1; // 0 = Write-Back caching, 1=Write-
> Through caching
> + UINT32 CacheDisabled : 1; // 0 = Cached, 1=Non-Cached
> + UINT32 MustBeZero2 : 4; // Must Be Zero
> + UINT32 Available : 3; // Ignored
> + UINT32 PageTableBaseAddressLow : 20; // Page Table Base Address
> Low
> +
> + UINT32 PageTableBaseAddressHigh : 20; // Page Table Base Address
> High
> + UINT32 MustBeZero3 : 12; // Must Be Zero
> } Bits;
> UINT64 Uint64;
> } IA32_PDPTE_PAE;
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 797fc2f600..982652b58b 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -26,7 +26,7 @@ PageTableLibSetPte4K (
> IN IA32_MAP_ATTRIBUTE *Mask
> )
> {
> - if (Mask->Bits.PageTableBaseAddress) {
> + if (Mask->Bits.PageTableBaseAddressLow || Mask-
> >Bits.PageTableBaseAddressHigh) {
> Pte4K->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS
> (Attribute) + Offset) | (Pte4K->Uint64 &
> ~IA32_PE_BASE_ADDRESS_MASK_40);
> }
>
> @@ -93,7 +93,7 @@ PageTableLibSetPleB (
> IN IA32_MAP_ATTRIBUTE *Mask
> )
> {
> - if (Mask->Bits.PageTableBaseAddress) {
> + if (Mask->Bits.PageTableBaseAddressLow || Mask-
> >Bits.PageTableBaseAddressHigh) {
> PleB->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS
> (Attribute) + Offset) | (PleB->Uint64 &
> ~IA32_PE_BASE_ADDRESS_MASK_39);
> }
>
> @@ -238,7 +238,7 @@ IsAttributesAndMaskValidForNonPresentEntry (
> //
> if ((Mask->Bits.ReadWrite == 0) || (Mask->Bits.UserSupervisor == 0) ||
> (Mask->Bits.WriteThrough == 0) || (Mask->Bits.CacheDisabled == 0) ||
> (Mask->Bits.Accessed == 0) || (Mask->Bits.Dirty == 0) || (Mask-
> >Bits.Pat == 0) || (Mask->Bits.Global == 0) ||
> - (Mask->Bits.PageTableBaseAddress == 0) || (Mask->Bits.ProtectionKey
> == 0) || (Mask->Bits.Nx == 0))
> + ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask-
> >Bits.PageTableBaseAddressHigh == 0)) || (Mask->Bits.ProtectionKey == 0)
> || (Mask->Bits.Nx == 0))
> {
> return RETURN_INVALID_PARAMETER;
> }
> @@ -396,7 +396,7 @@ PageTableLibMapInLevel (
> // This function is called when the memory length is less than the region
> length of the parent level.
> // No need to split the page when the attributes equal.
> //
> - if (Mask->Bits.PageTableBaseAddress == 0) {
> + if ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask-
> >Bits.PageTableBaseAddressHigh == 0)) {
> return RETURN_SUCCESS;
> }
>
> @@ -696,7 +696,7 @@ PageTableMap (
> return RETURN_INVALID_PARAMETER;
> }
>
> - if ((LinearAddress % SIZE_4KB != 0) || (Length % SIZE_4KB != 0)) {
> + if (((UINTN)LinearAddress % SIZE_4KB != 0) || ((UINTN)Length %
> SIZE_4KB != 0)) {
> //
> // LinearAddress and Length should be multiple of 4K.
> //
> @@ -738,12 +738,12 @@ PageTableMap (
> *IsModified = FALSE;
> }
>
> - ParentAttribute.Uint64 = 0;
> - ParentAttribute.Bits.PageTableBaseAddress = 1;
> - ParentAttribute.Bits.Present = 1;
> - ParentAttribute.Bits.ReadWrite = 1;
> - ParentAttribute.Bits.UserSupervisor = 1;
> - ParentAttribute.Bits.Nx = 0;
> + ParentAttribute.Uint64 = 0;
> + ParentAttribute.Bits.PageTableBaseAddressLow = 1;
> + ParentAttribute.Bits.Present = 1;
> + ParentAttribute.Bits.ReadWrite = 1;
> + ParentAttribute.Bits.UserSupervisor = 1;
> + ParentAttribute.Bits.Nx = 0;
>
> //
> // Query the required buffer size without modifying the page table.
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 17/21] UefiCpuPkg/CpuPageTableLib: Add check for page table creation
2023-03-23 7:40 ` [Patch V4 17/21] UefiCpuPkg/CpuPageTableLib: Add check for page table creation duntan
@ 2023-03-23 13:29 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 13:29 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Reviewed-by: Ray Ni <ray.ni@Intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V4 17/21] UefiCpuPkg/CpuPageTableLib: Add check for page
> table creation
>
> Add code to compare ParentPagingEntry Attribute&Mask and input
> Attribute&Mask to decide if new next level page table is needed
> in non-present ParentPagingEntry condition. This can help avoid
> unneccessary page table creation.
>
> For example, there is a page table in which [0, 1G] is mapped(Lv4[0]
> ,Lv3[0,0], a non-leaf level4 entry and a leaf level3 entry).And we
> only want to map [1G, 1G+2M] linear address still as non-present.
> The expected behaviour should be nothing happens in the process.
> However, previous code logic doesn't check if ParentPagingEntry
> Attribute&Mask and input Attribute&Mask are the same in non-present
> ParentPagingEntry condition. Then a new 4K memory is allocated for
> Lv2 since 1G+2M is not 1G-aligned.
> So when ParentPagingEntry is non-present, before allocate 4K memory
> for next level paging, we also check if ParentPagingEntry Attribute&
> Mask and input Attribute&Mask are the same.
>
> 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>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
> UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 10
> ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 982652b58b..55a756ad90 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -358,6 +358,16 @@ PageTableLibMapInLevel (
> return Status;
> }
>
> + //
> + // Check the attribute in ParentPagingEntry is equal to attribute calculated
> by input Attribue and Mask.
> + //
> + PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute
> (&ParentPagingEntry->PleB, ParentAttribute);
> + if ((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
> + == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) &
> IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))
> + {
> + return RETURN_SUCCESS;
> + }
> +
> //
> // The parent entry is CR3 or PML5E/PML4E/PDPTE/PDE.
> // It does NOT point to an existing page directory.
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 18/21] UefiCpuPkg: Combine branch for non-present and leaf ParentEntry
2023-03-23 7:40 ` [Patch V4 18/21] UefiCpuPkg: Combine branch for non-present and leaf ParentEntry duntan
@ 2023-03-23 13:32 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 13:32 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
> - //
> - // The parent entry is CR3 or PML5E/PML4E/PDPTE/PDE.
> + // When ParentPagingEntry is non-present, parent entry is CR3 or
> PML5E/PML4E/PDPTE/PDE.
> // It does NOT point to an existing page directory.
> + // When ParentPagingEntry is present, parent entry is leaf PDPTE_1G or
> PDE_2M. Split to 2M or 4K pages.
> + // Note: it's impossible the parent entry is a PTE_4K.
> //
> - ASSERT (Buffer == NULL || *BufferSize >= SIZE_4KB);
> - CreateNew = TRUE;
> - *BufferSize -= SIZE_4KB;
> + OneOfPagingEntry.Pnle.Uint64 = 0;
> + PleBAttribute.Uint64 = PageTableLibGetPleBMapAttribute
> (&ParentPagingEntry->PleB, ParentAttribute);
>
> - if (Modify) {
> - ParentPagingEntry->Uintn = (UINTN)Buffer + *BufferSize;
> - ZeroMem ((VOID *)ParentPagingEntry->Uintn, SIZE_4KB);
> + if (ParentPagingEntry->Pce.Present == 0) {
> //
> - // Set default attribute bits for PML5E/PML4E/PDPTE/PDE.
> + // [LinearAddress, LinearAddress + Length] contains non-present range.
> //
> - PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute,
> &AllOneMask);
> + Status = IsAttributesAndMaskValidForNonPresentEntry (Attribute,
> Mask);
> + if (RETURN_ERROR (Status)) {
> + return Status;
> + }
1. do you think put " OneOfPagingEntry.Pnle.Uint64 = 0;" in the if-body is better?
So both if and else initializes OneOfPagingEntry.
Other logic looks good to me.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 19/21] UefiCpuPkg/CpuPageTableLib: Enable PAE paging
2023-03-23 7:40 ` [Patch V4 19/21] UefiCpuPkg/CpuPageTableLib: Enable PAE paging duntan
@ 2023-03-23 13:38 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 13:38 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Overall logics look good to me.
2 minor comments.
> -#define IA32_PE_BASE_ADDRESS_MASK_40 0xFFFFFFFFFF000ull
> -#define IA32_PE_BASE_ADDRESS_MASK_39 0xFFFFFFFFFE000ull
> +#define IA32_PE_BASE_ADDRESS_MASK_40 0xFFFFFFFFFF000ull
> +#define IA32_PE_BASE_ADDRESS_MASK_39 0xFFFFFFFFFE000ull
> +#define IA32_PE_BASE_ADDRESS_MASK_PAE_PDPTE 0xFFFFFFFFFFFE0ull
1. This macro is not used.
>
> #define REGION_LENGTH(l) LShiftU64 (1, (l) * 9 + 3)
>
> +#define MAX_PAE_PDPTE_NUM 4
> +
> typedef enum {
> Pte = 1,
> Pde = 2,
> @@ -60,7 +63,8 @@ typedef union {
> UINT64 Uint64;
> } IA32_PAGE_NON_LEAF_ENTRY;
>
> -#define IA32_PNLE_PAGE_TABLE_BASE_ADDRESS(pa) ((pa)->Uint64 &
> IA32_PE_BASE_ADDRESS_MASK_40)
> +#define IA32_PNLE_PAGE_TABLE_BASE_ADDRESS(pa) ((pa)->Uint64 &
> IA32_PE_BASE_ADDRESS_MASK_40)
> +#define IA32_PAE_PDPTE_PAGE_TABLE_BASE_ADDRESS(pa) ((pa)->Uint64
> & IA32_PE_BASE_ADDRESS_MASK_PAE_PDPTE)
2. this macro is not used.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 20/21] UefiCpuPkg/CpuPageTableLib: Add RandomTest for PAE paging
2023-03-23 7:40 ` [Patch V4 20/21] UefiCpuPkg/CpuPageTableLib: Add RandomTest for " duntan
@ 2023-03-23 13:40 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 13:40 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V4 20/21] UefiCpuPkg/CpuPageTableLib: Add RandomTest
> for PAE paging
>
> Add RandomTest for PAE paging.
>
> 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>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>
> UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHo
> st.c | 2 ++
> UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c | 3
> +--
> UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c | 12
> ++++++++----
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> index e1efc84c82..8554eefa39 100644
> ---
> a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> +++
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> @@ -9,6 +9,7 @@
> #include "CpuPageTableLibUnitTest.h"
>
> // ----------------------------------------------------------------------- PageMode--
> TestCount-TestRangeCount---RandomOptions
> +static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPagingPae = { PagingPae, 100, 20, USE_RANDOM_ARRAY };
> static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging4Level = { Paging4Level, 100, 20,
> USE_RANDOM_ARRAY };
> static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging4Level1GB = { Paging4Level1GB, 100, 20,
> USE_RANDOM_ARRAY };
> static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging5Level = { Paging5Level, 100, 20,
> USE_RANDOM_ARRAY };
> @@ -880,6 +881,7 @@ UefiTestMain (
> goto EXIT;
> }
>
> + AddTestCase (RandomTestCase, "Random Test for PagingPae", "Random
> Test Case1", TestCaseforRandomTest, NULL, NULL,
> &mTestContextPagingPae);
> AddTestCase (RandomTestCase, "Random Test for Paging4Level", "Random
> Test Case1", TestCaseforRandomTest, NULL, NULL,
> &mTestContextPaging4Level);
> AddTestCase (RandomTestCase, "Random Test for Paging4Level1G",
> "Random Test Case2", TestCaseforRandomTest, NULL, NULL,
> &mTestContextPaging4Level1GB);
> AddTestCase (RandomTestCase, "Random Test for Paging5Level", "Random
> Test Case3", TestCaseforRandomTest, NULL, NULL,
> &mTestContextPaging5Level);
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> index 18a5010c30..7e79b01823 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> @@ -258,10 +258,9 @@ ValidateAndRandomeModifyPageTable (
> UNIT_TEST_STATUS Status;
> IA32_PAGING_ENTRY *PagingEntry;
>
> - if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) ||
> (PagingMode >= PagingModeMax)) {
> + if ((PagingMode == Paging32bit) || (PagingMode >= PagingModeMax)) {
> //
> // 32bit paging is never supported.
> - // PAE paging will be supported later.
> //
> return UNIT_TEST_ERROR_TEST_FAILED;
> }
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> index 22f179c21f..67776255c2 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> @@ -175,10 +175,9 @@ IsPageTableValid (
> return UNIT_TEST_PASSED;
> }
>
> - if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) ||
> (PagingMode >= PagingModeMax)) {
> + if ((PagingMode == Paging32bit) || (PagingMode >= PagingModeMax)) {
> //
> // 32bit paging is never supported.
> - // PAE paging will be supported later.
> //
> return UNIT_TEST_ERROR_TEST_FAILED;
> }
> @@ -187,7 +186,12 @@ IsPageTableValid (
> MaxLevel = (UINT8)(PagingMode >> 8);
>
> PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)PageTable;
> - for (Index = 0; Index < 512; Index++) {
> + for (Index = 0; Index < ((PagingMode == PagingPae) ? 4 : 512); Index++) {
> + if (PagingMode == PagingPae) {
> + UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero, 0);
> + UT_ASSERT_EQUAL (PagingEntry[Index].PdptePae.Bits.MustBeZero2, 0);
> + }
> +
> Status = IsPageTableEntryValid (&PagingEntry[Index], MaxLevel,
> MaxLeafLevel, Index << (9 * MaxLevel + 3));
> if (Status != UNIT_TEST_PASSED) {
> return Status;
> @@ -264,7 +268,7 @@ GetEntryFromPageTable (
> UINT64 Index;
> IA32_PAGING_ENTRY *PagingEntry;
>
> - if ((PagingMode == Paging32bit) || (PagingMode == PagingPae) ||
> (PagingMode >= PagingModeMax)) {
> + if ((PagingMode == Paging32bit) || (PagingMode >= PagingModeMax)) {
> //
> // 32bit paging is never supported.
> // PAE paging will be supported later.
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 21/21] UefiCpuPkg/CpuPageTableLib: Reduce the number of random tests
2023-03-23 7:40 ` [Patch V4 21/21] UefiCpuPkg/CpuPageTableLib: Reduce the number of random tests duntan
@ 2023-03-23 13:40 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-23 13:40 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, March 23, 2023 3:41 PM
> 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>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [Patch V4 21/21] UefiCpuPkg/CpuPageTableLib: Reduce the number
> of random tests
>
> Reduce the number of random tests. In previous patch, non-1:1
> mapping is enbaled and it may need more than an hour and a half
> for the CI test, which may lead to CI timeout. Reduce the number
> of random test count to pass the CI.
>
> 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>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> ---
>
> UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHo
> st.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> index 8554eefa39..c60302eeac 100644
> ---
> a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> +++
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> @@ -9,11 +9,11 @@
> #include "CpuPageTableLibUnitTest.h"
>
> // ----------------------------------------------------------------------- PageMode--
> TestCount-TestRangeCount---RandomOptions
> -static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPagingPae = { PagingPae, 100, 20, USE_RANDOM_ARRAY };
> -static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging4Level = { Paging4Level, 100, 20,
> USE_RANDOM_ARRAY };
> -static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging4Level1GB = { Paging4Level1GB, 100, 20,
> USE_RANDOM_ARRAY };
> -static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging5Level = { Paging5Level, 100, 20,
> USE_RANDOM_ARRAY };
> -static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging5Level1GB = { Paging5Level1GB, 100, 20,
> USE_RANDOM_ARRAY };
> +static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPagingPae = { PagingPae, 30, 20, USE_RANDOM_ARRAY };
> +static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging4Level = { Paging4Level, 30, 20,
> USE_RANDOM_ARRAY };
> +static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging4Level1GB = { Paging4Level1GB, 30, 20,
> USE_RANDOM_ARRAY };
> +static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging5Level = { Paging5Level, 30, 20,
> USE_RANDOM_ARRAY };
> +static CPU_PAGE_TABLE_LIB_RANDOM_TEST_CONTEXT
> mTestContextPaging5Level1GB = { Paging5Level1GB, 30, 20,
> USE_RANDOM_ARRAY };
>
> /**
> Check if the input parameters are not supported.
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr
2023-03-23 12:52 ` Ni, Ray
@ 2023-03-24 1:55 ` duntan
0 siblings, 0 replies; 44+ messages in thread
From: duntan @ 2023-03-24 1:55 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann
Thanks for the comments Ray. I will update the patches in next version patch set.
-----Why break out of the loop when meeting a non-present entry?
Because I think the parameter check is only needed once.
Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Thursday, March 23, 2023 8:53 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: RE: [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr
The commit message is very detailed.
But the function header comments are not.
Can you please update the function header comments to add 4 lines of @retval RETURN_INVALID_PARAMETER, each line describes one invalid case.
It's ok to have multiple lines starting with @retval RETURN_INVALID_PARAMETER>
> + // If (LinearAddress + Length - 1) is not in the same
> + ParentPagingEntry
> with (LinearAddress + Offset), then the remaining child PagingEntry
> + // starting from PagingEntryIndex of ParentPagingEntry is all
> + covered by
> [LinearAddress + Offset, LinearAddress + Length - 1].
> + //
> + PagingEntryIndexEnd = (BitFieldRead64 (LinearAddress + Length -
> + 1,
> BitStart + 9, 63) != BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ?
> 511 :
> + (UINTN)BitFieldRead64 (LinearAddress +
> + Length - 1, BitStart,
> BitStart + 9 - 1);
> + PagingEntry = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> + for (Index = PagingEntryIndex; Index <= PagingEntryIndexEnd; Index++) {
> + if (PagingEntry[Index].Pce.Present == 0) {
> + //
> + // [LinearAddress, LinearAddress + Length] contains non-present range.
> + //
> + Status = IsAttributesAndMaskValidForNonPresentEntry
> + (Attribute,
> Mask);
> + if (RETURN_ERROR (Status)) {
> + return Status;
> + }
> +
> + break;
Why break out of the loop when meeting a non-present entry?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [edk2-devel] [Patch V4 06/21] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry
[not found] ` <174F0C7EF4E09C53.19937@groups.io>
@ 2023-03-24 2:19 ` Ni, Ray
0 siblings, 0 replies; 44+ messages in thread
From: Ni, Ray @ 2023-03-24 2:19 UTC (permalink / raw)
To: devel@edk2.groups.io, Ni, Ray
Non-leaf entry doesn't have PAT bit.
So "& (~IA32_PE_BASE_ADDRESS_MASK_40)" is to make sure PAT bit in the original big-leaf entry
is not assigned to the corresponding bit (bit 12) in non-leaf entry.
Can you please add comments to explain why?
With that, Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Thursday, March 23, 2023 8:35 PM
> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [Patch V4 06/21] UefiCpuPkg/CpuPageTableLib: Fix
> issue when splitting leaf entry
>
> > + ParentPagingEntry->Uint64 = ((UINTN)(VOID *)PagingEntry) |
> > (ParentPagingEntry->Uint64 & (~IA32_PE_BASE_ADDRESS_MASK_40));
>
> In this branch, ParentPagingEntry is a leaf entry covering 2M or 1G physical
> address range.
> Then, why not use IA32_PE_BASE_ADDRESS_MASK_39? Because for the two
> types leaf entry, address
> only occupies 39 bits, not 40bits.
>
>
>
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2023-03-24 2:19 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-23 7:40 [Patch V4 00/21] Fix issues in CpuPageTableLib duntan
2023-03-23 7:40 ` [Patch V4 01/21] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition duntan
2023-03-23 7:40 ` [Patch V4 02/21] UefiCpuPkg/CpuPageTableLib: Add check for input Length duntan
2023-03-23 12:20 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 03/21] UefiCpuPkg/CpuPageTableLib:Initialize some LocalVariable at beginning duntan
2023-03-23 12:26 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 04/21] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan
2023-03-23 12:29 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 05/21] UefiCpuPkg/CpuPageTableLib:Clear PageSize bit(Bit7) for non-leaf duntan
2023-03-23 12:29 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 06/21] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry duntan
2023-03-23 12:35 ` Ni, Ray
[not found] ` <174F0C7EF4E09C53.19937@groups.io>
2023-03-24 2:19 ` [edk2-devel] " Ni, Ray
2023-03-23 7:40 ` [Patch V4 07/21] UefiCpuPkg/MpInitLib: Add code to initialize MapMask duntan
2023-03-23 12:37 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 08/21] UefiCpuPkg/CpuPageTableLib:Add check for Mask and Attr duntan
2023-03-23 12:52 ` Ni, Ray
2023-03-24 1:55 ` duntan
2023-03-23 7:40 ` [Patch V4 09/21] UefiCpuPkg/CpuPageTableLib: Add manual test to check " duntan
2023-03-23 12:57 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 10/21] UefiCpuPkg/CpuPageTableLib:Modify RandomBoolean() in RandomTest duntan
2023-03-23 12:59 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 11/21] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr duntan
2023-03-23 13:11 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 12/21] UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test duntan
2023-03-23 7:40 ` [Patch V4 13/21] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter duntan
2023-03-23 13:20 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 14/21] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified duntan
2023-03-23 13:23 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 15/21] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf duntan
2023-03-23 13:25 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 16/21] UefiCpuPkg: Modify UnitTest code since tested API is changed duntan
2023-03-23 7:40 ` [Patch V4 17/21] UefiCpuPkg/CpuPageTableLib: Add check for page table creation duntan
2023-03-23 13:29 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 18/21] UefiCpuPkg: Combine branch for non-present and leaf ParentEntry duntan
2023-03-23 13:32 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 19/21] UefiCpuPkg/CpuPageTableLib: Enable PAE paging duntan
2023-03-23 13:38 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 20/21] UefiCpuPkg/CpuPageTableLib: Add RandomTest for " duntan
2023-03-23 13:40 ` Ni, Ray
2023-03-23 7:40 ` [Patch V4 21/21] UefiCpuPkg/CpuPageTableLib: Reduce the number of random tests duntan
2023-03-23 13:40 ` Ni, Ray
2023-03-23 9:13 ` [edk2-devel] [Patch V4 00/21] Fix issues in CpuPageTableLib Gerd Hoffmann
2023-03-23 9:16 ` duntan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox