[Patch V2 00/14] Fix issues in CpuPageTableLib

[Patch V2 00/14] Fix issues in CpuPageTableLib

[duntan]

In the V2 patch set
1. Add more commit message about the 'Fix the non-1:1 mapping issue' patch
2. Add patches to enable PAE paing in CpuPageTable Lib and add random test for PAE paging
3. Add patches to add OUTPUT IsModified parameter for PageTableMap() and modify random test for this parameter
4. Add patches to fix IA32 build failure in CpuPageTableLib and mofify corresponding random test code.

Dun Tan (12):
  UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition
  UefiCpuPkg/CpuPageTableLib: Add check for input Length
  UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue
  UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry
  UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap
  UefiCpuPkg/CpuPageTableLib: Add manual test to check Mask and Attr
  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: Enable PAE paging
  UefiCpuPkg/CpuPageTableLib: Add RandomTest for PAE paging

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                              |  36 +++++++++++++++++++-----------------
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTable.h                         | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c                      | 187 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c                  | 309 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c                  |  13 ++++++++-----
 UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c                        |  12 +++++++-----
 7 files changed, 610 insertions(+), 278 deletions(-)

-- 
2.31.1.windows.1

[Patch V2 01/14] 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 '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>
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

Re: [Patch V2 01/14] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition

[Ni, Ray]

Dun,
The copyright year needs to change to 2023.
Code logic change is good to me.

Thanks,
Ray

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 01/14] UefiCpuPkg/CpuPageTableLib: Remove unneeded
> 'if' condition
> 
> 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>
> 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

Re: [Patch V2 01/14] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition

[Ni, Ray]

You can carry my Reviewed-by in next version if you add the copy right year change.

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 01/14] UefiCpuPkg/CpuPageTableLib: Remove unneeded
> 'if' condition
> 
> 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>
> 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

Re: [Patch V2 01/14] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition

[duntan]

Thanks Ray. Will update the copy right year in next version patch.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 9:24 AM
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 V2 01/14] UefiCpuPkg/CpuPageTableLib: Remove unneeded 'if' condition

You can carry my Reviewed-by in next version if you add the copy right year change.

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 01/14] UefiCpuPkg/CpuPageTableLib: Remove unneeded 
> 'if' condition
> 
> 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>
> 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 V2 02/14] UefiCpuPkg/CpuPageTableLib: Add check for input Length

[duntan]

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/Library/CpuPageTableLib/CpuPageTableMap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 47027917d9..4c9d70fa0a 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -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

Re: [Patch V2 02/14] UefiCpuPkg/CpuPageTableLib: Add check for input Length

[Ni, Ray]

The function header comments in lib header and C file should be updated as well
to document a new condition when success is returned.

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 02/14] 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/Library/CpuPageTableLib/CpuPageTableMap.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 47027917d9..4c9d70fa0a 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -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

Re: [Patch V2 02/14] UefiCpuPkg/CpuPageTableLib: Add check for input Length

[duntan]

Thanks Ray. Will update the corresponding function header comments in next version.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 9:25 AM
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 V2 02/14] UefiCpuPkg/CpuPageTableLib: Add check for input Length

The function header comments in lib header and C file should be updated as well to document a new condition when success is returned.

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 02/14] 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/Library/CpuPageTableLib/CpuPageTableMap.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> index 47027917d9..4c9d70fa0a 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
> @@ -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

[Patch V2 03/14] 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>
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 4c9d70fa0a..ee27238edb 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

Re: [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

[Ni, Ray]

Dun,
Can you split this patch to 2 patches?
One to move some local variable initialization to the beginning of the function.
The other to fix the bug. So the bug fix changes look smaller.

Thanks,
Ray

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 03/14] 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>
> 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 4c9d70fa0a..ee27238edb 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

Re: [Patch V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

[duntan]

Okay, I'll split this patch into 2 patches in next version

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 9:28 AM
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 V2 03/14] UefiCpuPkg/CpuPageTableLib: Fix the non-1:1 mapping issue

Dun,
Can you split this patch to 2 patches?
One to move some local variable initialization to the beginning of the function.
The other to fix the bug. So the bug fix changes look smaller.

Thanks,
Ray

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 03/14] 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>
> 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 4c9d70fa0a..ee27238edb 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 V2 04/14] 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>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index ee27238edb..0f3d0d684e 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -354,8 +354,15 @@ 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;
+      }
+
+      ParentPagingEntry->Uintn = (UINTN)(VOID *)PagingEntry;
 
       //
       // Set NOP attributes
