[PATCH v2] efi: arm64: treat regions with WT/WC set but WB cleared as memory

[Ard Biesheuvel]

On 26 August 2016 at 11:08, Ard Biesheuvel <ard.biesheuvel at linaro.org> wrote:
> On 26 August 2016 at 11:45, James Morse <james.morse at arm.com> wrote:
>> Hi Ard,
>>
>> On 25/08/16 17:17, Ard Biesheuvel wrote:
>>> Currently, memory regions are only recorded in the memblock memory table
>>> if they have the EFI_MEMORY_WB memory type attribute set. In case the
>>> region is of a reserved type, it is also marked as MEMBLOCK_NOMAP, which
>>> will leave it out of the linear mapping.
>>>
>>> However, memory regions may legally have the EFI_MEMORY_WT or EFI_MEMORY_WC
>>> attributes set, and the EFI_MEMORY_WB cleared, in which case the region in
>>> question is obviously backed by normal memory, but is not recorded in the
>>> memblock memory table at all.
>>
>> I've been hitting this when applying weird (but permissible) attributes to the
>> ACPI regions. Currently without WB we map these as device memory, then let
>> acpica make unaligned accesses to it.
>>
>> This patch fixes all that, and from the conversation on irc, the 'UC only' case
>> for ACPI tables is x86-legacy, and should never happen.
>>
>
> Indeed, deliberately exposing a region containing ACPI tables as
> something that *requires* strongly ordered access does not make sense.
>
> The problem here is that the UEFI memory map describes both occupied
> and available regions, and in the 'available' case, it is obvious that
> these bits describe the nature of the physical backing of the region,
> i.e., whether the bus fabric, memory, etc allow writeback/writethrough
> caching, write buffering etc. This explains why most regions have all
> the bits set.
>
> In the occupied case, this can be misinterpreted as describing the
> nature of the content, like an ACPI table containing fields that are
> not naturally aligned. I.e, I don't think clearing the UC bit here
> should be interpreted to mean that the payload *requires* WB/WT/WC. It
> is up to the kernel to complain if it interprets a region, and notices
> that the only supported mapping type conflicts with the nature of the
> accesses we intend to perform.
>
>>
>>> diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
>>> index c49d50e68aee..c2ac5975fd5d 100644
>>> --- a/drivers/firmware/efi/arm-init.c
>>> +++ b/drivers/firmware/efi/arm-init.c
>>> @@ -163,18 +163,22 @@ static __init int is_reserve_region(efi_memory_desc_t *md)
>>>       case EFI_BOOT_SERVICES_DATA:
>>>       case EFI_CONVENTIONAL_MEMORY:
>>>       case EFI_PERSISTENT_MEMORY:
>>> -             return 0;
>>> +             /*
>>> +              * According to the spec, these regions are no longer reserved
>>> +              * after calling ExitBootServices(). However, we can only use
>>> +              * them as System RAM if they can be mapped writeback cacheable.
>>> +              */
>>> +             return (md->attribute & EFI_MEMORY_WB);
>>>       default:
>>>               break;
>>
>> Micro-nit: break here in order to immediately return false looks a bit odd...
>>
>
> Yes, I could not decide what to do with it so I left it.
>
>>
>>>       }
>>> -     return is_normal_ram(md);
>>> +     return false;
>>>  }
>>
>>
>> Tested-by: James Morse <james.morse at arm.com>
>> Reviewed-by: James Morse <james.morse at arm.com>
>>
>

Matt,

Could we get this queued for v4.9 please?

Thanks,
Ard.