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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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