[PATCH 0/4] ArmPkg: add groundwork for DXE image protection

[PATCH 0/4] ArmPkg: add groundwork for DXE image protection

[Ard Biesheuvel]

The upcoming DXE image protection feature expects the EFI_CPU_ARCH_PROTOCOL
method SetMemoryAttributes() to deal with invocations that only modify
permission attributes, but leave the cacheability attributes alone. This
requires some groundwork to be performed in the MMU code for ARM.

Patch #1 is Jiewen's patch to retire EFI_MEMORY_WP, which is no longer
used as a permission attribute.

Patch #2 updates EfiAttributeToArmAttribute () so it can deal with
unspecified caching modes.

Patch #3 makes ARM deal with EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes()
calls that do not specify memory attributes. On ARM, we don't have code
that manages the permission bits in the page tables, so this does little
more than ignore such attributes.

Patch #4 implements the handling for AARCH64 to manage the permissions
bits without touching or caring about the memory type attributes.

Ard Biesheuvel (3):
  ArmPkg/CpuDxe: translate invalid memory types in
    EfiAttributeToArmAttribute
  ArmPkg/CpuDxe: ARM: ignore page table updates that only change
    permissions
  ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions

Jiewen Yao (1):
  ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  7 +-
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 24 ++++---
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  5 +-
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 76 +++++++++++++++-----
 4 files changed, 77 insertions(+), 35 deletions(-)

-- 
2.7.4

