[PATCH v2 0/9] ArmPkg: eradicate and deprecate by set/way cache ops

[PATCH v2 0/9] ArmPkg: eradicate and deprecate by set/way cache ops

[Ard Biesheuvel]

This is a combination of v1 'ArmPkg: eradicate and deprecate by set/way cache
ops' and v1 'ArmPkg/ArmLib: ASSERT() on misuse of set/way ops'

As it turns out, there were still some instances of set/way ops left in
the core code, in ArmMmuLib to be precise.

This series fixes ArmMmuLib to perform the appropriate cache invalidation
when populating page tables with the MMU and caches off, allowing us to
get rid of the cache clean/disable/enable sequences which are incorrect
and pointless at the same time.

I have incorporated some ArmLib changes that I posted separately before,
with the end result being that all uses pf set/way ops are gone from the
EDK2 core code, and the routines themselves will now ASSERT() when used
for anything other than managing the caches while the MMU is still off.
(Note that BeagleBoard in edk2-platforms still relies on this)

Changes since v1[s]:
- don't deprecate the set/way ops but make the usable before MMU is enabled
  only
- use a more elaborate sequence for invalidating the page table entries on
  32-bit ARM, to ensure we are compliant with the nooks and crannies of
  version 7 of the architecture.
- incorporate patch #1, which was still pending on the list, and is related
  (its PrePeiCore sibling was already reviewed and merged)

Ard Biesheuvel (9):
  ArmPlatformPkg/PrePi: replace set/way cache ops with by-VA ones
  ArmPkg/ArmMmuLib ARM: remove dummy constructor
  ArmPkg/ArmMmuLib ARM: split ArmMmuLibCore.c into core and update code
  ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries
  ArmPkg/ArmMmuLib AARCH64: cache-invalidate initial page table entries
  ArmPkg/ArmLib: move set/way helper functions into private header
  ArmPkg/ArmLib: clean up library includes
  ArmPkg/ArmLib: remove bogus protocol declaration
  ArmPkg/ArmLib: ASSERT on set/way cache ops being used with MMU on

 ArmPkg/Include/Library/ArmLib.h               |  18 -
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c    |  16 +-
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h    |  18 +
 ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S |   9 +-
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c          |  17 +-
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h          |  18 +
 ArmPkg/Library/ArmLib/ArmBaseLib.inf          |   6 +-
 ArmPkg/Library/ArmLib/ArmLib.c                |   2 -
 .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |   9 -
 .../Library/ArmMmuLib/Arm/ArmMmuLibConvert.c  |  32 ++
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c  | 498 ++----------------
 .../Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c   | 435 +++++++++++++++
 ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf    |   4 +
 ArmPlatformPkg/PrePi/PeiMPCore.inf            |   1 +
 ArmPlatformPkg/PrePi/PeiUniCore.inf           |   1 +
 ArmPlatformPkg/PrePi/PrePi.c                  |   8 +-
 16 files changed, 591 insertions(+), 501 deletions(-)
 create mode 100644 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
 create mode 100644 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c

-- 
2.17.1

[PATCH v2 1/9] ArmPlatformPkg/PrePi: replace set/way cache ops with by-VA ones

[Ard Biesheuvel]

Cache maintenance operations by set/way are only intended to be used
in the context of on/offlining a core, while it has been taken out of
the coherency domain. Any use intended to ensure that the contents of
the cache have made it to main memory is unreliable, since cacheline
migration and non-architected system caches may cause these contents
to linger elsewhere, without being visible in main memory once the
MMU and caches are disabled.

In KVM on Linux, there are horrid hacks in place to ensure that such
set/way operations are trapped, and replaced with a single by-VA
clean/invalidate of the entire guest VA space once the MMU state
changes, which can be costly, and is unnecessary if we manage the
caches a bit more carefully, and perform maintenance by virtual
address only.

So let's get rid of the call to ArmInvalidateDataCache () in the
PrePeiCore startup code, and instead, invalidate the UEFI memory
region by virtual address, which is the only memory region we will
be touching with the caches and MMU both disabled and enabled.
(This will lead to data corruption if data written with the MMU off
is shadowed by clean, stale cachelines that stick around when the
MMU is enabled again.)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Tested-By: Pete Batard <pete@akeo.ie>
---
 ArmPlatformPkg/PrePi/PeiMPCore.inf  | 1 +
 ArmPlatformPkg/PrePi/PeiUniCore.inf | 1 +
 ArmPlatformPkg/PrePi/PrePi.c        | 8 +++++---
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/ArmPlatformPkg/PrePi/PeiMPCore.inf b/ArmPlatformPkg/PrePi/PeiMPCore.inf
index 9c5da0d42a7b..053f9fd9e616 100644
--- a/ArmPlatformPkg/PrePi/PeiMPCore.inf
+++ b/ArmPlatformPkg/PrePi/PeiMPCore.inf
@@ -37,6 +37,7 @@ [Packages]
 
 [LibraryClasses]
   BaseLib
+  CacheMaintenanceLib
   DebugLib
   DebugAgentLib
   ArmLib
diff --git a/ArmPlatformPkg/PrePi/PeiUniCore.inf b/ArmPlatformPkg/PrePi/PeiUniCore.inf
index ee9b05b25337..78d218ae09ca 100644
--- a/ArmPlatformPkg/PrePi/PeiUniCore.inf
+++ b/ArmPlatformPkg/PrePi/PeiUniCore.inf
@@ -37,6 +37,7 @@ [Packages]
 
 [LibraryClasses]
   BaseLib
+  CacheMaintenanceLib
   DebugLib
   DebugAgentLib
   ArmLib
diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
index 2bb144958139..254fb331733e 100644
--- a/ArmPlatformPkg/PrePi/PrePi.c
+++ b/ArmPlatformPkg/PrePi/PrePi.c
@@ -8,6 +8,7 @@
 
 #include <PiPei.h>
 
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/DebugAgentLib.h>
 #include <Library/PrePiLib.h>
 #include <Library/PrintLib.h>
