[PATCH 0/5] ArmPkg: cosmetic cleanups for ArmMmuLib

[PATCH 0/5] ArmPkg: cosmetic cleanups for ArmMmuLib

[Ard Biesheuvel]

Now that the rewritten ArmMmuLib for AARCH64 seems to be functioning as
desired, we can apply some more polish and get rid of a couple of warts
that were left over from the original version.

Patches #1 and #2 move some code out of ArmMmuLib and into CpuDxe which
only exists for the benefit of the latter.

Patches #3 and #4 get rid of a couple of awkward helper functions that
can be replaced by simply arithmetic expressions.

Patch #5 drops a #define that is no longer used.

Ard Biesheuvel (5):
  ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()
  ArmPkg/CpuDxe: move PageAttributeToGcdAttribute() out of ArmMmuLib
  ArmPkg/ArmMmuLib: drop pointless LookupAddresstoRootTable() routine
  ArmPkg/ArmMmuLib: get rid of GetRootTranslationTableInfo()
  ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c           |  61 +++++++++
 ArmPkg/Drivers/CpuDxe/CpuDxe.h                |   7 --
 ArmPkg/Include/Chipset/AArch64.h              |   5 -
 .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 117 +++---------------
 4 files changed, 81 insertions(+), 109 deletions(-)

-- 
2.17.1

[PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

[Ard Biesheuvel]

Before getting rid of GetRootTranslationTableInfo() and the related
LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
version of the former to CpuDxe, which will be its only remaining
user. While at it, simplify it a bit, since in the CpuDxe cases,
both OUT arguments are always provided.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
 ArmPkg/Drivers/CpuDxe/CpuDxe.h      |  7 -------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 3b6c5e733709..24eb1c4221e3 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
 
+#define MIN_T0SZ        16
+#define BITS_PER_LEVEL  9
+
+STATIC
+VOID
+GetRootTranslationTableInfo (
+  IN  UINTN     T0SZ,
+  OUT UINTN     *RootTableLevel,
+  OUT UINTN     *RootTableEntryCount
+  )
+{
+  *RootTableLevel       = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
+  *RootTableEntryCount  = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
+}
+
 STATIC
 UINT64
 GetFirstPageAttribute (
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index b627c3c50ff8..3fe5c24d5e5b 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -134,13 +134,6 @@ GetMemoryRegion (
   OUT    UINTN                   *RegionAttributes
   );
 
-VOID
-GetRootTranslationTableInfo (
-  IN  UINTN    T0SZ,
-  OUT UINTN   *TableLevel,
-  OUT UINTN   *TableEntryCount
-  );
-
 EFI_STATUS
 SetGcdMemorySpaceAttributes (
   IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR    *MemorySpaceMap,
-- 
2.17.1

[PATCH 2/5] ArmPkg/CpuDxe: move PageAttributeToGcdAttribute() out of ArmMmuLib

[Ard Biesheuvel]

The routine PageAttributeToGcdAttribute() is exported by ArmMmuLib
but only ever used in the implementation of CpuDxe. So let's move
the function there and make it STATIC.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              | 46 ++++++++++++++++++++
 ArmPkg/Include/Chipset/AArch64.h                 |  5 ---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 45 -------------------
 3 files changed, 46 insertions(+), 50 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 24eb1c4221e3..29fa08f9e07c 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -30,6 +30,52 @@ GetRootTranslationTableInfo (
   *RootTableEntryCount  = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
 }
 
+STATIC
+UINT64
+PageAttributeToGcdAttribute (
+  IN UINT64 PageAttributes
+  )
+{
+  UINT64  GcdAttributes;
+
+  switch (PageAttributes & TT_ATTR_INDX_MASK) {
+  case TT_ATTR_INDX_DEVICE_MEMORY:
+    GcdAttributes = EFI_MEMORY_UC;
+    break;
+  case TT_ATTR_INDX_MEMORY_NON_CACHEABLE:
+    GcdAttributes = EFI_MEMORY_WC;
+    break;
+  case TT_ATTR_INDX_MEMORY_WRITE_THROUGH:
+    GcdAttributes = EFI_MEMORY_WT;
+    break;
+  case TT_ATTR_INDX_MEMORY_WRITE_BACK:
+    GcdAttributes = EFI_MEMORY_WB;
+    break;
+  default:
+    DEBUG ((DEBUG_ERROR,
+      "PageAttributeToGcdAttribute: PageAttributes:0x%lX not supported.\n",
+      PageAttributes));
+    ASSERT (0);
+    // The Global Coherency Domain (GCD) value is defined as a bit set.
+    // Returning 0 means no attribute has been set.
+    GcdAttributes = 0;
+  }
+
+  // Determine protection attributes
+  if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) ||
+      ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) {
+    // Read only cases map to write-protect
+    GcdAttributes |= EFI_MEMORY_RO;
+  }
+
+  // Process eXecute Never attribute
+  if ((PageAttributes & (TT_PXN_MASK | TT_UXN_MASK)) != 0) {
+    GcdAttributes |= EFI_MEMORY_XP;
+  }
+
+  return GcdAttributes;
+}
+
 STATIC
 UINT64
 GetFirstPageAttribute (
diff --git a/ArmPkg/Include/Chipset/AArch64.h b/ArmPkg/Include/Chipset/AArch64.h
index e3d877207b38..0ade5cce91c3 100644
--- a/ArmPkg/Include/Chipset/AArch64.h
+++ b/ArmPkg/Include/Chipset/AArch64.h
@@ -219,11 +219,6 @@ ArmReadCurrentEL (
   VOID
   );
 
-UINT64
-PageAttributeToGcdAttribute (
-  IN UINT64 PageAttributes
-  );
-
 UINTN
 ArmWriteCptr (
   IN  UINT64 Cptr
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 3b10ef58f0a2..d16e847218b7 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -57,51 +57,6 @@ ArmMemoryAttributeToPageAttribute (
   }
 }
 
-UINT64
-PageAttributeToGcdAttribute (
-  IN UINT64 PageAttributes
-  )
-{
-  UINT64  GcdAttributes;
-
-  switch (PageAttributes & TT_ATTR_INDX_MASK) {
-  case TT_ATTR_INDX_DEVICE_MEMORY:
-    GcdAttributes = EFI_MEMORY_UC;
-    break;
-  case TT_ATTR_INDX_MEMORY_NON_CACHEABLE:
-    GcdAttributes = EFI_MEMORY_WC;
-    break;
-  case TT_ATTR_INDX_MEMORY_WRITE_THROUGH:
-    GcdAttributes = EFI_MEMORY_WT;
-    break;
-  case TT_ATTR_INDX_MEMORY_WRITE_BACK:
-    GcdAttributes = EFI_MEMORY_WB;
-    break;
-  default:
-    DEBUG ((DEBUG_ERROR,
-      "PageAttributeToGcdAttribute: PageAttributes:0x%lX not supported.\n",
-      PageAttributes));
-    ASSERT (0);
-    // The Global Coherency Domain (GCD) value is defined as a bit set.
-    // Returning 0 means no attribute has been set.
-    GcdAttributes = 0;
-  }
-
-  // Determine protection attributes
-  if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) ||
-      ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) {
-    // Read only cases map to write-protect
-    GcdAttributes |= EFI_MEMORY_RO;
-  }
-
-  // Process eXecute Never attribute
-  if ((PageAttributes & (TT_PXN_MASK | TT_UXN_MASK)) != 0) {
-    GcdAttributes |= EFI_MEMORY_XP;
-  }
-
-  return GcdAttributes;
-}
-
 #define MIN_T0SZ        16
 #define BITS_PER_LEVEL  9
 
-- 
2.17.1

[PATCH 3/5] ArmPkg/ArmMmuLib: drop pointless LookupAddresstoRootTable() routine

[Ard Biesheuvel]

