public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ni, Ray" <ray.ni@intel.com>
To: devel@edk2.groups.io
Cc: Zhiguang Liu <zhiguang.liu@intel.com>, Eric Dong <eric.dong@intel.com>
Subject: [PATCH 08/10] CpuPageTableLib: Fix a bug to avoid unnecessary changing to page table
Date: Mon, 18 Jul 2022 21:18:29 +0800	[thread overview]
Message-ID: <20220718131831.660-9-ray.ni@intel.com> (raw)
In-Reply-To: <20220718131831.660-1-ray.ni@intel.com>

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>
Cc: Eric Dong <eric.dong@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


  parent reply	other threads:[~2022-07-18 13:18 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-18 13:18 [PATCH 00/10] UefiCpuPkg: Create CpuPageTableLib for manipulating X86 paging structs Ni, Ray
2022-07-18 13:18 ` [PATCH 01/10] " Ni, Ray
2022-07-18 13:49   ` [edk2-devel] " Gerd Hoffmann
2022-07-19  8:17     ` Ni, Ray
2022-08-15 16:23   ` Lendacky, Thomas
2022-08-16  2:25     ` Ni, Ray
2022-07-18 13:18 ` [PATCH 02/10] UefiCpuPkg/CpuPageTableLib: Return error on invalid parameters Ni, Ray
2022-07-18 13:18 ` [PATCH 03/10] CpuPageTableLib: Fix a bug when a bit is 1 in Attribute, 0 in Mask Ni, Ray
2022-07-18 13:18 ` [PATCH 04/10] CpuPageTableLib: Refactor the logic Ni, Ray
2022-07-18 13:18 ` [PATCH 05/10] CpuPageTableLib: Split the page entry when LA is aligned but PA is not Ni, Ray
2022-07-18 13:18 ` [PATCH 06/10] CpuPageTableLib: Avoid treating non-leaf entry as leaf one Ni, Ray
2022-07-18 13:18 ` [PATCH 07/10] CpuPageTableLib: Fix parent attributes are not inherited properly Ni, Ray
2022-07-18 13:18 ` Ni, Ray [this message]
2022-07-18 13:18 ` [PATCH 09/10] CpuPageTableLib: Fix bug that wrongly requires extra size for mapping Ni, Ray
2022-07-18 13:18 ` [PATCH 10/10] CpuPageTableLib: define IA32_PAGE_LEVEL enum type internally Ni, Ray
2022-08-09  3:46 ` [edk2-devel] [PATCH 00/10] UefiCpuPkg: Create CpuPageTableLib for manipulating X86 paging structs Dong, Eric

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220718131831.660-9-ray.ni@intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox