[Patch V6 00/22] Fix issues in CpuPageTableLib

[Patch V6 00/22] Fix issues in CpuPageTableLib

[duntan]

In the V6 atch set
In 'Fix the non-1:1 mapping issue', use MultU64x32 to avoid IA32 build failure.
In 'Fix issue when splitting leaf entry', add more precise comments to explain why IA32_PE_BASE_ADDRESS_MASK_40 is used.

Other patches are Reviewed-by Ray.

Dun Tan (20):
  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: Add LastMapEntry pointer
  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                              |  44 +++++++++++++++++++++++++-------------------
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h                         | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c                      | 317 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableParse.c                    |  25 +++++++++++++++++++++----
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c                  | 338 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c                  |  22 +++++++++++++++-------
 UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c                        |  11 +++++------
 8 files changed, 782 insertions(+), 327 deletions(-)

-- 
2.31.1.windows.1

[Patch V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

[duntan]

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>
Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: 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..b94ef07c56 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) + MultU64x32 (RegionLength, (UINT32)PagingEntryIndex);
+      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

[Patch V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry

[duntan]

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>
Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 57f1db203b..f09bb63ad1 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -363,21 +363,24 @@ 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
       // Note: Should NOT inherit the attributes from the original entry because a zero RW bit
       //       will make the entire region read-only even the child entries set the RW bit.
       //
+      // Non-leaf entry doesn't have PAT bit. So use ~IA32_PE_BASE_ADDRESS_MASK_40 is to make sure PAT bit
+      // (bit12) in original big-leaf entry is not assigned to PageTableBaseAddress field of non-leaf entry.
+      //
       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

Re: [Patch V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Friday, March 24, 2023 4:52 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 V6 06/22] UefiCpuPkg/CpuPageTableLib: Fix issue when
> splitting leaf entry
> 
> 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>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 19
> +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 57f1db203b..f09bb63ad1 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -363,21 +363,24 @@ 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
>        // Note: Should NOT inherit the attributes from the original entry because
> a zero RW bit
>        //       will make the entire region read-only even the child entries set the
> RW bit.
>        //
> +      // Non-leaf entry doesn't have PAT bit. So use
> ~IA32_PE_BASE_ADDRESS_MASK_40 is to make sure PAT bit
> +      // (bit12) in original big-leaf entry is not assigned to
> PageTableBaseAddress field of non-leaf entry.
> +      //
>        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

Re: [Patch V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Friday, March 24, 2023 4:52 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 V6 04/22] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1
> mapping issue
> 
> 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>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com>
> Acked-by: 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..b94ef07c56 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) +
> MultU64x32 (RegionLength, (UINT32)PagingEntryIndex);
> +      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