[PATCH 1/6] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition

[PATCH 1/6] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition

[duntan]

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: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Ray Ni <ray.ni@intel.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

[PATCH 0/6] Fix issues in CpuPageTableLib

[duntan]

1.UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition
2.UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
3.UefiCpuPkg/CpuPageTebleLib: Check input Mask in PageTableMap
4.UefiCpuPkg/CpuPageTableLib: Add manual TestCase to check input Mask
5.UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check Mask
6.UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test

Dun Tan (6):
  UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition
  UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
  UefiCpuPkg/CpuPageTebleLib: Check input Mask in PageTableMap
  UefiCpuPkg/CpuPageTableLib: Add manual TestCase to check input Mask
  UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check Mask
  UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test

 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c                      |  86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c                  | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------
 3 files changed, 311 insertions(+), 69 deletions(-)

-- 
2.31.1.windows.1

[PATCH 1/6] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition

[duntan]

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

[PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

[duntan]

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

[PATCH 3/6] UefiCpuPkg/CpuPageTebleLib: Check input Mask in PageTableMap

[duntan]

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

[PATCH 4/6] UefiCpuPkg/CpuPageTableLib: Add manual TestCase to check input Mask

[duntan]

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

[PATCH 5/6] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check Mask

[duntan]

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

[PATCH 6/6] UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test

[duntan]

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

Re: [edk2-devel] [PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

[Gerd Hoffmann]

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

Re: [edk2-devel] [PATCH 2/6] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

[duntan]

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