[edk2-devel] [PATCH v5 02/38] ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field

Ard Biesheuvel posted 38 patches 1 year, 3 months ago
[edk2-devel] [PATCH v5 02/38] ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field
Posted by Ard Biesheuvel 1 year, 3 months ago
With large page support out of the picture, we can treat bits 1 and 0 of
the page descriptor as individual valid and XN bits, instead of treating
XN as a page type. Doing so aligns the handling of the attribute with
the section descriptor layout, as well as the XN handling on AArch64,
and this is beneficial for maintainability.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Include/Chipset/ArmV7Mmu.h              |  8 +++-----
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 12 ++++++------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h
index 7501ebfdf97f..6a2584ceb303 100644
--- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
+++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
@@ -54,11 +54,9 @@
 #define TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Desc)  (((Desc) & 3UL) == TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE)
 
 // Translation table descriptor types
-#define TT_DESCRIPTOR_PAGE_TYPE_MASK       (3UL << 0)
-#define TT_DESCRIPTOR_PAGE_TYPE_FAULT      (0UL << 0)
-#define TT_DESCRIPTOR_PAGE_TYPE_PAGE       (2UL << 0)
-#define TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN    (3UL << 0)
-#define TT_DESCRIPTOR_PAGE_TYPE_LARGEPAGE  (1UL << 0)
+#define TT_DESCRIPTOR_PAGE_TYPE_MASK   (1UL << 1)
+#define TT_DESCRIPTOR_PAGE_TYPE_FAULT  (0UL << 1)
+#define TT_DESCRIPTOR_PAGE_TYPE_PAGE   (1UL << 1)
 
 // Section descriptor definitions
 #define TT_DESCRIPTOR_SECTION_SIZE  (0x00100000)
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 9ca00c976d5f..12d0f4c30f8e 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -104,12 +104,8 @@ UpdatePageEntries (
 
   // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
   // EntryValue: values at bit positions specified by EntryMask
-  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
-  if ((Attributes & EFI_MEMORY_XP) != 0) {
-    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
-  } else {
-    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
-  }
+  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK | TT_DESCRIPTOR_PAGE_XN_MASK;
+  EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
 
   // Although the PI spec is unclear on this, the GCD guarantees that only
   // one Attribute bit is set at a time, so the order of the conditionals below
@@ -148,6 +144,10 @@ UpdatePageEntries (
     EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
   }
 
+  if ((Attributes & EFI_MEMORY_XP) != 0) {
+    EntryValue |= TT_DESCRIPTOR_PAGE_XN_MASK;
+  }
+
   // Obtain page table base
   FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
 
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101107): https://edk2.groups.io/g/devel/message/101107
Mute This Topic: https://groups.io/mt/97585984/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 02/38] ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field
Posted by Leif Lindholm 1 year, 3 months ago
On Mon, Mar 13, 2023 at 18:16:38 +0100, Ard Biesheuvel wrote:
> With large page support out of the picture, we can treat bits 1 and 0 of
> the page descriptor as individual valid and XN bits, instead of treating
> XN as a page type. Doing so aligns the handling of the attribute with
> the section descriptor layout, as well as the XN handling on AArch64,
> and this is beneficial for maintainability.

On average, this is a big enough improvement that I'm happy for it to
go in, but it *does* take a step away from the actual architectural
definition.

I'll choose to see it as viewing aarch32 through aarch64 goggles,
which feels reasonable these days.

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  ArmPkg/Include/Chipset/ArmV7Mmu.h              |  8 +++-----
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c | 12 ++++++------
>  2 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/ArmPkg/Include/Chipset/ArmV7Mmu.h b/ArmPkg/Include/Chipset/ArmV7Mmu.h
> index 7501ebfdf97f..6a2584ceb303 100644
> --- a/ArmPkg/Include/Chipset/ArmV7Mmu.h
> +++ b/ArmPkg/Include/Chipset/ArmV7Mmu.h
> @@ -54,11 +54,9 @@
>  #define TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Desc)  (((Desc) & 3UL) == TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE)
>  
>  // Translation table descriptor types
> -#define TT_DESCRIPTOR_PAGE_TYPE_MASK       (3UL << 0)
> -#define TT_DESCRIPTOR_PAGE_TYPE_FAULT      (0UL << 0)
> -#define TT_DESCRIPTOR_PAGE_TYPE_PAGE       (2UL << 0)
> -#define TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN    (3UL << 0)
> -#define TT_DESCRIPTOR_PAGE_TYPE_LARGEPAGE  (1UL << 0)
> +#define TT_DESCRIPTOR_PAGE_TYPE_MASK   (1UL << 1)
> +#define TT_DESCRIPTOR_PAGE_TYPE_FAULT  (0UL << 1)
> +#define TT_DESCRIPTOR_PAGE_TYPE_PAGE   (1UL << 1)
>  
>  // Section descriptor definitions
>  #define TT_DESCRIPTOR_SECTION_SIZE  (0x00100000)
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
> index 9ca00c976d5f..12d0f4c30f8e 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
> @@ -104,12 +104,8 @@ UpdatePageEntries (
>  
>    // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
>    // EntryValue: values at bit positions specified by EntryMask
> -  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
> -  if ((Attributes & EFI_MEMORY_XP) != 0) {
> -    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
> -  } else {
> -    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> -  }
> +  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK | TT_DESCRIPTOR_PAGE_XN_MASK;
> +  EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
>  
>    // Although the PI spec is unclear on this, the GCD guarantees that only
>    // one Attribute bit is set at a time, so the order of the conditionals below
> @@ -148,6 +144,10 @@ UpdatePageEntries (
>      EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
>    }
>  
> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> +    EntryValue |= TT_DESCRIPTOR_PAGE_XN_MASK;
> +  }
> +
>    // Obtain page table base
>    FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
>  
> -- 
> 2.39.2


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101167): https://edk2.groups.io/g/devel/message/101167
Mute This Topic: https://groups.io/mt/97585984/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-