LookupAddresstoRootTable() uses a loop to go over its MaxAddress
argument, essentially to do a log2() and determine how many bits are
needed to represent it. Since the argument is the result of a shift-left
expression, there is some room for improvement here, and we can simply
use the bit count directly to calculate the value of T0SZ. At the same
time, we can omit calling GetRootTranslationTableInfo() to determine the
number of root table entries, and add a new helper that applies the
trivial calculation directly.

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

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index d16e847218b7..b6f3ef54aa26 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -59,6 +59,16 @@ ArmMemoryAttributeToPageAttribute (
 
 #define MIN_T0SZ        16
 #define BITS_PER_LEVEL  9
+#define MAX_VA_BITS     48
+
+STATIC
+UINTN
+GetRootTableEntryCount (
+  IN  UINTN T0SZ
+  )
+{
+  return TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
+}
 
 VOID
 GetRootTranslationTableInfo (
@@ -284,36 +294,6 @@ UpdateRegionMappingRecursive (
   return EFI_SUCCESS;
 }
 
-STATIC
-VOID
-LookupAddresstoRootTable (
-  IN  UINT64  MaxAddress,
-  OUT UINTN  *T0SZ,
-  OUT UINTN  *TableEntryCount
-  )
-{
-  UINTN TopBit;
-
-  // Check the parameters are not NULL
-  ASSERT ((T0SZ != NULL) && (TableEntryCount != NULL));
-
-  // Look for the highest bit set in MaxAddress
-  for (TopBit = 63; TopBit != 0; TopBit--) {
-    if ((1ULL << TopBit) & MaxAddress) {
-      // MaxAddress top bit is found
-      TopBit = TopBit + 1;
-      break;
-    }
-  }
-  ASSERT (TopBit != 0);
-
-  // Calculate T0SZ from the top bit of the MaxAddress
-  *T0SZ = 64 - TopBit;
-
-  // Get the Table info from T0SZ
-  GetRootTranslationTableInfo (*T0SZ, NULL, TableEntryCount);
-}
-
 STATIC
 EFI_STATUS
 UpdateRegionMapping (
@@ -508,6 +488,7 @@ ArmConfigureMmu (
   )
 {
   VOID*                         TranslationTable;
+  UINTN                         MaxAddressBits;
   UINT64                        MaxAddress;
   UINTN                         T0SZ;
   UINTN                         RootTableEntryCount;
@@ -526,11 +507,11 @@ ArmConfigureMmu (
   // into account the architectural limitations that result from UEFI's
   // use of 4 KB pages.
   //
-  MaxAddress = MIN (LShiftU64 (1ULL, ArmGetPhysicalAddressBits ()) - 1,
-                    MAX_ALLOC_ADDRESS);
+  MaxAddressBits = MIN (ArmGetPhysicalAddressBits (), MAX_VA_BITS);
+  MaxAddress = LShiftU64 (1ULL, MaxAddressBits) - 1;
 
-  // Lookup the Table Level to get the information
-  LookupAddresstoRootTable (MaxAddress, &T0SZ, &RootTableEntryCount);
+  T0SZ = 64 - MaxAddressBits;
+  RootTableEntryCount = GetRootTableEntryCount (T0SZ);
 
   //
   // Set TCR that allows us to retrieve T0SZ in the subsequent functions
-- 
2.17.1

[PATCH 4/5] ArmPkg/ArmMmuLib: get rid of GetRootTranslationTableInfo()

[Ard Biesheuvel]

Only a single call to GetRootTranslationTableInfo() remains, which
only provides the root table level. So let's create a new static
helper function that returns just this value, and use it instead.

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

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index b6f3ef54aa26..a82596d290f1 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -70,21 +70,13 @@ GetRootTableEntryCount (
   return TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
 }
 
-VOID
-GetRootTranslationTableInfo (
-  IN UINTN     T0SZ,
-  OUT UINTN   *TableLevel,
-  OUT UINTN   *TableEntryCount
+STATIC
+UINTN
+GetRootTableLevel (
+  IN  UINTN T0SZ
   )
 {
-  // Get the level of the root table
-  if (TableLevel) {
-    *TableLevel = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
-  }
-
-  if (TableEntryCount) {
-    *TableEntryCount = 1UL << (BITS_PER_LEVEL - (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL);
-  }
+  return (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
 }
 
 STATIC
@@ -303,7 +295,6 @@ UpdateRegionMapping (
   IN  UINT64  AttributeClearMask
   )
 {
-  UINTN     RootTableLevel;
   UINTN     T0SZ;
 
   if (((RegionStart | RegionLength) & EFI_PAGE_MASK)) {
@@ -311,11 +302,10 @@ UpdateRegionMapping (
   }
 
   T0SZ = ArmGetTCR () & TCR_T0SZ_MASK;
-  GetRootTranslationTableInfo (T0SZ, &RootTableLevel, NULL);
 
   return UpdateRegionMappingRecursive (RegionStart, RegionStart + RegionLength,
            AttributeSetMask, AttributeClearMask, ArmGetTTBR0BaseAddress (),
-           RootTableLevel);
+           GetRootTableLevel (T0SZ));
 }
 
 STATIC
-- 
2.17.1

[PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

[Ard Biesheuvel]

TT_ATTR_INDX_INVALID is #define'd but never used so drop it.

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

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index a82596d290f1..222ff817956f 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -19,9 +19,6 @@
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 
-// We use this index definition to define an invalid block entry
-#define TT_ATTR_INDX_INVALID    ((UINT32)~0)
-
 STATIC
 UINT64
 ArmMemoryAttributeToPageAttribute (
-- 
2.17.1

Re: [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

[Leif Lindholm]

On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
> Before getting rid of GetRootTranslationTableInfo() and the related
> LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
> version of the former to CpuDxe, which will be its only remaining
> user. While at it, simplify it a bit, since in the CpuDxe cases,
> both OUT arguments are always provided.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
>  ArmPkg/Drivers/CpuDxe/CpuDxe.h      |  7 -------
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index 3b6c5e733709..24eb1c4221e3 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
>  
> +#define MIN_T0SZ        16
> +#define BITS_PER_LEVEL  9
> +
> +STATIC
> +VOID
> +GetRootTranslationTableInfo (
> +  IN  UINTN     T0SZ,
> +  OUT UINTN     *RootTableLevel,
> +  OUT UINTN     *RootTableEntryCount
> +  )
> +{
> +  *RootTableLevel       = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
> +  *RootTableEntryCount  = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
> +}
> +
>  STATIC
>  UINT64
>  GetFirstPageAttribute (
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> index b627c3c50ff8..3fe5c24d5e5b 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> @@ -134,13 +134,6 @@ GetMemoryRegion (
>    OUT    UINTN                   *RegionAttributes
>    );
>  
> -VOID
> -GetRootTranslationTableInfo (

So, this may be super picky, but:
Deleting the prototype without making the definition also STATIC would
cause build errors with -Wmissing-prototypes (which someone might be
enabling explicitly or implicitly if say doing some code hardening on
the side).

Now, it's a valid point to say that -Wmissing-prototypes isn't in our
current CFLAGS, but I think it would be a good habit to get into.
So you get a:

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

regardless, but I'd appreciate if you could sling a STATIC into
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
pushing.

> -  IN  UINTN    T0SZ,
> -  OUT UINTN   *TableLevel,
> -  OUT UINTN   *TableEntryCount
> -  );
> -
>  EFI_STATUS
>  SetGcdMemorySpaceAttributes (
>    IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR    *MemorySpaceMap,
> -- 
> 2.17.1
> 

Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

[Ard Biesheuvel]

On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
> On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
>> Before getting rid of GetRootTranslationTableInfo() and the related
>> LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
>> version of the former to CpuDxe, which will be its only remaining
>> user. While at it, simplify it a bit, since in the CpuDxe cases,
>> both OUT arguments are always provided.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>   ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
>>   ArmPkg/Drivers/CpuDxe/CpuDxe.h      |  7 -------
>>   2 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> index 3b6c5e733709..24eb1c4221e3 100644
>> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> @@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>   
>>   #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
>>   
>> +#define MIN_T0SZ        16
>> +#define BITS_PER_LEVEL  9
>> +
>> +STATIC
>> +VOID
>> +GetRootTranslationTableInfo (
>> +  IN  UINTN     T0SZ,
>> +  OUT UINTN     *RootTableLevel,
>> +  OUT UINTN     *RootTableEntryCount
>> +  )
>> +{
>> +  *RootTableLevel       = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
>> +  *RootTableEntryCount  = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
>> +}
>> +
>>   STATIC
>>   UINT64
>>   GetFirstPageAttribute (
>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> index b627c3c50ff8..3fe5c24d5e5b 100644
>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>> @@ -134,13 +134,6 @@ GetMemoryRegion (
>>     OUT    UINTN                   *RegionAttributes
>>     );
>>   
>> -VOID
>> -GetRootTranslationTableInfo (
> 
> So, this may be super picky, but:
> Deleting the prototype without making the definition also STATIC would
> cause build errors with -Wmissing-prototypes (which someone might be
> enabling explicitly or implicitly if say doing some code hardening on
> the side).
> 
> Now, it's a valid point to say that -Wmissing-prototypes isn't in our
> current CFLAGS, but I think it would be a good habit to get into.
> So you get a:
> 
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> 
> regardless, but I'd appreciate if you could sling a STATIC into
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
> pushing.
> 

Well, while I see your point, please note that the prototype only exists 
in a local header file that only gets included by CpuDxe, and not by any 
of the other consumers of ArmMmuLib.


>> -  IN  UINTN    T0SZ,
>> -  OUT UINTN   *TableLevel,
>> -  OUT UINTN   *TableEntryCount
>> -  );
>> -
>>   EFI_STATUS
>>   SetGcdMemorySpaceAttributes (
>>     IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR    *MemorySpaceMap,
>> -- 
>> 2.17.1
>>
> 
> 
> 

Re: [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

[Leif Lindholm]

On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
> TT_ATTR_INDX_INVALID is #define'd but never used so drop it.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index a82596d290f1..222ff817956f 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -19,9 +19,6 @@
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  
> -// We use this index definition to define an invalid block entry
> -#define TT_ATTR_INDX_INVALID    ((UINT32)~0)
> -

Since this is separately defined also in
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
could this patch be tweaked to instead "consolidate" the definitions
to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
all of the other TT_ATTR_INDX_ definitions live?

/
    Leif

>  STATIC
>  UINT64
>  ArmMemoryAttributeToPageAttribute (
> -- 
> 2.17.1
> 

Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

[Leif Lindholm]

On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
> On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
> > On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
> > > Before getting rid of GetRootTranslationTableInfo() and the related
> > > LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
> > > version of the former to CpuDxe, which will be its only remaining
> > > user. While at it, simplify it a bit, since in the CpuDxe cases,
> > > both OUT arguments are always provided.
> > > 
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >   ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
> > >   ArmPkg/Drivers/CpuDxe/CpuDxe.h      |  7 -------
> > >   2 files changed, 15 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> > > index 3b6c5e733709..24eb1c4221e3 100644
> > > --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> > > +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> > > @@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >   #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
> > > +#define MIN_T0SZ        16
> > > +#define BITS_PER_LEVEL  9
> > > +
> > > +STATIC
> > > +VOID
> > > +GetRootTranslationTableInfo (
> > > +  IN  UINTN     T0SZ,
> > > +  OUT UINTN     *RootTableLevel,
> > > +  OUT UINTN     *RootTableEntryCount
> > > +  )
> > > +{
> > > +  *RootTableLevel       = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
> > > +  *RootTableEntryCount  = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
> > > +}
> > > +
> > >   STATIC
> > >   UINT64
> > >   GetFirstPageAttribute (
> > > diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > index b627c3c50ff8..3fe5c24d5e5b 100644
> > > --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > > @@ -134,13 +134,6 @@ GetMemoryRegion (
> > >     OUT    UINTN                   *RegionAttributes
> > >     );
> > > -VOID
> > > -GetRootTranslationTableInfo (
> > 
> > So, this may be super picky, but:
> > Deleting the prototype without making the definition also STATIC would
> > cause build errors with -Wmissing-prototypes (which someone might be
> > enabling explicitly or implicitly if say doing some code hardening on
> > the side).
> > 
> > Now, it's a valid point to say that -Wmissing-prototypes isn't in our
> > current CFLAGS, but I think it would be a good habit to get into.
> > So you get a:
> > 
> > Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> > 
> > regardless, but I'd appreciate if you could sling a STATIC into
> > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
> > pushing.
> > 
> 
> Well, while I see your point, please note that the prototype only exists in
> a local header file that only gets included by CpuDxe, and not by any of the
> other consumers of ArmMmuLib.

What I'm saying is that with -Wmissing-prototypes added, building
after applying this patch (but before applying the one that deletes
the function) gives:

"aarch64-linux-gnu-gcc"   -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc"   -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c -Wmissing-prototypes
/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: error: no previous prototype for ‘GetRootTranslationTableInfo’ [-Werror=missing-prototypes]
 GetRootTranslationTableInfo (
 ^~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

So it breaks bisect if you're experimenting with hardening.

As I said, I don't insist, but I want to be sure you realise that bit
before you decide.

/
    Leif

> 
> 
> > > -  IN  UINTN    T0SZ,
> > > -  OUT UINTN   *TableLevel,
> > > -  OUT UINTN   *TableEntryCount
> > > -  );
> > > -
> > >   EFI_STATUS
> > >   SetGcdMemorySpaceAttributes (
> > >     IN EFI_GCD_MEMORY_SPACE_DESCRIPTOR    *MemorySpaceMap,
> > > -- 
> > > 2.17.1
> > > 
> > 
> > 
> > 
> 

Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

[Ard Biesheuvel]

On 4/2/20 12:28 PM, Leif Lindholm wrote:
> On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
>> On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
>>> On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
>>>> Before getting rid of GetRootTranslationTableInfo() and the related
>>>> LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
>>>> version of the former to CpuDxe, which will be its only remaining
>>>> user. While at it, simplify it a bit, since in the CpuDxe cases,
>>>> both OUT arguments are always provided.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>>    ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 15 +++++++++++++++
>>>>    ArmPkg/Drivers/CpuDxe/CpuDxe.h      |  7 -------
>>>>    2 files changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>>>> index 3b6c5e733709..24eb1c4221e3 100644
>>>> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>>>> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>>>> @@ -15,6 +15,21 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>>    #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
>>>> +#define MIN_T0SZ        16
>>>> +#define BITS_PER_LEVEL  9
>>>> +
>>>> +STATIC
>>>> +VOID
>>>> +GetRootTranslationTableInfo (
>>>> +  IN  UINTN     T0SZ,
>>>> +  OUT UINTN     *RootTableLevel,
>>>> +  OUT UINTN     *RootTableEntryCount
>>>> +  )
>>>> +{
>>>> +  *RootTableLevel       = (T0SZ - MIN_T0SZ) / BITS_PER_LEVEL;
>>>> +  *RootTableEntryCount  = TT_ENTRY_COUNT >> (T0SZ - MIN_T0SZ) % BITS_PER_LEVEL;
>>>> +}
>>>> +
>>>>    STATIC
>>>>    UINT64
>>>>    GetFirstPageAttribute (
>>>> diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>>>> index b627c3c50ff8..3fe5c24d5e5b 100644
>>>> --- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>>>> +++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
>>>> @@ -134,13 +134,6 @@ GetMemoryRegion (
>>>>      OUT    UINTN                   *RegionAttributes
>>>>      );
>>>> -VOID
>>>> -GetRootTranslationTableInfo (
>>>
>>> So, this may be super picky, but:
>>> Deleting the prototype without making the definition also STATIC would
>>> cause build errors with -Wmissing-prototypes (which someone might be
>>> enabling explicitly or implicitly if say doing some code hardening on
>>> the side).
>>>
>>> Now, it's a valid point to say that -Wmissing-prototypes isn't in our
>>> current CFLAGS, but I think it would be a good habit to get into.
>>> So you get a:
>>>
>>> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>>>
>>> regardless, but I'd appreciate if you could sling a STATIC into
>>> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
>>> pushing.
>>>
>>
>> Well, while I see your point, please note that the prototype only exists in
>> a local header file that only gets included by CpuDxe, and not by any of the
>> other consumers of ArmMmuLib.
> 
> What I'm saying is that with -Wmissing-prototypes added, building
> after applying this patch (but before applying the one that deletes
> the function) gives:
> 
> "aarch64-linux-gnu-gcc"   -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc"   -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c -Wmissing-prototypes
> /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: error: no previous prototype for ‘GetRootTranslationTableInfo’ [-Werror=missing-prototypes]
>   GetRootTranslationTableInfo (
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> So it breaks bisect if you're experimenting with hardening.
> 
> As I said, I don't insist, but I want to be sure you realise that bit
> before you decide.
> 

But none of the other consumers of ArmMmuLib do an #include of 
ArmPkg/Drivers/CpuDxe/CpuDxe.h, right? So I am slightly puzzled by this 
result.

Re: [edk2-devel] [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

[Ard Biesheuvel]

On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote:
> On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
>> TT_ATTR_INDX_INVALID is #define'd but never used so drop it.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> index a82596d290f1..222ff817956f 100644
>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> @@ -19,9 +19,6 @@
>>   #include <Library/BaseLib.h>
>>   #include <Library/DebugLib.h>
>>   
>> -// We use this index definition to define an invalid block entry
>> -#define TT_ATTR_INDX_INVALID    ((UINT32)~0)
>> -
> 
> Since this is separately defined also in
> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
> could this patch be tweaked to instead "consolidate" the definitions
> to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
> all of the other TT_ATTR_INDX_ definitions live?
> 

That would imply that this value is somehow architected, which it is not.

ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this 
constant, and it only has meaning within the context of the routines 
therein.

Re: [edk2-devel] [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

[Leif Lindholm]

On Thu, Apr 02, 2020 at 12:29:40 +0200, Ard Biesheuvel wrote:
> On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote:
> > On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
> > > TT_ATTR_INDX_INVALID is #define'd but never used so drop it.
> > > 
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >   ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
> > >   1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > > index a82596d290f1..222ff817956f 100644
> > > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > > @@ -19,9 +19,6 @@
> > >   #include <Library/BaseLib.h>
> > >   #include <Library/DebugLib.h>
> > > -// We use this index definition to define an invalid block entry
> > > -#define TT_ATTR_INDX_INVALID    ((UINT32)~0)
> > > -
> > 
> > Since this is separately defined also in
> > ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
> > could this patch be tweaked to instead "consolidate" the definitions
> > to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
> > all of the other TT_ATTR_INDX_ definitions live?
> > 
> 
> That would imply that this value is somehow architected, which it is not.
> 
> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this
> constant, and it only has meaning within the context of the routines
> therein.

Hmm, ok. So all of those definitions should really move *out* of
ArmPkg/Include/Chipset/AArch64Mmu.h?

No, they can't, because some of the others are used in
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

So I'm still feeling that #defines using the same namespace should
live together in order to reduce risk of future screwups.
So, unrelated to this patch
(Reviewed-by: Leif Lindholm <leif@nuviainc.com>)
should the remaining TT_ATTR_INDX_INVALID be renamed, or should it be
moved to ArmPkg/Include/Chipset/AArch64Mmu.h?

/
    Leif

Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

[Leif Lindholm]

On Thu, Apr 02, 2020 at 12:29:30 +0200, Ard Biesheuvel wrote:
> On 4/2/20 12:28 PM, Leif Lindholm wrote:
> > On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
> > > On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
> > > > On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
> > > > > Before getting rid of GetRootTranslationTableInfo() and the related
> > > > > LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
> > > > > version of the former to CpuDxe, which will be its only remaining
> > > > > user. While at it, simplify it a bit, since in the CpuDxe cases,
> > > > > both OUT arguments are always provided.

Summary after discussing off-list:
This patch isn't *just* moving a public function private, but it's
replacing use of a function that was never meant to be public and had
a completely bonkers forward-declaration in a different module.

Ard has agreed to make the commit message more explicit on that point,
and I'm happy with that. So with that change, for the series:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> > > > So, this may be super picky, but:
> > > > Deleting the prototype without making the definition also STATIC would
> > > > cause build errors with -Wmissing-prototypes (which someone might be
> > > > enabling explicitly or implicitly if say doing some code hardening on
> > > > the side).
> > > > 
> > > > Now, it's a valid point to say that -Wmissing-prototypes isn't in our
> > > > current CFLAGS, but I think it would be a good habit to get into.
> > > > So you get a:
> > > > 
> > > > Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> > > > 
> > > > regardless, but I'd appreciate if you could sling a STATIC into
> > > > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c as well before
> > > > pushing.
> > > > 
> > > 
> > > Well, while I see your point, please note that the prototype only exists in
> > > a local header file that only gets included by CpuDxe, and not by any of the
> > > other consumers of ArmMmuLib.
> > 
> > What I'm saying is that with -Wmissing-prototypes added, building
> > after applying this patch (but before applying the one that deletes
> > the function) gives:
> > 
> > "aarch64-linux-gnu-gcc"   -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> > leif@vanye:/work/git/tianocore$ "aarch64-linux-gnu-gcc"   -g -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -ffunction-sections -fdata-sections -include AutoGen.h -fno-common -DSTRING_ARRAY_NAME=ArmMmuBaseLibStrings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -DMDEPKG_NDEBUG -DDISABLE_NEW_DEPRECATED_INTERFACES -mstrict-align -mgeneral-regs-only -c -o /work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/OUTPUT/AArch64/ArmMmuLibCore.obj -I/work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64 -I/work/git/edk2/ArmPkg/Library/ArmMmuLib -I/work/git/tianocore/Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/AARCH64/ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib/DEBUG -I/work/git/edk2/ArmPkg -I/work/git/edk2/ArmPkg/Include -I/work/git/edk2/EmbeddedPkg -I/work/git/edk2/EmbeddedPkg/Include -I/work/git/edk2/MdePkg -I/work/git/edk2/MdePkg/Include -I/work/git/edk2/MdePkg/Include/AArch64 /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c -Wmissing-prototypes
> > /work/git/edk2/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c:109:1: error: no previous prototype for ‘GetRootTranslationTableInfo’ [-Werror=missing-prototypes]
> >   GetRootTranslationTableInfo (
> >   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> > 
> > So it breaks bisect if you're experimenting with hardening.
> > 
> > As I said, I don't insist, but I want to be sure you realise that bit
> > before you decide.
> > 
> 
> But none of the other consumers of ArmMmuLib do an #include of
> ArmPkg/Drivers/CpuDxe/CpuDxe.h, right? So I am slightly puzzled by this
> result.
>

Re: [edk2-devel] [PATCH 5/5] ArmPkg/ArmMmuLib: drop unused TT_ATTR_INDX_INVALID CPP symbol

[Ard Biesheuvel]

On 4/2/20 12:57 PM, Leif Lindholm wrote:
> On Thu, Apr 02, 2020 at 12:29:40 +0200, Ard Biesheuvel wrote:
>> On 4/2/20 12:23 PM, Leif Lindholm via groups.io wrote:
>>> On Sat, Mar 28, 2020 at 11:43:21 +0100, Ard Biesheuvel wrote:
>>>> TT_ATTR_INDX_INVALID is #define'd but never used so drop it.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>>    ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 3 ---
>>>>    1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>>>> index a82596d290f1..222ff817956f 100644
>>>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>>>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>>>> @@ -19,9 +19,6 @@
>>>>    #include <Library/BaseLib.h>
>>>>    #include <Library/DebugLib.h>
>>>> -// We use this index definition to define an invalid block entry
>>>> -#define TT_ATTR_INDX_INVALID    ((UINT32)~0)
>>>> -
>>>
>>> Since this is separately defined also in
>>> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c (£"$%^£"$?!?!?)
>>> could this patch be tweaked to instead "consolidate" the definitions
>>> to a central location, like ArmPkg/Include/Chipset/AArch64Mmu.h where
>>> all of the other TT_ATTR_INDX_ definitions live?
>>>
>>
>> That would imply that this value is somehow architected, which it is not.
>>
>> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c is the only remaining user of this
>> constant, and it only has meaning within the context of the routines
>> therein.
> 
> Hmm, ok. So all of those definitions should really move *out* of
> ArmPkg/Include/Chipset/AArch64Mmu.h?
> 
> No, they can't, because some of the others are used in
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> 
> So I'm still feeling that #defines using the same namespace should
> live together in order to reduce risk of future screwups.
> So, unrelated to this patch
> (Reviewed-by: Leif Lindholm <leif@nuviainc.com>)
> should the remaining TT_ATTR_INDX_INVALID be renamed, or should it be
> moved to ArmPkg/Include/Chipset/AArch64Mmu.h?
> 

It should simply be renamed. They are not part of the same namespace, it 
is simply a local hack in CpuDxe to distinguish between valid indexes 
into the MAIR table and an 'unknown' placeholder value.

CpuDxe is on my list of things I'd like to clean up as well.

Re: [edk2-devel] [PATCH 1/5] ArmPkg/CpuDxe: use private copy of GetRootTranslationTableInfo()

[Ard Biesheuvel]

On 4/2/20 1:03 PM, Leif Lindholm wrote:
> On Thu, Apr 02, 2020 at 12:29:30 +0200, Ard Biesheuvel wrote:
>> On 4/2/20 12:28 PM, Leif Lindholm wrote:
>>> On Thu, Apr 02, 2020 at 12:20:54 +0200, Ard Biesheuvel wrote:
>>>> On 4/2/20 12:16 PM, Leif Lindholm via groups.io wrote:
>>>>> On Sat, Mar 28, 2020 at 11:43:17 +0100, Ard Biesheuvel wrote:
>>>>>> Before getting rid of GetRootTranslationTableInfo() and the related
>>>>>> LookupAddresstoRootTable() in AARCH64's version of ArmMmuLib, add a
>>>>>> version of the former to CpuDxe, which will be its only remaining
>>>>>> user. While at it, simplify it a bit, since in the CpuDxe cases,
>>>>>> both OUT arguments are always provided.
> 
> Summary after discussing off-list:
> This patch isn't *just* moving a public function private, but it's
> replacing use of a function that was never meant to be public and had
> a completely bonkers forward-declaration in a different module.
> 
> Ard has agreed to make the commit message more explicit on that point,
> and I'm happy with that. So with that change, for the series:
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> 

Pushed, thanks.