public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/5] ArmPkg, ArmVirtPkg ARM: enable non-executable stack
@ 2017-03-01 16:31 Ard Biesheuvel
  2017-03-01 16:31 ` [PATCH 1/5] ArmPkg/ArmMmuLib AARCH64: use correct return type for exported functions Ard Biesheuvel
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2017-03-01 16:31 UTC (permalink / raw)
  To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel

This refactors the ARM version of CpuDxe and ArmMmuLib to align more closely
with the AARCH64 version, which primarily comes down to moving
ArmSetMemoryAttributes() to ArmMmuLib, where it can be used by non-DXE
modules such as DxeIpl. This is a PEI module which is in charge of
configuring the non-executable DXE stack.

Ard Biesheuvel (5):
  ArmPkg/ArmMmuLib AARCH64: use correct return type for exported
    functions
  ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib
  ArmPkg/ArmMmuLib: remove VirtualMask arg from ArmSetMemoryAttributes
  ArmPkg/ArmMmuLib ARM: implement memory permission control routines
  ArmVirtPkg: enable non-executable DXE stack for all platforms

 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 368 -------------------
 ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  14 +-
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |   2 +-
 ArmPkg/Include/Library/ArmMmuLib.h               |   7 +
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  57 ++-
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     | 371 +++++++++++++++++++-
 ArmVirtPkg/ArmVirt.dsc.inc                       |   5 +
 ArmVirtPkg/ArmVirtQemu.dsc                       |   2 -
 ArmVirtPkg/ArmVirtQemuKernel.dsc                 |   2 -
 9 files changed, 405 insertions(+), 423 deletions(-)

-- 
2.7.4



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5] ArmPkg/ArmMmuLib AARCH64: use correct return type for exported functions
  2017-03-01 16:31 [PATCH 0/5] ArmPkg, ArmVirtPkg ARM: enable non-executable stack Ard Biesheuvel
@ 2017-03-01 16:31 ` Ard Biesheuvel
  2017-03-06 14:57   ` Leif Lindholm
  2017-03-01 16:31 ` [PATCH 2/5] ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib Ard Biesheuvel
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2017-03-01 16:31 UTC (permalink / raw)
  To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel

The routines ArmConfigureMmu() and SetMemoryAttributes() [*] are
declared as returning EFI_STATUS in the respective header files,
so align the definitions with that.

* SetMemoryAttributes() is declared in the wrong header (and defined in
  ArmMmuLib for AARCH64 and in CpuDxe for ARM)

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 52 ++++++++++----------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 2f8f99d44a31..df170d20a2c2 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -329,7 +329,7 @@ GetBlockEntryListFromAddress (
 }
 
 STATIC