[PATCH 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

[Ard Biesheuvel]

From: Jiewen Yao <jiewen.yao@intel.com>

Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,
according to UEFI spec, we should use EFI_MEMORY_RO for write protection.
The EFI_MEMORY_WP is the cache attribute instead of memory attribute.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  3 ++-
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 14 ++++++--------
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  5 +++--
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  3 ++-
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index d8bb41978066..15d5a8173233 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -3,6 +3,7 @@
 Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
 Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
 Portions copyright (c) 2011-2013, ARM Ltd. All rights reserved.<BR>
+Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -224,7 +225,7 @@ EfiAttributeToArmAttribute (
   ArmAttributes |= TT_AF;
 
   // Determine protection attributes
-  if (EfiAttributes & EFI_MEMORY_WP) {
+  if (EfiAttributes & EFI_MEMORY_RO) {
     ArmAttributes |= TT_AP_RO_RO;
   }
 
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 14fc22d7a59f..6dcfba69e879 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -3,6 +3,7 @@
 Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
 Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
 Portions copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
+Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -62,7 +63,7 @@ SectionToGcdAttributes (
   // determine protection attributes
   switch(SectionAttributes & TT_DESCRIPTOR_SECTION_AP_MASK) {
     case TT_DESCRIPTOR_SECTION_AP_NO_NO: // no read, no write
-      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
+      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
       break;
 
     case TT_DESCRIPTOR_SECTION_AP_RW_NO:
@@ -73,7 +74,7 @@ SectionToGcdAttributes (
     // read only cases map to write-protect
     case TT_DESCRIPTOR_SECTION_AP_RO_NO:
     case TT_DESCRIPTOR_SECTION_AP_RO_RO:
-      *GcdAttributes |= EFI_MEMORY_WP;
+      *GcdAttributes |= EFI_MEMORY_RO;
       break;
 
     default:
@@ -126,7 +127,7 @@ PageToGcdAttributes (
   // determine protection attributes
   switch(PageAttributes & TT_DESCRIPTOR_PAGE_AP_MASK) {
     case TT_DESCRIPTOR_PAGE_AP_NO_NO: // no read, no write
-      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
+      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
       break;
 
     case TT_DESCRIPTOR_PAGE_AP_RW_NO:
@@ -137,7 +138,7 @@ PageToGcdAttributes (
     // read only cases map to write-protect
     case TT_DESCRIPTOR_PAGE_AP_RO_NO:
     case TT_DESCRIPTOR_PAGE_AP_RO_RO:
-      *GcdAttributes |= EFI_MEMORY_WP;
+      *GcdAttributes |= EFI_MEMORY_RO;
       break;
 
     default:
@@ -730,9 +731,6 @@ EfiAttributeToArmAttribute (
       ArmAttributes = TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
       break;
 
-    case EFI_MEMORY_WP:
-    case EFI_MEMORY_XP:
-    case EFI_MEMORY_RP:
     case EFI_MEMORY_UCE:
     default:
       // Cannot be implemented UEFI definition unclear for ARM
@@ -743,7 +741,7 @@ EfiAttributeToArmAttribute (
   }
 
   // Determine protection attributes
-  if (EfiAttributes & EFI_MEMORY_WP) {
+  if (EfiAttributes & EFI_MEMORY_RO) {
     ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
   } else {
     ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index 723604d1df96..54d9b0163331 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -1,6 +1,7 @@
 /** @file
 *
 *  Copyright (c) 2013, ARM Limited. All rights reserved.
+*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -236,7 +237,7 @@ CpuConvertPagesToUncachedVirtualAddress (
   // be the PCI address. Code should always use the CPU address, and we will or in VirtualMask
   // to that address.
   //
-  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_WP, 0);
+  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_RO, 0);
   if (!EFI_ERROR (Status)) {
     Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_UC, VirtualMask);
   }
@@ -264,7 +265,7 @@ CpuReconvertPages (
   //
   // Unmap the aliased Address
   //
-  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_WP, 0);
+  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_RO, 0);
   if (!EFI_ERROR (Status)) {
     //
     // Restore atttributes
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 540069a59b2e..6aa970bc0514 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -3,6 +3,7 @@
 *
 *  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
 *  Copyright (c) 2016, Linaro Limited. All rights reserved.
+*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -89,7 +90,7 @@ PageAttributeToGcdAttribute (
   // 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_WP;
+    GcdAttributes |= EFI_MEMORY_RO;
   }
 
   // Process eXecute Never attribute
-- 
2.7.4

[PATCH 2/4] ArmPkg/CpuDxe: translate invalid memory types in EfiAttributeToArmAttribute

[Ard Biesheuvel]

The single user of EfiAttributeToArmAttribute () is the protocol
method EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), which uses the
return value to compare against the ARM attributes of an existing mapping,
to infer whether it is actually necessary to change anything, or whether
the requested update is redundant. This saves some cache and TLB
maintenance on 32-bit ARM systems that use uncached translation tables.

However, EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() may be invoked with
only permission bits set, in which case the implied requested action is to
update the permissions of the region without modifying the cacheability
attributes. This is currently not possible, because
EfiAttributeToArmAttribute () ASSERT()s [on AArch64] on Attributes arguments
that lack a cacheability bit.

So let's simply return TT_ATTR_INDX_INVALID (AArch64) or
TT_DESCRIPTOR_SECTION_TYPE_FAULT (ARM) in these cases (or'ed with the
appropriate permission bits). This way, the return value is equally
suitable for checking whether the attributes need to be modified, but
in a way that accommodates the use without a cacheability bit set.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 4 +---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c     | 3 ---
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index 15d5a8173233..7688846e70cb 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -216,9 +216,7 @@ EfiAttributeToArmAttribute (
     ArmAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK;
     break;
   default:
-    DEBUG ((EFI_D_ERROR, "EfiAttributeToArmAttribute: 0x%lX attributes is not supported.\n", EfiAttributes));
-    ASSERT (0);
-    ArmAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
+    ArmAttributes = TT_ATTR_INDX_MASK;
   }
 
   // Set the access flag to match the block attributes
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 6dcfba69e879..b6ba975b353a 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -733,10 +733,7 @@ EfiAttributeToArmAttribute (
 
     case EFI_MEMORY_UCE:
     default:
-      // Cannot be implemented UEFI definition unclear for ARM
-      // Cause a page fault if these ranges are accessed.
       ArmAttributes = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
-      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): Unsupported attribute %x will page fault on access\n", EfiAttributes));
       break;
   }
 
-- 
2.7.4

[PATCH 3/4] ArmPkg/CpuDxe: ARM: ignore page table updates that only change permissions

[Ard Biesheuvel]

Currently, we have not implemented support on 32-bit ARM for managing
permission bits in the page tables. Since the new DXE page protection
for PE/COFF images may invoke EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes()
with only permission attributes set, let's simply ignore those for now.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index b6ba975b353a..89e429925ba9 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -680,6 +680,13 @@ SetMemoryAttributes (
 {
   EFI_STATUS    Status;
 
+  //
+  // Ignore invocations that only modify permission bits
+  //
+  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
+    return EFI_SUCCESS;
+  }
+
   if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) {
     // Is the base and length a multiple of 1 MB?
     DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
-- 
2.7.4

[PATCH 4/4] ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions

[Ard Biesheuvel]

Since the new DXE page protection for PE/COFF images may invoke
EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() with only permission
attributes set, add support for this in the AARCH64 MMU code.

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

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 6aa970bc0514..764e54b5d747 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -28,6 +28,10 @@
 // We use this index definition to define an invalid block entry
 #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
 
+#define EFI_MEMORY_CACHETYPE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
+                                     EFI_MEMORY_WT | EFI_MEMORY_WB | \
+                                     EFI_MEMORY_UCE)
+
 STATIC
 UINT64
 ArmMemoryAttributeToPageAttribute (
@@ -101,25 +105,46 @@ PageAttributeToGcdAttribute (
   return GcdAttributes;
 }
 
-ARM_MEMORY_REGION_ATTRIBUTES
-GcdAttributeToArmAttribute (
+STATIC
+UINT64
+GcdAttributeToPageAttribute (
   IN UINT64 GcdAttributes
   )
 {
-  switch (GcdAttributes & 0xFF) {
+  UINT64 PageAttributes;
+
+  switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) {
   case EFI_MEMORY_UC:
-    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+    PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
+    break;
   case EFI_MEMORY_WC:
-    return ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
+    PageAttributes = TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
+    break;
   case EFI_MEMORY_WT:
-    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH;
+    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;
+    break;
   case EFI_MEMORY_WB:
-    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
+    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
+    break;
   default:
-    DEBUG ((EFI_D_ERROR, "GcdAttributeToArmAttribute: 0x%lX attributes is not supported.\n", GcdAttributes));
-    ASSERT (0);
-    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
+    PageAttributes = TT_ATTR_INDX_MASK;
+    break;
   }
+
+  if ((GcdAttributes & EFI_MEMORY_XP) != 0 ||
+      (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) == EFI_MEMORY_UC) {
+    if (ArmReadCurrentEL () == AARCH64_EL2) {
+      PageAttributes |= TT_XN_MASK;
+    } else {
+      PageAttributes |= TT_UXN_MASK | TT_PXN_MASK;
+    }
+  }
+
+  if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
+    PageAttributes |= TT_AP_RO_RO;
+  }
+
+  return PageAttributes | TT_AF;
 }
 
 #define MIN_T0SZ        16
@@ -434,17 +459,31 @@ SetMemoryAttributes (
   )
 {
   RETURN_STATUS                Status;
-  ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
   UINT64                      *TranslationTable;
-
-  MemoryRegion.PhysicalBase = BaseAddress;
-  MemoryRegion.VirtualBase = BaseAddress;
-  MemoryRegion.Length = Length;
-  MemoryRegion.Attributes = GcdAttributeToArmAttribute (Attributes);
+  UINT64                       PageAttributes;
+  UINT64                       PageAttributeMask;
+
+  PageAttributes = GcdAttributeToPageAttribute (Attributes);
+  PageAttributeMask = 0;
+
+  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
+    //
+    // No memory type was set in Attributes, so we are going to update the
+    // permissions only.
+    //
+    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;
+    PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
+                          TT_PXN_MASK | TT_XN_MASK);
+  }
 
   TranslationTable = ArmGetTTBR0BaseAddress ();
 
-  Status = FillTranslationTable (TranslationTable, &MemoryRegion);
+  Status = UpdateRegionMapping (
+             TranslationTable,
+             BaseAddress,
+             Length,
+             PageAttributes,
+             PageAttributeMask);
   if (RETURN_ERROR (Status)) {
     return Status;
   }
-- 
2.7.4

Re: [PATCH 2/4] ArmPkg/CpuDxe: translate invalid memory types in EfiAttributeToArmAttribute

[Leif Lindholm]

On Thu, Feb 09, 2017 at 05:38:09PM +0000, Ard Biesheuvel wrote:
> The single user of EfiAttributeToArmAttribute () is the protocol
> method EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), which uses the
> return value to compare against the ARM attributes of an existing mapping,
> to infer whether it is actually necessary to change anything, or whether
> the requested update is redundant. This saves some cache and TLB
> maintenance on 32-bit ARM systems that use uncached translation tables.
> 
> However, EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() may be invoked with
> only permission bits set, in which case the implied requested action is to
> update the permissions of the region without modifying the cacheability
> attributes. This is currently not possible, because
> EfiAttributeToArmAttribute () ASSERT()s [on AArch64] on Attributes arguments
> that lack a cacheability bit.
> 
> So let's simply return TT_ATTR_INDX_INVALID (AArch64) or
> TT_DESCRIPTOR_SECTION_TYPE_FAULT (ARM) in these cases (or'ed with the
> appropriate permission bits). This way, the return value is equally
> suitable for checking whether the attributes need to be modified, but
> in a way that accommodates the use without a cacheability bit set.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 4 +---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c     | 3 ---
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index 15d5a8173233..7688846e70cb 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -216,9 +216,7 @@ EfiAttributeToArmAttribute (
>      ArmAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK;
>      break;
>    default:
> -    DEBUG ((EFI_D_ERROR, "EfiAttributeToArmAttribute: 0x%lX attributes is not supported.\n", EfiAttributes));
> -    ASSERT (0);
> -    ArmAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
> +    ArmAttributes = TT_ATTR_INDX_MASK;

Commit message says TT_ATTR_INDX_INVALID - which one is intended to be
correct?

/
    Leif

>    }
>  
>    // Set the access flag to match the block attributes
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 6dcfba69e879..b6ba975b353a 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -733,10 +733,7 @@ EfiAttributeToArmAttribute (
>  
>      case EFI_MEMORY_UCE:
>      default:
> -      // Cannot be implemented UEFI definition unclear for ARM
> -      // Cause a page fault if these ranges are accessed.
>        ArmAttributes = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): Unsupported attribute %x will page fault on access\n", EfiAttributes));
>        break;
>    }
>  
> -- 
> 2.7.4
> 

Re: [PATCH 2/4] ArmPkg/CpuDxe: translate invalid memory types in EfiAttributeToArmAttribute

[Ard Biesheuvel]

On 10 February 2017 at 17:54, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Feb 09, 2017 at 05:38:09PM +0000, Ard Biesheuvel wrote:
>> The single user of EfiAttributeToArmAttribute () is the protocol
>> method EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), which uses the
>> return value to compare against the ARM attributes of an existing mapping,
>> to infer whether it is actually necessary to change anything, or whether
>> the requested update is redundant. This saves some cache and TLB
>> maintenance on 32-bit ARM systems that use uncached translation tables.
>>
>> However, EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() may be invoked with
>> only permission bits set, in which case the implied requested action is to
>> update the permissions of the region without modifying the cacheability
>> attributes. This is currently not possible, because
>> EfiAttributeToArmAttribute () ASSERT()s [on AArch64] on Attributes arguments
>> that lack a cacheability bit.
>>
>> So let's simply return TT_ATTR_INDX_INVALID (AArch64) or
>> TT_DESCRIPTOR_SECTION_TYPE_FAULT (ARM) in these cases (or'ed with the
>> appropriate permission bits). This way, the return value is equally
>> suitable for checking whether the attributes need to be modified, but
>> in a way that accommodates the use without a cacheability bit set.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 4 +---
>>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c     | 3 ---
>>  2 files changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> index 15d5a8173233..7688846e70cb 100644
>> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
>> @@ -216,9 +216,7 @@ EfiAttributeToArmAttribute (
>>      ArmAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK;
>>      break;
>>    default:
>> -    DEBUG ((EFI_D_ERROR, "EfiAttributeToArmAttribute: 0x%lX attributes is not supported.\n", EfiAttributes));
>> -    ASSERT (0);
>> -    ArmAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
>> +    ArmAttributes = TT_ATTR_INDX_MASK;
>
> Commit message says TT_ATTR_INDX_INVALID - which one is intended to be
> correct?
>

_MASK is the correct one. I will fix up the commit message when
applying (assuming there are no other concerns)

TT_ATTR_INDX_INVALID is named poorly, and cannnot be and'ed in a
meaningful way with other TT_ flags, so I stopped using it, but forgot
to update the log message

Re: [PATCH 3/4] ArmPkg/CpuDxe: ARM: ignore page table updates that only change permissions

[Leif Lindholm]

On Thu, Feb 09, 2017 at 05:38:10PM +0000, Ard Biesheuvel wrote:
> Currently, we have not implemented support on 32-bit ARM for managing
> permission bits in the page tables. Since the new DXE page protection
> for PE/COFF images may invoke EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes()
> with only permission attributes set, let's simply ignore those for now.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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

> ---
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index b6ba975b353a..89e429925ba9 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -680,6 +680,13 @@ SetMemoryAttributes (
>  {
>    EFI_STATUS    Status;
>  
> +  //
> +  // Ignore invocations that only modify permission bits
> +  //
> +  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
> +    return EFI_SUCCESS;
> +  }
> +
>    if(((BaseAddress & 0xFFFFF) == 0) && ((Length & 0xFFFFF) == 0)) {
>      // Is the base and length a multiple of 1 MB?
>      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): MMU section 0x%x length 0x%x to %lx\n", (UINTN)BaseAddress, (UINTN)Length, Attributes));
> -- 
> 2.7.4
> 

Re: [PATCH 4/4] ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions

[Leif Lindholm]

On Thu, Feb 09, 2017 at 05:38:11PM +0000, Ard Biesheuvel wrote:
> Since the new DXE page protection for PE/COFF images may invoke
> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() with only permission
> attributes set, add support for this in the AARCH64 MMU code.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 73 +++++++++++++++-----
>  1 file changed, 56 insertions(+), 17 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 6aa970bc0514..764e54b5d747 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -28,6 +28,10 @@
>  // We use this index definition to define an invalid block entry
>  #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
>  
> +#define EFI_MEMORY_CACHETYPE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
> +                                     EFI_MEMORY_WT | EFI_MEMORY_WB | \
> +                                     EFI_MEMORY_UCE)
> +

This is already duplicated between

ArmPkg/Drivers/CpuDxe/CpuDxe.h
and
UefiCpuPkg/CpuDxe/CpuDxe.h

Can we avoid adding more?

>  STATIC
>  UINT64
>  ArmMemoryAttributeToPageAttribute (
> @@ -101,25 +105,46 @@ PageAttributeToGcdAttribute (
>    return GcdAttributes;
>  }
>  
> -ARM_MEMORY_REGION_ATTRIBUTES
> -GcdAttributeToArmAttribute (
> +STATIC
> +UINT64
> +GcdAttributeToPageAttribute (
>    IN UINT64 GcdAttributes
>    )
>  {
> -  switch (GcdAttributes & 0xFF) {
> +  UINT64 PageAttributes;
> +
> +  switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) {
>    case EFI_MEMORY_UC:
> -    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +    PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
> +    break;
>    case EFI_MEMORY_WC:
> -    return ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> +    PageAttributes = TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
> +    break;
>    case EFI_MEMORY_WT:
> -    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH;
> +    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;

These TT_SH additions look like a bugfix - should they be a separate commit?

> +    break;
>    case EFI_MEMORY_WB:
> -    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> +    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
> +    break;
>    default:
> -    DEBUG ((EFI_D_ERROR, "GcdAttributeToArmAttribute: 0x%lX attributes is not supported.\n", GcdAttributes));
> -    ASSERT (0);
> -    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> +    PageAttributes = TT_ATTR_INDX_MASK;

OK, so you're doing the same thing here as in the ARM code.
This is a substantial change in behaviour (old behaviour: if unknown,
set to DEVICE; new behaviour: if unknown, set "all types permitted").
Am I missing something?

> +    break;
>    }
> +
> +  if ((GcdAttributes & EFI_MEMORY_XP) != 0 ||
> +      (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) == EFI_MEMORY_UC) {
> +    if (ArmReadCurrentEL () == AARCH64_EL2) {
> +      PageAttributes |= TT_XN_MASK;
> +    } else {
> +      PageAttributes |= TT_UXN_MASK | TT_PXN_MASK;
> +    }
> +  }
> +
> +  if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> +    PageAttributes |= TT_AP_RO_RO;
> +  }
> +
> +  return PageAttributes | TT_AF;
>  }
>  
>  #define MIN_T0SZ        16
> @@ -434,17 +459,31 @@ SetMemoryAttributes (
>    )
>  {
>    RETURN_STATUS                Status;
> -  ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
>    UINT64                      *TranslationTable;
> -
> -  MemoryRegion.PhysicalBase = BaseAddress;
> -  MemoryRegion.VirtualBase = BaseAddress;
> -  MemoryRegion.Length = Length;
> -  MemoryRegion.Attributes = GcdAttributeToArmAttribute (Attributes);
> +  UINT64                       PageAttributes;
> +  UINT64                       PageAttributeMask;
> +
> +  PageAttributes = GcdAttributeToPageAttribute (Attributes);
> +  PageAttributeMask = 0;
> +
> +  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
> +    //
> +    // No memory type was set in Attributes, so we are going to update the
> +    // permissions only.
> +    //
> +    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;
> +    PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
> +                          TT_PXN_MASK | TT_XN_MASK);
> +  }
>  
>    TranslationTable = ArmGetTTBR0BaseAddress ();
>  
> -  Status = FillTranslationTable (TranslationTable, &MemoryRegion);
> +  Status = UpdateRegionMapping (
> +             TranslationTable,
> +             BaseAddress,
> +             Length,
> +             PageAttributes,
> +             PageAttributeMask);
>    if (RETURN_ERROR (Status)) {
>      return Status;
>    }
> -- 
> 2.7.4
> 

Re: [PATCH 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

[Leif Lindholm]

On Thu, Feb 09, 2017 at 05:38:08PM +0000, Ard Biesheuvel wrote:
> From: Jiewen Yao <jiewen.yao@intel.com>
> 
> Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,
> according to UEFI spec, we should use EFI_MEMORY_RO for write protection.
> The EFI_MEMORY_WP is the cache attribute instead of memory attribute.
> 
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

No objections to this patch, but I would have expected it to be 4/4,
if it caused issues requiring the other 3 to be created?

/
    Leif

> ---
>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  3 ++-
>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 14 ++++++--------
>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  5 +++--
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  3 ++-
>  4 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index d8bb41978066..15d5a8173233 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -3,6 +3,7 @@
>  Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
>  Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
>  Portions copyright (c) 2011-2013, ARM Ltd. All rights reserved.<BR>
> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>  
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
> @@ -224,7 +225,7 @@ EfiAttributeToArmAttribute (
>    ArmAttributes |= TT_AF;
>  
>    // Determine protection attributes
> -  if (EfiAttributes & EFI_MEMORY_WP) {
> +  if (EfiAttributes & EFI_MEMORY_RO) {
>      ArmAttributes |= TT_AP_RO_RO;
>    }
>  
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index 14fc22d7a59f..6dcfba69e879 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -3,6 +3,7 @@
>  Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
>  Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
>  Portions copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>  
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
> @@ -62,7 +63,7 @@ SectionToGcdAttributes (
>    // determine protection attributes
>    switch(SectionAttributes & TT_DESCRIPTOR_SECTION_AP_MASK) {
>      case TT_DESCRIPTOR_SECTION_AP_NO_NO: // no read, no write
> -      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
> +      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
>        break;
>  
>      case TT_DESCRIPTOR_SECTION_AP_RW_NO:
> @@ -73,7 +74,7 @@ SectionToGcdAttributes (
>      // read only cases map to write-protect
>      case TT_DESCRIPTOR_SECTION_AP_RO_NO:
>      case TT_DESCRIPTOR_SECTION_AP_RO_RO:
> -      *GcdAttributes |= EFI_MEMORY_WP;
> +      *GcdAttributes |= EFI_MEMORY_RO;
>        break;
>  
>      default:
> @@ -126,7 +127,7 @@ PageToGcdAttributes (
>    // determine protection attributes
>    switch(PageAttributes & TT_DESCRIPTOR_PAGE_AP_MASK) {
>      case TT_DESCRIPTOR_PAGE_AP_NO_NO: // no read, no write
> -      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
> +      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
>        break;
>  
>      case TT_DESCRIPTOR_PAGE_AP_RW_NO:
> @@ -137,7 +138,7 @@ PageToGcdAttributes (
>      // read only cases map to write-protect
>      case TT_DESCRIPTOR_PAGE_AP_RO_NO:
>      case TT_DESCRIPTOR_PAGE_AP_RO_RO:
> -      *GcdAttributes |= EFI_MEMORY_WP;
> +      *GcdAttributes |= EFI_MEMORY_RO;
>        break;
>  
>      default:
> @@ -730,9 +731,6 @@ EfiAttributeToArmAttribute (
>        ArmAttributes = TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
>        break;
>  
> -    case EFI_MEMORY_WP:
> -    case EFI_MEMORY_XP:
> -    case EFI_MEMORY_RP:
>      case EFI_MEMORY_UCE:
>      default:
>        // Cannot be implemented UEFI definition unclear for ARM
> @@ -743,7 +741,7 @@ EfiAttributeToArmAttribute (
>    }
>  
>    // Determine protection attributes
> -  if (EfiAttributes & EFI_MEMORY_WP) {
> +  if (EfiAttributes & EFI_MEMORY_RO) {
>      ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
>    } else {
>      ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> index 723604d1df96..54d9b0163331 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> @@ -1,6 +1,7 @@
>  /** @file
>  *
>  *  Copyright (c) 2013, ARM Limited. All rights reserved.
> +*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD License
> @@ -236,7 +237,7 @@ CpuConvertPagesToUncachedVirtualAddress (
>    // be the PCI address. Code should always use the CPU address, and we will or in VirtualMask
>    // to that address.
>    //
> -  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_WP, 0);
> +  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_RO, 0);
>    if (!EFI_ERROR (Status)) {
>      Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_UC, VirtualMask);
>    }
> @@ -264,7 +265,7 @@ CpuReconvertPages (
>    //
>    // Unmap the aliased Address
>    //
> -  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_WP, 0);
> +  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_RO, 0);
>    if (!EFI_ERROR (Status)) {
>      //
>      // Restore atttributes
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 540069a59b2e..6aa970bc0514 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -3,6 +3,7 @@
>  *
>  *  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
>  *  Copyright (c) 2016, Linaro Limited. All rights reserved.
> +*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
>  *
>  *  This program and the accompanying materials
>  *  are licensed and made available under the terms and conditions of the BSD License
> @@ -89,7 +90,7 @@ PageAttributeToGcdAttribute (
>    // 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_WP;
> +    GcdAttributes |= EFI_MEMORY_RO;
>    }
>  
>    // Process eXecute Never attribute
> -- 
> 2.7.4
> 

Re: [PATCH 4/4] ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions

[Ard Biesheuvel]

On 10 February 2017 at 18:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Feb 09, 2017 at 05:38:11PM +0000, Ard Biesheuvel wrote:
>> Since the new DXE page protection for PE/COFF images may invoke
>> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() with only permission
>> attributes set, add support for this in the AARCH64 MMU code.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 73 +++++++++++++++-----
>>  1 file changed, 56 insertions(+), 17 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> index 6aa970bc0514..764e54b5d747 100644
>> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
>> @@ -28,6 +28,10 @@
>>  // We use this index definition to define an invalid block entry
>>  #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
>>
>> +#define EFI_MEMORY_CACHETYPE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
>> +                                     EFI_MEMORY_WT | EFI_MEMORY_WB | \
>> +                                     EFI_MEMORY_UCE)
>> +
>
> This is already duplicated between
>
> ArmPkg/Drivers/CpuDxe/CpuDxe.h
> and
> UefiCpuPkg/CpuDxe/CpuDxe.h
>
> Can we avoid adding more?
>

Good point. Mind if I move it to ArmMmuLib.h? (and keep the UefiCpuPkg
one alone)

>>  STATIC
>>  UINT64
>>  ArmMemoryAttributeToPageAttribute (
>> @@ -101,25 +105,46 @@ PageAttributeToGcdAttribute (
>>    return GcdAttributes;
>>  }
>>
>> -ARM_MEMORY_REGION_ATTRIBUTES
>> -GcdAttributeToArmAttribute (
>> +STATIC
>> +UINT64
>> +GcdAttributeToPageAttribute (
>>    IN UINT64 GcdAttributes
>>    )
>>  {
>> -  switch (GcdAttributes & 0xFF) {
>> +  UINT64 PageAttributes;
>> +
>> +  switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) {
>>    case EFI_MEMORY_UC:
>> -    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>> +    PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
>> +    break;
>>    case EFI_MEMORY_WC:
>> -    return ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
>> +    PageAttributes = TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
>> +    break;
>>    case EFI_MEMORY_WT:
>> -    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH;
>> +    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;
>
> These TT_SH additions look like a bugfix - should they be a separate commit?
>

No, it's the diff that is confusing here: GcdAttributeToArmAttribute()
is removed completely, and replaced with
GcdAttributeToPageAttribute(). Due to the case labels, these line up,
but they are completely unrelated.

>> +    break;
>>    case EFI_MEMORY_WB:
>> -    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
>> +    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
>> +    break;
>>    default:
>> -    DEBUG ((EFI_D_ERROR, "GcdAttributeToArmAttribute: 0x%lX attributes is not supported.\n", GcdAttributes));
>> -    ASSERT (0);
>> -    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>> +    PageAttributes = TT_ATTR_INDX_MASK;
>
> OK, so you're doing the same thing here as in the ARM code.
> This is a substantial change in behaviour (old behaviour: if unknown,
> set to DEVICE; new behaviour: if unknown, set "all types permitted").
> Am I missing something?
>

Again, completely different function

>> +    break;
>>    }
>> +
>> +  if ((GcdAttributes & EFI_MEMORY_XP) != 0 ||
>> +      (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) == EFI_MEMORY_UC) {
>> +    if (ArmReadCurrentEL () == AARCH64_EL2) {
>> +      PageAttributes |= TT_XN_MASK;
>> +    } else {
>> +      PageAttributes |= TT_UXN_MASK | TT_PXN_MASK;
>> +    }
>> +  }
>> +
>> +  if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
>> +    PageAttributes |= TT_AP_RO_RO;
>> +  }
>> +
>> +  return PageAttributes | TT_AF;
>>  }
>>
>>  #define MIN_T0SZ        16
>> @@ -434,17 +459,31 @@ SetMemoryAttributes (
>>    )
>>  {
>>    RETURN_STATUS                Status;
>> -  ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
>>    UINT64                      *TranslationTable;
>> -
>> -  MemoryRegion.PhysicalBase = BaseAddress;
>> -  MemoryRegion.VirtualBase = BaseAddress;
>> -  MemoryRegion.Length = Length;
>> -  MemoryRegion.Attributes = GcdAttributeToArmAttribute (Attributes);
>> +  UINT64                       PageAttributes;
>> +  UINT64                       PageAttributeMask;
>> +
>> +  PageAttributes = GcdAttributeToPageAttribute (Attributes);
>> +  PageAttributeMask = 0;
>> +
>> +  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
>> +    //
>> +    // No memory type was set in Attributes, so we are going to update the
>> +    // permissions only.
>> +    //
>> +    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;
>> +    PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
>> +                          TT_PXN_MASK | TT_XN_MASK);
>> +  }
>>
>>    TranslationTable = ArmGetTTBR0BaseAddress ();
>>
>> -  Status = FillTranslationTable (TranslationTable, &MemoryRegion);
>> +  Status = UpdateRegionMapping (
>> +             TranslationTable,
>> +             BaseAddress,
>> +             Length,
>> +             PageAttributes,
>> +             PageAttributeMask);
>>    if (RETURN_ERROR (Status)) {
>>      return Status;
>>    }
>> --
>> 2.7.4
>>

Re: [PATCH 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

[Ard Biesheuvel]

On 10 February 2017 at 18:17, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Feb 09, 2017 at 05:38:08PM +0000, Ard Biesheuvel wrote:
>> From: Jiewen Yao <jiewen.yao@intel.com>
>>
>> Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,
>> according to UEFI spec, we should use EFI_MEMORY_RO for write protection.
>> The EFI_MEMORY_WP is the cache attribute instead of memory attribute.
>>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> No objections to this patch, but I would have expected it to be 4/4,
> if it caused issues requiring the other 3 to be created?
>

Not quite: it is the feature itself that requires these fixes, and
this patch actually makes sense as 1/4, since it removes uses of
EFI_MEMORY_WP that are no longer appropriate. Implementing 2-4 with
EFI_MEMORY_WP instead of EFI_MEMORY_RO and then changing it at the end
would make no sense at all.

Re: [PATCH 1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

[Leif Lindholm]

On Fri, Feb 10, 2017 at 06:25:00PM +0000, Ard Biesheuvel wrote:
> On 10 February 2017 at 18:17, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Feb 09, 2017 at 05:38:08PM +0000, Ard Biesheuvel wrote:
> >> From: Jiewen Yao <jiewen.yao@intel.com>
> >>
> >> Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,
> >> according to UEFI spec, we should use EFI_MEMORY_RO for write protection.
> >> The EFI_MEMORY_WP is the cache attribute instead of memory attribute.
> >>
> >> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >
> > No objections to this patch, but I would have expected it to be 4/4,
> > if it caused issues requiring the other 3 to be created?
> >
> 
> Not quite: it is the feature itself that requires these fixes, and
> this patch actually makes sense as 1/4, since it removes uses of
> EFI_MEMORY_WP that are no longer appropriate. Implementing 2-4 with
> EFI_MEMORY_WP instead of EFI_MEMORY_RO and then changing it at the end
> would make no sense at all.

OK, so basically, the issue was already in the existing code?

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

Re: [PATCH 4/4] ArmPkg/ArmMmuLib: AARCH64: add support for modifying only permissions

[Leif Lindholm]

On Fri, Feb 10, 2017 at 06:23:23PM +0000, Ard Biesheuvel wrote:
> On 10 February 2017 at 18:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Feb 09, 2017 at 05:38:11PM +0000, Ard Biesheuvel wrote:
> >> Since the new DXE page protection for PE/COFF images may invoke
> >> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes() with only permission
> >> attributes set, add support for this in the AARCH64 MMU code.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 73 +++++++++++++++-----
> >>  1 file changed, 56 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> >> index 6aa970bc0514..764e54b5d747 100644
> >> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> >> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> >> @@ -28,6 +28,10 @@
> >>  // We use this index definition to define an invalid block entry
> >>  #define TT_ATTR_INDX_INVALID    ((UINT32)~0)
> >>
> >> +#define EFI_MEMORY_CACHETYPE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
> >> +                                     EFI_MEMORY_WT | EFI_MEMORY_WB | \
> >> +                                     EFI_MEMORY_UCE)
> >> +
> >
> > This is already duplicated between
> >
> > ArmPkg/Drivers/CpuDxe/CpuDxe.h
> > and
> > UefiCpuPkg/CpuDxe/CpuDxe.h
> >
> > Can we avoid adding more?
> >
> 
> Good point. Mind if I move it to ArmMmuLib.h? (and keep the UefiCpuPkg
> one alone)

That's fine for now.
They need to be squashed at some point, but that doesn't have to be now.

> >>  STATIC
> >>  UINT64
> >>  ArmMemoryAttributeToPageAttribute (
> >> @@ -101,25 +105,46 @@ PageAttributeToGcdAttribute (
> >>    return GcdAttributes;
> >>  }
> >>
> >> -ARM_MEMORY_REGION_ATTRIBUTES
> >> -GcdAttributeToArmAttribute (
> >> +STATIC
> >> +UINT64
> >> +GcdAttributeToPageAttribute (
> >>    IN UINT64 GcdAttributes
> >>    )
> >>  {
> >> -  switch (GcdAttributes & 0xFF) {
> >> +  UINT64 PageAttributes;
> >> +
> >> +  switch (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) {
> >>    case EFI_MEMORY_UC:
> >> -    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >> +    PageAttributes = TT_ATTR_INDX_DEVICE_MEMORY;
> >> +    break;
> >>    case EFI_MEMORY_WC:
> >> -    return ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> >> +    PageAttributes = TT_ATTR_INDX_MEMORY_NON_CACHEABLE;
> >> +    break;
> >>    case EFI_MEMORY_WT:
> >> -    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_THROUGH;
> >> +    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_THROUGH | TT_SH_INNER_SHAREABLE;
> >
> > These TT_SH additions look like a bugfix - should they be a separate commit?
> >
> 
> No, it's the diff that is confusing here: GcdAttributeToArmAttribute()
> is removed completely, and replaced with
> GcdAttributeToPageAttribute(). Due to the case labels, these line up,
> but they are completely unrelated.

Aaaah...

> 
> >> +    break;
> >>    case EFI_MEMORY_WB:
> >> -    return ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK;
> >> +    PageAttributes = TT_ATTR_INDX_MEMORY_WRITE_BACK | TT_SH_INNER_SHAREABLE;
> >> +    break;
> >>    default:
> >> -    DEBUG ((EFI_D_ERROR, "GcdAttributeToArmAttribute: 0x%lX attributes is not supported.\n", GcdAttributes));
> >> -    ASSERT (0);
> >> -    return ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
> >> +    PageAttributes = TT_ATTR_INDX_MASK;
> >
> > OK, so you're doing the same thing here as in the ARM code.
> > This is a substantial change in behaviour (old behaviour: if unknown,
> > set to DEVICE; new behaviour: if unknown, set "all types permitted").
> > Am I missing something?
> >
> 
> Again, completely different function

Sneaky :)

OK, I have no issues then.

/
    Leif

> >> +    break;
> >>    }
> >> +
> >> +  if ((GcdAttributes & EFI_MEMORY_XP) != 0 ||
> >> +      (GcdAttributes & EFI_MEMORY_CACHETYPE_MASK) == EFI_MEMORY_UC) {
> >> +    if (ArmReadCurrentEL () == AARCH64_EL2) {
> >> +      PageAttributes |= TT_XN_MASK;
> >> +    } else {
> >> +      PageAttributes |= TT_UXN_MASK | TT_PXN_MASK;
> >> +    }
> >> +  }
> >> +
> >> +  if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> >> +    PageAttributes |= TT_AP_RO_RO;
> >> +  }
> >> +
> >> +  return PageAttributes | TT_AF;
> >>  }
> >>
> >>  #define MIN_T0SZ        16
> >> @@ -434,17 +459,31 @@ SetMemoryAttributes (
> >>    )
> >>  {
> >>    RETURN_STATUS                Status;
> >> -  ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
> >>    UINT64                      *TranslationTable;
> >> -
> >> -  MemoryRegion.PhysicalBase = BaseAddress;
> >> -  MemoryRegion.VirtualBase = BaseAddress;
> >> -  MemoryRegion.Length = Length;
> >> -  MemoryRegion.Attributes = GcdAttributeToArmAttribute (Attributes);
> >> +  UINT64                       PageAttributes;
> >> +  UINT64                       PageAttributeMask;
> >> +
> >> +  PageAttributes = GcdAttributeToPageAttribute (Attributes);
> >> +  PageAttributeMask = 0;
> >> +
> >> +  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
> >> +    //
> >> +    // No memory type was set in Attributes, so we are going to update the
> >> +    // permissions only.
> >> +    //
> >> +    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;
> >> +    PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
> >> +                          TT_PXN_MASK | TT_XN_MASK);
> >> +  }
> >>
> >>    TranslationTable = ArmGetTTBR0BaseAddress ();
> >>
> >> -  Status = FillTranslationTable (TranslationTable, &MemoryRegion);
> >> +  Status = UpdateRegionMapping (
> >> +             TranslationTable,
> >> +             BaseAddress,
> >> +             Length,
> >> +             PageAttributes,
> >> +             PageAttributeMask);
> >>    if (RETURN_ERROR (Status)) {
> >>      return Status;
> >>    }
> >> --
> >> 2.7.4
> >>