@@ -178,8 +179,6 @@ CEntryPoint (
 
   // Data Cache enabled on Primary core when MMU is enabled.
   ArmDisableDataCache ();
-  // Invalidate Data cache
-  ArmInvalidateDataCache ();
   // Invalidate instruction cache
   ArmInvalidateInstructionCache ();
   // Enable Instruction Caches on all cores.
@@ -200,6 +199,10 @@ CEntryPoint (
 
   // If not primary Jump to Secondary Main
   if (ArmPlatformIsPrimaryCore (MpId)) {
+
+    InvalidateDataCacheRange ((VOID *)UefiMemoryBase,
+                              FixedPcdGet32(PcdSystemMemoryUefiRegionSize));
+
     // Goto primary Main.
     PrimaryMain (UefiMemoryBase, StacksBase, StartTimeStamp);
   } else {
@@ -209,4 +212,3 @@ CEntryPoint (
   // DXE Core should always load and never return
   ASSERT (FALSE);
 }
-
-- 
2.17.1

[PATCH v2 2/9] ArmPkg/ArmMmuLib ARM: remove dummy constructor

[Ard Biesheuvel]

Make the CONSTRUCTOR define in the .INF AARCH64 only, so we can drop
the empty stub that exists for ARM.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 9 ---------
 ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf   | 2 ++
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 74ac31de98cc..a6601258bee0 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -830,12 +830,3 @@ ArmClearMemoryRegionReadOnly (
 {
   return ArmSetMemoryAttributes (BaseAddress, Length, __EFI_MEMORY_RWX);
 }
-
-RETURN_STATUS
-EFIAPI
-ArmMmuBaseLibConstructor (
-  VOID
-  )
-{
-  return RETURN_SUCCESS;
-}
diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
index 5028a955afac..3dfe68ba48a6 100644
--- a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
+++ b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
@@ -14,6 +14,8 @@ [Defines]
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
   LIBRARY_CLASS                  = ArmMmuLib
+
+[Defines.AARCH64]
   CONSTRUCTOR                    = ArmMmuBaseLibConstructor
 
 [Sources.AARCH64]
-- 
2.17.1

[PATCH v2 3/9] ArmPkg/ArmMmuLib ARM: split ArmMmuLibCore.c into core and update code

[Ard Biesheuvel]

Unlike the AArch64 implementation of ArmMmuLib, which combines the
initial page table population code with the code that runs at later
stages to manage permission attributes in the page tables, ARM uses
two completely separate sets of routines for this.

Since ArmMmuLib is a static library, we can prevent duplication of
this code between different users, which usually only need one or
the other. (Note that LTO should also achieve the same.)

This also makes it easier to reason about modifying the cache
maintenance handling, and replace the set/way ops with by-VA
ops, since the code that performs the set/way ops only executes
when the MMU is still off.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c |  32 ++
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c    | 434 -------------------
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c  | 435 ++++++++++++++++++++
 ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf      |   2 +
 4 files changed, 469 insertions(+), 434 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
new file mode 100644
index 000000000000..e3b02a9fba57
--- /dev/null
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
@@ -0,0 +1,32 @@
+/** @file
+*  File managing the MMU for ARMv7 architecture
+*
+*  Copyright (c) 2011-2016, ARM Limited. All rights reserved.
+*
+*  SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <Uefi.h>
+
+#include <Library/ArmLib.h>
+
+#include <Chipset/ArmV7.h>
+
+UINT32
+ConvertSectionAttributesToPageAttributes (
+  IN UINT32   SectionAttributes,
+  IN BOOLEAN  IsLargePage
+  )
+{
+  UINT32 PageAttributes;
+
+  PageAttributes = 0;
+  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY (SectionAttributes, IsLargePage);
+  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_AP (SectionAttributes);
+  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_XN (SectionAttributes, IsLargePage);
+  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_NG (SectionAttributes);
+  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_S (SectionAttributes);
+
+  return PageAttributes;
+}
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index a6601258bee0..aca7a37facac 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -31,15 +31,6 @@
 #define ID_MMFR0_SHR_IMP_HW_COHERENT   1
 #define ID_MMFR0_SHR_IGNORED         0xf
 
-#define __EFI_MEMORY_RWX               0    // no restrictions
-
-#define CACHE_ATTRIBUTE_MASK   (EFI_MEMORY_UC | \
-                                EFI_MEMORY_WC | \
-                                EFI_MEMORY_WT | \
-                                EFI_MEMORY_WB | \
-                                EFI_MEMORY_UCE | \
-                                EFI_MEMORY_WP)
-
 UINTN
 EFIAPI
 ArmReadIdMmfr0 (
@@ -52,24 +43,6 @@ ArmHasMpExtensions (
   VOID
   );
 
-UINT32
-ConvertSectionAttributesToPageAttributes (
-  IN UINT32   SectionAttributes,
-  IN BOOLEAN  IsLargePage
-  )
-{
-  UINT32 PageAttributes;
-
-  PageAttributes = 0;
-  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_CACHE_POLICY (SectionAttributes, IsLargePage);
-  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_AP (SectionAttributes);
-  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_XN (SectionAttributes, IsLargePage);
-  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_NG (SectionAttributes);
-  PageAttributes |= TT_DESCRIPTOR_CONVERT_TO_PAGE_S (SectionAttributes);
-
-  return PageAttributes;
-}
-
 STATIC
 BOOLEAN
 PreferNonshareableMemory (
@@ -423,410 +396,3 @@ ArmConfigureMmu (
   ArmEnableMmu();
   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;
-  }
-
-  // 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,
-  OUT BOOLEAN                   *FlushTlbs OPTIONAL
-  )
-{
-  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) {
-    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 the order of the conditionals below
-  // is irrelevant. If no memory attribute is specified, we preserve whatever
-  // memory type is set in the page tables, and update the permission attributes
-  // only.
-  if (Attributes & EFI_MEMORY_UC) {
-    // 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) {
-    // 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) {
-    // 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) {
-    // 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
-  } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
-    // catch unsupported memory type attributes
-    ASSERT (FALSE);
-    return EFI_UNSUPPORTED;
-  }
-
-  if (Attributes & EFI_MEMORY_RO) {
-    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];
-      if (FlushTlbs != NULL) {
-        *FlushTlbs = TRUE;
-      }
-    }
-
-    // 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 (CurrentPageTableEntry  != PageTableEntry) {
-      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
-
-      // 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
-  )
-{
-  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 the order of the conditionals below
-  // is irrelevant. If no memory attribute is specified, we preserve whatever
-  // memory type is set in the page tables, and update the permission attributes
-  // only.
-  if (Attributes & EFI_MEMORY_UC) {
-    // 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) {
-    // 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) {
-    // 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) {
-    // 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
-  } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
-    // catch unsupported memory type attributes
-    ASSERT (FALSE);
-    return EFI_UNSUPPORTED;
-  }
-
-  if (Attributes & EFI_MEMORY_RO) {
-    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
-  } else {
-    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
-  }
-
-  if (Attributes & EFI_MEMORY_XP) {
-    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 converted 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,
-                 NULL);
-    } else {
-      // still a section entry
-
-      if (CurrentDescriptor != 0) {
-        // mask off appropriate fields
-        Descriptor = CurrentDescriptor & ~EntryMask;
-      } else {
-        Descriptor = ((UINTN)FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT;
-      }
-
-      // mask in new attributes and/or permissions
-      Descriptor |= EntryValue;
-
-      if (CurrentDescriptor  != Descriptor) {
-        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
-
-        // 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
-  )
-{
-  EFI_STATUS    Status;
-  UINT64        ChunkLength;
-  BOOLEAN       FlushTlbs;
-
-  if (BaseAddress > (UINT64)MAX_ADDRESS) {
-    return EFI_UNSUPPORTED;
-  }
-
-  Length = MIN (Length, (UINT64)MAX_ADDRESS - BaseAddress + 1);
-  if (Length == 0) {
-    return EFI_SUCCESS;
-  }
-
-  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,
-        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
-        BaseAddress, ChunkLength, Attributes));
-
-      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes);
-
-      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,
-        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
-        BaseAddress, ChunkLength, Attributes));
-
-      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
-                 &FlushTlbs);
-    }
-
-    if (EFI_ERROR (Status)) {
-      break;
-    }
-
-    BaseAddress += ChunkLength;
-    Length -= ChunkLength;
-  }
-
-  if (FlushTlbs) {
-    ArmInvalidateTlb ();
-  }
-  return Status;
-}
-
-EFI_STATUS
-ArmSetMemoryRegionNoExec (
-  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
-  IN  UINT64                    Length
-  )
-{
-  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_XP);
-}
-
-EFI_STATUS
-ArmClearMemoryRegionNoExec (
-  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
-  IN  UINT64                    Length
-  )
-{
-  return ArmSetMemoryAttributes (BaseAddress, Length, __EFI_MEMORY_RWX);
-}
-
-EFI_STATUS
-ArmSetMemoryRegionReadOnly (
-  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
-  IN  UINT64                    Length
-  )
-{
-  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO);
-}
-
-EFI_STATUS
-ArmClearMemoryRegionReadOnly (
-  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
-  IN  UINT64                    Length
-  )
-{
-  return ArmSetMemoryAttributes (BaseAddress, Length, __EFI_MEMORY_RWX);
-}
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
new file mode 100644
index 000000000000..3dafe1d964cd
--- /dev/null
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -0,0 +1,435 @@
+/** @file
+*  File managing the MMU for ARMv7 architecture
+*
+*  Copyright (c) 2011-2016, ARM Limited. All rights reserved.
+*
+*  SPDX-License-Identifier: BSD-2-Clause-Patent
+*
+**/
+
+#include <Uefi.h>
+
+#include <Library/ArmLib.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/CacheMaintenanceLib.h>
+#include <Library/MemoryAllocationLib.h>
+
+#include <Chipset/ArmV7.h>
+
+#define __EFI_MEMORY_RWX               0    // no restrictions
+
+#define CACHE_ATTRIBUTE_MASK   (EFI_MEMORY_UC | \
+                                EFI_MEMORY_WC | \
+                                EFI_MEMORY_WT | \
+                                EFI_MEMORY_WB | \
+                                EFI_MEMORY_UCE | \
+                                EFI_MEMORY_WP)
+
+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;
+  }
+
+  // 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,
+  OUT BOOLEAN                   *FlushTlbs OPTIONAL
+  )
+{
+  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) {
+    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 the order of the conditionals below
+  // is irrelevant. If no memory attribute is specified, we preserve whatever
+  // memory type is set in the page tables, and update the permission attributes
+  // only.
+  if (Attributes & EFI_MEMORY_UC) {
+    // 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) {
+    // 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) {
+    // 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) {
+    // 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
+  } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
+    // catch unsupported memory type attributes
+    ASSERT (FALSE);
+    return EFI_UNSUPPORTED;
+  }
+
+  if (Attributes & EFI_MEMORY_RO) {
+    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];
+      if (FlushTlbs != NULL) {
+        *FlushTlbs = TRUE;
+      }
+    }
+
+    // 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 (CurrentPageTableEntry  != PageTableEntry) {
+      Mva = (VOID *)(UINTN)((((UINTN)FirstLevelIdx) << TT_DESCRIPTOR_SECTION_BASE_SHIFT) + (PageTableIndex << TT_DESCRIPTOR_PAGE_BASE_SHIFT));
+
+      // 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
+  )
+{
+  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 the order of the conditionals below
+  // is irrelevant. If no memory attribute is specified, we preserve whatever
+  // memory type is set in the page tables, and update the permission attributes
+  // only.
+  if (Attributes & EFI_MEMORY_UC) {
+    // 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) {
+    // 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) {
+    // 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) {
+    // 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
+  } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
+    // catch unsupported memory type attributes
+    ASSERT (FALSE);
+    return EFI_UNSUPPORTED;
+  }
+
+  if (Attributes & EFI_MEMORY_RO) {
+    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
+  } else {
+    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
+  }
+
+  if (Attributes & EFI_MEMORY_XP) {
+    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 converted 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,
+                 NULL);
+    } else {
+      // still a section entry
+
+      if (CurrentDescriptor != 0) {
+        // mask off appropriate fields
+        Descriptor = CurrentDescriptor & ~EntryMask;
+      } else {
+        Descriptor = ((UINTN)FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT;
+      }
+
+      // mask in new attributes and/or permissions
+      Descriptor |= EntryValue;
+
+      if (CurrentDescriptor  != Descriptor) {
+        Mva = (VOID *)(UINTN)(((UINTN)FirstLevelIdx + i) << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
+
+        // 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
+  )
+{
+  EFI_STATUS    Status;
+  UINT64        ChunkLength;
+  BOOLEAN       FlushTlbs;
+
+  if (BaseAddress > (UINT64)MAX_ADDRESS) {
+    return EFI_UNSUPPORTED;
+  }
+
+  Length = MIN (Length, (UINT64)MAX_ADDRESS - BaseAddress + 1);
+  if (Length == 0) {
+    return EFI_SUCCESS;
+  }
+
+  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,
+        "SetMemoryAttributes(): MMU section 0x%lx length 0x%lx to %lx\n",
+        BaseAddress, ChunkLength, Attributes));
+
+      Status = UpdateSectionEntries (BaseAddress, ChunkLength, Attributes);
+
+      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,
+        "SetMemoryAttributes(): MMU page 0x%lx length 0x%lx to %lx\n",
+        BaseAddress, ChunkLength, Attributes));
+
+      Status = UpdatePageEntries (BaseAddress, ChunkLength, Attributes,
+                 &FlushTlbs);
+    }
+
+    if (EFI_ERROR (Status)) {
+      break;
+    }
+
+    BaseAddress += ChunkLength;
+    Length -= ChunkLength;
+  }
+
+  if (FlushTlbs) {
+    ArmInvalidateTlb ();
+  }
+  return Status;
+}
+
+EFI_STATUS
+ArmSetMemoryRegionNoExec (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_XP);
+}
+
+EFI_STATUS
+ArmClearMemoryRegionNoExec (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  return ArmSetMemoryAttributes (BaseAddress, Length, __EFI_MEMORY_RWX);
+}
+
+EFI_STATUS
+ArmSetMemoryRegionReadOnly (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  return ArmSetMemoryAttributes (BaseAddress, Length, EFI_MEMORY_RO);
+}
+
+EFI_STATUS
+ArmClearMemoryRegionReadOnly (
+  IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
+  IN  UINT64                    Length
+  )
+{
+  return ArmSetMemoryAttributes (BaseAddress, Length, __EFI_MEMORY_RWX);
+}
diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
index 3dfe68ba48a6..2a7e7147958c 100644
--- a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
+++ b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
@@ -23,7 +23,9 @@ [Sources.AARCH64]
   AArch64/ArmMmuLibReplaceEntry.S
 
 [Sources.ARM]
