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

On 26 August 2016 at 11:45, James Morse [off-list ref] wrote:

Hi Ard,

On 25/08/16 17:17, Ard Biesheuvel wrote:

quoted

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.

quoted

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.

quoted

      }
-     return is_normal_ram(md);
+     return false;
 }

Tested-by: James Morse <james.morse@arm.com>
Reviewed-by: James Morse <james.morse@arm.com>

Thanks,
Ard.