[PATCH v2 0/4] ArmPkg/ArmMmuLib: fixes and cleanups

[PATCH v2 0/4] ArmPkg/ArmMmuLib: fixes and cleanups

[Ard Biesheuvel]

Patch #1 replaces the needlessly complex routine to determine the number of
translation levels and the size and alignment of the root table based on the
T0SZ setting.

Patch #2 fixes the fix for allocating page aligned pages, which is a truism
and as such does not require any special care to begin with.

Patch #3 switches to a suitably aligned pool allocation for the root table
if its size is much smaller than a page.

Patch #4 makes the size of the VA space in the VM registers equal to the
GCD memory map size.

Ard Biesheuvel (4):
  ArmPkg/ArmMmuLib: deobfuscate GetRootTranslationTableInfo ()
  ArmPkg/ArmMmuLib: remove bogus alignment of page allocations
  ArmPkg/ArmMmuLib: use a pool allocation for the root table
  ArmPkg/ArmMmuLib: base page table VA size on GCD memory map size

 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 82 +++++++-------------
 ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf       |  4 +
 ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf        |  4 +
 3 files changed, 38 insertions(+), 52 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] ArmPkg/ArmMmuLib: deobfuscate GetRootTranslationTableInfo ()

[Ard Biesheuvel]

The relations between T0SZ, the number of translation levels and the
size/alignment of the root table can be expressed in simple arithmetic
expressions, so get rid of the lookup table.

Note that this disregards the fact that the maximum value of T0SZ is
39 not 42 (as one would expect for the smallest VA size using 2 levels)
but since this corresponds to a VA size of 32 MB and 4 MB, respectively,
neither of which are sufficient to run UEFI, we can safely ignore the
distinction.

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

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 6e05e6085011..84a689af7c8a 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -121,20 +121,8 @@ GcdAttributeToArmAttribute (
   }
 }
 
-// Describe the T0SZ values for each translation table level
-typedef struct {
-  UINTN     MinT0SZ;
-  UINTN     MaxT0SZ;
-  UINTN     LargestT0SZ; // Generally (MaxT0SZ == LargestT0SZ) but at the Level3 Table
-                         // the MaxT0SZ is not at the boundary of the table
-} T0SZ_DESCRIPTION_PER_LEVEL;
-
-// Map table for the corresponding Level of Table
-STATIC CONST T0SZ_DESCRIPTION_PER_LEVEL T0SZPerTableLevel[] = {
-    { 16, 24, 24 }, // Table Level 0
-    { 25, 33, 33 }, // Table Level 1
-    { 34, 39, 42 }  // Table Level 2
-};
+#define MIN_T0SZ        16
+#define BITS_PER_LEVEL  9
 
 VOID
 GetRootTranslationTableInfo (
@@ -143,28 +131,13 @@ GetRootTranslationTableInfo (
   OUT UINTN   *TableEntryCount
   )
 {
-  UINTN Index;
-
-  // Identify the level of the root table from the given T0SZ
-  for (Index = 0; Index < sizeof (T0SZPerTableLevel) / sizeof (T0SZ_DESCRIPTION_PER_LEVEL); Index++) {
-    if (T0SZ <= T0SZPerTableLevel[Index].MaxT0SZ) {
-      break;
-    }
-  }
-
-  // If we have not found the corresponding maximum T0SZ then we use the last one
-  if (Index == sizeof (T0SZPerTableLevel) / sizeof (T0SZ_DESCRIPTION_PER_LEVEL)) {
-    Index--;
-  }
-
   // Get the level of the root table
   if (TableLevel) {
-    *TableLevel = Index;
+    *TableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
   }
 
-  // The Size of the Table is 2^(T0SZ-LargestT0SZ)
   if (TableEntryCount) {
-    *TableEntryCount = 1 << (T0SZPerTableLevel[Index].LargestT0SZ - T0SZ + 1);
+    *TableEntryCount = 1UL << (BITS_PER_LEVEL - (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL);
   }
 }
 
-- 
2.7.4

[PATCH v2 2/4] ArmPkg/ArmMmuLib: remove bogus alignment of page allocations

[Ard Biesheuvel]

In commit 7d189f99d81c ("ArmPkg/Mmu: Fix bug of aligning new allocated
page table"), we fixed a flaw in the logic regarding alignment of newly
allocated translation table pages. However, we all failed to spot that
aligning page based allocations to page size is rather pointless to
begin with, so simply allocate a single page each time we add new pages
to the translation tables.

Also, drop the unnecessary cast.

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

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 84a689af7c8a..1ff584ec9eec 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -298,7 +298,7 @@ GetBlockEntryListFromAddress (
         }
 
         // Create a new translation table
-        TranslationTable = (UINT64*)AllocateAlignedPages (EFI_SIZE_TO_PAGES(TT_ENTRY_COUNT * sizeof(UINT64)), TT_ALIGNMENT_DESCRIPTION_TABLE);
+        TranslationTable = AllocatePages (1);
         if (TranslationTable == NULL) {
           return NULL;
         }
@@ -321,7 +321,7 @@ GetBlockEntryListFromAddress (
         //
 
         // Create a new translation table
-        TranslationTable = (UINT64*)AllocateAlignedPages (EFI_SIZE_TO_PAGES(TT_ENTRY_COUNT * sizeof(UINT64)), TT_ALIGNMENT_DESCRIPTION_TABLE);
+        TranslationTable = AllocatePages (1);
         if (TranslationTable == NULL) {
           return NULL;
         }
@@ -553,7 +553,6 @@ ArmConfigureMmu (
   )
 {
   VOID*                         TranslationTable;
-  UINTN                         TranslationTablePageCount;
   UINT32                        TranslationTableAttribute;
   ARM_MEMORY_REGION_DESCRIPTOR *MemoryTableEntry;
   UINT64                        MaxAddress;
@@ -640,8 +639,7 @@ ArmConfigureMmu (
   ArmSetTCR (TCR);
 
   // Allocate pages for translation table
-  TranslationTablePageCount = EFI_SIZE_TO_PAGES(RootTableEntryCount * sizeof(UINT64));
-  TranslationTable = (UINT64*)AllocateAlignedPages (TranslationTablePageCount, TT_ALIGNMENT_DESCRIPTION_TABLE);
+  TranslationTable = AllocatePages (1);
   if (TranslationTable == NULL) {
     return RETURN_OUT_OF_RESOURCES;
   }
@@ -718,7 +716,7 @@ ArmConfigureMmu (
   return RETURN_SUCCESS;
 
 FREE_TRANSLATION_TABLE:
-  FreePages (TranslationTable, TranslationTablePageCount);
+  FreePages (TranslationTable, 1);
   return Status;
 }
 
-- 
2.7.4

[PATCH v2 3/4] ArmPkg/ArmMmuLib: use a pool allocation for the root table

[Ard Biesheuvel]

Currently, we allocate a full page for the root translation table, even
if the configured translation only requires two entries (16 bytes) for
the root level, which happens to be the case for a 40 bit VA. Likewise,
for a 36-bit VA space, the root table only needs 16 entries of 8 bytes
each, adding up to 128 bytes.

So switch to a pool allocation for the root table if we can, but take into
account that the architecture requires it to be naturally aligned to its
size, i.e., a 64 byte table requires 64 byte alignment, whereas pool
allocations in general are only guaranteed to be aligned to 8 bytes.

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

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 1ff584ec9eec..57e789f68b3b 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -553,12 +553,14 @@ ArmConfigureMmu (
   )
 {
   VOID*                         TranslationTable;
+  VOID*                         TranslationTableBuffer;
   UINT32                        TranslationTableAttribute;
   ARM_MEMORY_REGION_DESCRIPTOR *MemoryTableEntry;
   UINT64                        MaxAddress;
   UINT64                        TopAddress;
   UINTN                         T0SZ;
   UINTN                         RootTableEntryCount;
+  UINTN                         RootTableEntrySize;
   UINT64                        TCR;
   RETURN_STATUS                 Status;
 
@@ -638,8 +640,19 @@ ArmConfigureMmu (
   // Set TCR
   ArmSetTCR (TCR);
 
-  // Allocate pages for translation table
-  TranslationTable = AllocatePages (1);
+  // Allocate pages for translation table. Pool allocations are 8 byte aligned,
+  // but we may require a higher alignment based on the size of the root table.
+  RootTableEntrySize = RootTableEntryCount * sizeof(UINT64);
+  if (RootTableEntrySize < EFI_PAGE_SIZE / 2) {
+    TranslationTableBuffer = AllocatePool (2 * RootTableEntrySize - 8);
+    //
+    // Naturally align the root table. Preserves possible NULL value
+    //
+    TranslationTable = (VOID *)((UINTN)(TranslationTableBuffer - 1) | (RootTableEntrySize - 1)) + 1;
+  } else {
+    TranslationTable = AllocatePages (1);
+    TranslationTableBuffer = NULL;
+  }
   if (TranslationTable == NULL) {
     return RETURN_OUT_OF_RESOURCES;
   }
@@ -653,10 +666,10 @@ ArmConfigureMmu (
   }
 
   if (TranslationTableSize != NULL) {
-    *TranslationTableSize = RootTableEntryCount * sizeof(UINT64);
+    *TranslationTableSize = RootTableEntrySize;
   }
 
-  ZeroMem (TranslationTable, RootTableEntryCount * sizeof(UINT64));
+  ZeroMem (TranslationTable, RootTableEntrySize);
 
   // Disable MMU and caches. ArmDisableMmu() also invalidates the TLBs
   ArmDisableMmu ();
@@ -716,7 +729,11 @@ ArmConfigureMmu (
   return RETURN_SUCCESS;
 
 FREE_TRANSLATION_TABLE:
-  FreePages (TranslationTable, 1);
+  if (TranslationTableBuffer != NULL) {
+    FreePool (TranslationTableBuffer);
+  } else {
+    FreePages (TranslationTable, 1);
+  }
   return Status;
 }
 
-- 
2.7.4

[PATCH v2 4/4] ArmPkg/ArmMmuLib: base page table VA size on GCD memory map size

[Ard Biesheuvel]

As reported by Eugene, the practice of sizing the address space in the
virtual memory system based on the maximum address in the table passed
to ArmConfigureMmu() is problematic, since it fails to take into account
the fact that the GCD memory space may be extended at a later time, both
for memory and for MMIO. So instead, choose the VA size identical to the
GCD memory map size, which is based on PcdPrePiCpuMemorySize on ARM
systems.

Reported-by: Eugene Cohen <eugene@hp.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 14 ++------------
 ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf       |  4 ++++
 ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf        |  4 ++++
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 57e789f68b3b..1fb3bbec6347 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -555,9 +555,7 @@ ArmConfigureMmu (
   VOID*                         TranslationTable;
   VOID*                         TranslationTableBuffer;
   UINT32                        TranslationTableAttribute;
-  ARM_MEMORY_REGION_DESCRIPTOR *MemoryTableEntry;
   UINT64                        MaxAddress;
-  UINT64                        TopAddress;
   UINTN                         T0SZ;
   UINTN                         RootTableEntryCount;
   UINTN                         RootTableEntrySize;
@@ -569,16 +567,8 @@ ArmConfigureMmu (
     return RETURN_INVALID_PARAMETER;
   }
 
-  // Identify the highest address of the memory table
-  MaxAddress = MemoryTable->PhysicalBase + MemoryTable->Length - 1;
-  MemoryTableEntry = MemoryTable;
-  while (MemoryTableEntry->Length != 0) {
-    TopAddress = MemoryTableEntry->PhysicalBase + MemoryTableEntry->Length - 1;
-    if (TopAddress > MaxAddress) {
-      MaxAddress = TopAddress;
-    }
-    MemoryTableEntry++;
-  }
+  // Cover the entire GCD memory space
+  MaxAddress = (1UL << PcdGet8 (PcdPrePiCpuMemorySize)) - 1;
 
   // Lookup the Table Level to get the information
   LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
index 1533c2944e8e..b9f264de8d26 100644
--- a/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
+++ b/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
@@ -32,6 +32,7 @@ [Sources.ARM]
 
 [Packages]
   ArmPkg/ArmPkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
 
 [LibraryClasses]
@@ -39,5 +40,8 @@ [LibraryClasses]
   CacheMaintenanceLib
   MemoryAllocationLib
 
+[Pcd.AARCH64]
+  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
+
 [Pcd.ARM]
   gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride
diff --git a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
index 14ebf8de673d..ecf13f790734 100644
--- a/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
+++ b/ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf
@@ -28,9 +28,13 @@ [Sources.AARCH64]
 
 [Packages]
   ArmPkg/ArmPkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
 
 [LibraryClasses]
   ArmLib
   CacheMaintenanceLib
   MemoryAllocationLib
+
+[Pcd.AARCH64]
+  gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize
-- 
2.7.4

Re: [PATCH v2 0/4] ArmPkg/ArmMmuLib: fixes and cleanups

[Leif Lindholm]

On Fri, Sep 09, 2016 at 11:48:36AM +0100, Ard Biesheuvel wrote:
> Patch #1 replaces the needlessly complex routine to determine the number of
> translation levels and the size and alignment of the root table based on the
> T0SZ setting.
> 
> Patch #2 fixes the fix for allocating page aligned pages, which is a truism
> and as such does not require any special care to begin with.
> 
> Patch #3 switches to a suitably aligned pool allocation for the root table
> if its size is much smaller than a page.
> 
> Patch #4 makes the size of the VA space in the VM registers equal to the
> GCD memory map size.

Right, took a while to get my head around some of your cleverer
arithmetic, but:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> Ard Biesheuvel (4):
>   ArmPkg/ArmMmuLib: deobfuscate GetRootTranslationTableInfo ()
>   ArmPkg/ArmMmuLib: remove bogus alignment of page allocations
>   ArmPkg/ArmMmuLib: use a pool allocation for the root table
>   ArmPkg/ArmMmuLib: base page table VA size on GCD memory map size
> 
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 82 +++++++-------------
>  ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf       |  4 +
>  ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf        |  4 +
>  3 files changed, 38 insertions(+), 52 deletions(-)
> 
> -- 
> 2.7.4
>