@@ -363,12 +370,6 @@ 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;
-      }
     }
   } else {
     //
-- 
2.31.1.windows.1

Re: [Patch V2 04/14] UefiCpuPkg/CpuPageTableLib: Fix issue when splitting leaf entry

[Ni, Ray]

> +
> +      ParentPagingEntry->Uintn = (UINTN)(VOID *)PagingEntry;

Only address field is set but attributes are cleared to zeros.
Then following code sets the attributes.
Still a hole at this point, right?

> 
>        //
>        // Set NOP attributes
> @@ -363,12 +370,6 @@ PageTableLibMapInLevel (
>        //       will make the entire region read-only even the child entries set the
> RW bit.
>        //
>        PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute,
> &AllOneMask);

[Patch V2 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr 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 and Present field of input
Attribute is 1.

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 | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 0f3d0d684e..56f762a15e 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -214,6 +214,33 @@ PageTableLibSetPnle (
   Pnle->Bits.CacheDisabled = 0;
 }
 
+/**
+  Check if any Non-Reserved field of Mask is 0 or Attribute->Bits.Present is 0
+  when creating new page table or mapping not-present range.
+
+  @param[in] Attribute    The attribute of the linear address range.
+  @param[in] Mask         The mask used for attribute to check.
+
+  @retval RETURN_INVALID_PARAMETER    There is 0-value field in Non-Reserved fields of Mask or Attribute->Bits.Present is 0.
+  @retval RETURN_SUCCESS              All Non-Reserved fields of Mask are not 0 and Attribute->Bits.Present is 1.
+**/
+RETURN_STATUS
+CheckMaskAndAttrForNotPresentEntry (
+  IN     IA32_MAP_ATTRIBUTE  *Attribute,
+  IN     IA32_MAP_ATTRIBUTE  *Mask
+  )
+{
+  if ((Attribute->Bits.Present == 0) || (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 +286,7 @@ PageTableLibMapInLevel (
   UINTN               Index;
   IA32_PAGING_ENTRY   *PagingEntry;
   UINTN               PagingEntryIndex;
+  UINTN               PagingEntryIndexLimit;
   IA32_PAGING_ENTRY   *CurrentPagingEntry;
   UINT64              RegionLength;
   UINT64              SubLength;
@@ -302,6 +330,14 @@ PageTableLibMapInLevel (
   //
 
   if (ParentPagingEntry->Pce.Present == 0) {
+    //
+    // [LinearAddress, LinearAddress + Length] contains not-present range.
+    //
+    Status = CheckMaskAndAttrForNotPresentEntry (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.
@@ -372,6 +408,23 @@ PageTableLibMapInLevel (
       PageTableLibSetPnle (&ParentPagingEntry->Pnle, &NopAttribute, &AllOneMask);
     }
   } else {
+    PagingEntry           = (IA32_PAGING_ENTRY *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry->Pnle);
+    PagingEntryIndexLimit = (BitFieldRead64 (LinearAddress + Length - 1, BitStart + 9, 63) > BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ? 511 :
+                            (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.
+        //
+        Status = CheckMaskAndAttrForNotPresentEntry (Attribute, Mask);
+        if (RETURN_ERROR (Status)) {
+          return Status;
+        }
+
+        break;
+      }
+    }
+
     //
     // It's a non-leaf entry
     //
@@ -419,7 +472,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

Re: [Patch V2 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap

[Ni, Ray]

> +**/
> +RETURN_STATUS
> +CheckMaskAndAttrForNotPresentEntry (
> +  IN     IA32_MAP_ATTRIBUTE  *Attribute,
> +  IN     IA32_MAP_ATTRIBUTE  *Mask
> +  )
> +{
> +  if ((Attribute->Bits.Present == 0) || (Mask->Bits.Present == 0) || (Mask-
> >Bits.ReadWrite == 0) ||

1. I think we can allow caller to set a not-present range as not-present.
Even though it's meaningless😊
So, I think we can remove the Attribute.Present check.


2. The function name can be more readable: how about IsAllAttributesSetForNonPresentEntry()?


> +      (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;
> +}

> +    PagingEntry           = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +    PagingEntryIndexLimit = (BitFieldRead64 (LinearAddress + Length - 1,
> BitStart + 9, 63) > BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ?
> 511 :

3. Can you add more comments for the code here?

Re: [Patch V2 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap

[duntan]

Thanks for the comments. Will do the code change in next version patch set.
Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 1:34 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 V2 05/14] UefiCpuPkg/CpuPageTebleLib: Check Mask and Attr in PageTableMap

> +**/
> +RETURN_STATUS
> +CheckMaskAndAttrForNotPresentEntry (
> +  IN     IA32_MAP_ATTRIBUTE  *Attribute,
> +  IN     IA32_MAP_ATTRIBUTE  *Mask
> +  )
> +{
> +  if ((Attribute->Bits.Present == 0) || (Mask->Bits.Present == 0) || (Mask-
> >Bits.ReadWrite == 0) ||

1. I think we can allow caller to set a not-present range as not-present.
Even though it's meaningless😊
So, I think we can remove the Attribute.Present check.


2. The function name can be more readable: how about IsAllAttributesSetForNonPresentEntry()?


> +      (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;
> +}

> +    PagingEntry           = (IA32_PAGING_ENTRY
> *)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-
> >Pnle);
> +    PagingEntryIndexLimit = (BitFieldRead64 (LinearAddress + Length - 1,
> BitStart + 9, 63) > BitFieldRead64 (LinearAddress + Offset, BitStart + 9, 63)) ?
> 511 :

3. Can you add more comments for the code here?

[Patch V2 06/14] UefiCpuPkg/CpuPageTableLib: Add manual test to check Mask and Attr

[duntan]

Add manual test case to check input Mask and Attribute. When
creating new page table or mapping not-present range in existing
page table, all the non-reserved fields of Mask and Present bit of
Attribute 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 | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
index 3014a03243..fe00a7f632 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
@@ -697,6 +697,114 @@ TestCaseManualChangeNx (
   return UNIT_TEST_PASSED;
 }
 
+/**
+  Check if the input Mask and Attribute 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
+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, 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 and Present bit of MapAttribute should be 1.
+  //
+  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] and set [2G - 8K, 2G] as RW. All fields of MapMask should be set and Present bit of MapAttribute should be 1.
+  //
+  PageTableBufferSize         = 0;
+  MapAttribute.Uint64         = SIZE_2GB - SIZE_8KB;
+  MapAttribute.Bits.ReadWrite = 1;
+  MapMask.Uint64              = MAX_UINT64;
+  Status                      = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute, &MapMask);
+  UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
+  MapAttribute.Bits.Present = 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 +854,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

Re: [Patch V2 06/14] UefiCpuPkg/CpuPageTableLib: Add manual test to check Mask and Attr

[Ni, Ray]

Please update test case to not expect failure when setting a non-present range as non-present .

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 06/14] UefiCpuPkg/CpuPageTableLib: Add manual test to
> check Mask and Attr
> 
> Add manual test case to check input Mask and Attribute. When
> creating new page table or mapping not-present range in existing
> page table, all the non-reserved fields of Mask and Present bit of
> Attribute 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/CpuPageTableLibUnitTestHo
> st.c | 110
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> index 3014a03243..fe00a7f632 100644
> ---
> a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> +++
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> @@ -697,6 +697,114 @@ TestCaseManualChangeNx (
>    return UNIT_TEST_PASSED;
>  }
> 
> +/**
> +  Check if the input Mask and Attribute 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
> +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, 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 and Present bit of MapAttribute should be 1.
> +  //
> +  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] and set [2G - 8K, 2G] as RW. All
> fields of MapMask should be set and Present bit of MapAttribute should be 1.
> +  //
> +  PageTableBufferSize         = 0;
> +  MapAttribute.Uint64         = SIZE_2GB - SIZE_8KB;
> +  MapAttribute.Bits.ReadWrite = 1;
> +  MapMask.Uint64              = MAX_UINT64;
> +  Status                      = PageTableMap (&PageTable, PagingMode, Buffer,
> &PageTableBufferSize, SIZE_2GB - SIZE_8KB, SIZE_8KB, &MapAttribute,
> &MapMask);
> +  UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
> +  MapAttribute.Bits.Present = 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 +854,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

[Patch V2 07/14] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr

[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 Mask or Present bit of Attribute in
generated random MapEntry 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 | 169 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c |   4 ++++
 2 files changed, 129 insertions(+), 44 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index 97a388ca1c..8293e3d8eb 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,64 @@ 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;
+  BOOLEAN             IsNotPresent;
+
+  MapsIndex       = MapEntrys->Count;
+  MapCount        = 0;
+  PreviousAddress = 0;
+  IsNotPresent    = FALSE;
 
   GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
+  Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
+
+  //
+  // Check if the generated MapEntrys->Maps[MapsIndex] contains not-present range.
+  //
+  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 = Map[Index].LinearAddress + Map[Index].Length;
+        }
+      }
+    } else {
+      IsNotPresent = TRUE;
+    }
+  }
 
   PageTableBufferSize = 0;
   Status              = PageTableMap (
@@ -638,6 +700,25 @@ SingleMapEntryTest (
                           &MapEntrys->Maps[MapsIndex].Attribute,
                           &MapEntrys->Maps[MapsIndex].Mask
                           );
+
+  //
+  // Return Status should be InvalidParameter when:
+  // 1. MapEntrys->Maps[MapsIndex] contains not-present range.
+  // 2. MapEntrys->Maps[MapsIndex].Mask contains zero value field or Attribute->Bits.Present is 0.
+  //
+  Attribute = &MapEntrys->Maps[MapsIndex].Attribute;
+  Mask      = &MapEntrys->Maps[MapsIndex].Mask;
+  if (((Attribute->Bits.Present == 0) || (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);
 
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
index 5bd70c0f65..11f7e607ca 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
@@ -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

Re: [Patch V2 07/14] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr

[Ni, Ray]

> 
> -/**
> -  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);
> +}
1. can you split the RandomBoolean() change in standalone patch?


2. can you simplify below code to only use for-loop?
> +  //
> +  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 = Map[Index].LinearAddress + Map[Index].Length;
> +        }
> +      }
> +    } else {
> +      IsNotPresent = TRUE;
> +    }
> +  }
> 
>    PageTableBufferSize = 0;
>    Status              = PageTableMap (
> @@ -638,6 +700,25 @@ SingleMapEntryTest (
>                            &MapEntrys->Maps[MapsIndex].Attribute,
>                            &MapEntrys->Maps[MapsIndex].Mask
>                            );
> +
> +  //
> +  // Return Status should be InvalidParameter when:
> +  // 1. MapEntrys->Maps[MapsIndex] contains not-present range.
> +  // 2. MapEntrys->Maps[MapsIndex].Mask contains zero value field or
> Attribute->Bits.Present is 0.
> +  //
> +  Attribute = &MapEntrys->Maps[MapsIndex].Attribute;
> +  Mask      = &MapEntrys->Maps[MapsIndex].Mask;
> +  if (((Attribute->Bits.Present == 0) || (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);
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> index 5bd70c0f65..11f7e607ca 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> @@ -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

Re: [Patch V2 07/14] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr

[duntan]

Thanks Ray for the comments. Will create a new patch to split the RandomBoolean() and simplify the code loop in random test

Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 1:49 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>; Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: RE: [Patch V2 07/14] UefiCpuPkg/CpuPageTableLib:Modify RandomTest to check Mask/Attr

> 
> -/**
> -  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); 
> +}
1. can you split the RandomBoolean() change in standalone patch?


2. can you simplify below code to only use for-loop?
> +  //
> +  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 = Map[Index].LinearAddress + Map[Index].Length;
> +        }
> +      }
> +    } else {
> +      IsNotPresent = TRUE;
> +    }
> +  }
> 
>    PageTableBufferSize = 0;
>    Status              = PageTableMap (
> @@ -638,6 +700,25 @@ SingleMapEntryTest (
>                            &MapEntrys->Maps[MapsIndex].Attribute,
>                            &MapEntrys->Maps[MapsIndex].Mask
>                            );
> +
> +  //
> +  // Return Status should be InvalidParameter when:
> +  // 1. MapEntrys->Maps[MapsIndex] contains not-present range.
> +  // 2. MapEntrys->Maps[MapsIndex].Mask contains zero value field or
> Attribute->Bits.Present is 0.
> +  //
> +  Attribute = &MapEntrys->Maps[MapsIndex].Attribute;
> +  Mask      = &MapEntrys->Maps[MapsIndex].Mask;
> +  if (((Attribute->Bits.Present == 0) || (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);
> 
> diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> index 5bd70c0f65..11f7e607ca 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/TestHelper.c
> @@ -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

[Patch V2 08/14] 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 fe00a7f632..6f27411d4b 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: [Patch V2 08/14] UefiCpuPkg/CpuPageTableLib: Enable non-1:1 mapping in random test

[Ni, Ray]

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

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 08/14] UefiCpuPkg/CpuPageTableLib: Enable non-1:1
> mapping in random test
> 
> 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/CpuPageTableLibUnitTestHo
> st.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> index fe00a7f632..6f27411d4b 100644
> ---
> a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> +++
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.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

[Patch V2 09/14] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter.

[duntan]

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                      | 47 ++++++++++++++++++++++++++++++++++++++++-------
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c | 66 +++++++++++++++++++++++++++++++++---------------------------------
 UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c                  |  6 ++++--
 UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c                        |  6 ++++--
 5 files changed, 84 insertions(+), 45 deletions(-)

diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index 2dc9b7d18e..118dff20f4 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.
@@ -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 56f762a15e..4e8ac9b981 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -262,6 +262,7 @@ CheckMaskAndAttrForNotPresentEntry (
                                     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_SUCCESS            PageTable is created/updated successfully.
 **/
@@ -278,7 +279,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   OPTIONAL
   )
 {
   RETURN_STATUS       Status;
@@ -302,6 +304,8 @@ PageTableLibMapInLevel (
   IA32_MAP_ATTRIBUTE  ChildMask;
   IA32_MAP_ATTRIBUTE  CurrentMask;
   IA32_MAP_ATTRIBUTE  LocalParentAttribute;
+  IA32_PAGING_ENTRY   ParentPagingEntryContent;
+  IA32_PAGING_ENTRY   PrevLeafPagingEntryContent;
 
   ASSERT (Level != 0);
   ASSERT ((Attribute != NULL) && (Mask != NULL));
@@ -314,8 +318,9 @@ PageTableLibMapInLevel (
   NopAttribute.Bits.ReadWrite      = 1;
   NopAttribute.Bits.UserSupervisor = 1;
 
-  LocalParentAttribute.Uint64 = ParentAttribute->Uint64;
-  ParentAttribute             = &LocalParentAttribute;
+  LocalParentAttribute.Uint64     = ParentAttribute->Uint64;
+  ParentAttribute                 = &LocalParentAttribute;
+  ParentPagingEntryContent.Uint64 = ParentPagingEntry->Uint64;
 
   //
   // RegionLength: 256T (1 << 48) 512G (1 << 39), 1G (1 << 30), 2M (1 << 21) or 4K (1 << 12).
@@ -542,7 +547,17 @@ PageTableLibMapInLevel (
           ASSERT (CreateNew || (Mask->Bits.Nx == 0) || (Attribute->Bits.Nx == 1));
         }
 
+        //
+        // Check if any leaf PagingEntry is modified.
+        //
+        PrevLeafPagingEntryContent.Uint64 = CurrentPagingEntry->Uint64;
         PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, &CurrentMask);
+
+        if (PrevLeafPagingEntryContent.Uint64 != CurrentPagingEntry->Uint64) {
+          if (IsModified != NULL) {
+            *IsModified = TRUE;
+          }
+        }
       }
     } else {
       //
@@ -565,7 +580,8 @@ PageTableLibMapInLevel (
                  Length,
                  Offset,
                  Attribute,
-                 Mask
+                 Mask,
+                 IsModified
                  );
       if (RETURN_ERROR (Status)) {
         return Status;
@@ -577,6 +593,15 @@ PageTableLibMapInLevel (
     Index++;
   }
 
+  //
+  // Check if ParentPagingEntry entry is modified.
+  //
+  if (ParentPagingEntryContent.Uint64 != ParentPagingEntry->Uint64) {
+    if (IsModified != NULL) {
+      *IsModified = TRUE;
+    }
+  }
+
   return RETURN_SUCCESS;
 }
 
@@ -597,6 +622,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.
@@ -616,7 +642,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;
@@ -661,6 +688,10 @@ PageTableMap (
     return RETURN_INVALID_PARAMETER;
   }
 
+  if (IsModified != NULL) {
+    *IsModified = FALSE;
+  }
+
   MaxLeafLevel     = (IA32_PAGE_LEVEL)(UINT8)PagingMode;
   MaxLevel         = (IA32_PAGE_LEVEL)(UINT8)(PagingMode >> 8);
   MaxLinearAddress = LShiftU64 (1, 12 + MaxLevel * 9);
@@ -703,7 +734,8 @@ PageTableMap (
                    Length,
                    0,
                    Attribute,
-                   Mask
+                   Mask,
+                   NULL
                    );
   if (RETURN_ERROR (Status)) {
     return Status;
@@ -735,7 +767,8 @@ PageTableMap (
              Length,
              0,
              Attribute,
-             Mask
+             Mask,
+             IsModified
              );
   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 6f27411d4b..3df6436af3 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,13 +741,13 @@ TestCaseToCheckMapMaskAndAttr (
   //
   // 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);
+  Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_1GB, &MapAttribute, &MapMask, NULL);
   UT_ASSERT_EQUAL (Status, RETURN_INVALID_PARAMETER);
   MapMask.Uint64 = MAX_UINT64;
-  Status         = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_1GB, &MapAttribute, &MapMask);
+  Status         = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, 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, 0, SIZE_1GB, &MapAttribute, &MapMask);
+  Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, 0, SIZE_1GB, &MapAttribute, &MapMask, NULL);
   UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
 
   //
@@ -758,13 +758,13 @@ TestCaseToCheckMapMaskAndAttr (
   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);
+  Status                    = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_1GB, SIZE_1GB - SIZE_8KB, &MapAttribute, &MapMask, NULL);
   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);
+  Status         = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_1GB, SIZE_1GB - 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, SIZE_1GB, SIZE_1GB - SIZE_8KB, &MapAttribute, &MapMask);
+  Status = PageTableMap (&PageTable, PagingMode, Buffer, &PageTableBufferSize, SIZE_1GB, SIZE_1GB - SIZE_8KB, &MapAttribute, &MapMask, NULL);
   UT_ASSERT_EQUAL (Status, RETURN_SUCCESS);
 
   //
@@ -774,10 +774,10 @@ TestCaseToCheckMapMaskAndAttr (
   MapAttribute.Uint64         = SIZE_2GB - SIZE_8KB;
   MapAttribute.Bits.ReadWrite = 1;
   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_INVALID_PARAMETER);
   MapAttribute.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);
 
   MapCount = 0;
diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index 8293e3d8eb..b2965d61fb 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -698,7 +698,8 @@ SingleMapEntryTest (
                           MapEntrys->Maps[MapsIndex].LinearAddress,
                           MapEntrys->Maps[MapsIndex].Length,
                           &MapEntrys->Maps[MapsIndex].Attribute,
-                          &MapEntrys->Maps[MapsIndex].Mask
+                          &MapEntrys->Maps[MapsIndex].Mask,
+                          NULL
                           );
 
   //
@@ -736,7 +737,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 7cf91ed9c4..c10121ede5 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
+++ b/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
@@ -60,7 +60,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));
@@ -75,7 +76,8 @@ CreatePageTable (
              Address,
              Length,
              &MapAttribute,
-             &MapMask
+             &MapMask,
+             NULL
              );
   ASSERT_EFI_ERROR (Status);
   return PageTable;
-- 
2.31.1.windows.1

Re: [Patch V2 09/14] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter.

[Ni, Ray]

> +  IA32_PAGING_ENTRY   ParentPagingEntryContent;
1. how about "OriginalParentPagingEntry"?

> +  IA32_PAGING_ENTRY   PrevLeafPagingEntryContent;
2. how about "OriginalCurrentPagingEntry"?

> 
> +  //
> +  // Check if ParentPagingEntry entry is modified.
> +  //
3. Can you add more comments to explain why checking parent entry content is enough?

> +  if (ParentPagingEntryContent.Uint64 != ParentPagingEntry->Uint64) {
> +    if (IsModified != NULL) {
4. Can you always pass a non-NULL IsModified to MapInLevel()?

5. By the way, it's good that this patch doesn't enhance test case to test IsModified return value.

Re: [Patch V2 09/14] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter.

[duntan]

Thanks for the comments. Will change the code and add the comments in next version patch.
Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 2:02 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 V2 09/14] UefiCpuPkg/CpuPageTableLib: Add OUTPUT IsModified parameter.

> +  IA32_PAGING_ENTRY   ParentPagingEntryContent;
1. how about "OriginalParentPagingEntry"?

> +  IA32_PAGING_ENTRY   PrevLeafPagingEntryContent;
2. how about "OriginalCurrentPagingEntry"?

> 
> +  //
> +  // Check if ParentPagingEntry entry is modified.
> +  //
3. Can you add more comments to explain why checking parent entry content is enough?

> +  if (ParentPagingEntryContent.Uint64 != ParentPagingEntry->Uint64) {
> +    if (IsModified != NULL) {
4. Can you always pass a non-NULL IsModified to MapInLevel()?

5. By the way, it's good that this patch doesn't enhance test case to test IsModified return value.

[Patch V2 10/14] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified

[duntan]

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 | 63 ++++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
index b2965d61fb..8f8f0a5a9f 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;
@@ -647,25 +649,33 @@ SingleMapEntryTest (
   IA32_MAP_ATTRIBUTE  *Attribute;
   UINT64              PreviousAddress;
   BOOLEAN             IsNotPresent;
+  BOOLEAN             IsModified;
 
   MapsIndex       = MapEntrys->Count;
   MapCount        = 0;
   PreviousAddress = 0;
   IsNotPresent    = FALSE;
+  IsModified      = FALSE;
 
   GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
   Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
 
+  if (MapCount != 0) {
+    //
+    // Allocate memory for Map
+    // Note the memory is only used in this one Single MapEntry Test
+    //
+    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) {
     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 {
@@ -699,7 +709,7 @@ SingleMapEntryTest (
                           MapEntrys->Maps[MapsIndex].Length,
                           &MapEntrys->Maps[MapsIndex].Attribute,
                           &MapEntrys->Maps[MapsIndex].Mask,
-                          NULL
+                          &IsModified
                           );
 
   //
@@ -738,7 +748,7 @@ SingleMapEntryTest (
                MapEntrys->Maps[MapsIndex].Length,
                &MapEntrys->Maps[MapsIndex].Attribute,
                &MapEntrys->Maps[MapsIndex].Mask,
-               NULL
+               &IsModified
                );
   }
 
@@ -752,18 +762,33 @@ 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 (MapCount2 == 0) {
+      UT_ASSERT_EQUAL (IsModified, FALSE);
+    } 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);
@@ -773,17 +798,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);
@@ -797,6 +822,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

Re: [Patch V2 10/14] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified

[Ni, Ray]

> 
>    GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
>    Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
> 
> +  if (MapCount != 0) {
> +    //
> +    // Allocate memory for Map
> +    // Note the memory is only used in this one Single MapEntry Test
> +    //
> +    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) {
>      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;

1. can you split the Parse() API call into a standalone patch?

> +    if (MapCount2 == 0) {
> +      UT_ASSERT_EQUAL (IsModified, FALSE);
2. no need to treat "MapCount == 0" as special. CompareMem() should be able to accept 0-length bytes.

> +    } else if (CompareMem (Map, Map2, MapCount2 * sizeof
> (IA32_MAP_ENTRY)) != 0) {
> +      UT_ASSERT_EQUAL (IsModified, TRUE);
> +    } else {
> +      UT_ASSERT_EQUAL (IsModified, FALSE);
> +    }
>    }

Re: [Patch V2 10/14] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified

[duntan]

Thanks for the comments. I'll split this patch and remove the unneeded 'if' condition check.

Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 2:09 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>; Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: RE: [Patch V2 10/14] UefiCpuPkg/CpuPageTableLib: Modify RandomTest to check IsModified

> 
>    GenerateSingleRandomMapEntry (MaxAddress, MapEntrys);
>    Status = PageTableParse (*PageTable, PagingMode, NULL, &MapCount);
> 
> +  if (MapCount != 0) {
> +    //
> +    // Allocate memory for Map
> +    // Note the memory is only used in this one Single MapEntry Test
> +    //
> +    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) {
>      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;

1. can you split the Parse() API call into a standalone patch?

> +    if (MapCount2 == 0) {
> +      UT_ASSERT_EQUAL (IsModified, FALSE);
2. no need to treat "MapCount == 0" as special. CompareMem() should be able to accept 0-length bytes.

> +    } else if (CompareMem (Map, Map2, MapCount2 * sizeof
> (IA32_MAP_ENTRY)) != 0) {
> +      UT_ASSERT_EQUAL (IsModified, TRUE);
> +    } else {
> +      UT_ASSERT_EQUAL (IsModified, FALSE);
> +    }
>    }

[Patch V2 11/14] UefiCpuPkg/CpuPageTableLib: Enable PAE paging

[duntan]

Remove the limitation check for PagingPae to enable creating or
updating PAE page table in CpuPageTableLib. The origin code is
naturally adapted 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/CpuPageTableMap.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index 4e8ac9b981..d99a21a0fc 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -658,10 +658,9 @@ PageTableMap (
     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;
   }
@@ -694,11 +693,11 @@ 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;
   }
@@ -771,6 +770,14 @@ PageTableMap (
              IsModified
              );
   if (!RETURN_ERROR (Status)) {
+    if (PagingMode == PagingPae) {
+      //
+      // These fields of PAE paging PDPTE should be 0 according to SDM.
+      //
+      TopPagingEntry.PdptePae.Bits.MustBeZero  = 0;
+      TopPagingEntry.PdptePae.Bits.MustBeZero2 = 0;
+    }
+
     *PageTable = (UINTN)(TopPagingEntry.Uintn & IA32_PE_BASE_ADDRESS_MASK_40);
   }
 
-- 
2.31.1.windows.1

Re: [Patch V2 11/14] UefiCpuPkg/CpuPageTableLib: Enable PAE paging

[Ni, Ray]

> +    if (PagingMode == PagingPae) {
> +      //
> +      // These fields of PAE paging PDPTE should be 0 according to SDM.
> +      //

1. can you update comments to explain such as:
"These fields are treated as ReadWrite, .... by the common map logic. So they might be set to 1."

> +      TopPagingEntry.PdptePae.Bits.MustBeZero  = 0;
> +      TopPagingEntry.PdptePae.Bits.MustBeZero2 = 0;
2. Above code sets the CR3 value. You should update the 4 PDPTE entries.

Re: [Patch V2 11/14] UefiCpuPkg/CpuPageTableLib: Enable PAE paging

[duntan]

I'll add more comments and modify the code to set 4 PDPTE entries. Also will add unit test code check the MustBeZero fields for PAE paging.
Thanks for the comments.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 2:24 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 V2 11/14] UefiCpuPkg/CpuPageTableLib: Enable PAE paging

> +    if (PagingMode == PagingPae) {
> +      //
> +      // These fields of PAE paging PDPTE should be 0 according to SDM.
> +      //

1. can you update comments to explain such as:
"These fields are treated as ReadWrite, .... by the common map logic. So they might be set to 1."

> +      TopPagingEntry.PdptePae.Bits.MustBeZero  = 0;
> +      TopPagingEntry.PdptePae.Bits.MustBeZero2 = 0;
2. Above code sets the CR3 value. You should update the 4 PDPTE entries.

[Patch V2 12/14] UefiCpuPkg/CpuPageTableLib: Add RandomTest for PAE paging

[duntan]

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                  | 5 ++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
index 3df6436af3..06a8fd3c02 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 };
@@ -865,6 +866,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 8f8f0a5a9f..a7f45fb175 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
@@ -251,10 +251,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 11f7e607ca..614bd6bbf1 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;
   }
@@ -264,7 +263,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

Re: [Patch V2 12/14] UefiCpuPkg/CpuPageTableLib: Add RandomTest for PAE paging

[Ni, Ray]

Can you please check that the reserved fields in 4 PDPTE entries are set to 0 in the test?

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Wednesday, March 8, 2023 6:08 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 V2 12/14] 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                  | 5 ++-
> --
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTest
> Host.c
> index 3df6436af3..06a8fd3c02 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 };
> @@ -865,6 +866,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 8f8f0a5a9f..a7f45fb175 100644
> --- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> +++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/RandomTest.c
> @@ -251,10 +251,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 11f7e607ca..614bd6bbf1 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;
>    }
> @@ -264,7 +263,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

[Patch V2 13/14] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf

[duntan]

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 +++++++++++-----------
 UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c   |   6 +++---
 4 files changed, 93 insertions(+), 92 deletions(-)

diff --git a/UefiCpuPkg/Include/Library/CpuPageTableLib.h b/UefiCpuPkg/Include/Library/CpuPageTableLib.h
index 118dff20f4..a04040123f 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..882719546f 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 High
+
+    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 d99a21a0fc..fd5c5b50d2 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);
   }
 
@@ -233,7 +233,7 @@ CheckMaskAndAttrForNotPresentEntry (
   if ((Attribute->Bits.Present == 0) || (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))
+      ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask->Bits.PageTableBaseAddressHigh == 0)) || (Mask->Bits.ProtectionKey == 0) || (Mask->Bits.Nx == 0))
   {
     return RETURN_INVALID_PARAMETER;
   }
