On 26 August 2016 at 11:08, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote: > On 26 August 2016 at 11:45, James Morse <james.morse@xxxxxxx> 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@xxxxxxx> >> Reviewed-by: James Morse <james.morse@xxxxxxx> >> > Matt, Could we get this queued for v4.9 please? Thanks, Ard. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
