From patchwork Fri Jun 24 11:44:48 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 70823 Delivered-To: patch@linaro.org Received: by 10.140.28.4 with SMTP id 4csp903487qgy; Fri, 24 Jun 2016 04:44:54 -0700 (PDT) X-Received: by 10.98.89.85 with SMTP id n82mr6475307pfb.23.1466768694549; Fri, 24 Jun 2016 04:44:54 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 69si6453809pfr.155.2016.06.24.04.44.54; Fri, 24 Jun 2016 04:44:54 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751425AbcFXLow (ORCPT + 30 others); Fri, 24 Jun 2016 07:44:52 -0400 Received: from mail-io0-f171.google.com ([209.85.223.171]:34364 "EHLO mail-io0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751063AbcFXLou (ORCPT ); Fri, 24 Jun 2016 07:44:50 -0400 Received: by mail-io0-f171.google.com with SMTP id g13so89951968ioj.1 for ; Fri, 24 Jun 2016 04:44:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=SpmtEWeFBR1AVdqLI9OvEXnIBRmtNVAHFAns7ACPwJE=; b=WzL0Ri02EZ9Zy2ykYHzT208QZyw//IDofTTStlqhGOq7Es7DjDQFdvR3j7SjVvnd9R mccBnM1k19o9UTFae1wxLhye+IevhRXw/T+BLcOdicSyPpDx0S2mPgZ93qjZzggcext0 yzGAo+T08/l4nuvE1wEOuGk/4qNy7vi4NhXdw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=SpmtEWeFBR1AVdqLI9OvEXnIBRmtNVAHFAns7ACPwJE=; b=H71t+KyaTS09ImR4sqMCOoa1IcnfrJVXlN0NC97AY4RF7b30BFfa58mJwiLtLBZIk7 gpqnjhJIztwge9pNOQK8YjjlvO1rCmKmUZ5ZRfFblEiSkfSPUdDYZ2go/0BPI3Wp+gOo jJrM5HDLP1zHaHxB7KSolYnmSH3KPgiPh7g70z8i9ewb8js7dt85urE9gWWxF0KrK0KQ BvNOXtrYnJhnUbdbPXMXgB/lEhnjD1S/6rHZBmd0q7/tJfCbjn/LH7on38en8mCXA9yO hKejRyIv/XNVe9KA0twVKZJEhs/5bSqwQJ7BdCEZrmnEjo3AcDI6k1Bk1Edz8jMKbNfp h5ig== X-Gm-Message-State: ALyK8tKrfCVg5c0H/36suWykwWxvLsaoCXXe6zM9YW327LSygiCgl4mxfUaY2rDJoSnXlLBJ9jHm7buJfawdcC1H X-Received: by 10.107.164.211 with SMTP id d80mr5208430ioj.130.1466768689565; Fri, 24 Jun 2016 04:44:49 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.214.6 with HTTP; Fri, 24 Jun 2016 04:44:48 -0700 (PDT) In-Reply-To: <1466681690-5850-4-git-send-email-matt@codeblueprint.co.uk> References: <1466681690-5850-1-git-send-email-matt@codeblueprint.co.uk> <1466681690-5850-4-git-send-email-matt@codeblueprint.co.uk> From: Ard Biesheuvel Date: Fri, 24 Jun 2016 13:44:48 +0200 Message-ID: Subject: Re: [PATCH 03/11] efi: Refactor efi_memmap_init_early() into arch-neutral code To: Matt Fleming Cc: Dave Young , "linux-kernel@vger.kernel.org" , "linux-efi@vger.kernel.org" , Leif Lindholm , Peter Jones , Borislav Petkov , Mark Rutland Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Matt, On 23 June 2016 at 13:34, Matt Fleming wrote: > Every EFI architecture apart from ia64 needs to setup the EFI memory > map at efi.memmap, and the code for doing that is essentially the same > across all implementations. Therefore, it makes sense to factor this > out into the common code under drivers/firmware/efi/. > > The only slight variation is the data structure out of which we pull > the initial memory map information, such as physical address, memory > descriptor size and version, etc. We can address this by passing a > generic data structure (struct efi_memory_map_data) as the argument to > efi_memmap_init_early() which contains the minimum info required for > initialising the memory map. > > In the process, this patch also fixes a few undesirable implementation > differences: > > - ARM and arm64 were failing to clear the EFI_MEMMAP bit when > unmapping the early EFI memory map. EFI_MEMMAP indicates whether > the EFI memory map is mapped (not the regions contained within) and > can be traversed. It's more correct to set the bit as soon as we > memremap() the passed in EFI memmap. > This patch (and the series) looks mostly fine to me, but this patch does break ARM currently, please see below. > - Rename efi_unmmap_memmap() to efi_memmap_unmap() to adhere to the > regular naming scheme. > > This patch also uses a read-write mapping for the memory map instead > of the read-only mapping currently used on ARM and arm64. x86 needs > the ability to update the memory map in-place when assigning virtual > addresses to regions (efi_map_region()) and tagging regions when > reserving boot services (efi_reserve_boot_services()). > > There's no way for the generic fake_mem code to know which mapping to > use without introducing some arch-specific constant/hook, so just use > read-write since read-only is of dubious value for the EFI memory map. > > Cc: Ard Biesheuvel > Cc: Leif Lindholm > Cc: Peter Jones > Cc: Borislav Petkov > Cc: Mark Rutland > Cc: Dave Young > Signed-off-by: Matt Fleming > --- > arch/x86/include/asm/efi.h | 1 - > arch/x86/platform/efi/efi.c | 66 +++++++++++------------------------------ > arch/x86/platform/efi/quirks.c | 4 +-- > drivers/firmware/efi/arm-init.c | 17 +++++------ > drivers/firmware/efi/efi.c | 46 ++++++++++++++++++++++++++++ > drivers/firmware/efi/fake_mem.c | 15 ++++++---- > include/linux/efi.h | 16 ++++++++++ > 7 files changed, 98 insertions(+), 67 deletions(-) > > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h > index 78d1e7467eae..67983035bd7b 100644 > --- a/arch/x86/include/asm/efi.h > +++ b/arch/x86/include/asm/efi.h > @@ -118,7 +118,6 @@ extern int __init efi_memblock_x86_reserve_range(void); > extern pgd_t * __init efi_call_phys_prolog(void); > extern void __init efi_call_phys_epilog(pgd_t *save_pgd); > extern void __init efi_print_memmap(void); > -extern void __init efi_unmap_memmap(void); > extern void __init efi_memory_uc(u64 addr, unsigned long size); > extern void __init efi_map_region(efi_memory_desc_t *md); > extern void __init efi_map_region_fixed(efi_memory_desc_t *md); > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c > index 7c3d9092c668..b434c887229c 100644 > --- a/arch/x86/platform/efi/efi.c > +++ b/arch/x86/platform/efi/efi.c > @@ -188,7 +188,9 @@ static void __init do_add_efi_memmap(void) > int __init efi_memblock_x86_reserve_range(void) > { > struct efi_info *e = &boot_params.efi_info; > + struct efi_memory_map_data data; > phys_addr_t pmap; > + int rv; > > if (efi_enabled(EFI_PARAVIRT)) > return 0; > @@ -203,11 +205,17 @@ int __init efi_memblock_x86_reserve_range(void) > #else > pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32)); > #endif > - efi.memmap.phys_map = pmap; > - efi.memmap.nr_map = e->efi_memmap_size / > - e->efi_memdesc_size; > - efi.memmap.desc_size = e->efi_memdesc_size; > - efi.memmap.desc_version = e->efi_memdesc_version; > + data.phys_map = pmap; > + data.size = e->efi_memmap_size; > + data.desc_size = e->efi_memdesc_size; > + data.desc_version = e->efi_memdesc_version; > + > + rv = efi_memmap_init_early(&data); > + if (rv) > + return rv; > + > + if (add_efi_memmap) > + do_add_efi_memmap(); > > WARN(efi.memmap.desc_version != 1, > "Unexpected EFI_MEMORY_DESCRIPTOR version %ld", > @@ -234,19 +242,6 @@ void __init efi_print_memmap(void) > } > } > > -void __init efi_unmap_memmap(void) > -{ > - unsigned long size; > - > - clear_bit(EFI_MEMMAP, &efi.flags); > - > - size = efi.memmap.nr_map * efi.memmap.desc_size; > - if (efi.memmap.map) { > - early_memunmap(efi.memmap.map, size); > - efi.memmap.map = NULL; > - } > -} > - > static int __init efi_systab_init(void *phys) > { > if (efi_enabled(EFI_64BIT)) { > @@ -430,33 +425,6 @@ static int __init efi_runtime_init(void) > return 0; > } > > -static int __init efi_memmap_init(void) > -{ > - unsigned long addr, size; > - > - if (efi_enabled(EFI_PARAVIRT)) > - return 0; > - > - /* Map the EFI memory map */ > - size = efi.memmap.nr_map * efi.memmap.desc_size; > - addr = (unsigned long)efi.memmap.phys_map; > - > - efi.memmap.map = early_memremap(addr, size); > - if (efi.memmap.map == NULL) { > - pr_err("Could not map the memory map!\n"); > - return -ENOMEM; > - } > - > - efi.memmap.map_end = efi.memmap.map + size; > - > - if (add_efi_memmap) > - do_add_efi_memmap(); > - > - set_bit(EFI_MEMMAP, &efi.flags); > - > - return 0; > -} > - > void __init efi_init(void) > { > efi_char16_t *c16; > @@ -514,11 +482,11 @@ void __init efi_init(void) > if (!efi_runtime_supported()) > pr_info("No EFI runtime due to 32/64-bit mismatch with kernel\n"); > else { > - if (efi_runtime_disabled() || efi_runtime_init()) > + if (efi_runtime_disabled() || efi_runtime_init()) { > + efi_memmap_unmap(); > return; > + } > } > - if (efi_memmap_init()) > - return; > > if (efi_enabled(EFI_DBG)) > efi_print_memmap(); > @@ -855,7 +823,7 @@ static void __init kexec_enter_virtual_mode(void) > * non-native EFI > */ > if (!efi_is_native()) { > - efi_unmap_memmap(); > + efi_memmap_unmap(); > clear_bit(EFI_RUNTIME_SERVICES, &efi.flags); > return; > } > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c > index 4480c06cade7..0af180004e74 100644 > --- a/arch/x86/platform/efi/quirks.c > +++ b/arch/x86/platform/efi/quirks.c > @@ -266,7 +266,7 @@ void __init efi_free_boot_services(void) > free_bootmem_late(start, size); > } > > - efi_unmap_memmap(); > + efi_memmap_unmap(); > } > > /* > @@ -344,7 +344,7 @@ void __init efi_apply_memmap_quirks(void) > */ > if (!efi_runtime_supported()) { > pr_info("Setup done, disabling due to 32/64-bit mismatch\n"); > - efi_unmap_memmap(); > + efi_memmap_unmap(); > } > > /* UV2+ BIOS has a fix for this issue. UV1 still needs the quirk. */ > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > index c49d50e68aee..5a2df3fefccc 100644 > --- a/drivers/firmware/efi/arm-init.c > +++ b/drivers/firmware/efi/arm-init.c > @@ -211,12 +211,11 @@ static __init void reserve_regions(void) > memblock_mark_nomap(paddr, size); > > } > - > - set_bit(EFI_MEMMAP, &efi.flags); > } > > void __init efi_init(void) > { > + struct efi_memory_map_data data; > struct efi_fdt_params params; > > /* Grab UEFI information placed in FDT by stub */ > @@ -225,9 +224,12 @@ void __init efi_init(void) > > efi_system_table = params.system_table; > > - efi.memmap.phys_map = params.mmap; > - efi.memmap.map = early_memremap_ro(params.mmap, params.mmap_size); > - if (efi.memmap.map == NULL) { > + data.desc_version = params.desc_ver; > + data.desc_size = params.desc_size; > + data.size = params.mmap_size; > + data.phys_map = params.mmap; > + > + if (efi_memmap_init_early(&data) < 0) { > /* > * If we are booting via UEFI, the UEFI memory map is the only > * description of memory we have, so there is little point in > @@ -235,9 +237,6 @@ void __init efi_init(void) > */ > panic("Unable to map EFI memory map.\n"); > } > - efi.memmap.map_end = efi.memmap.map + params.mmap_size; > - efi.memmap.desc_size = params.desc_size; > - efi.memmap.desc_version = params.desc_ver; > > WARN(efi.memmap.desc_version != 1, > "Unexpected EFI_MEMORY_DESCRIPTOR version %ld", > @@ -248,7 +247,7 @@ void __init efi_init(void) > > reserve_regions(); > efi_memattr_init(); > - early_memunmap(efi.memmap.map, params.mmap_size); > + efi_memmap_unmap(); > > memblock_reserve(params.mmap & PAGE_MASK, > PAGE_ALIGN(params.mmap_size + > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > index 05509f3aaee8..3e6dce71f54d 100644 > --- a/drivers/firmware/efi/efi.c > +++ b/drivers/firmware/efi/efi.c > @@ -448,6 +448,52 @@ int __init efi_config_init(efi_config_table_type_t *arch_tables) > return ret; > } > > +/** > + * efi_memmap_init_early - Map the EFI memory map data structure > + * @data: EFI memory map data > + * > + * Use early_memremap() to map the passed in EFI memory map and assign > + * it to efi.memmap. > + */ > +int __init efi_memmap_init_early(struct efi_memory_map_data *data) > +{ > + struct efi_memory_map map; > + > + if (efi_enabled(EFI_PARAVIRT)) > + return 0; > + > + map.phys_map = data->phys_map; > + > + map.map = early_memremap(data->phys_map, data->size); > + if (!map.map) { > + pr_err("Could not map the memory map!\n"); > + return -ENOMEM; > + } > + > + map.nr_map = data->size / data->desc_size; > + map.map_end = map.map + data->size; > + > + map.desc_version = data->desc_version; > + map.desc_size = data->desc_size; > + > + set_bit(EFI_MEMMAP, &efi.flags); > + > + efi.memmap = map; > + > + return 0; > +} > + > +void __init efi_memmap_unmap(void) > +{ > + unsigned long size; > + > + size = efi.memmap.desc_size * efi.memmap.nr_map; > + > + early_memunmap(efi.memmap.map, size); > + efi.memmap.map = NULL; This assignment breaks the calculation of mapsize in arm_enable_runtime_services(), so you should probably fold the following hunk into this patch. With that change (or an equivalent one): Tested-by: Ard Biesheuvel Reviewed-by: Ard Biesheuvel diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c index ce1424672d89..1884347a3ef6 100644 --- a/drivers/firmware/efi/arm-runtime.c +++ b/drivers/firmware/efi/arm-runtime.c @@ -109,7 +109,7 @@ static int __init arm_enable_runtime_services(void) pr_info("Remapping and enabling EFI services.\n"); - mapsize = efi.memmap.map_end - efi.memmap.map; + mapsize = efi.memmap.desc_size * efi.memmap.nr_map; if (efi_memmap_init_late(efi.memmap.phys_map, mapsize)) { pr_err("Failed to remap EFI memory map\n");