@@ -376,7 +376,7 @@ PageTableLibMapInLevel (
     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)))) &&
-        (  (Mask->Bits.PageTableBaseAddress == 0)
+        (  ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask->Bits.PageTableBaseAddressHigh == 0))
         || ((IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (&PleBAttribute) + PagingEntryIndex * RegionLength)
             == (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset))))
     {
@@ -676,7 +676,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.
     //
@@ -710,12 +710,12 @@ PageTableMap (
     TopPagingEntry.Pce.Nx             = 0;
   }
 
-  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.
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c b/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
index c10121ede5..05a40bb225 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
+++ b/UefiCpuPkg/Library/MpInitLib/X64/CreatePageTable.c
@@ -37,9 +37,9 @@ CreatePageTable (
   MapAttribute.Bits.Present   = 1;
   MapAttribute.Bits.ReadWrite = 1;
 
-  MapMask.Bits.PageTableBaseAddress = 1;
-  MapMask.Bits.Present              = 1;
-  MapMask.Bits.ReadWrite            = 1;
+  MapMask.Bits.PageTableBaseAddressLow = 1;
+  MapMask.Bits.Present                 = 1;
+  MapMask.Bits.ReadWrite               = 1;
 
   PageTable           = 0;
   PageTableBufferSize = 0;
-- 
2.31.1.windows.1

Re: [Patch V2 13/14] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf

[Ni, Ray]

> +    UINT32    PageTableBaseAddressLow  : 19; // Page Table Base Address
> High

1. Comments say "High". Should be "Low".

> +
> 
> -  MapMask.Bits.PageTableBaseAddress = 1;
> -  MapMask.Bits.Present              = 1;
> -  MapMask.Bits.ReadWrite            = 1;
> +  MapMask.Bits.PageTableBaseAddressLow = 1;
> +  MapMask.Bits.Present                 = 1;
> +  MapMask.Bits.ReadWrite               = 1;

2. Can you please create a separate patch to initialize MapMask to 0?
Missing the initialization doesn't cause functionality issue but looks confusing.



> 
>    PageTable           = 0;
>    PageTableBufferSize = 0;
> --
> 2.31.1.windows.1

Re: [Patch V2 13/14] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf

[duntan]

I'll modify the comments and add a new patch to initialize MapMask to 0.
Thanks for the comment.

Thanks,
Dun

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, March 15, 2023 2:36 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: RE: [Patch V2 13/14] UefiCpuPkg: Fix IA32 build failure in CpuPageTableLib.inf

> +    UINT32    PageTableBaseAddressLow  : 19; // Page Table Base Address
> High

1. Comments say "High". Should be "Low".

> +
> 
> -  MapMask.Bits.PageTableBaseAddress = 1;
> -  MapMask.Bits.Present              = 1;
> -  MapMask.Bits.ReadWrite            = 1;
> +  MapMask.Bits.PageTableBaseAddressLow = 1;
> +  MapMask.Bits.Present                 = 1;
> +  MapMask.Bits.ReadWrite               = 1;

2. Can you please create a separate patch to initialize MapMask to 0?
Missing the initialization doesn't cause functionality issue but looks confusing.



> 
>    PageTable           = 0;
>    PageTableBufferSize = 0;
> --
> 2.31.1.windows.1

[Patch V2 14/14] UefiCpuPkg: Modify UnitTest code since tested API is changed

[duntan]

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>
Cc: 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 06a8fd3c02..34e5852579 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/UnitTest/CpuPageTableLibUnitTestHost.c
@@ -423,15 +423,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
   //
@@ -461,9 +460,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);
@@ -481,20 +480,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 a7f45fb175..56d894cc04 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) {
@@ -363,10 +370,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;
@@ -413,8 +422,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;
     }
   }
@@ -424,8 +434,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;
     }
   }
@@ -442,8 +454,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;
       }
     }
   }
@@ -457,8 +470,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) {
@@ -721,7 +734,7 @@ SingleMapEntryTest (
   if (((Attribute->Bits.Present == 0) || (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)) &&
+       ((Mask->Bits.PageTableBaseAddressLow == 0) && (Mask->Bits.PageTableBaseAddressHigh == 0)) || (Mask->Bits.ProtectionKey == 0) || (Mask->Bits.Nx == 0)) &&
       IsNotPresent)
   {
     RemoveLastMapEntry (MapEntrys);
@@ -1000,21 +1013,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 614bd6bbf1..64c42c5135 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) {
@@ -232,7 +232,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

Re: [Patch V2 14/14] UefiCpuPkg: Modify UnitTest code since tested API is changed

[Ni, Ray]

> @@ -413,8 +422,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;

1. "& IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK" is not needed.

Re: [Patch V2 14/14] UefiCpuPkg: Modify UnitTest code since tested API is changed

[Zhiguang Liu]

Hi Ray,

The Address could be not 4k align, "& IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK" is to clear the low 12 bit to avoid impact the attribute.
I think it is needed

Thanks
Zhiguang

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, March 15, 2023 2:43 PM
> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: RE: [Patch V2 14/14] UefiCpuPkg: Modify UnitTest code since tested
> API is changed
> 
> > @@ -413,8 +422,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;
> 
> 1. "& IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK" is not
> needed.

Re: [Patch V2 14/14] UefiCpuPkg: Modify UnitTest code since tested API is changed

[Ni, Ray]

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

> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Wednesday, March 15, 2023 5:26 PM
> To: Ni, Ray <ray.ni@intel.com>; 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>
> Subject: RE: [Patch V2 14/14] UefiCpuPkg: Modify UnitTest code since tested
> API is changed
> 
> Hi Ray,
> 
> The Address could be not 4k align, "&
> IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK" is to clear the
> low 12 bit to avoid impact the attribute.
> I think it is needed
> 
> Thanks
> Zhiguang
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Wednesday, March 15, 2023 2:43 PM
> > To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
> > Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> > Subject: RE: [Patch V2 14/14] UefiCpuPkg: Modify UnitTest code since
> tested
> > API is changed
> >
> > > @@ -413,8 +422,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;
> >
> > 1. "& IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS_MASK" is not
> > needed.