CpuPageTableLib: Fix a bug to avoid unnecessary changing to page table

With the following paging structure that maps [0, 2G] with ReadWrite
bit set.
PML4[0] --> PDPTE[0] --> PDE[0-255]
              \-> PDPTE[1] --> PDE[0-255]

If ReadWrite bit is cleared in PML4[0] and PageTableMap() is called
to change [0, 2M] as read-only, today's logic unnecessarily changes
the paging structure in 2 aspects:
1. When setting PageTableBaseAddress in the entry, the code clears
    all attributes.
2. Even the ReadWrite bit in parent entry is not set, the code clears
    the ReadWrite bit in the leaf entry.

First change is wrong. It should not change other attributes when
setting the PA.
Second change is unnecessary. Because the parent entry already
declares the whole region as read-only, there is no need to clear
ReadWrite bit in the leaf entry again.

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Eric Dong <eric.dong@intel.com>

1 files changed, 32 insertions, 9 deletions

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index db24205cbd..b1ff14e2b0 100644
--- a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
+++ b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
@@ -27,10 +27,7 @@ PageTableLibSetPte4K (
   )

 {

   if (Mask->Bits.PageTableBaseAddress) {

-    //

-    // Reset all attributes when the physical address is changed.

-    //

-    Pte4K->Uint64 = IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset;

+    Pte4K->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (Pte4K->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_40);

   }

 

   if (Mask->Bits.Present) {

@@ -97,10 +94,7 @@ PageTableLibSetPleB (
   )

 {

   if (Mask->Bits.PageTableBaseAddress) {

-    //

-    // Reset all attributes when the physical address is changed.

-    //

-    PleB->Uint64 = IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset;

+    PleB->Uint64 = (IA32_MAP_ATTRIBUTE_PAGE_TABLE_BASE_ADDRESS (Attribute) + Offset) | (PleB->Uint64 & ~IA32_PE_BASE_ADDRESS_MASK_39);

   }

 

   PleB->Bits.MustBeOne = 1;

@@ -277,6 +271,7 @@ PageTableLibMapInLevel (
   IA32_PAGING_ENTRY   OneOfPagingEntry;

   IA32_MAP_ATTRIBUTE  ChildAttribute;

   IA32_MAP_ATTRIBUTE  ChildMask;

+  IA32_MAP_ATTRIBUTE  CurrentMask;

 

   ASSERT (Level != 0);

   ASSERT ((Attribute != NULL) && (Mask != NULL));

@@ -464,7 +459,35 @@ PageTableLibMapInLevel (
       // Create one entry mapping the entire region (1G, 2M or 4K).

       //

       if (Modify) {

-        PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, Mask);

+        //

+        // When the inheritable attributes in parent entry could override the child attributes,

+        // e.g.: Present/ReadWrite/UserSupervisor is 0 in parent entry, or

+        //       Nx is 1 in parent entry,

+        // we just skip setting any value to these attributes in child.

+        // We add assertion to make sure the requested settings don't conflict with parent attributes in this case.

+        //

+        CurrentMask.Uint64 = Mask->Uint64;

+        if (ParentAttribute->Bits.Present == 0) {

+          CurrentMask.Bits.Present = 0;

+          ASSERT (CreateNew || (Mask->Bits.Present == 0) || (Attribute->Bits.Present == 0));

+        }

+

+        if (ParentAttribute->Bits.ReadWrite == 0) {

+          CurrentMask.Bits.ReadWrite = 0;

+          ASSERT (CreateNew || (Mask->Bits.ReadWrite == 0) || (Attribute->Bits.ReadWrite == 0));

+        }

+

+        if (ParentAttribute->Bits.UserSupervisor == 0) {

+          CurrentMask.Bits.UserSupervisor = 0;

+          ASSERT (CreateNew || (Mask->Bits.UserSupervisor == 0) || (Attribute->Bits.UserSupervisor == 0));

+        }

+

+        if (ParentAttribute->Bits.Nx == 1) {

+          CurrentMask.Bits.Nx = 0;

+          ASSERT (CreateNew || (Mask->Bits.Nx == 0) || (Attribute->Bits.Nx == 1));

+        }

+

+        PageTableLibSetPle (Level, CurrentPagingEntry, Offset, Attribute, &CurrentMask);

       }

     } else {

       //