-RETURN_STATUS
+EFI_STATUS
 UpdateRegionMapping (
   IN  UINT64  *RootTable,
   IN  UINT64  RegionStart,
@@ -347,7 +347,7 @@ UpdateRegionMapping (
   // Ensure the Length is aligned on 4KB boundary
   if ((RegionLength == 0) || ((RegionLength & (SIZE_4KB - 1)) != 0)) {
     ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
-    return RETURN_INVALID_PARAMETER;
+    return EFI_INVALID_PARAMETER;
   }
 
   do {
@@ -357,7 +357,7 @@ UpdateRegionMapping (
     BlockEntry = GetBlockEntryListFromAddress (RootTable, RegionStart, &TableLevel, &BlockEntrySize, &LastBlockEntry);
     if (BlockEntry == NULL) {
       // GetBlockEntryListFromAddress() return NULL when it fails to allocate new pages from the Translation Tables
-      return RETURN_OUT_OF_RESOURCES;
+      return EFI_OUT_OF_RESOURCES;
     }
 
     if (TableLevel != 3) {
@@ -385,11 +385,11 @@ UpdateRegionMapping (
     } while ((RegionLength >= BlockEntrySize) && (BlockEntry <= LastBlockEntry));
   } while (RegionLength != 0);
 
-  return RETURN_SUCCESS;
+  return EFI_SUCCESS;
 }
 
 STATIC
-RETURN_STATUS
+EFI_STATUS
 FillTranslationTable (
   IN  UINT64                        *RootTable,
   IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryRegion
@@ -446,7 +446,7 @@ GcdAttributeToPageAttribute (
   return PageAttributes | TT_AF;
 }
 
-RETURN_STATUS
+EFI_STATUS
 SetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN UINT64                    Length,
@@ -454,7 +454,7 @@ SetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS      VirtualMask
   )
 {
-  RETURN_STATUS                Status;
+  EFI_STATUS                   Status;
   UINT64                      *TranslationTable;
   UINT64                       PageAttributes;
   UINT64                       PageAttributeMask;
@@ -480,18 +480,18 @@ SetMemoryAttributes (
              Length,
              PageAttributes,
              PageAttributeMask);
-  if (RETURN_ERROR (Status)) {
+  if (EFI_ERROR (Status)) {
     return Status;
   }
 
   // Invalidate all TLB entries so changes are synced
   ArmInvalidateTlb ();
 
-  return RETURN_SUCCESS;
+  return EFI_SUCCESS;
 }
 
 STATIC
-RETURN_STATUS
+EFI_STATUS
 SetMemoryRegionAttribute (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length,
@@ -499,23 +499,23 @@ SetMemoryRegionAttribute (
   IN  UINT64                    BlockEntryMask
   )
 {
-  RETURN_STATUS                Status;
+  EFI_STATUS                   Status;
   UINT64                       *RootTable;
 
   RootTable = ArmGetTTBR0BaseAddress ();
 
   Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, BlockEntryMask);
-  if (RETURN_ERROR (Status)) {
+  if (EFI_ERROR (Status)) {
     return Status;
   }
 
   // Invalidate all TLB entries so changes are synced
   ArmInvalidateTlb ();
 
-  return RETURN_SUCCESS;
+  return EFI_SUCCESS;
 }
 
-RETURN_STATUS
+EFI_STATUS
 ArmSetMemoryRegionNoExec (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length
@@ -536,7 +536,7 @@ ArmSetMemoryRegionNoExec (
            ~TT_ADDRESS_MASK_BLOCK_ENTRY);
 }
 
-RETURN_STATUS
+EFI_STATUS
 ArmClearMemoryRegionNoExec (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length
@@ -554,7 +554,7 @@ ArmClearMemoryRegionNoExec (
            Mask);
 }
 
-RETURN_STATUS
+EFI_STATUS
 ArmSetMemoryRegionReadOnly (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length
@@ -567,7 +567,7 @@ ArmSetMemoryRegionReadOnly (
            ~TT_ADDRESS_MASK_BLOCK_ENTRY);
 }
 
-RETURN_STATUS
+EFI_STATUS
 ArmClearMemoryRegionReadOnly (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length
@@ -580,7 +580,7 @@ ArmClearMemoryRegionReadOnly (
            ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK));
 }
 
-RETURN_STATUS
+EFI_STATUS
 EFIAPI
 ArmConfigureMmu (
   IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable,
@@ -594,11 +594,11 @@ ArmConfigureMmu (
   UINTN                         T0SZ;
   UINTN                         RootTableEntryCount;
   UINT64                        TCR;
-  RETURN_STATUS                 Status;
+  EFI_STATUS                    Status;
 
   if(MemoryTable == NULL) {
     ASSERT (MemoryTable != NULL);
-    return RETURN_INVALID_PARAMETER;
+    return EFI_INVALID_PARAMETER;
   }
 
   // Cover the entire GCD memory space
@@ -632,7 +632,7 @@ ArmConfigureMmu (
     } else {
       DEBUG ((EFI_D_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 RETURN_UNSUPPORTED;
+      return EFI_UNSUPPORTED;
     }
   } else if (ArmReadCurrentEL () == AARCH64_EL1) {
     // Due to Cortex-A57 erratum #822227 we must set TG1[1] == 1, regardless of EPD1.
@@ -654,11 +654,11 @@ ArmConfigureMmu (
     } else {
       DEBUG ((EFI_D_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 RETURN_UNSUPPORTED;
+      return EFI_UNSUPPORTED;
     }
   } else {
     ASSERT (0); // UEFI is only expected to run at EL2 and EL1, not EL3.
-    return RETURN_UNSUPPORTED;
+    return EFI_UNSUPPORTED;
   }
 
   //
@@ -680,7 +680,7 @@ ArmConfigureMmu (
   // Allocate pages for translation table
   TranslationTable = AllocatePages (1);
   if (TranslationTable == NULL) {
-    return RETURN_OUT_OF_RESOURCES;
+    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
@@ -719,7 +719,7 @@ ArmConfigureMmu (
     DEBUG_CODE_END ();
 
     Status = FillTranslationTable (TranslationTable, MemoryTable);
-    if (RETURN_ERROR (Status)) {
+    if (EFI_ERROR (Status)) {
       goto FREE_TRANSLATION_TABLE;
     }
     MemoryTable++;
@@ -739,7 +739,7 @@ ArmConfigureMmu (
   ArmEnableDataCache ();
 
   ArmEnableMmu ();
-  return RETURN_SUCCESS;
+  return EFI_SUCCESS;
 
 FREE_TRANSLATION_TABLE:
   FreePages (TranslationTable, 1);
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/5] ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib
  2017-03-01 16:31 [PATCH 0/5] ArmPkg, ArmVirtPkg ARM: enable non-executable stack Ard Biesheuvel
  2017-03-01 16:31 ` [PATCH 1/5] ArmPkg/ArmMmuLib AARCH64: use correct return type for exported functions Ard Biesheuvel
@ 2017-03-01 16:31 ` Ard Biesheuvel
  2017-03-06 16:03   ` Leif Lindholm
  2017-03-01 16:31 ` [PATCH 3/5] ArmPkg/ArmMmuLib: remove VirtualMask arg from ArmSetMemoryAttributes Ard Biesheuvel
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2017-03-01 16:31 UTC (permalink / raw)
  To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel

... where it belongs, since AARCH64 already keeps it there, and
non DXE users of ArmMmuLib (such as DxeIpl, for the non-executable
stack) may need its functionality as well.

While at it, rename SetMemoryAttributes to ArmSetMemoryAttributes,
and make any functions that are not exported STATIC. Also, replace
an explicit gBS->AllocatePages() call [which is DXE specific] with
MemoryAllocationLib::AllocatePages().

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 368 --------------------
 ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  14 +-
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |   2 +-
 ArmPkg/Include/Library/ArmMmuLib.h               |   8 +
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |   2 +-
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     | 368 ++++++++++++++++++++
 6 files changed, 379 insertions(+), 383 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 6322d301060e..b985dd743f02 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -343,374 +343,6 @@ SyncCacheConfig (
   return EFI_SUCCESS;
 }
 
-
-
-EFI_STATUS
-UpdatePageEntries (
-  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
-  IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
-  )
-{
-  EFI_STATUS    Status;
-  UINT32        EntryValue;
-  UINT32        EntryMask;
-  UINT32        FirstLevelIdx;
-  UINT32        Offset;
-  UINT32        NumPageEntries;
-  UINT32        Descriptor;
-  UINT32        p;
-  UINT32        PageTableIndex;
-  UINT32        PageTableEntry;
-  UINT32        CurrentPageTableEntry;
-  VOID          *Mva;
-
-  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
-  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
-
-  Status = EFI_SUCCESS;
-
-  // 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;
-  }
-
-  // Although the PI spec is unclear on this the GCD guarantees that only
-  // one Attribute bit is set at a time, so we can safely use a switch statement
-  if ((Attributes & EFI_MEMORY_UC) != 0) {
-    // modify cacheability attributes
-    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-    // map to strongly ordered
-    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
-  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
-    // modify cacheability attributes
-    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-    // map to normal non-cachable
-    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
-  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
-    // modify cacheability attributes
-    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-    // write through with no-allocate
-    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
-  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
-    // modify cacheability attributes
-    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
-    // write back (with allocate)
-    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
-  }
-
-  if ((Attributes & EFI_MEMORY_RO) != 0) {
-    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
-  } else {
-    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
-  }
-
-  // Obtain page table base
-  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
-
-  // Calculate number of 4KB page table entries to change
-  NumPageEntries = Length / TT_DESCRIPTOR_PAGE_SIZE;
-
-  // Iterate for the number of 4KB pages to change
-  Offset = 0;
-  for(p = 0; p < NumPageEntries; p++) {
-    // Calculate index into first level translation table for page table value
-
-    FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress + Offset) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
-    ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
-
-    // Read the descriptor from the first level page table
-    Descriptor = FirstLevelTable[FirstLevelIdx];
-
-    // Does this descriptor need to be converted from section entry to 4K pages?
-    if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Descriptor)) {
-      Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
-      if (EFI_ERROR(Status)) {
-        // Exit for loop
-        break;
-      }
-
-      // Re-read descriptor
-      Descriptor = FirstLevelTable[FirstLevelIdx];
-    }
-
-    // Obtain page table base address
-    PageTable = (ARM_PAGE_TABLE_ENTRY *)TT_DESCRIPTOR_PAGE_BASE_ADDRESS(Descriptor);
-
-    // Calculate index into the page table
-    PageTableIndex = ((BaseAddress + Offset) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
-    ASSERT (PageTableIndex < TRANSLATION_TABLE_PAGE_COUNT);
-
-    // Get the entry
-    CurrentPageTableEntry = PageTable[PageTableIndex];
-
-    // Mask off appropriate fields
-    PageTableEntry = CurrentPageTableEntry & ~EntryMask;
-
-    // Mask in new attributes and/or permissions
-    PageTableEntry |= EntryValue;
-
-    if (VirtualMask != 0) {
-      // Make this virtual address point at a physical page
-      PageTableEntry &= ~VirtualMask;
-    }
-
-    if (CurrentPageTableEntry  != PageTableEntry) {
-      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
-      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
-        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
-        // Note assumes switch(Attributes), not ARMv7 possibilities
-        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
-      }
-
-      // Only need to update if we are changing the entry
-      PageTable[PageTableIndex] = PageTableEntry;
-      ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
-    }
-
-    Status = EFI_SUCCESS;
-    Offset += TT_DESCRIPTOR_PAGE_SIZE;
-
-  } // End first level translation table loop
-
-  return Status;
-}
-
-
-
-EFI_STATUS
-UpdateSectionEntries (
-  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
-  IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
-  )
-{
-  EFI_STATUS    Status = EFI_SUCCESS;
-  UINT32        EntryMask;
-  UINT32        EntryValue;
-  UINT32        FirstLevelIdx;
-  UINT32        NumSections;
-  UINT32        i;
-  UINT32        CurrentDescriptor;
-  UINT32        Descriptor;
-  VOID          *Mva;
-  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
-
-  // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
-  // EntryValue: values at bit positions specified by EntryMask
-
-  // Make sure we handle a section range that is unmapped
-  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
-              TT_DESCRIPTOR_SECTION_AP_MASK;
-  EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
-
-  // Although the PI spec is unclear on this the GCD guarantees that only
-  // one Attribute bit is set at a time, so we can safely use a switch statement
-  if ((Attributes & EFI_MEMORY_UC) != 0) {
-    // modify cacheability attributes
-    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-    // map to strongly ordered
-    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
-  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
-    // modify cacheability attributes
-    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-    // map to normal non-cachable
-    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
-  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
-    // modify cacheability attributes
-    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-    // write through with no-allocate
-    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
-  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
-    // modify cacheability attributes
-    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
-    // write back (with allocate)
-    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
-  }
-
-  if ((Attributes & EFI_MEMORY_RO) != 0) {
-    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
-  } else {
-    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
-  }
-
-  if ((Attributes & EFI_MEMORY_XP) != 0) {
-    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
-  }
-
-  // obtain page table base
-  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
-
-  // calculate index into first level translation table for start of modification
-  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
-  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
-
-  // calculate number of 1MB first level entries this applies to
-  NumSections = Length / TT_DESCRIPTOR_SECTION_SIZE;
-
-  // iterate through each descriptor
-  for(i=0; i<NumSections; i++) {
-    CurrentDescriptor = FirstLevelTable[FirstLevelIdx + i];
-
-    // has this descriptor already been coverted to pages?
-    if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
-      // forward this 1MB range to page table function instead
-      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
-    } else {
-      // still a section entry
-
-      // mask off appropriate fields
-      Descriptor = CurrentDescriptor & ~EntryMask;
-
-      // mask in new attributes and/or permissions
-      Descriptor |= EntryValue;
-      if (VirtualMask != 0) {
-        Descriptor &= ~VirtualMask;
-      }
-
-      if (CurrentDescriptor  != Descriptor) {
-        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
-        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
-          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
-          // Note assumes switch(Attributes), not ARMv7 possabilities
-          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
-        }
-
-        // Only need to update if we are changing the descriptor
-        FirstLevelTable[FirstLevelIdx + i] = Descriptor;
-        ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
-      }
-
-      Status = EFI_SUCCESS;
-    }
-  }
-
-  return Status;
-}
-
-EFI_STATUS
-ConvertSectionToPages (
-  IN EFI_PHYSICAL_ADDRESS  BaseAddress
-  )
-{
-  EFI_STATUS              Status;
-  EFI_PHYSICAL_ADDRESS    PageTableAddr;
-  UINT32                  FirstLevelIdx;
-  UINT32                  SectionDescriptor;
-  UINT32                  PageTableDescriptor;
-  UINT32                  PageDescriptor;
-  UINT32                  Index;
-
-  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
-  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
-
-  DEBUG ((EFI_D_PAGE, "Converting section at 0x%x to pages\n", (UINTN)BaseAddress));
-
-  // Obtain page table base
-  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
-
-  // Calculate index into first level translation table for start of modification
-  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
-  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
-
-  // Get section attributes and convert to page attributes
-  SectionDescriptor = FirstLevelTable[FirstLevelIdx];
-  PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor, FALSE);
-
-  // Allocate a page table for the 4KB entries (we use up a full page even though we only need 1KB)
-  Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData, 1, &PageTableAddr);
-  if (EFI_ERROR(Status)) {
-    return Status;
-  }
-
-  PageTable = (volatile ARM_PAGE_TABLE_ENTRY *)(UINTN)PageTableAddr;
-
-  // Write the page table entries out
-  for (Index = 0; Index < TRANSLATION_TABLE_PAGE_COUNT; Index++) {
-    PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;
-  }
-
-  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
-  WriteBackInvalidateDataCacheRange ((VOID *)(UINTN)PageTableAddr, TT_DESCRIPTOR_PAGE_SIZE);
-
-  // Formulate page table entry, Domain=0, NS=0
-  PageTableDescriptor = (((UINTN)PageTableAddr) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
-
-  // Write the page table entry out, replacing section entry
-  FirstLevelTable[FirstLevelIdx] = PageTableDescriptor;
-
-  return EFI_SUCCESS;
-}
-
-
-
-EFI_STATUS
-SetMemoryAttributes (
-  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
-  IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
-  )
-{
-  EFI_STATUS    Status;
-  UINT64        ChunkLength;
-  BOOLEAN       FlushTlbs;
-
-  FlushTlbs = FALSE;
-  while (Length > 0) {
-    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
-        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
-
-      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
-
-      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
-        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
-        BaseAddress, ChunkLength, Attributes));
-
-      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
-                 VirtualMask);
-
-      FlushTlbs = TRUE;
-    } else {
-
-      //
-      // Process page by page until the next section boundary, but only if
-      // we have more than a section's worth of area to deal with after that.
-      //
-      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
-                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
-      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
-        ChunkLength = Length;
-      }
-
-      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
-        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
-        BaseAddress, ChunkLength, Attributes));
-
-      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
-                 VirtualMask);
-    }
-
-    if (EFI_ERROR (Status)) {
-      break;
-    }
-
-    BaseAddress += ChunkLength;
-    Length -= ChunkLength;
-  }
-
-  if (FlushTlbs) {
-    ArmInvalidateTlb ();
-  }
-  return Status;
-}
-
 UINT64
 EfiAttributeToArmAttribute (
   IN UINT64                    EfiAttributes
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index a46db8d25754..a0f71e69ec09 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -19,6 +19,7 @@
 #include <Uefi.h>
 
 #include <Library/ArmLib.h>
+#include <Library/ArmMmuLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
 #include <Library/PcdLib.h>
@@ -112,11 +113,6 @@ SyncCacheConfig (
   IN  EFI_CPU_ARCH_PROTOCOL *CpuProtocol
   );
 
-EFI_STATUS
-ConvertSectionToPages (
-  IN EFI_PHYSICAL_ADDRESS  BaseAddress
-  );
-
 /**
  * Publish ARM Processor Data table in UEFI SYSTEM Table.
  * @param  HobStart               Pointer to the beginning of the HOB List from PEI.
@@ -132,14 +128,6 @@ PublishArmProcessorTable(
   VOID
   );
 
-EFI_STATUS
-SetMemoryAttributes (
-  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
-  IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
-  );
-
 // The ARM Attributes might be defined on 64-bit (case of the long format description table)
 UINT64
 EfiAttributeToArmAttribute (
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index 0f36a058407a..d0a3fedd3aa7 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -210,7 +210,7 @@ CpuSetMemoryAttributes (
   if (EFI_ERROR (Status) || (RegionArmAttributes != ArmAttributes) ||
       ((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))
   {
-    return SetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
+    return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
   } else {
     return EFI_SUCCESS;
   }
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
index c1d43872d548..d3a302fa8125 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -62,4 +62,12 @@ ArmReplaceLiveTranslationEntry (
   IN  UINT64  Value
   );
 
+EFI_STATUS
+ArmSetMemoryAttributes (
+  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN UINT64                    Length,
+  IN UINT64                    Attributes,
+  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  );
+
 #endif
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index df170d20a2c2..77f108971f3e 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -447,7 +447,7 @@ GcdAttributeToPageAttribute (
 }
 
 EFI_STATUS
-SetMemoryAttributes (
+ArmSetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN UINT64                    Length,
   IN UINT64                    Attributes,
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 4b6f4ce392b7..93980d6d12db 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -16,6 +16,7 @@
 #include <Uefi.h>
 #include <Chipset/ArmV7.h>
 #include <Library/BaseMemoryLib.h>
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/ArmLib.h>
 #include <Library/BaseLib.h>
@@ -36,6 +37,12 @@
 #define ID_MMFR0_SHR_IMP_HW_COHERENT   1
 #define ID_MMFR0_SHR_IGNORED         0xf
 
+// First Level Descriptors
+typedef UINT32    ARM_FIRST_LEVEL_DESCRIPTOR;
+
+// Second Level Descriptors
+typedef UINT32    ARM_PAGE_TABLE_ENTRY;
+
 UINTN
 EFIAPI
 ArmReadIdMmfr0 (
@@ -406,6 +413,367 @@ ArmConfigureMmu (
   return RETURN_SUCCESS;
 }
 
+STATIC
+EFI_STATUS
+ConvertSectionToPages (
+  IN EFI_PHYSICAL_ADDRESS  BaseAddress
+  )
+{
+  UINT32                  FirstLevelIdx;
+  UINT32                  SectionDescriptor;
+  UINT32                  PageTableDescriptor;
+  UINT32                  PageDescriptor;
+  UINT32                  Index;
+
+  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
+  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
+
+  DEBUG ((EFI_D_PAGE, "Converting section at 0x%x to pages\n", (UINTN)BaseAddress));
+
+  // Obtain page table base
+  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
+
+  // Calculate index into first level translation table for start of modification
+  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
+  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
+
+  // Get section attributes and convert to page attributes
+  SectionDescriptor = FirstLevelTable[FirstLevelIdx];
+  PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor, FALSE);
+
+  // Allocate a page table for the 4KB entries (we use up a full page even though we only need 1KB)
+  PageTable = (volatile ARM_PAGE_TABLE_ENTRY *)AllocatePages (1);
+  if (PageTable == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  // Write the page table entries out
+  for (Index = 0; Index < TRANSLATION_TABLE_PAGE_COUNT; Index++) {
+    PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;
+  }
+
+  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
+  WriteBackInvalidateDataCacheRange ((VOID *)PageTable, TT_DESCRIPTOR_PAGE_SIZE);
+
+  // Formulate page table entry, Domain=0, NS=0
+  PageTableDescriptor = (((UINTN)PageTable) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
+
+  // Write the page table entry out, replacing section entry
+  FirstLevelTable[FirstLevelIdx] = PageTableDescriptor;
+
+  return EFI_SUCCESS;
+}
+
+STATIC
+EFI_STATUS
+UpdatePageEntries (
+  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN UINT64                    Length,
+  IN UINT64                    Attributes,
+  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  )
+{
+  EFI_STATUS    Status;
+  UINT32        EntryValue;
+  UINT32        EntryMask;
+  UINT32        FirstLevelIdx;
+  UINT32        Offset;
+  UINT32        NumPageEntries;
+  UINT32        Descriptor;
+  UINT32        p;
+  UINT32        PageTableIndex;
+  UINT32        PageTableEntry;
+  UINT32        CurrentPageTableEntry;
+  VOID          *Mva;
+
+  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
+  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
+
+  Status = EFI_SUCCESS;
+
+  // 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;
+  }
+
+  // Although the PI spec is unclear on this the GCD guarantees that only
+  // one Attribute bit is set at a time, so we can safely use a switch statement
+  if ((Attributes & EFI_MEMORY_UC) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // map to strongly ordered
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
+  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // map to normal non-cachable
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
+  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // write through with no-allocate
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
+  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
+    // write back (with allocate)
+    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
+  }
+
+  if ((Attributes & EFI_MEMORY_RO) != 0) {
+    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
+  } else {
+    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
+  }
+
+  // Obtain page table base
+  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
+
+  // Calculate number of 4KB page table entries to change
+  NumPageEntries = Length / TT_DESCRIPTOR_PAGE_SIZE;
+
+  // Iterate for the number of 4KB pages to change
+  Offset = 0;
+  for(p = 0; p < NumPageEntries; p++) {
+    // Calculate index into first level translation table for page table value
+
+    FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress + Offset) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
+    ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
+
+    // Read the descriptor from the first level page table
+    Descriptor = FirstLevelTable[FirstLevelIdx];
+
+    // Does this descriptor need to be converted from section entry to 4K pages?
+    if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Descriptor)) {
+      Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
+      if (EFI_ERROR(Status)) {
+        // Exit for loop
+        break;
+      }
+
+      // Re-read descriptor
+      Descriptor = FirstLevelTable[FirstLevelIdx];
+    }
+
+    // Obtain page table base address
+    PageTable = (ARM_PAGE_TABLE_ENTRY *)TT_DESCRIPTOR_PAGE_BASE_ADDRESS(Descriptor);
+
+    // Calculate index into the page table
+    PageTableIndex = ((BaseAddress + Offset) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
+    ASSERT (PageTableIndex < TRANSLATION_TABLE_PAGE_COUNT);
+
+    // Get the entry
+    CurrentPageTableEntry = PageTable[PageTableIndex];
+
+    // Mask off appropriate fields
+    PageTableEntry = CurrentPageTableEntry & ~EntryMask;
+
+    // Mask in new attributes and/or permissions
+    PageTableEntry |= EntryValue;
+
+    if (VirtualMask != 0) {
+      // Make this virtual address point at a physical page
+      PageTableEntry &= ~VirtualMask;
+    }
+
+    if (CurrentPageTableEntry  != PageTableEntry) {
+      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
+      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
+        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
+        // Note assumes switch(Attributes), not ARMv7 possibilities
+        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
+      }
+
+      // Only need to update if we are changing the entry
+      PageTable[PageTableIndex] = PageTableEntry;
+      ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
+    }
+
+    Status = EFI_SUCCESS;
+    Offset += TT_DESCRIPTOR_PAGE_SIZE;
+
+  } // End first level translation table loop
+
+  return Status;
+}
+
+STATIC
+EFI_STATUS
+UpdateSectionEntries (
+  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN UINT64                    Length,
+  IN UINT64                    Attributes,
+  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  )
+{
+  EFI_STATUS    Status = EFI_SUCCESS;
+  UINT32        EntryMask;
+  UINT32        EntryValue;
+  UINT32        FirstLevelIdx;
+  UINT32        NumSections;
+  UINT32        i;
+  UINT32        CurrentDescriptor;
+  UINT32        Descriptor;
+  VOID          *Mva;
+  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
+
+  // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
+  // EntryValue: values at bit positions specified by EntryMask
+
+  // Make sure we handle a section range that is unmapped
+  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
+              TT_DESCRIPTOR_SECTION_AP_MASK;
+  EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
+
+  // Although the PI spec is unclear on this the GCD guarantees that only
+  // one Attribute bit is set at a time, so we can safely use a switch statement
+  if ((Attributes & EFI_MEMORY_UC) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // map to strongly ordered
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
+  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // map to normal non-cachable
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
+  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // write through with no-allocate
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
+  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
+    // modify cacheability attributes
+    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
+    // write back (with allocate)
+    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
+  }
+
+  if ((Attributes & EFI_MEMORY_RO) != 0) {
+    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
+  } else {
+    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
+  }
+
+  if ((Attributes & EFI_MEMORY_XP) != 0) {
+    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
+  }
+
+  // obtain page table base
+  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
+
+  // calculate index into first level translation table for start of modification
+  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
+  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
+
+  // calculate number of 1MB first level entries this applies to
+  NumSections = Length / TT_DESCRIPTOR_SECTION_SIZE;
+
+  // iterate through each descriptor
+  for(i=0; i<NumSections; i++) {
+    CurrentDescriptor = FirstLevelTable[FirstLevelIdx + i];
+
+    // has this descriptor already been coverted to pages?
+    if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
+      // forward this 1MB range to page table function instead
+      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
+    } else {
+      // still a section entry
+
+      // mask off appropriate fields
+      Descriptor = CurrentDescriptor & ~EntryMask;
+
+      // mask in new attributes and/or permissions
+      Descriptor |= EntryValue;
+      if (VirtualMask != 0) {
+        Descriptor &= ~VirtualMask;
+      }
+
+      if (CurrentDescriptor  != Descriptor) {
+        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
+        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
+          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
+          // Note assumes switch(Attributes), not ARMv7 possabilities
+          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
+        }
+
+        // Only need to update if we are changing the descriptor
+        FirstLevelTable[FirstLevelIdx + i] = Descriptor;
+        ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
+      }
+
+      Status = EFI_SUCCESS;
+    }
+  }
+
+  return Status;
+}
+
+EFI_STATUS
+ArmSetMemoryAttributes (
+  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN UINT64                    Length,
+  IN UINT64                    Attributes,
+  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  )
+{
+  EFI_STATUS    Status;
+  UINT64        ChunkLength;
+  BOOLEAN       FlushTlbs;
+
+  FlushTlbs = FALSE;
+  while (Length > 0) {
+    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
+        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
+
+      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
+
+      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
+        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
+        BaseAddress, ChunkLength, Attributes));
+
+      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
+                 VirtualMask);
+
+      FlushTlbs = TRUE;
+    } else {
+
+      //
+      // Process page by page until the next section boundary, but only if
+      // we have more than a section's worth of area to deal with after that.
+      //
+      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
+                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
+      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
+        ChunkLength = Length;
+      }
+
+      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
+        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
+        BaseAddress, ChunkLength, Attributes));
+
+      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
+                 VirtualMask);
+    }
+
+    if (EFI_ERROR (Status)) {
+      break;
+    }
+
+    BaseAddress += ChunkLength;
+    Length -= ChunkLength;
+  }
+
+  if (FlushTlbs) {
+    ArmInvalidateTlb ();
+  }
+  return Status;
+}
+
 RETURN_STATUS
 ArmSetMemoryRegionNoExec (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/5] ArmPkg/ArmMmuLib: remove VirtualMask arg from ArmSetMemoryAttributes
  2017-03-01 16:31 [PATCH 0/5] ArmPkg, ArmVirtPkg ARM: enable non-executable stack Ard Biesheuvel
  2017-03-01 16:31 ` [PATCH 1/5] ArmPkg/ArmMmuLib AARCH64: use correct return type for exported functions Ard Biesheuvel
  2017-03-01 16:31 ` [PATCH 2/5] ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib Ard Biesheuvel
@ 2017-03-01 16:31 ` Ard Biesheuvel
  2017-03-06 16:06   ` Leif Lindholm
  2017-03-01 16:31 ` [PATCH 4/5] ArmPkg/ArmMmuLib ARM: implement memory permission control routines Ard Biesheuvel
  2017-03-01 16:31 ` [PATCH 5/5] ArmVirtPkg: enable non-executable DXE stack for all platforms Ard Biesheuvel
  4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2017-03-01 16:31 UTC (permalink / raw)
  To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel

We no longer make use of the ArmMmuLib 'feature' to create aliased
memory ranges with mismatched attributes, and in fact, it was only
wired up in the ARM version to begin with.

So remove the VirtualMask argument from ArmSetMemoryAttributes()'s
prototype, and remove the dead code that referred to it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  2 +-
 ArmPkg/Include/Library/ArmMmuLib.h               |  3 +--
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  3 +--
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     | 25 +++++---------------
 4 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index d0a3fedd3aa7..8150486217cf 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -210,7 +210,7 @@ CpuSetMemoryAttributes (
   if (EFI_ERROR (Status) || (RegionArmAttributes != ArmAttributes) ||
       ((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))
   {
-    return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
+    return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes);
   } else {
     return EFI_SUCCESS;
   }
diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
index d3a302fa8125..fb7fd006417c 100644
--- a/ArmPkg/Include/Library/ArmMmuLib.h
+++ b/ArmPkg/Include/Library/ArmMmuLib.h
@@ -66,8 +66,7 @@ EFI_STATUS
 ArmSetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  IN UINT64                    Attributes
   );
 
 #endif
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 77f108971f3e..8bd1c6fad95f 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -450,8 +450,7 @@ EFI_STATUS
 ArmSetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  IN UINT64                    Attributes
   )
 {
   EFI_STATUS                   Status;
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 93980d6d12db..1112660b434e 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -469,8 +469,7 @@ EFI_STATUS
 UpdatePageEntries (
   IN EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  IN UINT64                    Attributes
   )
 {
   EFI_STATUS    Status;
@@ -575,11 +574,6 @@ UpdatePageEntries (
     // Mask in new attributes and/or permissions
     PageTableEntry |= EntryValue;
 
-    if (VirtualMask != 0) {
-      // Make this virtual address point at a physical page
-      PageTableEntry &= ~VirtualMask;
-    }
-
     if (CurrentPageTableEntry  != PageTableEntry) {
       Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
       if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
@@ -606,8 +600,7 @@ EFI_STATUS
 UpdateSectionEntries (
   IN EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  IN UINT64                    Attributes
   )
 {
   EFI_STATUS    Status = EFI_SUCCESS;
@@ -680,7 +673,7 @@ UpdateSectionEntries (
     // has this descriptor already been coverted to pages?
     if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
       // forward this 1MB range to page table function instead
-      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
+      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes);
     } else {
       // still a section entry
 
@@ -689,9 +682,6 @@ UpdateSectionEntries (
 
       // mask in new attributes and/or permissions
       Descriptor |= EntryValue;
-      if (VirtualMask != 0) {
-        Descriptor &= ~VirtualMask;
-      }
 
       if (CurrentDescriptor  != Descriptor) {
         Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
@@ -717,8 +707,7 @@ EFI_STATUS
 ArmSetMemoryAttributes (
   IN EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN UINT64                    Length,
-  IN UINT64                    Attributes,
-  IN EFI_PHYSICAL_ADDRESS      VirtualMask
+  IN UINT64                    Attributes
   )
 {
   EFI_STATUS    Status;
@@ -736,8 +725,7 @@ ArmSetMemoryAttributes (
         "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
         BaseAddress, ChunkLength, Attributes));
 
-      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
-                 VirtualMask);
+      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes);
 
       FlushTlbs = TRUE;
     } else {
@@ -756,8 +744,7 @@ ArmSetMemoryAttributes (
         "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
         BaseAddress, ChunkLength, Attributes));
 
-      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
-                 VirtualMask);
+      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes);
     }
 
     if (EFI_ERROR (Status)) {
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/5] ArmPkg/ArmMmuLib ARM: implement memory permission control routines
  2017-03-01 16:31 [PATCH 0/5] ArmPkg, ArmVirtPkg ARM: enable non-executable stack Ard Biesheuvel
                   ` (2 preceding siblings ...)
  2017-03-01 16:31 ` [PATCH 3/5] ArmPkg/ArmMmuLib: remove VirtualMask arg from ArmSetMemoryAttributes Ard Biesheuvel
@ 2017-03-01 16:31 ` Ard Biesheuvel
  2017-03-06 16:11   ` Leif Lindholm
  2017-03-01 16:31 ` [PATCH 5/5] ArmVirtPkg: enable non-executable DXE stack for all platforms Ard Biesheuvel
  4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2017-03-01 16:31 UTC (permalink / raw)
  To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel

Now that we have the prerequisite functionality available in ArmMmuLib,
wire it up into ArmSetMemoryRegionNoExec, ArmClearMemoryRegionNoExec,
ArmSetMemoryRegionReadOnly and ArmClearMemoryRegionReadOnly. This is
used by the non-executable stack feature that is configured by DxeIpl.

NOTE: The current implementation will not combine RO and XP attributes,
      i.e., setting/clearing a region no-exec will unconditionally
      clear the read-only attribute, and vice versa. Currently, we
      only use ArmSetMemoryRegionNoExec(), so for now, we should be
      able to live with this.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 1112660b434e..55601328d93e 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -761,40 +761,40 @@ ArmSetMemoryAttributes (
   return Status;
 }
 
-RETURN_STATUS
+EFI_STATUS
 ArmSetMemoryRegionNoExec (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length
   )
 {
-  return RETURN_UNSUPPORTED;
+  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_XP);
 }
 
-RETURN_STATUS
+EFI_STATUS
 ArmClearMemoryRegionNoExec (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length
   )
 {
-  return RETURN_UNSUPPORTED;
+  return ArmSetMemoryAttributes (BaseAddress, Length, 0);
 }
 
-RETURN_STATUS
+EFI_STATUS
 ArmSetMemoryRegionReadOnly (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length
   )
 {
-  return RETURN_UNSUPPORTED;
+  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO);
 }
 
-RETURN_STATUS
+EFI_STATUS
 ArmClearMemoryRegionReadOnly (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length
   )
 {
-  return RETURN_UNSUPPORTED;
+  return ArmSetMemoryAttributes (BaseAddress, Length, 0);
 }
 
 RETURN_STATUS
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/5] ArmVirtPkg: enable non-executable DXE stack for all platforms
  2017-03-01 16:31 [PATCH 0/5] ArmPkg, ArmVirtPkg ARM: enable non-executable stack Ard Biesheuvel
                   ` (3 preceding siblings ...)
  2017-03-01 16:31 ` [PATCH 4/5] ArmPkg/ArmMmuLib ARM: implement memory permission control routines Ard Biesheuvel
@ 2017-03-01 16:31 ` Ard Biesheuvel
  2017-03-01 19:10   ` Laszlo Ersek
  4 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2017-03-01 16:31 UTC (permalink / raw)
  To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel

Now that ARM has grown support for managing memory permissions in
ArmMmuLib, we can enable the non-executable DXE stack for all virt
platforms.

Note that this is not [entirely] redundant: the non-executable stack
is configured before DxeCore is invoked. The image and memory protection
features configured during DXE only take affect when the CPU arch
protocol implementation is registered.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc       | 5 +++++
 ArmVirtPkg/ArmVirtQemu.dsc       | 2 --
 ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 --
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index acfb71d3ff6c..e2d3dcce7945 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -386,6 +386,11 @@ [PcdsFixedAtBuild.common]
   #
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
 
+  #
+  # Enable the non-executable DXE stack. (This gets set up by DxeIpl)
+  #
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
+
 [PcdsFixedAtBuild.ARM]
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
 
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 615e1fca4877..477dfdcfc764 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -152,8 +152,6 @@ [PcdsFixedAtBuild.common]
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
 
 [PcdsFixedAtBuild.AARCH64]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
-
   # KVM limits it IPA space to 40 bits (1 TB), so there is no need to
   # support anything bigger, even if the host hardware does
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index e4902690123c..fd39c2802a85 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -163,8 +163,6 @@ [PcdsFixedAtBuild.AARCH64]
   #
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
 
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
-
   # KVM limits it IPA space to 40 bits (1 TB), so there is no need to
   # support anything bigger, even if the host hardware does
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] ArmVirtPkg: enable non-executable DXE stack for all platforms
  2017-03-01 16:31 ` [PATCH 5/5] ArmVirtPkg: enable non-executable DXE stack for all platforms Ard Biesheuvel
@ 2017-03-01 19:10   ` Laszlo Ersek
  2017-03-01 19:10     ` Ard Biesheuvel
  0 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2017-03-01 19:10 UTC (permalink / raw)
  To: Ard Biesheuvel, edk2-devel, leif.lindholm

On 03/01/17 17:31, Ard Biesheuvel wrote:
> Now that ARM has grown support for managing memory permissions in
> ArmMmuLib, we can enable the non-executable DXE stack for all virt
> platforms.
> 
> Note that this is not [entirely] redundant: the non-executable stack
> is configured before DxeCore is invoked. The image and memory protection
> features configured during DXE only take affect when the CPU arch
> protocol implementation is registered.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc       | 5 +++++
>  ArmVirtPkg/ArmVirtQemu.dsc       | 2 --
>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 --
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index acfb71d3ff6c..e2d3dcce7945 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -386,6 +386,11 @@ [PcdsFixedAtBuild.common]
>    #
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
>  
> +  #
> +  # Enable the non-executable DXE stack. (This gets set up by DxeIpl)
> +  #
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
> +
>  [PcdsFixedAtBuild.ARM]
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
>  
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 615e1fca4877..477dfdcfc764 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -152,8 +152,6 @@ [PcdsFixedAtBuild.common]
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
>  
>  [PcdsFixedAtBuild.AARCH64]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
> -
>    # KVM limits it IPA space to 40 bits (1 TB), so there is no need to
>    # support anything bigger, even if the host hardware does
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index e4902690123c..fd39c2802a85 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -163,8 +163,6 @@ [PcdsFixedAtBuild.AARCH64]
>    #
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
>  
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
> -
>    # KVM limits it IPA space to 40 bits (1 TB), so there is no need to
>    # support anything bigger, even if the host hardware does
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
> 

This doesn't just extend PcdSetNxForStack from AARCH64 from ARM, but
also from QEMU to Xen. Is that your intent? If so,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] ArmVirtPkg: enable non-executable DXE stack for all platforms
  2017-03-01 19:10   ` Laszlo Ersek
@ 2017-03-01 19:10     ` Ard Biesheuvel
  0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2017-03-01 19:10 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Leif Lindholm

On 1 March 2017 at 19:10, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/01/17 17:31, Ard Biesheuvel wrote:
>> Now that ARM has grown support for managing memory permissions in
>> ArmMmuLib, we can enable the non-executable DXE stack for all virt
>> platforms.
>>
>> Note that this is not [entirely] redundant: the non-executable stack
>> is configured before DxeCore is invoked. The image and memory protection
>> features configured during DXE only take affect when the CPU arch
>> protocol implementation is registered.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/ArmVirt.dsc.inc       | 5 +++++
>>  ArmVirtPkg/ArmVirtQemu.dsc       | 2 --
>>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 2 --
>>  3 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index acfb71d3ff6c..e2d3dcce7945 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -386,6 +386,11 @@ [PcdsFixedAtBuild.common]
>>    #
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
>>
>> +  #
>> +  # Enable the non-executable DXE stack. (This gets set up by DxeIpl)
>> +  #
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>> +
>>  [PcdsFixedAtBuild.ARM]
>>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
>>
>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>> index 615e1fca4877..477dfdcfc764 100644
>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>> @@ -152,8 +152,6 @@ [PcdsFixedAtBuild.common]
>>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
>>
>>  [PcdsFixedAtBuild.AARCH64]
>> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>> -
>>    # KVM limits it IPA space to 40 bits (1 TB), so there is no need to
>>    # support anything bigger, even if the host hardware does
>>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
>> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>> index e4902690123c..fd39c2802a85 100644
>> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
>> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
>> @@ -163,8 +163,6 @@ [PcdsFixedAtBuild.AARCH64]
>>    #
>>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
>>
>> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>> -
>>    # KVM limits it IPA space to 40 bits (1 TB), so there is no need to
>>    # support anything bigger, even if the host hardware does
>>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
>>
>
> This doesn't just extend PcdSetNxForStack from AARCH64 from ARM, but
> also from QEMU to Xen. Is that your intent? If so,
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Yes, it is, but I will mention that in the commit log

Thanks,
Ard.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] ArmPkg/ArmMmuLib AARCH64: use correct return type for exported functions
  2017-03-01 16:31 ` [PATCH 1/5] ArmPkg/ArmMmuLib AARCH64: use correct return type for exported functions Ard Biesheuvel
@ 2017-03-06 14:57   ` Leif Lindholm
  0 siblings, 0 replies; 14+ messages in thread
From: Leif Lindholm @ 2017-03-06 14:57 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel, lersek

On Wed, Mar 01, 2017 at 04:31:39PM +0000, Ard Biesheuvel wrote:
> The routines ArmConfigureMmu() and SetMemoryAttributes() [*] are
> declared as returning EFI_STATUS in the respective header files,
> so align the definitions with that.
> 

Ouch. Well, in general, this is just not very EDK2:ish.

> * SetMemoryAttributes() is declared in the wrong header (and defined in
>   ArmMmuLib for AARCH64 and in CpuDxe for ARM)
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
No need to wait for rest of series (or prereqs for), this one stands alone.

> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 52 ++++++++++----------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 2f8f99d44a31..df170d20a2c2 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -329,7 +329,7 @@ GetBlockEntryListFromAddress (
>  }
>  
>  STATIC
> -RETURN_STATUS
> +EFI_STATUS
>  UpdateRegionMapping (
>    IN  UINT64  *RootTable,
>    IN  UINT64  RegionStart,
> @@ -347,7 +347,7 @@ UpdateRegionMapping (
>    // Ensure the Length is aligned on 4KB boundary
>    if ((RegionLength == 0) || ((RegionLength & (SIZE_4KB - 1)) != 0)) {
>      ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
> -    return RETURN_INVALID_PARAMETER;
> +    return EFI_INVALID_PARAMETER;
>    }
>  
>    do {
> @@ -357,7 +357,7 @@ UpdateRegionMapping (
>      BlockEntry = GetBlockEntryListFromAddress (RootTable, RegionStart, &TableLevel, &BlockEntrySize, &LastBlockEntry);
>      if (BlockEntry == NULL) {
>        // GetBlockEntryListFromAddress() return NULL when it fails to allocate new pages from the Translation Tables
> -      return RETURN_OUT_OF_RESOURCES;
> +      return EFI_OUT_OF_RESOURCES;
>      }
>  
>      if (TableLevel != 3) {
> @@ -385,11 +385,11 @@ UpdateRegionMapping (
>      } while ((RegionLength >= BlockEntrySize) && (BlockEntry <= LastBlockEntry));
>    } while (RegionLength != 0);
>  
> -  return RETURN_SUCCESS;
> +  return EFI_SUCCESS;
>  }
>  
>  STATIC
> -RETURN_STATUS
> +EFI_STATUS
>  FillTranslationTable (
>    IN  UINT64                        *RootTable,
>    IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryRegion
> @@ -446,7 +446,7 @@ GcdAttributeToPageAttribute (
>    return PageAttributes | TT_AF;
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  SetMemoryAttributes (
>    IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN UINT64                    Length,
> @@ -454,7 +454,7 @@ SetMemoryAttributes (
>    IN EFI_PHYSICAL_ADDRESS      VirtualMask
>    )
>  {
> -  RETURN_STATUS                Status;
> +  EFI_STATUS                   Status;
>    UINT64                      *TranslationTable;
>    UINT64                       PageAttributes;
>    UINT64                       PageAttributeMask;
> @@ -480,18 +480,18 @@ SetMemoryAttributes (
>               Length,
>               PageAttributes,
>               PageAttributeMask);
> -  if (RETURN_ERROR (Status)) {
> +  if (EFI_ERROR (Status)) {
>      return Status;
>    }
>  
>    // Invalidate all TLB entries so changes are synced
>    ArmInvalidateTlb ();
>  
> -  return RETURN_SUCCESS;
> +  return EFI_SUCCESS;
>  }
>  
>  STATIC
> -RETURN_STATUS
> +EFI_STATUS
>  SetMemoryRegionAttribute (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length,
> @@ -499,23 +499,23 @@ SetMemoryRegionAttribute (
>    IN  UINT64                    BlockEntryMask
>    )
>  {
> -  RETURN_STATUS                Status;
> +  EFI_STATUS                   Status;
>    UINT64                       *RootTable;
>  
>    RootTable = ArmGetTTBR0BaseAddress ();
>  
>    Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, BlockEntryMask);
> -  if (RETURN_ERROR (Status)) {
> +  if (EFI_ERROR (Status)) {
>      return Status;
>    }
>  
>    // Invalidate all TLB entries so changes are synced
>    ArmInvalidateTlb ();
>  
> -  return RETURN_SUCCESS;
> +  return EFI_SUCCESS;
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmSetMemoryRegionNoExec (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
> @@ -536,7 +536,7 @@ ArmSetMemoryRegionNoExec (
>             ~TT_ADDRESS_MASK_BLOCK_ENTRY);
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmClearMemoryRegionNoExec (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
> @@ -554,7 +554,7 @@ ArmClearMemoryRegionNoExec (
>             Mask);
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmSetMemoryRegionReadOnly (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
> @@ -567,7 +567,7 @@ ArmSetMemoryRegionReadOnly (
>             ~TT_ADDRESS_MASK_BLOCK_ENTRY);
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmClearMemoryRegionReadOnly (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
> @@ -580,7 +580,7 @@ ArmClearMemoryRegionReadOnly (
>             ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK));
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  EFIAPI
>  ArmConfigureMmu (
>    IN  ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable,
> @@ -594,11 +594,11 @@ ArmConfigureMmu (
>    UINTN                         T0SZ;
>    UINTN                         RootTableEntryCount;
>    UINT64                        TCR;
> -  RETURN_STATUS                 Status;
> +  EFI_STATUS                    Status;
>  
>    if(MemoryTable == NULL) {
>      ASSERT (MemoryTable != NULL);
> -    return RETURN_INVALID_PARAMETER;
> +    return EFI_INVALID_PARAMETER;
>    }
>  
>    // Cover the entire GCD memory space
> @@ -632,7 +632,7 @@ ArmConfigureMmu (
>      } else {
>        DEBUG ((EFI_D_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 RETURN_UNSUPPORTED;
> +      return EFI_UNSUPPORTED;
>      }
>    } else if (ArmReadCurrentEL () == AARCH64_EL1) {
>      // Due to Cortex-A57 erratum #822227 we must set TG1[1] == 1, regardless of EPD1.
> @@ -654,11 +654,11 @@ ArmConfigureMmu (
>      } else {
>        DEBUG ((EFI_D_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 RETURN_UNSUPPORTED;
> +      return EFI_UNSUPPORTED;
>      }
>    } else {
>      ASSERT (0); // UEFI is only expected to run at EL2 and EL1, not EL3.
> -    return RETURN_UNSUPPORTED;
> +    return EFI_UNSUPPORTED;
>    }
>  
>    //
> @@ -680,7 +680,7 @@ ArmConfigureMmu (
>    // Allocate pages for translation table
>    TranslationTable = AllocatePages (1);
>    if (TranslationTable == NULL) {
> -    return RETURN_OUT_OF_RESOURCES;
> +    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
> @@ -719,7 +719,7 @@ ArmConfigureMmu (
>      DEBUG_CODE_END ();
>  
>      Status = FillTranslationTable (TranslationTable, MemoryTable);
> -    if (RETURN_ERROR (Status)) {
> +    if (EFI_ERROR (Status)) {
>        goto FREE_TRANSLATION_TABLE;
>      }
>      MemoryTable++;
> @@ -739,7 +739,7 @@ ArmConfigureMmu (
>    ArmEnableDataCache ();
>  
>    ArmEnableMmu ();
> -  return RETURN_SUCCESS;
> +  return EFI_SUCCESS;
>  
>  FREE_TRANSLATION_TABLE:
>    FreePages (TranslationTable, 1);
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib
  2017-03-01 16:31 ` [PATCH 2/5] ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib Ard Biesheuvel
@ 2017-03-06 16:03   ` Leif Lindholm
  2017-03-06 16:05     ` Ard Biesheuvel
  0 siblings, 1 reply; 14+ messages in thread
From: Leif Lindholm @ 2017-03-06 16:03 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel, lersek

n Wed, Mar 01, 2017 at 04:31:40PM +0000, Ard Biesheuvel wrote:
> ... where it belongs, since AARCH64 already keeps it there, and
> non DXE users of ArmMmuLib (such as DxeIpl, for the non-executable
> stack) may need its functionality as well.
> 
> While at it, rename SetMemoryAttributes to ArmSetMemoryAttributes,
> and make any functions that are not exported STATIC. Also, replace
> an explicit gBS->AllocatePages() call [which is DXE specific] with
> MemoryAllocationLib::AllocatePages().
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 368 --------------------
>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  14 +-
>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |   2 +-
>  ArmPkg/Include/Library/ArmMmuLib.h               |   8 +
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |   2 +-
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     | 368 ++++++++++++++++++++
>  6 files changed, 379 insertions(+), 383 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 6322d301060e..b985dd743f02 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -343,374 +343,6 @@ SyncCacheConfig (
>    return EFI_SUCCESS;
>  }
>  
> -
> -
> -EFI_STATUS
> -UpdatePageEntries (
> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> -  IN UINT64                    Length,
> -  IN UINT64                    Attributes,
> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> -  )
> -{
> -  EFI_STATUS    Status;
> -  UINT32        EntryValue;
> -  UINT32        EntryMask;
> -  UINT32        FirstLevelIdx;
> -  UINT32        Offset;
> -  UINT32        NumPageEntries;
> -  UINT32        Descriptor;
> -  UINT32        p;
> -  UINT32        PageTableIndex;
> -  UINT32        PageTableEntry;
> -  UINT32        CurrentPageTableEntry;
> -  VOID          *Mva;
> -
> -  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> -  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
> -
> -  Status = EFI_SUCCESS;
> -
> -  // 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;
> -  }
> -
> -  // Although the PI spec is unclear on this the GCD guarantees that only
> -  // one Attribute bit is set at a time, so we can safely use a switch statement
> -  if ((Attributes & EFI_MEMORY_UC) != 0) {
> -    // modify cacheability attributes
> -    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> -    // map to strongly ordered
> -    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> -  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> -    // modify cacheability attributes
> -    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> -    // map to normal non-cachable
> -    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> -  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> -    // modify cacheability attributes
> -    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> -    // write through with no-allocate
> -    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> -  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> -    // modify cacheability attributes
> -    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> -    // write back (with allocate)
> -    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> -  }
> -
> -  if ((Attributes & EFI_MEMORY_RO) != 0) {
> -    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
> -  } else {
> -    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
> -  }
> -
> -  // Obtain page table base
> -  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> -
> -  // Calculate number of 4KB page table entries to change
> -  NumPageEntries = Length / TT_DESCRIPTOR_PAGE_SIZE;
> -
> -  // Iterate for the number of 4KB pages to change
> -  Offset = 0;
> -  for(p = 0; p < NumPageEntries; p++) {
> -    // Calculate index into first level translation table for page table value
> -
> -    FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress + Offset) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> -    ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> -
> -    // Read the descriptor from the first level page table
> -    Descriptor = FirstLevelTable[FirstLevelIdx];
> -
> -    // Does this descriptor need to be converted from section entry to 4K pages?
> -    if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Descriptor)) {
> -      Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> -      if (EFI_ERROR(Status)) {
> -        // Exit for loop
> -        break;
> -      }
> -
> -      // Re-read descriptor
> -      Descriptor = FirstLevelTable[FirstLevelIdx];
> -    }
> -
> -    // Obtain page table base address
> -    PageTable = (ARM_PAGE_TABLE_ENTRY *)TT_DESCRIPTOR_PAGE_BASE_ADDRESS(Descriptor);
> -
> -    // Calculate index into the page table
> -    PageTableIndex = ((BaseAddress + Offset) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
> -    ASSERT (PageTableIndex < TRANSLATION_TABLE_PAGE_COUNT);
> -
> -    // Get the entry
> -    CurrentPageTableEntry = PageTable[PageTableIndex];
> -
> -    // Mask off appropriate fields
> -    PageTableEntry = CurrentPageTableEntry & ~EntryMask;
> -
> -    // Mask in new attributes and/or permissions
> -    PageTableEntry |= EntryValue;
> -
> -    if (VirtualMask != 0) {
> -      // Make this virtual address point at a physical page
> -      PageTableEntry &= ~VirtualMask;
> -    }
> -
> -    if (CurrentPageTableEntry  != PageTableEntry) {
> -      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
> -      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
> -        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
> -        // Note assumes switch(Attributes), not ARMv7 possibilities
> -        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
> -      }
> -
> -      // Only need to update if we are changing the entry
> -      PageTable[PageTableIndex] = PageTableEntry;
> -      ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
> -    }
> -
> -    Status = EFI_SUCCESS;
> -    Offset += TT_DESCRIPTOR_PAGE_SIZE;
> -
> -  } // End first level translation table loop
> -
> -  return Status;
> -}
> -
> -
> -
> -EFI_STATUS
> -UpdateSectionEntries (
> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> -  IN UINT64                    Length,
> -  IN UINT64                    Attributes,
> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> -  )
> -{
> -  EFI_STATUS    Status = EFI_SUCCESS;
> -  UINT32        EntryMask;
> -  UINT32        EntryValue;
> -  UINT32        FirstLevelIdx;
> -  UINT32        NumSections;
> -  UINT32        i;
> -  UINT32        CurrentDescriptor;
> -  UINT32        Descriptor;
> -  VOID          *Mva;
> -  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> -
> -  // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
> -  // EntryValue: values at bit positions specified by EntryMask
> -
> -  // Make sure we handle a section range that is unmapped
> -  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
> -              TT_DESCRIPTOR_SECTION_AP_MASK;
> -  EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
> -
> -  // Although the PI spec is unclear on this the GCD guarantees that only
> -  // one Attribute bit is set at a time, so we can safely use a switch statement
> -  if ((Attributes & EFI_MEMORY_UC) != 0) {
> -    // modify cacheability attributes
> -    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> -    // map to strongly ordered
> -    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> -  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> -    // modify cacheability attributes
> -    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> -    // map to normal non-cachable
> -    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> -  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> -    // modify cacheability attributes
> -    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> -    // write through with no-allocate
> -    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> -  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> -    // modify cacheability attributes
> -    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> -    // write back (with allocate)
> -    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> -  }
> -
> -  if ((Attributes & EFI_MEMORY_RO) != 0) {
> -    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
> -  } else {
> -    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> -  }
> -
> -  if ((Attributes & EFI_MEMORY_XP) != 0) {
> -    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
> -  }
> -
> -  // obtain page table base
> -  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> -
> -  // calculate index into first level translation table for start of modification
> -  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> -  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> -
> -  // calculate number of 1MB first level entries this applies to
> -  NumSections = Length / TT_DESCRIPTOR_SECTION_SIZE;
> -
> -  // iterate through each descriptor
> -  for(i=0; i<NumSections; i++) {
> -    CurrentDescriptor = FirstLevelTable[FirstLevelIdx + i];
> -
> -    // has this descriptor already been coverted to pages?
> -    if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
> -      // forward this 1MB range to page table function instead
> -      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
> -    } else {
> -      // still a section entry
> -
> -      // mask off appropriate fields
> -      Descriptor = CurrentDescriptor & ~EntryMask;
> -
> -      // mask in new attributes and/or permissions
> -      Descriptor |= EntryValue;
> -      if (VirtualMask != 0) {
> -        Descriptor &= ~VirtualMask;
> -      }
> -
> -      if (CurrentDescriptor  != Descriptor) {
> -        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> -        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
> -          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
> -          // Note assumes switch(Attributes), not ARMv7 possabilities
> -          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
> -        }
> -
> -        // Only need to update if we are changing the descriptor
> -        FirstLevelTable[FirstLevelIdx + i] = Descriptor;
> -        ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
> -      }
> -
> -      Status = EFI_SUCCESS;
> -    }
> -  }
> -
> -  return Status;
> -}
> -
> -EFI_STATUS
> -ConvertSectionToPages (
> -  IN EFI_PHYSICAL_ADDRESS  BaseAddress
> -  )
> -{
> -  EFI_STATUS              Status;
> -  EFI_PHYSICAL_ADDRESS    PageTableAddr;
> -  UINT32                  FirstLevelIdx;
> -  UINT32                  SectionDescriptor;
> -  UINT32                  PageTableDescriptor;
> -  UINT32                  PageDescriptor;
> -  UINT32                  Index;
> -
> -  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> -  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
> -
> -  DEBUG ((EFI_D_PAGE, "Converting section at 0x%x to pages\n", (UINTN)BaseAddress));
> -
> -  // Obtain page table base
> -  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> -
> -  // Calculate index into first level translation table for start of modification
> -  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> -  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> -
> -  // Get section attributes and convert to page attributes
> -  SectionDescriptor = FirstLevelTable[FirstLevelIdx];
> -  PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor, FALSE);
> -
> -  // Allocate a page table for the 4KB entries (we use up a full page even though we only need 1KB)
> -  Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData, 1, &PageTableAddr);
> -  if (EFI_ERROR(Status)) {
> -    return Status;
> -  }
> -
> -  PageTable = (volatile ARM_PAGE_TABLE_ENTRY *)(UINTN)PageTableAddr;
> -
> -  // Write the page table entries out
> -  for (Index = 0; Index < TRANSLATION_TABLE_PAGE_COUNT; Index++) {
> -    PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;
> -  }
> -
> -  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
> -  WriteBackInvalidateDataCacheRange ((VOID *)(UINTN)PageTableAddr, TT_DESCRIPTOR_PAGE_SIZE);
> -
> -  // Formulate page table entry, Domain=0, NS=0
> -  PageTableDescriptor = (((UINTN)PageTableAddr) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
> -
> -  // Write the page table entry out, replacing section entry
> -  FirstLevelTable[FirstLevelIdx] = PageTableDescriptor;
> -
> -  return EFI_SUCCESS;
> -}
> -
> -
> -
> -EFI_STATUS
> -SetMemoryAttributes (
> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> -  IN UINT64                    Length,
> -  IN UINT64                    Attributes,
> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> -  )
> -{
> -  EFI_STATUS    Status;
> -  UINT64        ChunkLength;
> -  BOOLEAN       FlushTlbs;
> -
> -  FlushTlbs = FALSE;
> -  while (Length > 0) {
> -    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> -        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
> -
> -      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
> -
> -      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
> -        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
> -        BaseAddress, ChunkLength, Attributes));
> -
> -      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
> -                 VirtualMask);
> -
> -      FlushTlbs = TRUE;
> -    } else {
> -
> -      //
> -      // Process page by page until the next section boundary, but only if
> -      // we have more than a section's worth of area to deal with after that.
> -      //
> -      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
> -                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
> -      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
> -        ChunkLength = Length;
> -      }
> -
> -      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
> -        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
> -        BaseAddress, ChunkLength, Attributes));
> -
> -      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
> -                 VirtualMask);
> -    }
> -
> -    if (EFI_ERROR (Status)) {
> -      break;
> -    }
> -
> -    BaseAddress += ChunkLength;
> -    Length -= ChunkLength;
> -  }
> -
> -  if (FlushTlbs) {
> -    ArmInvalidateTlb ();
> -  }
> -  return Status;
> -}
> -
>  UINT64
>  EfiAttributeToArmAttribute (
>    IN UINT64                    EfiAttributes
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> index a46db8d25754..a0f71e69ec09 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> @@ -19,6 +19,7 @@
>  #include <Uefi.h>
>  
>  #include <Library/ArmLib.h>
> +#include <Library/ArmMmuLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/PcdLib.h>
> @@ -112,11 +113,6 @@ SyncCacheConfig (
>    IN  EFI_CPU_ARCH_PROTOCOL *CpuProtocol
>    );
>  
> -EFI_STATUS
> -ConvertSectionToPages (
> -  IN EFI_PHYSICAL_ADDRESS  BaseAddress
> -  );
> -
>  /**
>   * Publish ARM Processor Data table in UEFI SYSTEM Table.
>   * @param  HobStart               Pointer to the beginning of the HOB List from PEI.
> @@ -132,14 +128,6 @@ PublishArmProcessorTable(
>    VOID
>    );
>  
> -EFI_STATUS
> -SetMemoryAttributes (
> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> -  IN UINT64                    Length,
> -  IN UINT64                    Attributes,
> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> -  );
> -
>  // The ARM Attributes might be defined on 64-bit (case of the long format description table)
>  UINT64
>  EfiAttributeToArmAttribute (
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> index 0f36a058407a..d0a3fedd3aa7 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> @@ -210,7 +210,7 @@ CpuSetMemoryAttributes (
>    if (EFI_ERROR (Status) || (RegionArmAttributes != ArmAttributes) ||
>        ((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))
>    {
> -    return SetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
> +    return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
>    } else {
>      return EFI_SUCCESS;
>    }
> diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
> index c1d43872d548..d3a302fa8125 100644
> --- a/ArmPkg/Include/Library/ArmMmuLib.h
> +++ b/ArmPkg/Include/Library/ArmMmuLib.h
> @@ -62,4 +62,12 @@ ArmReplaceLiveTranslationEntry (
>    IN  UINT64  Value
>    );
>  
> +EFI_STATUS
> +ArmSetMemoryAttributes (
> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN UINT64                    Length,
> +  IN UINT64                    Attributes,
> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> +  );
> +
>  #endif
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index df170d20a2c2..77f108971f3e 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -447,7 +447,7 @@ GcdAttributeToPageAttribute (
>  }
>  
>  EFI_STATUS
> -SetMemoryAttributes (
> +ArmSetMemoryAttributes (
>    IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN UINT64                    Length,
>    IN UINT64                    Attributes,
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index 4b6f4ce392b7..93980d6d12db 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -16,6 +16,7 @@
>  #include <Uefi.h>
>  #include <Chipset/ArmV7.h>
>  #include <Library/BaseMemoryLib.h>
> +#include <Library/CacheMaintenanceLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/ArmLib.h>
>  #include <Library/BaseLib.h>
> @@ -36,6 +37,12 @@
>  #define ID_MMFR0_SHR_IMP_HW_COHERENT   1
>  #define ID_MMFR0_SHR_IGNORED         0xf
>  
> +// First Level Descriptors
> +typedef UINT32    ARM_FIRST_LEVEL_DESCRIPTOR;
> +
> +// Second Level Descriptors
> +typedef UINT32    ARM_PAGE_TABLE_ENTRY;
> +

Copied from ArmPkg/Drivers/CpuDxe/Arm/Mmu.c, but not deleted there.
Can it be, or can it be moved out into a header somewhere?

No other comments.

/
    Leif

>  UINTN
>  EFIAPI
>  ArmReadIdMmfr0 (
> @@ -406,6 +413,367 @@ ArmConfigureMmu (
>    return RETURN_SUCCESS;
>  }
>  
> +STATIC
> +EFI_STATUS
> +ConvertSectionToPages (
> +  IN EFI_PHYSICAL_ADDRESS  BaseAddress
> +  )
> +{
> +  UINT32                  FirstLevelIdx;
> +  UINT32                  SectionDescriptor;
> +  UINT32                  PageTableDescriptor;
> +  UINT32                  PageDescriptor;
> +  UINT32                  Index;
> +
> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> +  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
> +
> +  DEBUG ((EFI_D_PAGE, "Converting section at 0x%x to pages\n", (UINTN)BaseAddress));
> +
> +  // Obtain page table base
> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> +
> +  // Calculate index into first level translation table for start of modification
> +  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> +  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> +
> +  // Get section attributes and convert to page attributes
> +  SectionDescriptor = FirstLevelTable[FirstLevelIdx];
> +  PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor, FALSE);
> +
> +  // Allocate a page table for the 4KB entries (we use up a full page even though we only need 1KB)
> +  PageTable = (volatile ARM_PAGE_TABLE_ENTRY *)AllocatePages (1);
> +  if (PageTable == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  // Write the page table entries out
> +  for (Index = 0; Index < TRANSLATION_TABLE_PAGE_COUNT; Index++) {
> +    PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;
> +  }
> +
> +  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
> +  WriteBackInvalidateDataCacheRange ((VOID *)PageTable, TT_DESCRIPTOR_PAGE_SIZE);
> +
> +  // Formulate page table entry, Domain=0, NS=0
> +  PageTableDescriptor = (((UINTN)PageTable) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
> +
> +  // Write the page table entry out, replacing section entry
> +  FirstLevelTable[FirstLevelIdx] = PageTableDescriptor;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +UpdatePageEntries (
> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN UINT64                    Length,
> +  IN UINT64                    Attributes,
> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> +  )
> +{
> +  EFI_STATUS    Status;
> +  UINT32        EntryValue;
> +  UINT32        EntryMask;
> +  UINT32        FirstLevelIdx;
> +  UINT32        Offset;
> +  UINT32        NumPageEntries;
> +  UINT32        Descriptor;
> +  UINT32        p;
> +  UINT32        PageTableIndex;
> +  UINT32        PageTableEntry;
> +  UINT32        CurrentPageTableEntry;
> +  VOID          *Mva;
> +
> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> +  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
> +
> +  Status = EFI_SUCCESS;
> +
> +  // 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;
> +  }
> +
> +  // Although the PI spec is unclear on this the GCD guarantees that only
> +  // one Attribute bit is set at a time, so we can safely use a switch statement
> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> +    // map to strongly ordered
> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> +    // map to normal non-cachable
> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> +    // write through with no-allocate
> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> +    // write back (with allocate)
> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
> +  } else {
> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
> +  }
> +
> +  // Obtain page table base
> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> +
> +  // Calculate number of 4KB page table entries to change
> +  NumPageEntries = Length / TT_DESCRIPTOR_PAGE_SIZE;
> +
> +  // Iterate for the number of 4KB pages to change
> +  Offset = 0;
> +  for(p = 0; p < NumPageEntries; p++) {
> +    // Calculate index into first level translation table for page table value
> +
> +    FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress + Offset) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> +    ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> +
> +    // Read the descriptor from the first level page table
> +    Descriptor = FirstLevelTable[FirstLevelIdx];
> +
> +    // Does this descriptor need to be converted from section entry to 4K pages?
> +    if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Descriptor)) {
> +      Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> +      if (EFI_ERROR(Status)) {
> +        // Exit for loop
> +        break;
> +      }
> +
> +      // Re-read descriptor
> +      Descriptor = FirstLevelTable[FirstLevelIdx];
> +    }
> +
> +    // Obtain page table base address
> +    PageTable = (ARM_PAGE_TABLE_ENTRY *)TT_DESCRIPTOR_PAGE_BASE_ADDRESS(Descriptor);
> +
> +    // Calculate index into the page table
> +    PageTableIndex = ((BaseAddress + Offset) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
> +    ASSERT (PageTableIndex < TRANSLATION_TABLE_PAGE_COUNT);
> +
> +    // Get the entry
> +    CurrentPageTableEntry = PageTable[PageTableIndex];
> +
> +    // Mask off appropriate fields
> +    PageTableEntry = CurrentPageTableEntry & ~EntryMask;
> +
> +    // Mask in new attributes and/or permissions
> +    PageTableEntry |= EntryValue;
> +
> +    if (VirtualMask != 0) {
> +      // Make this virtual address point at a physical page
> +      PageTableEntry &= ~VirtualMask;
> +    }
> +
> +    if (CurrentPageTableEntry  != PageTableEntry) {
> +      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
> +      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
> +        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
> +        // Note assumes switch(Attributes), not ARMv7 possibilities
> +        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
> +      }
> +
> +      // Only need to update if we are changing the entry
> +      PageTable[PageTableIndex] = PageTableEntry;
> +      ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
> +    }
> +
> +    Status = EFI_SUCCESS;
> +    Offset += TT_DESCRIPTOR_PAGE_SIZE;
> +
> +  } // End first level translation table loop
> +
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +UpdateSectionEntries (
> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN UINT64                    Length,
> +  IN UINT64                    Attributes,
> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> +  )
> +{
> +  EFI_STATUS    Status = EFI_SUCCESS;
> +  UINT32        EntryMask;
> +  UINT32        EntryValue;
> +  UINT32        FirstLevelIdx;
> +  UINT32        NumSections;
> +  UINT32        i;
> +  UINT32        CurrentDescriptor;
> +  UINT32        Descriptor;
> +  VOID          *Mva;
> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> +
> +  // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
> +  // EntryValue: values at bit positions specified by EntryMask
> +
> +  // Make sure we handle a section range that is unmapped
> +  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
> +              TT_DESCRIPTOR_SECTION_AP_MASK;
> +  EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
> +
> +  // Although the PI spec is unclear on this the GCD guarantees that only
> +  // one Attribute bit is set at a time, so we can safely use a switch statement
> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> +    // map to strongly ordered
> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> +    // map to normal non-cachable
> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> +    // write through with no-allocate
> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> +    // modify cacheability attributes
> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> +    // write back (with allocate)
> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
> +  } else {
> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> +  }
> +
> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> +    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
> +  }
> +
> +  // obtain page table base
> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> +
> +  // calculate index into first level translation table for start of modification
> +  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> +  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> +
> +  // calculate number of 1MB first level entries this applies to
> +  NumSections = Length / TT_DESCRIPTOR_SECTION_SIZE;
> +
> +  // iterate through each descriptor
> +  for(i=0; i<NumSections; i++) {
> +    CurrentDescriptor = FirstLevelTable[FirstLevelIdx + i];
> +
> +    // has this descriptor already been coverted to pages?
> +    if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
> +      // forward this 1MB range to page table function instead
> +      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
> +    } else {
> +      // still a section entry
> +
> +      // mask off appropriate fields
> +      Descriptor = CurrentDescriptor & ~EntryMask;
> +
> +      // mask in new attributes and/or permissions
> +      Descriptor |= EntryValue;
> +      if (VirtualMask != 0) {
> +        Descriptor &= ~VirtualMask;
> +      }
> +
> +      if (CurrentDescriptor  != Descriptor) {
> +        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> +        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
> +          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
> +          // Note assumes switch(Attributes), not ARMv7 possabilities
> +          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
> +        }
> +
> +        // Only need to update if we are changing the descriptor
> +        FirstLevelTable[FirstLevelIdx + i] = Descriptor;
> +        ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
> +      }
> +
> +      Status = EFI_SUCCESS;
> +    }
> +  }
> +
> +  return Status;
> +}
> +
> +EFI_STATUS
> +ArmSetMemoryAttributes (
> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> +  IN UINT64                    Length,
> +  IN UINT64                    Attributes,
> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> +  )
> +{
> +  EFI_STATUS    Status;
> +  UINT64        ChunkLength;
> +  BOOLEAN       FlushTlbs;
> +
> +  FlushTlbs = FALSE;
> +  while (Length > 0) {
> +    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> +        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
> +
> +      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
> +
> +      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
> +        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
> +        BaseAddress, ChunkLength, Attributes));
> +
> +      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
> +                 VirtualMask);
> +
> +      FlushTlbs = TRUE;
> +    } else {
> +
> +      //
> +      // Process page by page until the next section boundary, but only if
> +      // we have more than a section's worth of area to deal with after that.
> +      //
> +      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
> +                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
> +      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
> +        ChunkLength = Length;
> +      }
> +
> +      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
> +        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
> +        BaseAddress, ChunkLength, Attributes));
> +
> +      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
> +                 VirtualMask);
> +    }
> +
> +    if (EFI_ERROR (Status)) {
> +      break;
> +    }
> +
> +    BaseAddress += ChunkLength;
> +    Length -= ChunkLength;
> +  }
> +
> +  if (FlushTlbs) {
> +    ArmInvalidateTlb ();
> +  }
> +  return Status;
> +}
> +
>  RETURN_STATUS
>  ArmSetMemoryRegionNoExec (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib
  2017-03-06 16:03   ` Leif Lindholm
@ 2017-03-06 16:05     ` Ard Biesheuvel
  2017-03-06 16:21       ` Leif Lindholm
  0 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2017-03-06 16:05 UTC (permalink / raw)
  To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Laszlo Ersek

On 6 March 2017 at 17:03, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> n Wed, Mar 01, 2017 at 04:31:40PM +0000, Ard Biesheuvel wrote:
>> ... where it belongs, since AARCH64 already keeps it there, and
>> non DXE users of ArmMmuLib (such as DxeIpl, for the non-executable
>> stack) may need its functionality as well.
>>
>> While at it, rename SetMemoryAttributes to ArmSetMemoryAttributes,
>> and make any functions that are not exported STATIC. Also, replace
>> an explicit gBS->AllocatePages() call [which is DXE specific] with
>> MemoryAllocationLib::AllocatePages().
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 368 --------------------
>>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                   |  14 +-
>>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |   2 +-
>>  ArmPkg/Include/Library/ArmMmuLib.h               |   8 +
>>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |   2 +-
>>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     | 368 ++++++++++++++++++++
>>  6 files changed, 379 insertions(+), 383 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> index 6322d301060e..b985dd743f02 100644
>> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
>> @@ -343,374 +343,6 @@ SyncCacheConfig (
>>    return EFI_SUCCESS;
>>  }
>>
>> -
>> -
>> -EFI_STATUS
>> -UpdatePageEntries (
>> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>> -  IN UINT64                    Length,
>> -  IN UINT64                    Attributes,
>> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
>> -  )
>> -{
>> -  EFI_STATUS    Status;
>> -  UINT32        EntryValue;
>> -  UINT32        EntryMask;
>> -  UINT32        FirstLevelIdx;
>> -  UINT32        Offset;
>> -  UINT32        NumPageEntries;
>> -  UINT32        Descriptor;
>> -  UINT32        p;
>> -  UINT32        PageTableIndex;
>> -  UINT32        PageTableEntry;
>> -  UINT32        CurrentPageTableEntry;
>> -  VOID          *Mva;
>> -
>> -  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
>> -  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
>> -
>> -  Status = EFI_SUCCESS;
>> -
>> -  // 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;
>> -  }
>> -
>> -  // Although the PI spec is unclear on this the GCD guarantees that only
>> -  // one Attribute bit is set at a time, so we can safely use a switch statement
>> -  if ((Attributes & EFI_MEMORY_UC) != 0) {
>> -    // modify cacheability attributes
>> -    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> -    // map to strongly ordered
>> -    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
>> -  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
>> -    // modify cacheability attributes
>> -    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> -    // map to normal non-cachable
>> -    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
>> -  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
>> -    // modify cacheability attributes
>> -    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> -    // write through with no-allocate
>> -    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
>> -  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
>> -    // modify cacheability attributes
>> -    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> -    // write back (with allocate)
>> -    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
>> -  }
>> -
>> -  if ((Attributes & EFI_MEMORY_RO) != 0) {
>> -    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
>> -  } else {
>> -    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
>> -  }
>> -
>> -  // Obtain page table base
>> -  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
>> -
>> -  // Calculate number of 4KB page table entries to change
>> -  NumPageEntries = Length / TT_DESCRIPTOR_PAGE_SIZE;
>> -
>> -  // Iterate for the number of 4KB pages to change
>> -  Offset = 0;
>> -  for(p = 0; p < NumPageEntries; p++) {
>> -    // Calculate index into first level translation table for page table value
>> -
>> -    FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress + Offset) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
>> -    ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
>> -
>> -    // Read the descriptor from the first level page table
>> -    Descriptor = FirstLevelTable[FirstLevelIdx];
>> -
>> -    // Does this descriptor need to be converted from section entry to 4K pages?
>> -    if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Descriptor)) {
>> -      Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
>> -      if (EFI_ERROR(Status)) {
>> -        // Exit for loop
>> -        break;
>> -      }
>> -
>> -      // Re-read descriptor
>> -      Descriptor = FirstLevelTable[FirstLevelIdx];
>> -    }
>> -
>> -    // Obtain page table base address
>> -    PageTable = (ARM_PAGE_TABLE_ENTRY *)TT_DESCRIPTOR_PAGE_BASE_ADDRESS(Descriptor);
>> -
>> -    // Calculate index into the page table
>> -    PageTableIndex = ((BaseAddress + Offset) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
>> -    ASSERT (PageTableIndex < TRANSLATION_TABLE_PAGE_COUNT);
>> -
>> -    // Get the entry
>> -    CurrentPageTableEntry = PageTable[PageTableIndex];
>> -
>> -    // Mask off appropriate fields
>> -    PageTableEntry = CurrentPageTableEntry & ~EntryMask;
>> -
>> -    // Mask in new attributes and/or permissions
>> -    PageTableEntry |= EntryValue;
>> -
>> -    if (VirtualMask != 0) {
>> -      // Make this virtual address point at a physical page
>> -      PageTableEntry &= ~VirtualMask;
>> -    }
>> -
>> -    if (CurrentPageTableEntry  != PageTableEntry) {
>> -      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
>> -      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
>> -        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
>> -        // Note assumes switch(Attributes), not ARMv7 possibilities
>> -        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
>> -      }
>> -
>> -      // Only need to update if we are changing the entry
>> -      PageTable[PageTableIndex] = PageTableEntry;
>> -      ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
>> -    }
>> -
>> -    Status = EFI_SUCCESS;
>> -    Offset += TT_DESCRIPTOR_PAGE_SIZE;
>> -
>> -  } // End first level translation table loop
>> -
>> -  return Status;
>> -}
>> -
>> -
>> -
>> -EFI_STATUS
>> -UpdateSectionEntries (
>> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>> -  IN UINT64                    Length,
>> -  IN UINT64                    Attributes,
>> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
>> -  )
>> -{
>> -  EFI_STATUS    Status = EFI_SUCCESS;
>> -  UINT32        EntryMask;
>> -  UINT32        EntryValue;
>> -  UINT32        FirstLevelIdx;
>> -  UINT32        NumSections;
>> -  UINT32        i;
>> -  UINT32        CurrentDescriptor;
>> -  UINT32        Descriptor;
>> -  VOID          *Mva;
>> -  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
>> -
>> -  // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
>> -  // EntryValue: values at bit positions specified by EntryMask
>> -
>> -  // Make sure we handle a section range that is unmapped
>> -  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
>> -              TT_DESCRIPTOR_SECTION_AP_MASK;
>> -  EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
>> -
>> -  // Although the PI spec is unclear on this the GCD guarantees that only
>> -  // one Attribute bit is set at a time, so we can safely use a switch statement
>> -  if ((Attributes & EFI_MEMORY_UC) != 0) {
>> -    // modify cacheability attributes
>> -    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> -    // map to strongly ordered
>> -    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
>> -  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
>> -    // modify cacheability attributes
>> -    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> -    // map to normal non-cachable
>> -    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
>> -  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
>> -    // modify cacheability attributes
>> -    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> -    // write through with no-allocate
>> -    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
>> -  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
>> -    // modify cacheability attributes
>> -    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> -    // write back (with allocate)
>> -    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
>> -  }
>> -
>> -  if ((Attributes & EFI_MEMORY_RO) != 0) {
>> -    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
>> -  } else {
>> -    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
>> -  }
>> -
>> -  if ((Attributes & EFI_MEMORY_XP) != 0) {
>> -    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
>> -  }
>> -
>> -  // obtain page table base
>> -  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
>> -
>> -  // calculate index into first level translation table for start of modification
>> -  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
>> -  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
>> -
>> -  // calculate number of 1MB first level entries this applies to
>> -  NumSections = Length / TT_DESCRIPTOR_SECTION_SIZE;
>> -
>> -  // iterate through each descriptor
>> -  for(i=0; i<NumSections; i++) {
>> -    CurrentDescriptor = FirstLevelTable[FirstLevelIdx + i];
>> -
>> -    // has this descriptor already been coverted to pages?
>> -    if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
>> -      // forward this 1MB range to page table function instead
>> -      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
>> -    } else {
>> -      // still a section entry
>> -
>> -      // mask off appropriate fields
>> -      Descriptor = CurrentDescriptor & ~EntryMask;
>> -
>> -      // mask in new attributes and/or permissions
>> -      Descriptor |= EntryValue;
>> -      if (VirtualMask != 0) {
>> -        Descriptor &= ~VirtualMask;
>> -      }
>> -
>> -      if (CurrentDescriptor  != Descriptor) {
>> -        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
>> -        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
>> -          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
>> -          // Note assumes switch(Attributes), not ARMv7 possabilities
>> -          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
>> -        }
>> -
>> -        // Only need to update if we are changing the descriptor
>> -        FirstLevelTable[FirstLevelIdx + i] = Descriptor;
>> -        ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
>> -      }
>> -
>> -      Status = EFI_SUCCESS;
>> -    }
>> -  }
>> -
>> -  return Status;
>> -}
>> -
>> -EFI_STATUS
>> -ConvertSectionToPages (
>> -  IN EFI_PHYSICAL_ADDRESS  BaseAddress
>> -  )
>> -{
>> -  EFI_STATUS              Status;
>> -  EFI_PHYSICAL_ADDRESS    PageTableAddr;
>> -  UINT32                  FirstLevelIdx;
>> -  UINT32                  SectionDescriptor;
>> -  UINT32                  PageTableDescriptor;
>> -  UINT32                  PageDescriptor;
>> -  UINT32                  Index;
>> -
>> -  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
>> -  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
>> -
>> -  DEBUG ((EFI_D_PAGE, "Converting section at 0x%x to pages\n", (UINTN)BaseAddress));
>> -
>> -  // Obtain page table base
>> -  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
>> -
>> -  // Calculate index into first level translation table for start of modification
>> -  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
>> -  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
>> -
>> -  // Get section attributes and convert to page attributes
>> -  SectionDescriptor = FirstLevelTable[FirstLevelIdx];
>> -  PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor, FALSE);
>> -
>> -  // Allocate a page table for the 4KB entries (we use up a full page even though we only need 1KB)
>> -  Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData, 1, &PageTableAddr);
>> -  if (EFI_ERROR(Status)) {
>> -    return Status;
>> -  }
>> -
>> -  PageTable = (volatile ARM_PAGE_TABLE_ENTRY *)(UINTN)PageTableAddr;
>> -
>> -  // Write the page table entries out
>> -  for (Index = 0; Index < TRANSLATION_TABLE_PAGE_COUNT; Index++) {
>> -    PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;
>> -  }
>> -
>> -  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
>> -  WriteBackInvalidateDataCacheRange ((VOID *)(UINTN)PageTableAddr, TT_DESCRIPTOR_PAGE_SIZE);
>> -
>> -  // Formulate page table entry, Domain=0, NS=0
>> -  PageTableDescriptor = (((UINTN)PageTableAddr) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
>> -
>> -  // Write the page table entry out, replacing section entry
>> -  FirstLevelTable[FirstLevelIdx] = PageTableDescriptor;
>> -
>> -  return EFI_SUCCESS;
>> -}
>> -
>> -
>> -
>> -EFI_STATUS
>> -SetMemoryAttributes (
>> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>> -  IN UINT64                    Length,
>> -  IN UINT64                    Attributes,
>> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
>> -  )
>> -{
>> -  EFI_STATUS    Status;
>> -  UINT64        ChunkLength;
>> -  BOOLEAN       FlushTlbs;
>> -
>> -  FlushTlbs = FALSE;
>> -  while (Length > 0) {
>> -    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
>> -        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
>> -
>> -      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
>> -
>> -      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
>> -        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
>> -        BaseAddress, ChunkLength, Attributes));
>> -
>> -      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
>> -                 VirtualMask);
>> -
>> -      FlushTlbs = TRUE;
>> -    } else {
>> -
>> -      //
>> -      // Process page by page until the next section boundary, but only if
>> -      // we have more than a section's worth of area to deal with after that.
>> -      //
>> -      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
>> -                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
>> -      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
>> -        ChunkLength = Length;
>> -      }
>> -
>> -      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
>> -        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
>> -        BaseAddress, ChunkLength, Attributes));
>> -
>> -      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
>> -                 VirtualMask);
>> -    }
>> -
>> -    if (EFI_ERROR (Status)) {
>> -      break;
>> -    }
>> -
>> -    BaseAddress += ChunkLength;
>> -    Length -= ChunkLength;
>> -  }
>> -
>> -  if (FlushTlbs) {
>> -    ArmInvalidateTlb ();
>> -  }
>> -  return Status;
>> -}
>> -
>>  UINT64
>>  EfiAttributeToArmAttribute (
>>    IN UINT64                    EfiAttributes
>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> index a46db8d25754..a0f71e69ec09 100644
>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> @@ -19,6 +19,7 @@
>>  #include <Uefi.h>
>>
>>  #include <Library/ArmLib.h>
>> +#include <Library/ArmMmuLib.h>
>>  #include <Library/BaseMemoryLib.h>
>>  #include <Library/DebugLib.h>
>>  #include <Library/PcdLib.h>
>> @@ -112,11 +113,6 @@ SyncCacheConfig (
>>    IN  EFI_CPU_ARCH_PROTOCOL *CpuProtocol
>>    );
>>
>> -EFI_STATUS
>> -ConvertSectionToPages (
>> -  IN EFI_PHYSICAL_ADDRESS  BaseAddress
>> -  );
>> -
>>  /**
>>   * Publish ARM Processor Data table in UEFI SYSTEM Table.
>>   * @param  HobStart               Pointer to the beginning of the HOB List from PEI.
>> @@ -132,14 +128,6 @@ PublishArmProcessorTable(
>>    VOID
>>    );
>>
>> -EFI_STATUS
>> -SetMemoryAttributes (
>> -  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>> -  IN UINT64                    Length,
>> -  IN UINT64                    Attributes,
>> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
>> -  );
>> -
>>  // The ARM Attributes might be defined on 64-bit (case of the long format description table)
>>  UINT64
>>  EfiAttributeToArmAttribute (
>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
>> index 0f36a058407a..d0a3fedd3aa7 100644
>> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
>> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
>> @@ -210,7 +210,7 @@ CpuSetMemoryAttributes (
>>    if (EFI_ERROR (Status) || (RegionArmAttributes != ArmAttributes) ||
>>        ((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))
>>    {
>> -    return SetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
>> +    return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
>>    } else {
>>      return EFI_SUCCESS;
>>    }
>> diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
>> index c1d43872d548..d3a302fa8125 100644
>> --- a/ArmPkg/Include/Library/ArmMmuLib.h
>> +++ b/ArmPkg/Include/Library/ArmMmuLib.h
>> @@ -62,4 +62,12 @@ ArmReplaceLiveTranslationEntry (
>>    IN  UINT64  Value
>>    );
>>
>> +EFI_STATUS
>> +ArmSetMemoryAttributes (
>> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>> +  IN UINT64                    Length,
>> +  IN UINT64                    Attributes,
>> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
>> +  );
>> +
>>  #endif
>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> index df170d20a2c2..77f108971f3e 100644
>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> @@ -447,7 +447,7 @@ GcdAttributeToPageAttribute (
>>  }
>>
>>  EFI_STATUS
>> -SetMemoryAttributes (
>> +ArmSetMemoryAttributes (
>>    IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>>    IN UINT64                    Length,
>>    IN UINT64                    Attributes,
>> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
>> index 4b6f4ce392b7..93980d6d12db 100644
>> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
>> @@ -16,6 +16,7 @@
>>  #include <Uefi.h>
>>  #include <Chipset/ArmV7.h>
>>  #include <Library/BaseMemoryLib.h>
>> +#include <Library/CacheMaintenanceLib.h>
>>  #include <Library/MemoryAllocationLib.h>
>>  #include <Library/ArmLib.h>
>>  #include <Library/BaseLib.h>
>> @@ -36,6 +37,12 @@
>>  #define ID_MMFR0_SHR_IMP_HW_COHERENT   1
>>  #define ID_MMFR0_SHR_IGNORED         0xf
>>
>> +// First Level Descriptors
>> +typedef UINT32    ARM_FIRST_LEVEL_DESCRIPTOR;
>> +
>> +// Second Level Descriptors
>> +typedef UINT32    ARM_PAGE_TABLE_ENTRY;
>> +
>
> Copied from ArmPkg/Drivers/CpuDxe/Arm/Mmu.c, but not deleted there.
> Can it be, or can it be moved out into a header somewhere?
>
> No other comments.
>

It is used in both places, so I'd need to put in in a header file

ArmPkg/Include/Chipset/ArmV7Mmu.h comes to mind ...


>>  UINTN
>>  EFIAPI
>>  ArmReadIdMmfr0 (
>> @@ -406,6 +413,367 @@ ArmConfigureMmu (
>>    return RETURN_SUCCESS;
>>  }
>>
>> +STATIC
>> +EFI_STATUS
>> +ConvertSectionToPages (
>> +  IN EFI_PHYSICAL_ADDRESS  BaseAddress
>> +  )
>> +{
>> +  UINT32                  FirstLevelIdx;
>> +  UINT32                  SectionDescriptor;
>> +  UINT32                  PageTableDescriptor;
>> +  UINT32                  PageDescriptor;
>> +  UINT32                  Index;
>> +
>> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
>> +  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
>> +
>> +  DEBUG ((EFI_D_PAGE, "Converting section at 0x%x to pages\n", (UINTN)BaseAddress));
>> +
>> +  // Obtain page table base
>> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
>> +
>> +  // Calculate index into first level translation table for start of modification
>> +  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
>> +  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
>> +
>> +  // Get section attributes and convert to page attributes
>> +  SectionDescriptor = FirstLevelTable[FirstLevelIdx];
>> +  PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor, FALSE);
>> +
>> +  // Allocate a page table for the 4KB entries (we use up a full page even though we only need 1KB)
>> +  PageTable = (volatile ARM_PAGE_TABLE_ENTRY *)AllocatePages (1);
>> +  if (PageTable == NULL) {
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  // Write the page table entries out
>> +  for (Index = 0; Index < TRANSLATION_TABLE_PAGE_COUNT; Index++) {
>> +    PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;
>> +  }
>> +
>> +  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
>> +  WriteBackInvalidateDataCacheRange ((VOID *)PageTable, TT_DESCRIPTOR_PAGE_SIZE);
>> +
>> +  // Formulate page table entry, Domain=0, NS=0
>> +  PageTableDescriptor = (((UINTN)PageTable) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
>> +
>> +  // Write the page table entry out, replacing section entry
>> +  FirstLevelTable[FirstLevelIdx] = PageTableDescriptor;
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +UpdatePageEntries (
>> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>> +  IN UINT64                    Length,
>> +  IN UINT64                    Attributes,
>> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
>> +  )
>> +{
>> +  EFI_STATUS    Status;
>> +  UINT32        EntryValue;
>> +  UINT32        EntryMask;
>> +  UINT32        FirstLevelIdx;
>> +  UINT32        Offset;
>> +  UINT32        NumPageEntries;
>> +  UINT32        Descriptor;
>> +  UINT32        p;
>> +  UINT32        PageTableIndex;
>> +  UINT32        PageTableEntry;
>> +  UINT32        CurrentPageTableEntry;
>> +  VOID          *Mva;
>> +
>> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
>> +  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
>> +
>> +  Status = EFI_SUCCESS;
>> +
>> +  // 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;
>> +  }
>> +
>> +  // Although the PI spec is unclear on this the GCD guarantees that only
>> +  // one Attribute bit is set at a time, so we can safely use a switch statement
>> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
>> +    // modify cacheability attributes
>> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> +    // map to strongly ordered
>> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
>> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
>> +    // modify cacheability attributes
>> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> +    // map to normal non-cachable
>> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
>> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
>> +    // modify cacheability attributes
>> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> +    // write through with no-allocate
>> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
>> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
>> +    // modify cacheability attributes
>> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
>> +    // write back (with allocate)
>> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
>> +  }
>> +
>> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
>> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
>> +  } else {
>> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
>> +  }
>> +
>> +  // Obtain page table base
>> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
>> +
>> +  // Calculate number of 4KB page table entries to change
>> +  NumPageEntries = Length / TT_DESCRIPTOR_PAGE_SIZE;
>> +
>> +  // Iterate for the number of 4KB pages to change
>> +  Offset = 0;
>> +  for(p = 0; p < NumPageEntries; p++) {
>> +    // Calculate index into first level translation table for page table value
>> +
>> +    FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress + Offset) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
>> +    ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
>> +
>> +    // Read the descriptor from the first level page table
>> +    Descriptor = FirstLevelTable[FirstLevelIdx];
>> +
>> +    // Does this descriptor need to be converted from section entry to 4K pages?
>> +    if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Descriptor)) {
>> +      Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
>> +      if (EFI_ERROR(Status)) {
>> +        // Exit for loop
>> +        break;
>> +      }
>> +
>> +      // Re-read descriptor
>> +      Descriptor = FirstLevelTable[FirstLevelIdx];
>> +    }
>> +
>> +    // Obtain page table base address
>> +    PageTable = (ARM_PAGE_TABLE_ENTRY *)TT_DESCRIPTOR_PAGE_BASE_ADDRESS(Descriptor);
>> +
>> +    // Calculate index into the page table
>> +    PageTableIndex = ((BaseAddress + Offset) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
>> +    ASSERT (PageTableIndex < TRANSLATION_TABLE_PAGE_COUNT);
>> +
>> +    // Get the entry
>> +    CurrentPageTableEntry = PageTable[PageTableIndex];
>> +
>> +    // Mask off appropriate fields
>> +    PageTableEntry = CurrentPageTableEntry & ~EntryMask;
>> +
>> +    // Mask in new attributes and/or permissions
>> +    PageTableEntry |= EntryValue;
>> +
>> +    if (VirtualMask != 0) {
>> +      // Make this virtual address point at a physical page
>> +      PageTableEntry &= ~VirtualMask;
>> +    }
>> +
>> +    if (CurrentPageTableEntry  != PageTableEntry) {
>> +      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
>> +      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
>> +        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
>> +        // Note assumes switch(Attributes), not ARMv7 possibilities
>> +        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
>> +      }
>> +
>> +      // Only need to update if we are changing the entry
>> +      PageTable[PageTableIndex] = PageTableEntry;
>> +      ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
>> +    }
>> +
>> +    Status = EFI_SUCCESS;
>> +    Offset += TT_DESCRIPTOR_PAGE_SIZE;
>> +
>> +  } // End first level translation table loop
>> +
>> +  return Status;
>> +}
>> +
>> +STATIC
>> +EFI_STATUS
>> +UpdateSectionEntries (
>> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>> +  IN UINT64                    Length,
>> +  IN UINT64                    Attributes,
>> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
>> +  )
>> +{
>> +  EFI_STATUS    Status = EFI_SUCCESS;
>> +  UINT32        EntryMask;
>> +  UINT32        EntryValue;
>> +  UINT32        FirstLevelIdx;
>> +  UINT32        NumSections;
>> +  UINT32        i;
>> +  UINT32        CurrentDescriptor;
>> +  UINT32        Descriptor;
>> +  VOID          *Mva;
>> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
>> +
>> +  // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
>> +  // EntryValue: values at bit positions specified by EntryMask
>> +
>> +  // Make sure we handle a section range that is unmapped
>> +  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
>> +              TT_DESCRIPTOR_SECTION_AP_MASK;
>> +  EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
>> +
>> +  // Although the PI spec is unclear on this the GCD guarantees that only
>> +  // one Attribute bit is set at a time, so we can safely use a switch statement
>> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
>> +    // modify cacheability attributes
>> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> +    // map to strongly ordered
>> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
>> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
>> +    // modify cacheability attributes
>> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> +    // map to normal non-cachable
>> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
>> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
>> +    // modify cacheability attributes
>> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> +    // write through with no-allocate
>> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
>> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
>> +    // modify cacheability attributes
>> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
>> +    // write back (with allocate)
>> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
>> +  }
>> +
>> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
>> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
>> +  } else {
>> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
>> +  }
>> +
>> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
>> +    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
>> +  }
>> +
>> +  // obtain page table base
>> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
>> +
>> +  // calculate index into first level translation table for start of modification
>> +  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
>> +  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
>> +
>> +  // calculate number of 1MB first level entries this applies to
>> +  NumSections = Length / TT_DESCRIPTOR_SECTION_SIZE;
>> +
>> +  // iterate through each descriptor
>> +  for(i=0; i<NumSections; i++) {
>> +    CurrentDescriptor = FirstLevelTable[FirstLevelIdx + i];
>> +
>> +    // has this descriptor already been coverted to pages?
>> +    if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
>> +      // forward this 1MB range to page table function instead
>> +      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
>> +    } else {
>> +      // still a section entry
>> +
>> +      // mask off appropriate fields
>> +      Descriptor = CurrentDescriptor & ~EntryMask;
>> +
>> +      // mask in new attributes and/or permissions
>> +      Descriptor |= EntryValue;
>> +      if (VirtualMask != 0) {
>> +        Descriptor &= ~VirtualMask;
>> +      }
>> +
>> +      if (CurrentDescriptor  != Descriptor) {
>> +        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
>> +        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
>> +          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
>> +          // Note assumes switch(Attributes), not ARMv7 possabilities
>> +          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
>> +        }
>> +
>> +        // Only need to update if we are changing the descriptor
>> +        FirstLevelTable[FirstLevelIdx + i] = Descriptor;
>> +        ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
>> +      }
>> +
>> +      Status = EFI_SUCCESS;
>> +    }
>> +  }
>> +
>> +  return Status;
>> +}
>> +
>> +EFI_STATUS
>> +ArmSetMemoryAttributes (
>> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>> +  IN UINT64                    Length,
>> +  IN UINT64                    Attributes,
>> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
>> +  )
>> +{
>> +  EFI_STATUS    Status;
>> +  UINT64        ChunkLength;
>> +  BOOLEAN       FlushTlbs;
>> +
>> +  FlushTlbs = FALSE;
>> +  while (Length > 0) {
>> +    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
>> +        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
>> +
>> +      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
>> +
>> +      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
>> +        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
>> +        BaseAddress, ChunkLength, Attributes));
>> +
>> +      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
>> +                 VirtualMask);
>> +
>> +      FlushTlbs = TRUE;
>> +    } else {
>> +
>> +      //
>> +      // Process page by page until the next section boundary, but only if
>> +      // we have more than a section's worth of area to deal with after that.
>> +      //
>> +      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
>> +                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
>> +      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
>> +        ChunkLength = Length;
>> +      }
>> +
>> +      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
>> +        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
>> +        BaseAddress, ChunkLength, Attributes));
>> +
>> +      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
>> +                 VirtualMask);
>> +    }
>> +
>> +    if (EFI_ERROR (Status)) {
>> +      break;
>> +    }
>> +
>> +    BaseAddress += ChunkLength;
>> +    Length -= ChunkLength;
>> +  }
>> +
>> +  if (FlushTlbs) {
>> +    ArmInvalidateTlb ();
>> +  }
>> +  return Status;
>> +}
>> +
>>  RETURN_STATUS
>>  ArmSetMemoryRegionNoExec (
>>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>> --
>> 2.7.4
>>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] ArmPkg/ArmMmuLib: remove VirtualMask arg from ArmSetMemoryAttributes
  2017-03-01 16:31 ` [PATCH 3/5] ArmPkg/ArmMmuLib: remove VirtualMask arg from ArmSetMemoryAttributes Ard Biesheuvel
@ 2017-03-06 16:06   ` Leif Lindholm
  0 siblings, 0 replies; 14+ messages in thread
From: Leif Lindholm @ 2017-03-06 16:06 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel, lersek

On Wed, Mar 01, 2017 at 04:31:41PM +0000, Ard Biesheuvel wrote:
> We no longer make use of the ArmMmuLib 'feature' to create aliased
> memory ranges with mismatched attributes, and in fact, it was only
> wired up in the ARM version to begin with.
> 
> So remove the VirtualMask argument from ArmSetMemoryAttributes()'s
> prototype, and remove the dead code that referred to it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  2 +-
>  ArmPkg/Include/Library/ArmMmuLib.h               |  3 +--
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  3 +--
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c     | 25 +++++---------------
>  4 files changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> index d0a3fedd3aa7..8150486217cf 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> @@ -210,7 +210,7 @@ CpuSetMemoryAttributes (
>    if (EFI_ERROR (Status) || (RegionArmAttributes != ArmAttributes) ||
>        ((BaseAddress + Length) > (RegionBaseAddress + RegionLength)))
>    {
> -    return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes, 0);
> +    return ArmSetMemoryAttributes (BaseAddress, Length, EfiAttributes);
>    } else {
>      return EFI_SUCCESS;
>    }
> diff --git a/ArmPkg/Include/Library/ArmMmuLib.h b/ArmPkg/Include/Library/ArmMmuLib.h
> index d3a302fa8125..fb7fd006417c 100644
> --- a/ArmPkg/Include/Library/ArmMmuLib.h
> +++ b/ArmPkg/Include/Library/ArmMmuLib.h
> @@ -66,8 +66,7 @@ EFI_STATUS
>  ArmSetMemoryAttributes (
>    IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN UINT64                    Length,
> -  IN UINT64                    Attributes,
> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> +  IN UINT64                    Attributes
>    );
>  
>  #endif
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 77f108971f3e..8bd1c6fad95f 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -450,8 +450,7 @@ EFI_STATUS
>  ArmSetMemoryAttributes (
>    IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN UINT64                    Length,
> -  IN UINT64                    Attributes,
> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> +  IN UINT64                    Attributes
>    )
>  {
>    EFI_STATUS                   Status;
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index 93980d6d12db..1112660b434e 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -469,8 +469,7 @@ EFI_STATUS
>  UpdatePageEntries (
>    IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN UINT64                    Length,
> -  IN UINT64                    Attributes,
> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> +  IN UINT64                    Attributes
>    )
>  {
>    EFI_STATUS    Status;
> @@ -575,11 +574,6 @@ UpdatePageEntries (
>      // Mask in new attributes and/or permissions
>      PageTableEntry |= EntryValue;
>  
> -    if (VirtualMask != 0) {
> -      // Make this virtual address point at a physical page
> -      PageTableEntry &= ~VirtualMask;
> -    }
> -
>      if (CurrentPageTableEntry  != PageTableEntry) {
>        Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
>        if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
> @@ -606,8 +600,7 @@ EFI_STATUS
>  UpdateSectionEntries (
>    IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN UINT64                    Length,
> -  IN UINT64                    Attributes,
> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> +  IN UINT64                    Attributes
>    )
>  {
>    EFI_STATUS    Status = EFI_SUCCESS;
> @@ -680,7 +673,7 @@ UpdateSectionEntries (
>      // has this descriptor already been coverted to pages?
>      if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
>        // forward this 1MB range to page table function instead
> -      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
> +      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes);
>      } else {
>        // still a section entry
>  
> @@ -689,9 +682,6 @@ UpdateSectionEntries (
>  
>        // mask in new attributes and/or permissions
>        Descriptor |= EntryValue;
> -      if (VirtualMask != 0) {
> -        Descriptor &= ~VirtualMask;
> -      }
>  
>        if (CurrentDescriptor  != Descriptor) {
>          Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> @@ -717,8 +707,7 @@ EFI_STATUS
>  ArmSetMemoryAttributes (
>    IN EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN UINT64                    Length,
> -  IN UINT64                    Attributes,
> -  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> +  IN UINT64                    Attributes
>    )
>  {
>    EFI_STATUS    Status;
> @@ -736,8 +725,7 @@ ArmSetMemoryAttributes (
>          "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
>          BaseAddress, ChunkLength, Attributes));
>  
> -      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
> -                 VirtualMask);
> +      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes);
>  
>        FlushTlbs = TRUE;
>      } else {
> @@ -756,8 +744,7 @@ ArmSetMemoryAttributes (
>          "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
>          BaseAddress, ChunkLength, Attributes));
>  
> -      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
> -                 VirtualMask);
> +      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes);
>      }
>  
>      if (EFI_ERROR (Status)) {
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] ArmPkg/ArmMmuLib ARM: implement memory permission control routines
  2017-03-01 16:31 ` [PATCH 4/5] ArmPkg/ArmMmuLib ARM: implement memory permission control routines Ard Biesheuvel
@ 2017-03-06 16:11   ` Leif Lindholm
  0 siblings, 0 replies; 14+ messages in thread
From: Leif Lindholm @ 2017-03-06 16:11 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel, lersek

On Wed, Mar 01, 2017 at 04:31:42PM +0000, Ard Biesheuvel wrote:
> Now that we have the prerequisite functionality available in ArmMmuLib,
> wire it up into ArmSetMemoryRegionNoExec, ArmClearMemoryRegionNoExec,
> ArmSetMemoryRegionReadOnly and ArmClearMemoryRegionReadOnly. This is
> used by the non-executable stack feature that is configured by DxeIpl.
> 
> NOTE: The current implementation will not combine RO and XP attributes,
>       i.e., setting/clearing a region no-exec will unconditionally
>       clear the read-only attribute, and vice versa. Currently, we
>       only use ArmSetMemoryRegionNoExec(), so for now, we should be
>       able to live with this.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index 1112660b434e..55601328d93e 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -761,40 +761,40 @@ ArmSetMemoryAttributes (
>    return Status;
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS

Could these RETURN_->EFI_ fixes be folded into 1/5 instead (if you've
not already pushed it by the time you get here)?

>  ArmSetMemoryRegionNoExec (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_XP);
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmClearMemoryRegionNoExec (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  return ArmSetMemoryAttributes (BaseAddress, Length, 0);

I'd be slightly happier if there was a #define for that 0, throughout.
Alternatively, replace with a macro called ArmClearMemoryAttributes().

/
    Leif

>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmSetMemoryRegionReadOnly (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO);
>  }
>  
> -RETURN_STATUS
> +EFI_STATUS
>  ArmClearMemoryRegionReadOnly (
>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
>    IN  UINT64                    Length
>    )
>  {
> -  return RETURN_UNSUPPORTED;
> +  return ArmSetMemoryAttributes (BaseAddress, Length, 0);
>  }
>  
>  RETURN_STATUS
> -- 
> 2.7.4
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib
  2017-03-06 16:05     ` Ard Biesheuvel
@ 2017-03-06 16:21       ` Leif Lindholm
  0 siblings, 0 replies; 14+ messages in thread
From: Leif Lindholm @ 2017-03-06 16:21 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Laszlo Ersek

On Mon, Mar 06, 2017 at 05:05:58PM +0100, Ard Biesheuvel wrote:
> >> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> >> index 4b6f4ce392b7..93980d6d12db 100644
> >> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> >> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> >> @@ -16,6 +16,7 @@
> >>  #include <Uefi.h>
> >>  #include <Chipset/ArmV7.h>
> >>  #include <Library/BaseMemoryLib.h>
> >> +#include <Library/CacheMaintenanceLib.h>
> >>  #include <Library/MemoryAllocationLib.h>
> >>  #include <Library/ArmLib.h>
> >>  #include <Library/BaseLib.h>
> >> @@ -36,6 +37,12 @@
> >>  #define ID_MMFR0_SHR_IMP_HW_COHERENT   1
> >>  #define ID_MMFR0_SHR_IGNORED         0xf
> >>
> >> +// First Level Descriptors
> >> +typedef UINT32    ARM_FIRST_LEVEL_DESCRIPTOR;
> >> +
> >> +// Second Level Descriptors
> >> +typedef UINT32    ARM_PAGE_TABLE_ENTRY;
> >> +
> >
> > Copied from ArmPkg/Drivers/CpuDxe/Arm/Mmu.c, but not deleted there.
> > Can it be, or can it be moved out into a header somewhere?
> >
> > No other comments.
> >
> 
> It is used in both places, so I'd need to put in in a header file
> 
> ArmPkg/Include/Chipset/ArmV7Mmu.h comes to mind ...

Works for me.
If you fold that in:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

> >>  UINTN
> >>  EFIAPI
> >>  ArmReadIdMmfr0 (
> >> @@ -406,6 +413,367 @@ ArmConfigureMmu (
> >>    return RETURN_SUCCESS;
> >>  }
> >>
> >> +STATIC
> >> +EFI_STATUS
> >> +ConvertSectionToPages (
> >> +  IN EFI_PHYSICAL_ADDRESS  BaseAddress
> >> +  )
> >> +{
> >> +  UINT32                  FirstLevelIdx;
> >> +  UINT32                  SectionDescriptor;
> >> +  UINT32                  PageTableDescriptor;
> >> +  UINT32                  PageDescriptor;
> >> +  UINT32                  Index;
> >> +
> >> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> >> +  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
> >> +
> >> +  DEBUG ((EFI_D_PAGE, "Converting section at 0x%x to pages\n", (UINTN)BaseAddress));
> >> +
> >> +  // Obtain page table base
> >> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> >> +
> >> +  // Calculate index into first level translation table for start of modification
> >> +  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> >> +  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> >> +
> >> +  // Get section attributes and convert to page attributes
> >> +  SectionDescriptor = FirstLevelTable[FirstLevelIdx];
> >> +  PageDescriptor = TT_DESCRIPTOR_PAGE_TYPE_PAGE | ConvertSectionAttributesToPageAttributes (SectionDescriptor, FALSE);
> >> +
> >> +  // Allocate a page table for the 4KB entries (we use up a full page even though we only need 1KB)
> >> +  PageTable = (volatile ARM_PAGE_TABLE_ENTRY *)AllocatePages (1);
> >> +  if (PageTable == NULL) {
> >> +    return EFI_OUT_OF_RESOURCES;
> >> +  }
> >> +
> >> +  // Write the page table entries out
> >> +  for (Index = 0; Index < TRANSLATION_TABLE_PAGE_COUNT; Index++) {
> >> +    PageTable[Index] = TT_DESCRIPTOR_PAGE_BASE_ADDRESS(BaseAddress + (Index << 12)) | PageDescriptor;
> >> +  }
> >> +
> >> +  // Flush d-cache so descriptors make it back to uncached memory for subsequent table walks
> >> +  WriteBackInvalidateDataCacheRange ((VOID *)PageTable, TT_DESCRIPTOR_PAGE_SIZE);
> >> +
> >> +  // Formulate page table entry, Domain=0, NS=0
> >> +  PageTableDescriptor = (((UINTN)PageTable) & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) | TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
> >> +
> >> +  // Write the page table entry out, replacing section entry
> >> +  FirstLevelTable[FirstLevelIdx] = PageTableDescriptor;
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >> +STATIC
> >> +EFI_STATUS
> >> +UpdatePageEntries (
> >> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> >> +  IN UINT64                    Length,
> >> +  IN UINT64                    Attributes,
> >> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> >> +  )
> >> +{
> >> +  EFI_STATUS    Status;
> >> +  UINT32        EntryValue;
> >> +  UINT32        EntryMask;
> >> +  UINT32        FirstLevelIdx;
> >> +  UINT32        Offset;
> >> +  UINT32        NumPageEntries;
> >> +  UINT32        Descriptor;
> >> +  UINT32        p;
> >> +  UINT32        PageTableIndex;
> >> +  UINT32        PageTableEntry;
> >> +  UINT32        CurrentPageTableEntry;
> >> +  VOID          *Mva;
> >> +
> >> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> >> +  volatile ARM_PAGE_TABLE_ENTRY         *PageTable;
> >> +
> >> +  Status = EFI_SUCCESS;
> >> +
> >> +  // 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;
> >> +  }
> >> +
> >> +  // Although the PI spec is unclear on this the GCD guarantees that only
> >> +  // one Attribute bit is set at a time, so we can safely use a switch statement
> >> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // map to strongly ordered
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // map to normal non-cachable
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // write through with no-allocate
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // write back (with allocate)
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> +  }
> >> +
> >> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
> >> +  } else {
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
> >> +  }
> >> +
> >> +  // Obtain page table base
> >> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> >> +
> >> +  // Calculate number of 4KB page table entries to change
> >> +  NumPageEntries = Length / TT_DESCRIPTOR_PAGE_SIZE;
> >> +
> >> +  // Iterate for the number of 4KB pages to change
> >> +  Offset = 0;
> >> +  for(p = 0; p < NumPageEntries; p++) {
> >> +    // Calculate index into first level translation table for page table value
> >> +
> >> +    FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress + Offset) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> >> +    ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> >> +
> >> +    // Read the descriptor from the first level page table
> >> +    Descriptor = FirstLevelTable[FirstLevelIdx];
> >> +
> >> +    // Does this descriptor need to be converted from section entry to 4K pages?
> >> +    if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(Descriptor)) {
> >> +      Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> >> +      if (EFI_ERROR(Status)) {
> >> +        // Exit for loop
> >> +        break;
> >> +      }
> >> +
> >> +      // Re-read descriptor
> >> +      Descriptor = FirstLevelTable[FirstLevelIdx];
> >> +    }
> >> +
> >> +    // Obtain page table base address
> >> +    PageTable = (ARM_PAGE_TABLE_ENTRY *)TT_DESCRIPTOR_PAGE_BASE_ADDRESS(Descriptor);
> >> +
> >> +    // Calculate index into the page table
> >> +    PageTableIndex = ((BaseAddress + Offset) & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT;
> >> +    ASSERT (PageTableIndex < TRANSLATION_TABLE_PAGE_COUNT);
> >> +
> >> +    // Get the entry
> >> +    CurrentPageTableEntry = PageTable[PageTableIndex];
> >> +
> >> +    // Mask off appropriate fields
> >> +    PageTableEntry = CurrentPageTableEntry & ~EntryMask;
> >> +
> >> +    // Mask in new attributes and/or permissions
> >> +    PageTableEntry |= EntryValue;
> >> +
> >> +    if (VirtualMask != 0) {
> >> +      // Make this virtual address point at a physical page
> >> +      PageTableEntry &= ~VirtualMask;
> >> +    }
> >> +
> >> +    if (CurrentPageTableEntry  != PageTableEntry) {
> >> +      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
> >> +      if ((CurrentPageTableEntry & TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) == TT_DESCRIPTOR_PAGE_CACHEABLE_MASK) {
> >> +        // The current section mapping is cacheable so Clean/Invalidate the MVA of the page
> >> +        // Note assumes switch(Attributes), not ARMv7 possibilities
> >> +        WriteBackInvalidateDataCacheRange (Mva, TT_DESCRIPTOR_PAGE_SIZE);
> >> +      }
> >> +
> >> +      // Only need to update if we are changing the entry
> >> +      PageTable[PageTableIndex] = PageTableEntry;
> >> +      ArmUpdateTranslationTableEntry ((VOID *)&PageTable[PageTableIndex], Mva);
> >> +    }
> >> +
> >> +    Status = EFI_SUCCESS;
> >> +    Offset += TT_DESCRIPTOR_PAGE_SIZE;
> >> +
> >> +  } // End first level translation table loop
> >> +
> >> +  return Status;
> >> +}
> >> +
> >> +STATIC
> >> +EFI_STATUS
> >> +UpdateSectionEntries (
> >> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> >> +  IN UINT64                    Length,
> >> +  IN UINT64                    Attributes,
> >> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> >> +  )
> >> +{
> >> +  EFI_STATUS    Status = EFI_SUCCESS;
> >> +  UINT32        EntryMask;
> >> +  UINT32        EntryValue;
> >> +  UINT32        FirstLevelIdx;
> >> +  UINT32        NumSections;
> >> +  UINT32        i;
> >> +  UINT32        CurrentDescriptor;
> >> +  UINT32        Descriptor;
> >> +  VOID          *Mva;
> >> +  volatile ARM_FIRST_LEVEL_DESCRIPTOR   *FirstLevelTable;
> >> +
> >> +  // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
> >> +  // EntryValue: values at bit positions specified by EntryMask
> >> +
> >> +  // Make sure we handle a section range that is unmapped
> >> +  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
> >> +              TT_DESCRIPTOR_SECTION_AP_MASK;
> >> +  EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
> >> +
> >> +  // Although the PI spec is unclear on this the GCD guarantees that only
> >> +  // one Attribute bit is set at a time, so we can safely use a switch statement
> >> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // map to strongly ordered
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // map to normal non-cachable
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // write through with no-allocate
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // write back (with allocate)
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> +  }
> >> +
> >> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
> >> +  } else {
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> >> +  }
> >> +
> >> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
> >> +  }
> >> +
> >> +  // obtain page table base
> >> +  FirstLevelTable = (ARM_FIRST_LEVEL_DESCRIPTOR *)ArmGetTTBR0BaseAddress ();
> >> +
> >> +  // calculate index into first level translation table for start of modification
> >> +  FirstLevelIdx = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(BaseAddress) >> TT_DESCRIPTOR_SECTION_BASE_SHIFT;
> >> +  ASSERT (FirstLevelIdx < TRANSLATION_TABLE_SECTION_COUNT);
> >> +
> >> +  // calculate number of 1MB first level entries this applies to
> >> +  NumSections = Length / TT_DESCRIPTOR_SECTION_SIZE;
> >> +
> >> +  // iterate through each descriptor
> >> +  for(i=0; i<NumSections; i++) {
> >> +    CurrentDescriptor = FirstLevelTable[FirstLevelIdx + i];
> >> +
> >> +    // has this descriptor already been coverted to pages?
> >> +    if (TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE(CurrentDescriptor)) {
> >> +      // forward this 1MB range to page table function instead
> >> +      Status = UpdatePageEntries ((FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT, TT_DESCRIPTOR_SECTION_SIZE, Attributes, VirtualMask);
> >> +    } else {
> >> +      // still a section entry
> >> +
> >> +      // mask off appropriate fields
> >> +      Descriptor = CurrentDescriptor & ~EntryMask;
> >> +
> >> +      // mask in new attributes and/or permissions
> >> +      Descriptor |= EntryValue;
> >> +      if (VirtualMask != 0) {
> >> +        Descriptor &= ~VirtualMask;
> >> +      }
> >> +
> >> +      if (CurrentDescriptor  != Descriptor) {
> >> +        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelTable) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
> >> +        if ((CurrentDescriptor & TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) == TT_DESCRIPTOR_SECTION_CACHEABLE_MASK) {
> >> +          // The current section mapping is cacheable so Clean/Invalidate the MVA of the section
> >> +          // Note assumes switch(Attributes), not ARMv7 possabilities
> >> +          WriteBackInvalidateDataCacheRange (Mva, SIZE_1MB);
> >> +        }
> >> +
> >> +        // Only need to update if we are changing the descriptor
> >> +        FirstLevelTable[FirstLevelIdx + i] = Descriptor;
> >> +        ArmUpdateTranslationTableEntry ((VOID *)&FirstLevelTable[FirstLevelIdx + i], Mva);
> >> +      }
> >> +
> >> +      Status = EFI_SUCCESS;
> >> +    }
> >> +  }
> >> +
> >> +  return Status;
> >> +}
> >> +
> >> +EFI_STATUS
> >> +ArmSetMemoryAttributes (
> >> +  IN EFI_PHYSICAL_ADDRESS      BaseAddress,
> >> +  IN UINT64                    Length,
> >> +  IN UINT64                    Attributes,
> >> +  IN EFI_PHYSICAL_ADDRESS      VirtualMask
> >> +  )
> >> +{
> >> +  EFI_STATUS    Status;
> >> +  UINT64        ChunkLength;
> >> +  BOOLEAN       FlushTlbs;
> >> +
> >> +  FlushTlbs = FALSE;
> >> +  while (Length > 0) {
> >> +    if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> >> +        Length >= TT_DESCRIPTOR_SECTION_SIZE) {
> >> +
> >> +      ChunkLength = Length - Length % TT_DESCRIPTOR_SECTION_SIZE;
> >> +
> >> +      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
> >> +        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
> >> +        BaseAddress, ChunkLength, Attributes));
> >> +
> >> +      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes,
> >> +                 VirtualMask);
> >> +
> >> +      FlushTlbs = TRUE;
> >> +    } else {
> >> +
> >> +      //
> >> +      // Process page by page until the next section boundary, but only if
> >> +      // we have more than a section's worth of area to deal with after that.
> >> +      //
> >> +      ChunkLength = TT_DESCRIPTOR_SECTION_SIZE -
> >> +                    (BaseAddress % TT_DESCRIPTOR_SECTION_SIZE);
> >> +      if (ChunkLength + TT_DESCRIPTOR_SECTION_SIZE > Length) {
> >> +        ChunkLength = Length;
> >> +      }
> >> +
> >> +      DEBUG ((DEBUG_PAGE | DEBUG_INFO,
> >> +        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
> >> +        BaseAddress, ChunkLength, Attributes));
> >> +
> >> +      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
> >> +                 VirtualMask);
> >> +    }
> >> +
> >> +    if (EFI_ERROR (Status)) {
> >> +      break;
> >> +    }
> >> +
> >> +    BaseAddress += ChunkLength;
> >> +    Length -= ChunkLength;
> >> +  }
> >> +
> >> +  if (FlushTlbs) {
> >> +    ArmInvalidateTlb ();
> >> +  }
> >> +  return Status;
> >> +}
> >> +
> >>  RETURN_STATUS
> >>  ArmSetMemoryRegionNoExec (
> >>    IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
> >> --
> >> 2.7.4
> >>


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-03-06 16:21 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-01 16:31 [PATCH 0/5] ArmPkg, ArmVirtPkg ARM: enable non-executable stack Ard Biesheuvel
2017-03-01 16:31 ` [PATCH 1/5] ArmPkg/ArmMmuLib AARCH64: use correct return type for exported functions Ard Biesheuvel
2017-03-06 14:57   ` Leif Lindholm
2017-03-01 16:31 ` [PATCH 2/5] ArmPkg: move ARM version of SetMemoryAttributes to ArmMmuLib Ard Biesheuvel
2017-03-06 16:03   ` Leif Lindholm
2017-03-06 16:05     ` Ard Biesheuvel
2017-03-06 16:21       ` Leif Lindholm
2017-03-01 16:31 ` [PATCH 3/5] ArmPkg/ArmMmuLib: remove VirtualMask arg from ArmSetMemoryAttributes Ard Biesheuvel
2017-03-06 16:06   ` Leif Lindholm
2017-03-01 16:31 ` [PATCH 4/5] ArmPkg/ArmMmuLib ARM: implement memory permission control routines Ard Biesheuvel
2017-03-06 16:11   ` Leif Lindholm
2017-03-01 16:31 ` [PATCH 5/5] ArmVirtPkg: enable non-executable DXE stack for all platforms Ard Biesheuvel
2017-03-01 19:10   ` Laszlo Ersek
2017-03-01 19:10     ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox