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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel