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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, Feb 15, 2017 at 05:11:54PM +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_MASK (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>

With this updated commit message:
Reviewed-by: Leif Lindholm <leif.lindholm@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
>