+  Arm/ArmMmuLibConvert.c
   Arm/ArmMmuLibCore.c
+  Arm/ArmMmuLibUpdate.c
   Arm/ArmMmuLibV7Support.S   |GCC
   Arm/ArmMmuLibV7Support.asm |RVCT
 
-- 
2.17.1

[PATCH v2 4/9] ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries

[Ard Biesheuvel]

In the ARM version of ArmMmuLib, we are currently relying on set/way
invalidation to ensure that the caches are in a consistent state with
respect to main memory once we turn the MMU on. Even if set/way
operations were the appropriate method to achieve this, doing an
invalidate-all first and then populating the page table entries creates
a window where page table entries could be loaded speculatively into
the caches before we modify them, and shadow the new values that we
write there.

So let's get rid of the blanket clean/invalidate operations, and instead,
invalidate each section entry before and after it is updated (to address
all the little corner cases that the ARMv7 spec permits), and invalidate
sets of level 2 entries in blocks, using the generic invalidation routine
from CacheMaintenanceLib

On ARMv7, cache maintenance may be required also when the MMU is
enabled, in case the page table walker is not cache coherent. However,
the code being updated here is guaranteed to run only when the MMU is
still off, and so we can disregard the case when the MMU and caches
are on.

Since the MMU and D-cache are already off when we reach this point, we
can drop the MMU and D-cache disables as well. Maintenance of the I-cache
is unnecessary, since we are not modifying any code, and the installed
mapping is guaranteed to be 1:1. This means we can also leave it enabled
while the page table population code is running.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 55 +++++++++++++++-----
 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index aca7a37facac..7c7cad2c3d9d 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -178,11 +178,25 @@ PopulateLevel2PageTable (
 
   ASSERT (FirstPageOffset + Pages <= TRANSLATION_TABLE_PAGE_COUNT);
 
+  //
+  // Invalidate once to prevent page table updates to hit in the
+  // caches inadvertently.
+  //
+  InvalidateDataCacheRange ((UINT32 *)TranslationTable + FirstPageOffset,
+    RemainLength / TT_DESCRIPTOR_PAGE_SIZE * sizeof (*PageEntry));
+
   for (Index = 0; Index < Pages; Index++) {
     *PageEntry++     =  TT_DESCRIPTOR_PAGE_BASE_ADDRESS(PhysicalBase) | PageAttributes;
     PhysicalBase += TT_DESCRIPTOR_PAGE_SIZE;
   }
 
+  //
+  // Invalidate again to ensure that any line fetches that may have occurred
+  // [speculatively] since the previous invalidate are evicted again.
+  //
+  ArmDataMemoryBarrier ();
+  InvalidateDataCacheRange ((UINT32 *)TranslationTable + FirstPageOffset,
+    RemainLength / TT_DESCRIPTOR_PAGE_SIZE * sizeof (*PageEntry));
 }
 
 STATIC
