[edk2-devel] [PATCH 08/10] CpuPageTableLib: Fix a bug to avoid unnecessary changing to page table

[Ni, Ray]

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....@intel.com>
Signed-off-by: Ray Ni <ray...@intel.com>
Cc: Eric Dong <eric.d...@intel.com>
---
 .../Library/CpuPageTableLib/CpuPageTableMap.c | 41 +++++++++++++++----
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c 
b/UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
index ac5d1c79f4..1205119fc8 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 {
       //
-- 
2.35.1.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91457): https://edk2.groups.io/g/devel/message/91457
Mute This Topic: https://groups.io/mt/92458166/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-