[PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: final cleanups

[PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: final cleanups

[Ard Biesheuvel]

Some mostly cosmetic tweaks to the AArch64 MMU code that are mostly unrelated
to the actual fixes and improvements  I posted earlier, so I am posting them
separately.

Ard Biesheuvel (2):
  ArmPkg/ArmMmuLib AARCH64: drop pointless page table memory type check
  ArmPkg/ArmMmuLib AARCH64: cosmetic fixups

 .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 71 ++++++++++---------
 1 file changed, 37 insertions(+), 34 deletions(-)

-- 
2.17.1

[PATCH 1/2] ArmPkg/ArmMmuLib AARCH64: drop pointless page table memory type check

[Ard Biesheuvel]

This is the AARCH64 counterpart of commit 1f3b1eb3082206e4, to remove
a pointless check against the memory type of the allocations that the
page tables happened to land in. On ArmV8, we use writeback cacheable
exclusively for all memory.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 221175ca6535..f2eec7191328 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -497,7 +497,6 @@ ArmConfigureMmu (
   )
 {
   VOID*                         TranslationTable;
-  UINT32                        TranslationTableAttribute;
   UINT64                        MaxAddress;
   UINTN                         T0SZ;
   UINTN                         RootTableEntryCount;
@@ -618,18 +617,7 @@ ArmConfigureMmu (
     RootTableEntryCount * sizeof(UINT64));
   ZeroMem (TranslationTable, RootTableEntryCount * sizeof(UINT64));
 
-  TranslationTableAttribute = TT_ATTR_INDX_INVALID;
   while (MemoryTable->Length != 0) {
-
-    DEBUG_CODE_BEGIN ();
-      // Find the memory attribute for the Translation Table
-      if ((UINTN)TranslationTable >= MemoryTable->PhysicalBase &&
-          (UINTN)TranslationTable + EFI_PAGE_SIZE <= MemoryTable->PhysicalBase +
-                                                          MemoryTable->Length) {
-        TranslationTableAttribute = MemoryTable->Attributes;
-      }
-    DEBUG_CODE_END ();
-
     Status = FillTranslationTable (TranslationTable, MemoryTable);
     if (EFI_ERROR (Status)) {
       goto FREE_TRANSLATION_TABLE;
@@ -637,9 +625,6 @@ ArmConfigureMmu (
     MemoryTable++;
   }
 
-  ASSERT (TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK ||
-          TranslationTableAttribute == ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_WRITE_BACK);
-
   ArmSetMAIR (MAIR_ATTR(TT_ATTR_INDX_DEVICE_MEMORY, MAIR_ATTR_DEVICE_MEMORY) |                      // mapped to EFI_MEMORY_UC
               MAIR_ATTR(TT_ATTR_INDX_MEMORY_NON_CACHEABLE, MAIR_ATTR_NORMAL_MEMORY_NON_CACHEABLE) | // mapped to EFI_MEMORY_WC
               MAIR_ATTR(TT_ATTR_INDX_MEMORY_WRITE_THROUGH, MAIR_ATTR_NORMAL_MEMORY_WRITE_THROUGH) | // mapped to EFI_MEMORY_WT
-- 
2.17.1

[PATCH 2/2] ArmPkg/ArmMmuLib AARCH64: cosmetic fixups

[Ard Biesheuvel]

Some cosmetic fixups to the AArch64 MMU code:
- reflow overly long lines unless it hurts legibility
- add/remove whitespace according to the [de facto] coding style
- use camel case for goto labels

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 56 +++++++++++++-------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index f2eec7191328..a43d468b73ca 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -47,7 +47,7 @@ ArmMemoryAttributeToPageAttribute (
     return TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
 
   default:
-    ASSERT(0);
+    ASSERT (0);
   case ARM_MEMORY_REGION_ATTRIBUTE_DEVICE:
   case ARM_MEMORY_REGION_ATTRIBUTE_NONSECURE_DEVICE:
     if (ArmReadCurrentEL () == AARCH64_EL2)
@@ -78,7 +78,9 @@ PageAttributeToGcdAttribute (
     GcdAttributes = EFI_MEMORY_WB;
     break;
   default:
-    DEBUG ((EFI_D_ERROR, "PageAttributeToGcdAttribute: PageAttributes:0x%lX not supported.\n", PageAttributes));
+    DEBUG ((DEBUG_ERROR,
+      "PageAttributeToGcdAttribute: PageAttributes:0x%lX not supported.\n",
+      PageAttributes));
     ASSERT (0);
     // The Global Coherency Domain (GCD) value is defined as a bit set.
     // Returning 0 means no attribute has been set.
@@ -86,13 +88,14 @@ PageAttributeToGcdAttribute (
   }
 
   // Determine protection attributes
-  if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) || ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) {
+  if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) ||
+      ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) {
     // Read only cases map to write-protect
     GcdAttributes |= EFI_MEMORY_RO;
   }
 
   // Process eXecute Never attribute
-  if ((PageAttributes & (TT_PXN_MASK | TT_UXN_MASK)) != 0 ) {
+  if ((PageAttributes & (TT_PXN_MASK | TT_UXN_MASK)) != 0) {
     GcdAttributes |= EFI_MEMORY_XP;
   }
 
@@ -503,7 +506,7 @@ ArmConfigureMmu (
   UINT64                        TCR;
   EFI_STATUS                    Status;
 
-  if(MemoryTable == NULL) {
+  if (MemoryTable == NULL) {
     ASSERT (MemoryTable != NULL);
     return EFI_INVALID_PARAMETER;
   }
@@ -544,7 +547,9 @@ ArmConfigureMmu (
     } else if (MaxAddress < SIZE_256TB) {
       TCR |= TCR_PS_256TB;
     } else {
-      DEBUG ((EFI_D_ERROR, "ArmConfigureMmu: The MaxAddress 0x%lX is not supported by this MMU configuration.\n", MaxAddress));
+      DEBUG ((DEBUG_ERROR,
+        "ArmConfigureMmu: The MaxAddress 0x%lX is not supported by this MMU configuration.\n",
+        MaxAddress));
       ASSERT (0); // Bigger than 48-bit memory space are not supported
       return EFI_UNSUPPORTED;
     }
@@ -566,7 +571,9 @@ ArmConfigureMmu (
     } else if (MaxAddress < SIZE_256TB) {
       TCR |= TCR_IPS_256TB;
     } else {
-      DEBUG ((EFI_D_ERROR, "ArmConfigureMmu: The MaxAddress 0x%lX is not supported by this MMU configuration.\n", MaxAddress));
+      DEBUG ((DEBUG_ERROR,
+        "ArmConfigureMmu: The MaxAddress 0x%lX is not supported by this MMU configuration.\n",
+        MaxAddress));
       ASSERT (0); // Bigger than 48-bit memory space are not supported
       return EFI_UNSUPPORTED;
     }
@@ -596,9 +603,12 @@ ArmConfigureMmu (
   if (TranslationTable == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
-  // We set TTBR0 just after allocating the table to retrieve its location from the subsequent
-  // functions without needing to pass this value across the functions. The MMU is only enabled
-  // after the translation tables are populated.
+  //
+  // We set TTBR0 just after allocating the table to retrieve its location from
+  // the subsequent functions without needing to pass this value across the
+  // functions. The MMU is only enabled after the translation tables are
+  // populated.
+  //
   ArmSetTTBR0 (TranslationTable);
 
   if (TranslationTableBase != NULL) {
@@ -606,7 +616,7 @@ ArmConfigureMmu (
   }
 
   if (TranslationTableSize != NULL) {
-    *TranslationTableSize = RootTableEntryCount * sizeof(UINT64);
+    *TranslationTableSize = RootTableEntryCount * sizeof (UINT64);
   }
 
   //
@@ -614,21 +624,29 @@ ArmConfigureMmu (
   // when populating the page tables.
   //
   InvalidateDataCacheRange (TranslationTable,
-    RootTableEntryCount * sizeof(UINT64));
-  ZeroMem (TranslationTable, RootTableEntryCount * sizeof(UINT64));
+    RootTableEntryCount * sizeof (UINT64));
+  ZeroMem (TranslationTable, RootTableEntryCount * sizeof (UINT64));
 
   while (MemoryTable->Length != 0) {
     Status = FillTranslationTable (TranslationTable, MemoryTable);
     if (EFI_ERROR (Status)) {
-      goto FREE_TRANSLATION_TABLE;
+      goto FreeTranslationTable;
     }
     MemoryTable++;
   }
 
-  ArmSetMAIR (MAIR_ATTR(TT_ATTR_INDX_DEVICE_MEMORY, MAIR_ATTR_DEVICE_MEMORY) |                      // mapped to EFI_MEMORY_UC
-              MAIR_ATTR(TT_ATTR_INDX_MEMORY_NON_CACHEABLE, MAIR_ATTR_NORMAL_MEMORY_NON_CACHEABLE) | // mapped to EFI_MEMORY_WC
-              MAIR_ATTR(TT_ATTR_INDX_MEMORY_WRITE_THROUGH, MAIR_ATTR_NORMAL_MEMORY_WRITE_THROUGH) | // mapped to EFI_MEMORY_WT
-              MAIR_ATTR(TT_ATTR_INDX_MEMORY_WRITE_BACK, MAIR_ATTR_NORMAL_MEMORY_WRITE_BACK));       // mapped to EFI_MEMORY_WB
+  //
+  // EFI_MEMORY_UC ==> MAIR_ATTR_DEVICE_MEMORY
+  // EFI_MEMORY_WC ==> MAIR_ATTR_NORMAL_MEMORY_NON_CACHEABLE
+  // EFI_MEMORY_WT ==> MAIR_ATTR_NORMAL_MEMORY_WRITE_THROUGH
+  // EFI_MEMORY_WB ==> MAIR_ATTR_NORMAL_MEMORY_WRITE_BACK
+  //
+  ArmSetMAIR (
+    MAIR_ATTR (TT_ATTR_INDX_DEVICE_MEMORY,        MAIR_ATTR_DEVICE_MEMORY)               |
+    MAIR_ATTR (TT_ATTR_INDX_MEMORY_NON_CACHEABLE, MAIR_ATTR_NORMAL_MEMORY_NON_CACHEABLE) |
+    MAIR_ATTR (TT_ATTR_INDX_MEMORY_WRITE_THROUGH, MAIR_ATTR_NORMAL_MEMORY_WRITE_THROUGH) |
+    MAIR_ATTR (TT_ATTR_INDX_MEMORY_WRITE_BACK,    MAIR_ATTR_NORMAL_MEMORY_WRITE_BACK)
+    );
 
   ArmDisableAlignmentCheck ();
   ArmEnableStackAlignmentCheck ();
@@ -638,7 +656,7 @@ ArmConfigureMmu (
   ArmEnableMmu ();
   return EFI_SUCCESS;
 
-FREE_TRANSLATION_TABLE:
+FreeTranslationTable:
   FreePages (TranslationTable, 1);
   return Status;
 }
-- 
2.17.1

Re: [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: final cleanups

[Leif Lindholm]

On Sat, Mar 07, 2020 at 10:10:06 +0100, Ard Biesheuvel wrote:
> Some mostly cosmetic tweaks to the AArch64 MMU code that are mostly unrelated
> to the actual fixes and improvements  I posted earlier, so I am posting them
> separately.

For the series:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

Thanks!

> Ard Biesheuvel (2):
>   ArmPkg/ArmMmuLib AARCH64: drop pointless page table memory type check
>   ArmPkg/ArmMmuLib AARCH64: cosmetic fixups
> 
>  .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 71 ++++++++++---------
>  1 file changed, 37 insertions(+), 34 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [edk2-devel] [PATCH 0/2] ArmPkg/ArmMmuLib AARCH64: final cleanups

[Laszlo Ersek]

On 03/07/20 10:10, Ard Biesheuvel wrote:
> Some mostly cosmetic tweaks to the AArch64 MMU code that are mostly unrelated
> to the actual fixes and improvements  I posted earlier, so I am posting them
> separately.
> 
> Ard Biesheuvel (2):
>   ArmPkg/ArmMmuLib AARCH64: drop pointless page table memory type check
>   ArmPkg/ArmMmuLib AARCH64: cosmetic fixups
> 
>  .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 71 ++++++++++---------
>  1 file changed, 37 insertions(+), 34 deletions(-)
> 

Commit range
748fea6279efc20de3fef483deb4b774f3c34906..4249278aa686a695c0904b08801134ab289a0b3e,
via <https://github.com/tianocore/edk2/pull/431>.

Thanks
Laszlo