@@ -253,11 +267,28 @@ FillTranslationTable (
   SectionEntry    = TRANSLATION_TABLE_ENTRY_FOR_VIRTUAL_ADDRESS(TranslationTable, MemoryRegion->VirtualBase);
 
   while (RemainLength != 0) {
+    //
+    // Ensure that the assignment of the page table entry will not hit
+    // in the cache. Whether this could occur is IMPLEMENTATION DEFINED
+    // and thus permitted by the ARMv7 architecture.
+    //
+    ArmInvalidateDataCacheEntryByMVA ((UINTN)SectionEntry);
+    ArmDataSynchronizationBarrier ();
+
     if (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE == 0 &&
         RemainLength >= TT_DESCRIPTOR_SECTION_SIZE) {
       // Case: Physical address aligned on the Section Size (1MB) && the length
       // is greater than the Section Size
-      *SectionEntry++ = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | Attributes;
+      *SectionEntry = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | Attributes;
+
+      //
+      // Issue a DMB to ensure that the page table entry update made it to
+      // memory before we issue the invalidate, otherwise, a subsequent
+      // speculative fetch could observe the old value.
+      //
+      ArmDataMemoryBarrier ();
+      ArmInvalidateDataCacheEntryByMVA ((UINTN)SectionEntry++);
+
       PhysicalBase += TT_DESCRIPTOR_SECTION_SIZE;
       RemainLength -= TT_DESCRIPTOR_SECTION_SIZE;
     } else {
@@ -267,9 +298,17 @@ FillTranslationTable (
       // Case: Physical address aligned on the Section Size (1MB) && the length
       //       does not fill a section
       // Case: Physical address NOT aligned on the Section Size (1MB)
-      PopulateLevel2PageTable (SectionEntry++, PhysicalBase, PageMapLength,
+      PopulateLevel2PageTable (SectionEntry, PhysicalBase, PageMapLength,
         MemoryRegion->Attributes);
 
+      //
+      // Issue a DMB to ensure that the page table entry update made it to
+      // memory before we issue the invalidate, otherwise, a subsequent
+      // speculative fetch could observe the old value.
+      //
+      ArmDataMemoryBarrier ();
+      ArmInvalidateDataCacheEntryByMVA ((UINTN)SectionEntry++);
+
       // If it is the last entry
       if (RemainLength < TT_DESCRIPTOR_SECTION_SIZE) {
         break;
@@ -349,18 +388,6 @@ ArmConfigureMmu (
     }
   }
 
-  ArmCleanInvalidateDataCache ();
-  ArmInvalidateInstructionCache ();
-
-  ArmDisableDataCache ();
-  ArmDisableInstructionCache();
-  // TLBs are also invalidated when calling ArmDisableMmu()
-  ArmDisableMmu ();
-
-  // Make sure nothing sneaked into the cache
-  ArmCleanInvalidateDataCache ();
-  ArmInvalidateInstructionCache ();
-
   ArmSetTTBR0 ((VOID *)(UINTN)(((UINTN)TranslationTable & ~TRANSLATION_TABLE_SECTION_ALIGNMENT_MASK) | (TTBRAttributes & 0x7F)));
 
   //
-- 
2.17.1

[PATCH v2 5/9] ArmPkg/ArmMmuLib AARCH64: cache-invalidate initial page table entries

[Ard Biesheuvel]

In the AARCH64 version of ArmMmuLib, we are currently relying on
set/way invalidation to ensure that the caches are in a consistent
state with respect to main memory once we turn the MMU on. Even if
set/way operations were the appropriate method to achieve this, doing
an invalidate-all first and then populating the page table entries
creates a window where page table entries could be loaded speculatively
into the caches before we modify them, and shadow the new values that
we write there.

So let's get rid of the blanket clean/invalidate operations, and
instead, update ArmUpdateTranslationTableEntry () to invalidate each
page table entry *after* it is written if the MMU is still disabled
at this point.

On ARMv8, it is guaranteed that memory accesses done by the page table
walker are cache coherent, and so we can ignore the case where the
MMU is on.

Since the MMU and D-cache are already off when we reach this point, we
can drop the MMU and D-cache disables as well. Maintenance of the I-cache
is unnecessary, since we are not modifying any code, and the installed
mapping is guaranteed to be 1:1. This means we can also leave it enabled
while the page table population code is running.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S    | 9 ++++++++-
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 9 ---------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
index 1adf960377a2..f744cd6738b9 100644
--- a/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
+++ b/ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S
@@ -13,6 +13,8 @@
 .set DAIF_RD_FIQ_BIT,   (1 << 6)
 .set DAIF_RD_IRQ_BIT,   (1 << 7)
 
+.set SCTLR_ELx_M_BIT_POS, (0)
+
 ASM_FUNC(ArmReadMidr)
   mrs     x0, midr_el1        // Read from Main ID Register (MIDR)
   ret
@@ -122,11 +124,16 @@ ASM_FUNC(ArmUpdateTranslationTableEntry)
    lsr     x1, x1, #12
    EL1_OR_EL2_OR_EL3(x0)
 1: tlbi    vaae1, x1             // TLB Invalidate VA , EL1
+   mrs     x2, sctlr_el1
    b       4f
 2: tlbi    vae2, x1              // TLB Invalidate VA , EL2
+   mrs     x2, sctlr_el2
    b       4f
 3: tlbi    vae3, x1              // TLB Invalidate VA , EL3
-4: dsb     nsh
+   mrs     x2, sctlr_el3
+4: tbnz    x2, SCTLR_ELx_M_BIT_POS, 5f
+   dc      ivac, x0              // invalidate in Dcache if MMU is still off
+5: dsb     nsh
    isb
    ret
 
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index e8f5c69e3136..204e33c75f95 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -699,15 +699,6 @@ ArmConfigureMmu (
 
   ZeroMem (TranslationTable, RootTableEntryCount * sizeof(UINT64));
 
-  // Disable MMU and caches. ArmDisableMmu() also invalidates the TLBs
-  ArmDisableMmu ();
-  ArmDisableDataCache ();
-  ArmDisableInstructionCache ();
-
-  // Make sure nothing sneaked into the cache
-  ArmCleanInvalidateDataCache ();
-  ArmInvalidateInstructionCache ();
-
   TranslationTableAttribute = TT_ATTR_INDX_INVALID;
   while (MemoryTable->Length != 0) {
 
-- 
2.17.1

[PATCH v2 6/9] ArmPkg/ArmLib: move set/way helper functions into private header

[Ard Biesheuvel]

The clean/invalidate helper functions that operate on a single cache
line identified by set, way and level in a special, architected format
are only used by the implementations of the clean/invalidate routines
that operate on the entire cache hierarchy, as exposed by ArmLib.

The latter routines will be deprecated soon, so move the helpers out
of ArmLib.h and into a private header so they are safe from abuse.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Include/Library/ArmLib.h            | 18 ------------------
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h | 18 ++++++++++++++++++
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h       | 18 ++++++++++++++++++
 3 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index e76a46d5f4ce..5a27b7c2fc27 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -211,24 +211,6 @@ ArmCleanInvalidateDataCacheEntryByMVA (
   IN  UINTN   Address
   );
 
-VOID
-EFIAPI
-ArmInvalidateDataCacheEntryBySetWay (
-  IN  UINTN  SetWayFormat
-  );
-
-VOID
-EFIAPI
-ArmCleanDataCacheEntryBySetWay (
-  IN  UINTN  SetWayFormat
-  );
-
-VOID
-EFIAPI
-ArmCleanInvalidateDataCacheEntryBySetWay (
-  IN  UINTN   SetWayFormat
-  );
-
 VOID
 EFIAPI
 ArmEnableDataCache (
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
index ab9bcf553c4d..b2c8a8ea0b84 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h
@@ -17,5 +17,23 @@ AArch64AllDataCachesOperation (
   IN  AARCH64_CACHE_OPERATION  DataCacheOperation
   );
 
+VOID
+EFIAPI
+ArmInvalidateDataCacheEntryBySetWay (
+  IN  UINTN  SetWayFormat
+  );
+
+VOID
+EFIAPI
+ArmCleanDataCacheEntryBySetWay (
+  IN  UINTN  SetWayFormat
+  );
+
+VOID
+EFIAPI
+ArmCleanInvalidateDataCacheEntryBySetWay (
+  IN  UINTN   SetWayFormat
+  );
+
 #endif // __AARCH64_LIB_H__
 
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h
index c52fb9a1b484..93183e67230e 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h
@@ -30,5 +30,23 @@ ArmV7AllDataCachesOperation (
   IN  ARM_V7_CACHE_OPERATION  DataCacheOperation
   );
 
+VOID
+EFIAPI
+ArmInvalidateDataCacheEntryBySetWay (
+  IN  UINTN  SetWayFormat
+  );
+
+VOID
+EFIAPI
+ArmCleanDataCacheEntryBySetWay (
+  IN  UINTN  SetWayFormat
+  );
+
+VOID
+EFIAPI
+ArmCleanInvalidateDataCacheEntryBySetWay (
+  IN  UINTN   SetWayFormat
+  );
+
 #endif // __ARM_V7_LIB_H__
 
-- 
2.17.1

[PATCH v2 7/9] ArmPkg/ArmLib: clean up library includes

[Ard Biesheuvel]

Suspiciously, ArmLib's INF does not contain a [LibraryClasses]
section at all, but it turns out that all the library includes
it contains (except for ArmLib.h itself) are actually bogus so
let's just drop all of them. While at it, replace <Uefi.h> with
the more accurate <Base.h> for a BASE type module, and put the
includes in a consistent order.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c |  9 +++++----
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c       | 10 ++++++----
 ArmPkg/Library/ArmLib/ArmLib.c             |  2 --
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
index 0ed8dae9a4b0..924bf48020e0 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
@@ -7,11 +7,12 @@
 
 **/
 
-#include <Uefi.h>
-#include <Chipset/AArch64.h>
+#include <Base.h>
+
 #include <Library/ArmLib.h>
-#include <Library/BaseLib.h>
-#include <Library/IoLib.h>
+
+#include <Chipset/AArch64.h>
+
 #include "AArch64Lib.h"
 #include "ArmLibPrivate.h"
 
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
index 38516d4f1b87..5d93aa6e0b8c 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
@@ -6,11 +6,13 @@
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
-#include <Uefi.h>
-#include <Chipset/ArmV7.h>
+
+#include <Base.h>
+
 #include <Library/ArmLib.h>
-#include <Library/BaseLib.h>
-#include <Library/IoLib.h>
+
+#include <Chipset/ArmV7.h>
+
 #include "ArmV7Lib.h"
 #include "ArmLibPrivate.h"
 
diff --git a/ArmPkg/Library/ArmLib/ArmLib.c b/ArmPkg/Library/ArmLib/ArmLib.c
index c682c3ab6339..3905d02c5e7e 100644
--- a/ArmPkg/Library/ArmLib/ArmLib.c
+++ b/ArmPkg/Library/ArmLib/ArmLib.c
@@ -10,8 +10,6 @@
 #include <Base.h>
 
 #include <Library/ArmLib.h>
-#include <Library/DebugLib.h>
-#include <Library/PcdLib.h>
 
 #include "ArmLibPrivate.h"
 
-- 
2.17.1

[PATCH v2 8/9] ArmPkg/ArmLib: remove bogus protocol declaration

[Ard Biesheuvel]

ArmLib is a BASE type library, which should not depend or
even be aware on DXE type protocols. So drop the reference
to gEfiCpuArchProtocolGuid.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmLib/ArmBaseLib.inf | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/ArmPkg/Library/ArmLib/ArmBaseLib.inf b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
index 5e70990872f2..106a09f821e1 100644
--- a/ArmPkg/Library/ArmLib/ArmBaseLib.inf
+++ b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
@@ -48,8 +48,5 @@ [Packages]
   ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
 
-[Protocols]
-  gEfiCpuArchProtocolGuid
-
 [FeaturePcd.ARM]
   gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride
-- 
2.17.1

[PATCH v2 9/9] ArmPkg/ArmLib: ASSERT on set/way cache ops being used with MMU on

[Ard Biesheuvel]

On ARMv7 and up, doing cache maintenance by set/way is only
permitted in the context of on/offlining a core, and any other
uses should be avoided. Add ASSERT()s in the right place to
ensure that any uses with the MMU enabled are caught in DEBUG
builds.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c | 7 +++++++
 ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c       | 7 +++++++
 ArmPkg/Library/ArmLib/ArmBaseLib.inf       | 3 +++
 3 files changed, 17 insertions(+)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
index 924bf48020e0..3fbd591192e2 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c
@@ -10,6 +10,7 @@
 #include <Base.h>
 
 #include <Library/ArmLib.h>
+#include <Library/DebugLib.h>
 
 #include <Chipset/AArch64.h>
 
@@ -41,6 +42,8 @@ ArmInvalidateDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   AArch64DataCacheOperation (ArmInvalidateDataCacheEntryBySetWay);
 }
@@ -51,6 +54,8 @@ ArmCleanInvalidateDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   AArch64DataCacheOperation (ArmCleanInvalidateDataCacheEntryBySetWay);
 }
@@ -61,6 +66,8 @@ ArmCleanDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   AArch64DataCacheOperation (ArmCleanDataCacheEntryBySetWay);
 }
diff --git a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
index 5d93aa6e0b8c..2c4a23e1a1b2 100644
--- a/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
+++ b/ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c
@@ -10,6 +10,7 @@
 #include <Base.h>
 
 #include <Library/ArmLib.h>
+#include <Library/DebugLib.h>
 
 #include <Chipset/ArmV7.h>
 
@@ -41,6 +42,8 @@ ArmInvalidateDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   ArmV7DataCacheOperation (ArmInvalidateDataCacheEntryBySetWay);
 }
@@ -51,6 +54,8 @@ ArmCleanInvalidateDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   ArmV7DataCacheOperation (ArmCleanInvalidateDataCacheEntryBySetWay);
 }
@@ -61,6 +66,8 @@ ArmCleanDataCache (
   VOID
   )
 {
+  ASSERT (!ArmMmuEnabled ());
+
   ArmDataSynchronizationBarrier ();
   ArmV7DataCacheOperation (ArmCleanDataCacheEntryBySetWay);
 }
diff --git a/ArmPkg/Library/ArmLib/ArmBaseLib.inf b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
index 106a09f821e1..f61c71b673d1 100644
--- a/ArmPkg/Library/ArmLib/ArmBaseLib.inf
+++ b/ArmPkg/Library/ArmLib/ArmBaseLib.inf
@@ -44,6 +44,9 @@ [Sources.AARCH64]
   AArch64/AArch64Support.S
   AArch64/AArch64ArchTimerSupport.S
 
+[LibraryClasses]
+  DebugLib
+
 [Packages]
   ArmPkg/ArmPkg.dec
   MdePkg/MdePkg.dec
-- 
2.17.1

Re: [PATCH v2 1/9] ArmPlatformPkg/PrePi: replace set/way cache ops with by-VA ones

[Leif Lindholm]

On Wed, Mar 04, 2020 at 19:12:38 +0100, Ard Biesheuvel wrote:
> Cache maintenance operations by set/way are only intended to be used
> in the context of on/offlining a core, while it has been taken out of
> the coherency domain. Any use intended to ensure that the contents of
> the cache have made it to main memory is unreliable, since cacheline
> migration and non-architected system caches may cause these contents
> to linger elsewhere, without being visible in main memory once the
> MMU and caches are disabled.
> 
> In KVM on Linux, there are horrid hacks in place to ensure that such
> set/way operations are trapped, and replaced with a single by-VA
> clean/invalidate of the entire guest VA space once the MMU state
> changes, which can be costly, and is unnecessary if we manage the
> caches a bit more carefully, and perform maintenance by virtual
> address only.
> 
> So let's get rid of the call to ArmInvalidateDataCache () in the
> PrePeiCore startup code, and instead, invalidate the UEFI memory
> region by virtual address, which is the only memory region we will
> be touching with the caches and MMU both disabled and enabled.
> (This will lead to data corruption if data written with the MMU off
> is shadowed by clean, stale cachelines that stick around when the
> MMU is enabled again.)
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Tested-By: Pete Batard <pete@akeo.ie>
> ---
>  ArmPlatformPkg/PrePi/PeiMPCore.inf  | 1 +
>  ArmPlatformPkg/PrePi/PeiUniCore.inf | 1 +
>  ArmPlatformPkg/PrePi/PrePi.c        | 8 +++++---
>  3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPlatformPkg/PrePi/PeiMPCore.inf b/ArmPlatformPkg/PrePi/PeiMPCore.inf
> index 9c5da0d42a7b..053f9fd9e616 100644
> --- a/ArmPlatformPkg/PrePi/PeiMPCore.inf
> +++ b/ArmPlatformPkg/PrePi/PeiMPCore.inf
> @@ -37,6 +37,7 @@ [Packages]
>  
>  [LibraryClasses]
>    BaseLib
> +  CacheMaintenanceLib
>    DebugLib
>    DebugAgentLib
>    ArmLib
> diff --git a/ArmPlatformPkg/PrePi/PeiUniCore.inf b/ArmPlatformPkg/PrePi/PeiUniCore.inf
> index ee9b05b25337..78d218ae09ca 100644
> --- a/ArmPlatformPkg/PrePi/PeiUniCore.inf
> +++ b/ArmPlatformPkg/PrePi/PeiUniCore.inf
> @@ -37,6 +37,7 @@ [Packages]
>  
>  [LibraryClasses]
>    BaseLib
> +  CacheMaintenanceLib
>    DebugLib
>    DebugAgentLib
>    ArmLib
> diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
> index 2bb144958139..254fb331733e 100644
> --- a/ArmPlatformPkg/PrePi/PrePi.c
> +++ b/ArmPlatformPkg/PrePi/PrePi.c
> @@ -8,6 +8,7 @@
>  
>  #include <PiPei.h>
>  
> +#include <Library/CacheMaintenanceLib.h>
>  #include <Library/DebugAgentLib.h>
>  #include <Library/PrePiLib.h>
>  #include <Library/PrintLib.h>
> @@ -178,8 +179,6 @@ CEntryPoint (
>  
>    // Data Cache enabled on Primary core when MMU is enabled.
>    ArmDisableDataCache ();
> -  // Invalidate Data cache
> -  ArmInvalidateDataCache ();
>    // Invalidate instruction cache
>    ArmInvalidateInstructionCache ();
>    // Enable Instruction Caches on all cores.
> @@ -200,6 +199,10 @@ CEntryPoint (
>  
>    // If not primary Jump to Secondary Main
>    if (ArmPlatformIsPrimaryCore (MpId)) {
> +
> +    InvalidateDataCacheRange ((VOID *)UefiMemoryBase,
> +                              FixedPcdGet32(PcdSystemMemoryUefiRegionSize));

Space before that (.
With that:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> +
>      // Goto primary Main.
>      PrimaryMain (UefiMemoryBase, StacksBase, StartTimeStamp);
>    } else {
> @@ -209,4 +212,3 @@ CEntryPoint (
>    // DXE Core should always load and never return
>    ASSERT (FALSE);
>  }
> -
> -- 
> 2.17.1
> 

Re: [PATCH v2 0/9] ArmPkg: eradicate and deprecate by set/way cache ops

[Leif Lindholm]

On Wed, Mar 04, 2020 at 19:12:37 +0100, Ard Biesheuvel wrote:
> This is a combination of v1 'ArmPkg: eradicate and deprecate by set/way cache
> ops' and v1 'ArmPkg/ArmLib: ASSERT() on misuse of set/way ops'
> 
> As it turns out, there were still some instances of set/way ops left in
> the core code, in ArmMmuLib to be precise.
> 
> This series fixes ArmMmuLib to perform the appropriate cache invalidation
> when populating page tables with the MMU and caches off, allowing us to
> get rid of the cache clean/disable/enable sequences which are incorrect
> and pointless at the same time.
> 
> I have incorporated some ArmLib changes that I posted separately before,
> with the end result being that all uses pf set/way ops are gone from the
> EDK2 core code, and the routines themselves will now ASSERT() when used
> for anything other than managing the caches while the MMU is still off.
> (Note that BeagleBoard in edk2-platforms still relies on this)
> 
> Changes since v1[s]:
> - don't deprecate the set/way ops but make the usable before MMU is enabled
>   only
> - use a more elaborate sequence for invalidating the page table entries on
>   32-bit ARM, to ensure we are compliant with the nooks and crannies of
>   version 7 of the architecture.
> - incorporate patch #1, which was still pending on the list, and is related
>   (its PrePeiCore sibling was already reviewed and merged)
> 
> Ard Biesheuvel (9):
>   ArmPlatformPkg/PrePi: replace set/way cache ops with by-VA ones
>   ArmPkg/ArmMmuLib ARM: remove dummy constructor
>   ArmPkg/ArmMmuLib ARM: split ArmMmuLibCore.c into core and update code
>   ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries
>   ArmPkg/ArmMmuLib AARCH64: cache-invalidate initial page table entries
>   ArmPkg/ArmLib: move set/way helper functions into private header
>   ArmPkg/ArmLib: clean up library includes
>   ArmPkg/ArmLib: remove bogus protocol declaration
>   ArmPkg/ArmLib: ASSERT on set/way cache ops being used with MMU on

Apart from the minor nit pointed out (which you can fix before
committing) - for the series:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

Thanks!

>  ArmPkg/Include/Library/ArmLib.h               |  18 -
>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c    |  16 +-
>  ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h    |  18 +
>  ArmPkg/Library/ArmLib/AArch64/ArmLibSupport.S |   9 +-
>  ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c          |  17 +-
>  ArmPkg/Library/ArmLib/Arm/ArmV7Lib.h          |  18 +
>  ArmPkg/Library/ArmLib/ArmBaseLib.inf          |   6 +-
>  ArmPkg/Library/ArmLib/ArmLib.c                |   2 -
>  .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |   9 -
>  .../Library/ArmMmuLib/Arm/ArmMmuLibConvert.c  |  32 ++
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c  | 498 ++----------------
>  .../Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c   | 435 +++++++++++++++
>  ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf    |   4 +
>  ArmPlatformPkg/PrePi/PeiMPCore.inf            |   1 +
>  ArmPlatformPkg/PrePi/PeiUniCore.inf           |   1 +
>  ArmPlatformPkg/PrePi/PrePi.c                  |   8 +-
>  16 files changed, 591 insertions(+), 501 deletions(-)
>  create mode 100644 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibConvert.c
>  create mode 100644 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
> 
> -- 
> 2.17.1
> 

Re: [PATCH v2 0/9] ArmPkg: eradicate and deprecate by set/way cache ops

[Ard Biesheuvel]

On Thu, 5 Mar 2020 at 17:29, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Wed, Mar 04, 2020 at 19:12:37 +0100, Ard Biesheuvel wrote:
> > This is a combination of v1 'ArmPkg: eradicate and deprecate by set/way cache
> > ops' and v1 'ArmPkg/ArmLib: ASSERT() on misuse of set/way ops'
> >
> > As it turns out, there were still some instances of set/way ops left in
> > the core code, in ArmMmuLib to be precise.
> >
> > This series fixes ArmMmuLib to perform the appropriate cache invalidation
> > when populating page tables with the MMU and caches off, allowing us to
> > get rid of the cache clean/disable/enable sequences which are incorrect
> > and pointless at the same time.
> >
> > I have incorporated some ArmLib changes that I posted separately before,
> > with the end result being that all uses pf set/way ops are gone from the
> > EDK2 core code, and the routines themselves will now ASSERT() when used
> > for anything other than managing the caches while the MMU is still off.
> > (Note that BeagleBoard in edk2-platforms still relies on this)
> >
> > Changes since v1[s]:
> > - don't deprecate the set/way ops but make the usable before MMU is enabled
> >   only
> > - use a more elaborate sequence for invalidating the page table entries on
> >   32-bit ARM, to ensure we are compliant with the nooks and crannies of
> >   version 7 of the architecture.
> > - incorporate patch #1, which was still pending on the list, and is related
> >   (its PrePeiCore sibling was already reviewed and merged)
> >
> > Ard Biesheuvel (9):
> >   ArmPlatformPkg/PrePi: replace set/way cache ops with by-VA ones
> >   ArmPkg/ArmMmuLib ARM: remove dummy constructor
> >   ArmPkg/ArmMmuLib ARM: split ArmMmuLibCore.c into core and update code
> >   ArmPkg/ArmMmuLib ARM: cache-invalidate initial page table entries
> >   ArmPkg/ArmMmuLib AARCH64: cache-invalidate initial page table entries
> >   ArmPkg/ArmLib: move set/way helper functions into private header
> >   ArmPkg/ArmLib: clean up library includes
> >   ArmPkg/ArmLib: remove bogus protocol declaration
> >   ArmPkg/ArmLib: ASSERT on set/way cache ops being used with MMU on
>
> Apart from the minor nit pointed out (which you can fix before
> committing) - for the series:
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>

Thanks. These are merged now, along with the followup series, and the
only vaguely related cleanups for ARM.

I did spot one other issue on AArch64 though, so I'll send out another
followup patch to address that.