* [PATCH 1/6] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition
2023-03-03 10:03 [PATCH 0/6] Fix issues in CpuPageTableLib duntan
@ 2023-03-03 10:03 ` duntan
2023-03-03 10:03 ` [PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: duntan @ 2023-03-03 10:03 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 check for (ParentPagingEntry->Pnle.Bits.Present == 0)
won't is always FALSE.
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 | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 37713ec659..47027917d9 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -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] 9+ messages in thread* [PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
2023-03-03 10:03 [PATCH 0/6] Fix issues in CpuPageTableLib duntan
2023-03-03 10:03 ` [PATCH 1/6] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition duntan
@ 2023-03-03 10:03 ` duntan
2023-03-06 7:13 ` [edk2-devel] " Gerd Hoffmann
2023-03-03 10:03 ` [PATCH 3/6] UefiCpuPkg/CpuPageTebleLib: Check input Mask in PageTableMap duntan
` (3 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: duntan @ 2023-03-03 10:03 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Fix the non-1:1 mapping issue in PageTableMap () of CpuPageTableLib
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 | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 47027917d9..d2f35aa375 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,13 @@ 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);
+
//
// ParentPagingEntry ONLY is deferenced for checking Present and MustBeOne bits
// when Modify is FALSE.
@@ -325,8 +333,11 @@ PageTableLibMapInLevel (
// the actual attributes of grand-parents when determing the memory type.
//
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)))
+ if ((((IA32_MAP_ATTRIBUTE_ATTRIBUTES (&PleBAttribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask))
+ == (IA32_MAP_ATTRIBUTE_ATTRIBUTES (Attribute) & IA32_MAP_ATTRIBUTE_ATTRIBUTES (Mask)))) &&
+ ( (Mask->Bits.PageTableBaseAddress == 0)
+ || ((IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&PleBAttribute) + PagingEntryIndex * RegionLength)
+ == (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset))))
{
//
// This function is called when the memory length is less than the region length of the parent level.
@@ -353,8 +364,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,14 +435,11 @@ 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;
+ RegionMask = RegionLength - 1;
+ RegionStart = (LinearAddress + Offset) & ~RegionMask;
ParentAttribute->Uint64 = PageTableLibGetPnleMapAttribute (&ParentPagingEntry->Pnle, ParentAttribute);
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [edk2-devel] [PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
2023-03-03 10:03 ` [PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan
@ 2023-03-06 7:13 ` Gerd Hoffmann
2023-03-08 10:11 ` duntan
0 siblings, 1 reply; 9+ messages in thread
From: Gerd Hoffmann @ 2023-03-06 7:13 UTC (permalink / raw)
To: devel, dun.tan; +Cc: Eric Dong, Ray Ni, Rahul Kumar
On Fri, Mar 03, 2023 at 06:03:32PM +0800, duntan wrote:
> Fix the non-1:1 mapping issue in PageTableMap () of CpuPageTableLib
This needs a more verbose commit message. It's not clear what the bug
is and how you are fixing it.
take care,
Gerd
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
2023-03-06 7:13 ` [edk2-devel] " Gerd Hoffmann
@ 2023-03-08 10:11 ` duntan
0 siblings, 0 replies; 9+ messages in thread
From: duntan @ 2023-03-08 10:11 UTC (permalink / raw)
To: devel@edk2.groups.io, kraxel@redhat.com
Cc: Dong, Eric, Ni, Ray, Kumar, Rahul R
Thanks for the comments Gerd. More detailed commit message has been added in V2 patch set.
Thanks,
Dun
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd Hoffmann
Sent: Monday, March 6, 2023 3:14 PM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
On Fri, Mar 03, 2023 at 06:03:32PM +0800, duntan wrote:
> Fix the non-1:1 mapping issue in PageTableMap () of CpuPageTableLib
This needs a more verbose commit message. It's not clear what the bug is and how you are fixing it.
take care,
Gerd
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/6] UefiCpuPkg/CpuPageTebleLib: Check input Mask in PageTableMap
2023-03-03 10:03 [PATCH 0/6] Fix issues in CpuPageTableLib duntan
2023-03-03 10:03 ` [PATCH 1/6] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition duntan
2023-03-03 10:03 ` [PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue duntan
@ 2023-03-03 10:03 ` duntan
2023-03-03 10:03 ` [PATCH 4/6] UefiCpuPkg/CpuPageTableLib: Add manual TestCase to check input Mask duntan
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: duntan @ 2023-03-03 10:03 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
When creating new page table or mapping not-present range in
existing page table, we need to make sure all the non-reserved
fields of input Mask are not 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 | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 49 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index d2f35aa375..21fdfb53c1 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -214,6 +214,28 @@ PageTableLibSetPnle (
Pnle->Bits.CacheDisabled = 0;
}
+/**
+ Check if any Non-Reserved field of Mask is 0. When creating new page table or mapping not-present
+ range, we need to make sure all the non-reserved fields of input Mask are not 0.
+
+ @param[in] Mask The mask used for attribute to check.
+**/
+RETURN_STATUS
+CheckMaskNonReservedBit (
+ IN IA32_MAP_ATTRIBUTE *Mask
+ )
+{
+ if ((Mask->Bits.Present == 0) || (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;
+ }
+
+ return RETURN_SUCCESS;
+}
+
/**
Update page table to map [LinearAddress, LinearAddress + Length) with specified attribute in the specified level.
@@ -259,6 +281,7 @@ PageTableLibMapInLevel (
UINTN Index;
IA32_PAGING_ENTRY *PagingEntry;
UINTN PagingEntryIndex;
+ UINTN PagingEntryIndexLimit;
IA32_PAGING_ENTRY *CurrentPagingEntry;
UINT64 RegionLength;
UINT64 SubLength;
@@ -302,6 +325,15 @@ PageTableLibMapInLevel (
//
if (ParentPagingEntry->Pce.Present == 0) {
+ //
+ // [LinearAddress, LinearAddress + Length] contains not-present range, we need to
+ // make sure all the non-reserved fields of Mask are not 0.
+ //
+ Status = CheckMaskNonReservedBit (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.
@@ -371,6 +403,23 @@ PageTableLibMapInLevel (
}
}
} else {
+ PagingEntry = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
+ PagingEntryIndexLimit = (UINTN)BitFieldRead64 (LinearAddress + Length - 1, BitStart, BitStart + 9 - 1);
+ for (Index = PagingEntryIndex; Index <= PagingEntryIndexLimit; Index++) {
+ if (PagingEntry[Index].Pce.Present == 0) {
+ //
+ // [LinearAddress, LinearAddress + Length] contains not-present range, we need to
+ // make sure all the non-reserved fields of Mask are not 0.
+ //
+ Status = CheckMaskNonReservedBit (Mask);
+ if (RETURN_ERROR (Status)) {
+ return Status;
+ }
+
+ break;
+ }
+ }
+
//
// It's a non-leaf entry
//
@@ -418,7 +467,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;
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/6] UefiCpuPkg/CpuPageTableLib: Add manual TestCase to check input Mask
2023-03-03 10:03 [PATCH 0/6] Fix issues in CpuPageTableLib duntan
` (2 preceding siblings ...)
2023-03-03 10:03 ` [PATCH 3/6] UefiCpuPkg/CpuPageTebleLib: Check input Mask in PageTableMap duntan
@ 2023-03-03 10:03 ` duntan
2023-03-03 10:03 ` [PATCH 5/6] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check Mask duntan
2023-03-03 10:03 ` [PATCH 6/6] UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test duntan
5 siblings, 0 replies; 9+ messages in thread
From: duntan @ 2023-03-03 10:03 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann
Add manual test case to check input Mask. When creating new page
table or mapping not-present range in existing page table, all the
non-reserved fields of Mask should not 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/UnitTest/CpuPageTableLibUnitTestHost.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 121 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
index 3014a03243..5957b80d6e 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
@@ -697,6 +697,126 @@ TestCaseManualChangeNx (
return UNIT_TEST_PASSED;
}
+/**
+ Check if the input Mask is expected when creating new page table or map not-present
+ range in 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
+TestCaseToCheckMapMask (
+ 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, 1G]. All fields of MapMask should be set.
+ //
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_1GB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+ MapMask.Uint64 = MAX_UINT64;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_1GB, &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_1GB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
+
+ //
+ // Update Page table to cover [1G, 2G - 8K]. All fields of MapMask should be set.
+ //
+ PageTableBufferSize = 0;
+ MapAttribute.Uint64 = SIZE_1GB;
+ MapAttribute.Bits.Present = 1;
+ MapMask.Uint64 = 0;
+ MapMask.Bits.Present = 1;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_1GB, SIZE_1GB - SIZE_8KB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+ MapMask.Uint64 = MAX_UINT64;
+ Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_1GB, SIZE_1GB - 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, SIZE_1GB, SIZE_1GB - SIZE_8KB, &MapAttribute, &MapMask);
+ UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
+
+ //
+ // Update Page table to cover [2G - 8K, 2G]. All fields of MapMask should be set.
+ //
+ PageTableBufferSize = 0;
+ MapAttribute.Uint64 = SIZE_2GB - SIZE_8KB;
+ MapAttribute.Bits.Present = 1;
+ 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_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);
+
+ //
+ // Update Page table to set [2G - 8K, 2G] as RW. Only need to set the MapMask.Bits.ReadWrite to 1.
+ //
+ PageTableBufferSize = 0;
+ MapAttribute.Uint64 = 0;
+ MapAttribute.Bits.ReadWrite = 1;
+ MapMask.Uint64 = 0;
+ MapMask.Bits.ReadWrite = 1;
+ 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 +866,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", TestCaseToCheckMapMask, NULL, NULL, NULL);
//
// Populate the Random Test Cases.
//
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 5/6] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check Mask
2023-03-03 10:03 [PATCH 0/6] Fix issues in CpuPageTableLib duntan
` (3 preceding siblings ...)
2023-03-03 10:03 ` [PATCH 4/6] UefiCpuPkg/CpuPageTableLib: Add manual TestCase to check input Mask duntan
@ 2023-03-03 10:03 ` duntan
2023-03-03 10:03 ` [PATCH 6/6] UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test duntan
5 siblings, 0 replies; 9+ messages in thread
From: duntan @ 2023-03-03 10:03 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 mapping not-present range in existing page table, if
any non-reserved field of input Mask are 0, the return status of
PageTableMap () should be RETURN_INVALID_PARAMETER.
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 | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------
1 file changed, 120 insertions(+), 44 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index 97a388ca1c..69d01db451 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -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;
@@ -274,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
@@ -299,7 +319,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,12 +343,21 @@ 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 {
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;
}
@@ -338,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 ()) {
- MapEntrys->Maps[MapsIndex].Mask.Bits.PageTableBaseAddress = 0;
- }
}
MapEntrys->Count += 1;
@@ -609,23 +630,65 @@ 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;
+ UINTN PreviousAddress;
+ BOOLEAN IsNotPresent;
+
+ MapsIndex = MapEntrys->Count;
+ MapCount = 0;
+ PreviousAddress = 0;
+ IsNotPresent = FALSE;
GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
+ Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
+
+ //
+ // Return Status should be InvalidParameter when:
+ // 1. MapEntrys->Maps[MapsIndex] contains not-present range.
+ // 2. MapEntrys->Maps[MapsIndex].Mask contains zero value field.
+ //
+ if (MapEntrys->Maps[MapsIndex].Length > 0) {
+ 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);
+
+ if (Map[MapCount - 1].LinearAddress + Map[MapCount - 1].Length < MapEntrys->Maps[MapsIndex].LinearAddress + MapEntrys->Maps[MapsIndex].Length) {
+ IsNotPresent = TRUE;
+ } else {
+ for (Index = 0; Index < MapCount; Index++) {
+ if ((PreviousAddress < Map[Index].LinearAddress) &&
+ (MapEntrys->Maps[MapsIndex].LinearAddress < Map[Index].LinearAddress) &&
+ ((MapEntrys->Maps[MapsIndex].LinearAddress + MapEntrys->Maps[MapsIndex].Length) > PreviousAddress))
+ {
+ //
+ // MapEntrys->Maps[MapsIndex] contains not-present range in exsiting page table.
+ //
+ IsNotPresent = TRUE;
+ break;
+ }
+
+ PreviousAddress = (UINTN)(Map[Index].LinearAddress + Map[Index].Length);
+ }
+ }
+ } else {
+ IsNotPresent = TRUE;
+ }
+ }
PageTableBufferSize = 0;
Status = PageTableMap (
@@ -638,6 +701,19 @@ SingleMapEntryTest (
&MapEntrys->Maps[MapsIndex].Attribute,
&MapEntrys->Maps[MapsIndex].Mask
);
+
+ Mask = &MapEntrys->Maps[MapsIndex].Mask;
+ if (((Mask->Bits.Present == 0) || (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)) &&
+ IsNotPresent)
+ {
+ RemoveLastMapEntry (MapEntrys);
+ UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+ return UNIT_TEST_PASSED;
+ }
+
if (PageTableBufferSize != 0) {
UT_ASSERT_EQUAL (Status, RETURN_BUFFER_TOO_SMALL);
--
2.31.1.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 6/6] UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test
2023-03-03 10:03 [PATCH 0/6] Fix issues in CpuPageTableLib duntan
` (4 preceding siblings ...)
2023-03-03 10:03 ` [PATCH 5/6] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check Mask duntan
@ 2023-03-03 10:03 ` duntan
5 siblings, 0 replies; 9+ messages in thread
From: duntan @ 2023-03-03 10:03 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>
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 | 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 5957b80d6e..65b717b9ac 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] 9+ messages in thread