From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755264AbcDKNR6 (ORCPT ); Mon, 11 Apr 2016 09:17:58 -0400 Received: from mail-ig0-f172.google.com ([209.85.213.172]:38258 "EHLO mail-ig0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754861AbcDKNR5 (ORCPT ); Mon, 11 Apr 2016 09:17:57 -0400 MIME-Version: 1.0 In-Reply-To: <1460379809-18490-3-git-send-email-matt@codeblueprint.co.uk> References: <1460379809-18490-1-git-send-email-matt@codeblueprint.co.uk> <1460379809-18490-3-git-send-email-matt@codeblueprint.co.uk> Date: Mon, 11 Apr 2016 15:17:55 +0200 Message-ID: Subject: Re: [PATCH 2/2] efi: Remove global 'memmap' From: Ard Biesheuvel To: Matt Fleming Cc: "linux-efi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Tony Luck , "Luck, Tony" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11 April 2016 at 15:03, Matt Fleming wrote: > Abolish the poorly named EFI memory map named 'memmap'. It is shadowed > by a bunch of local definitions in various files and having two ways > to access the EFI memory map ('efi->memmap' vs 'memmap') is rather > confusing. > > Furthermore, ia64 doesn't even provide this global object, which has > caused issues when trying to write generic EFI memmap code. > > Replace all occurrences with efi.memmap. > > Cc: Ard Biesheuvel > Cc: "Luck, Tony" > Signed-off-by: Matt Fleming > --- > arch/x86/platform/efi/efi.c | 86 +++++++++++++++++++++----------------- > drivers/firmware/efi/arm-init.c | 20 ++++----- > drivers/firmware/efi/arm-runtime.c | 13 +++--- > drivers/firmware/efi/fake_mem.c | 40 +++++++++--------- > include/linux/efi.h | 7 ++-- > 5 files changed, 86 insertions(+), 80 deletions(-) > > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c > index bd8f03301b59..88d2fb2cb3ef 100644 > --- a/arch/x86/platform/efi/efi.c > +++ b/arch/x86/platform/efi/efi.c > @@ -56,8 +56,6 @@ > > #define EFI_DEBUG > > -struct efi_memory_map memmap; > - > static struct efi efi_phys __initdata; > static efi_system_table_t efi_systab __initdata; > > @@ -207,15 +205,13 @@ int __init efi_memblock_x86_reserve_range(void) > #else > pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32)); > #endif > - memmap.phys_map = pmap; > - memmap.nr_map = e->efi_memmap_size / > + efi.memmap.phys_map = pmap; > + efi.memmap.nr_map = e->efi_memmap_size / > e->efi_memdesc_size; > - memmap.desc_size = e->efi_memdesc_size; > - memmap.desc_version = e->efi_memdesc_version; > - > - memblock_reserve(pmap, memmap.nr_map * memmap.desc_size); > + efi.memmap.desc_size = e->efi_memdesc_size; > + efi.memmap.desc_version = e->efi_memdesc_version; > > - efi.memmap = &memmap; > + memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size); > > return 0; > } > @@ -240,10 +236,14 @@ void __init efi_print_memmap(void) > > void __init efi_unmap_memmap(void) > { > + unsigned long size; > + > clear_bit(EFI_MEMMAP, &efi.flags); > - if (memmap.map) { > - early_memunmap(memmap.map, memmap.nr_map * memmap.desc_size); > - memmap.map = NULL; > + > + size = efi.memmap.nr_map * efi.memmap.desc_size; > + if (efi.memmap.map) { > + early_memunmap(efi.memmap.map, size); > + efi.memmap.map = NULL; > } > } > > @@ -432,17 +432,22 @@ static int __init efi_runtime_init(void) > > static int __init efi_memmap_init(void) > { > + unsigned long addr, size; > + > if (efi_enabled(EFI_PARAVIRT)) > return 0; > > /* Map the EFI memory map */ > - memmap.map = early_memremap((unsigned long)memmap.phys_map, > - memmap.nr_map * memmap.desc_size); > - if (memmap.map == NULL) { > + 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; > } > - memmap.map_end = memmap.map + (memmap.nr_map * memmap.desc_size); > + > + efi.memmap.map_end = efi.memmap.map + size; > > if (add_efi_memmap) > do_add_efi_memmap(); > @@ -544,7 +549,6 @@ void __init efi_set_executable(efi_memory_desc_t *md, bool executable) > void __init runtime_code_page_mkexec(void) > { > efi_memory_desc_t *md; > - void *p; > > /* Make EFI runtime service code area executable */ > for_each_efi_memory_desc(md) { > @@ -594,7 +598,6 @@ void __init old_map_region(efi_memory_desc_t *md) > /* Merge contiguous regions of the same type and attribute */ > static void __init efi_merge_regions(void) > { > - void *p; > efi_memory_desc_t *md, *prev_md = NULL; > > for_each_efi_memory_desc(md) { > @@ -640,6 +643,7 @@ static void __init get_systab_virt_addr(efi_memory_desc_t *md) > static void __init save_runtime_map(void) > { > #ifdef CONFIG_KEXEC_CORE > + unsigned long desc_size; > efi_memory_desc_t *md; > void *tmp, *q = NULL; > int count = 0; > @@ -647,21 +651,23 @@ static void __init save_runtime_map(void) > if (efi_enabled(EFI_OLD_MEMMAP)) > return; > > + desc_size = efi.memmap.desc_size; > + > for_each_efi_memory_desc(md) { > if (!(md->attribute & EFI_MEMORY_RUNTIME) || > (md->type == EFI_BOOT_SERVICES_CODE) || > (md->type == EFI_BOOT_SERVICES_DATA)) > continue; > - tmp = krealloc(q, (count + 1) * memmap.desc_size, GFP_KERNEL); > + tmp = krealloc(q, (count + 1) * desc_size, GFP_KERNEL); > if (!tmp) > goto out; > q = tmp; > > - memcpy(q + count * memmap.desc_size, md, memmap.desc_size); > + memcpy(q + count * desc_size, md, desc_size); > count++; > } > > - efi_runtime_map_setup(q, count, memmap.desc_size); > + efi_runtime_map_setup(q, count, desc_size); > return; > > out: > @@ -701,10 +707,10 @@ static inline void *efi_map_next_entry_reverse(void *entry) > { > /* Initial call */ > if (!entry) > - return memmap.map_end - memmap.desc_size; > + return efi.memmap.map_end - efi.memmap.desc_size; > > - entry -= memmap.desc_size; > - if (entry < memmap.map) > + entry -= efi.memmap.desc_size; > + if (entry < efi.memmap.map) > return NULL; > > return entry; > @@ -746,10 +752,10 @@ static void *efi_map_next_entry(void *entry) > > /* Initial call */ > if (!entry) > - return memmap.map; > + return efi.memmap.map; > > - entry += memmap.desc_size; > - if (entry >= memmap.map_end) > + entry += efi.memmap.desc_size; > + if (entry >= efi.memmap.map_end) > return NULL; > > return entry; > @@ -763,8 +769,11 @@ static void * __init efi_map_regions(int *count, int *pg_shift) > { > void *p, *new_memmap = NULL; > unsigned long left = 0; > + unsigned long desc_size; > efi_memory_desc_t *md; > > + desc_size = efi.memmap.desc_size; > + > p = NULL; > while ((p = efi_map_next_entry(p))) { > md = p; > @@ -779,7 +788,7 @@ static void * __init efi_map_regions(int *count, int *pg_shift) > efi_map_region(md); > get_systab_virt_addr(md); > > - if (left < memmap.desc_size) { > + if (left < desc_size) { > new_memmap = realloc_pages(new_memmap, *pg_shift); > if (!new_memmap) > return NULL; > @@ -788,10 +797,9 @@ static void * __init efi_map_regions(int *count, int *pg_shift) > (*pg_shift)++; > } > > - memcpy(new_memmap + (*count * memmap.desc_size), md, > - memmap.desc_size); > + memcpy(new_memmap + (*count * desc_size), md, desc_size); > > - left -= memmap.desc_size; > + left -= desc_size; > (*count)++; > } > > @@ -835,10 +843,10 @@ static void __init kexec_enter_virtual_mode(void) > > BUG_ON(!efi.systab); > > - num_pages = ALIGN(memmap.nr_map * memmap.desc_size, PAGE_SIZE); > + num_pages = ALIGN(efi.memmap.nr_map * efi.memmap.desc_size, PAGE_SIZE); > num_pages >>= PAGE_SHIFT; > > - if (efi_setup_page_tables(memmap.phys_map, num_pages)) { > + if (efi_setup_page_tables(efi.memmap.phys_map, num_pages)) { > clear_bit(EFI_RUNTIME_SERVICES, &efi.flags); > return; > } > @@ -922,16 +930,16 @@ static void __init __efi_enter_virtual_mode(void) > > if (efi_is_native()) { > status = phys_efi_set_virtual_address_map( > - memmap.desc_size * count, > - memmap.desc_size, > - memmap.desc_version, > + efi.memmap.desc_size * count, > + efi.memmap.desc_size, > + efi.memmap.desc_version, > (efi_memory_desc_t *)__pa(new_memmap)); > } else { > status = efi_thunk_set_virtual_address_map( > efi_phys.set_virtual_address_map, > - memmap.desc_size * count, > - memmap.desc_size, > - memmap.desc_version, > + efi.memmap.desc_size * count, > + efi.memmap.desc_size, > + efi.memmap.desc_version, > (efi_memory_desc_t *)__pa(new_memmap)); > } > > diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c > index 96e7fe548164..275187a86a19 100644 > --- a/drivers/firmware/efi/arm-init.c > +++ b/drivers/firmware/efi/arm-init.c > @@ -20,8 +20,6 @@ > > #include > > -struct efi_memory_map memmap; > - > u64 efi_system_table; > > static int __init is_normal_ram(efi_memory_desc_t *md) > @@ -40,7 +38,7 @@ static phys_addr_t efi_to_phys(unsigned long addr) > { > efi_memory_desc_t *md; > > - for_each_efi_memory_desc_in_map(&memmap, md) { > + for_each_efi_memory_desc(md) { > if (!(md->attribute & EFI_MEMORY_RUNTIME)) > continue; > if (md->virt_addr == 0) > @@ -145,7 +143,7 @@ static __init void reserve_regions(void) > if (efi_enabled(EFI_DBG)) > pr_info("Processing EFI memory map:\n"); > > - for_each_efi_memory_desc_in_map(&memmap, md) { > + for_each_efi_memory_desc(md) { > paddr = md->phys_addr; > npages = md->num_pages; > > @@ -186,9 +184,9 @@ void __init efi_init(void) > > efi_system_table = params.system_table; > > - memmap.phys_map = params.mmap; > - memmap.map = early_memremap_ro(params.mmap, params.mmap_size); > - if (memmap.map == NULL) { > + efi.memmap.phys_map = params.mmap; > + efi.memmap.map = early_memremap_ro(params.mmap, params.mmap_size); > + if (efi.memmap.map == NULL) { > /* > * If we are booting via UEFI, the UEFI memory map is the only > * description of memory we have, so there is little point in > @@ -196,15 +194,15 @@ void __init efi_init(void) > */ > panic("Unable to map EFI memory map.\n"); > } > - memmap.map_end = memmap.map + params.mmap_size; > - memmap.desc_size = params.desc_size; > - memmap.desc_version = params.desc_ver; > + efi.memmap.map_end = efi.memmap.map + params.mmap_size; > + efi.memmap.desc_size = params.desc_size; > + efi.memmap.desc_version = params.desc_ver; > > if (uefi_init() < 0) > return; > > reserve_regions(); > - early_memunmap(memmap.map, params.mmap_size); > + early_memunmap(efi.memmap.map, params.mmap_size); > memblock_mark_nomap(params.mmap & PAGE_MASK, > PAGE_ALIGN(params.mmap_size + > (params.mmap & ~PAGE_MASK))); > diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c > index 1cfbfaf57a2d..0416d5d33e74 100644 > --- a/drivers/firmware/efi/arm-runtime.c > +++ b/drivers/firmware/efi/arm-runtime.c > @@ -89,6 +89,7 @@ static bool __init efi_virtmap_init(void) > */ > static int __init arm_enable_runtime_services(void) > { > + phys_addr_t phys_map; Is the sole purpose of this variable to prevent breaking the 80-column rule? If so, please be aware that I intend to propose a patch that replaces the ioremap_cache() below with a call to memremap(), but this is another change that is gated by Russell merging my memremap patches for ARM > u64 mapsize; > > if (!efi_enabled(EFI_BOOT)) { > @@ -103,15 +104,15 @@ static int __init arm_enable_runtime_services(void) > > pr_info("Remapping and enabling EFI services.\n"); > > - mapsize = memmap.map_end - memmap.map; > - memmap.map = (__force void *)ioremap_cache(memmap.phys_map, > - mapsize); > - if (!memmap.map) { > + mapsize = efi.memmap.map_end - efi.memmap.map; > + phys_map = efi.memmap.phys_map; > + > + efi.memmap.map = (__force void *)ioremap_cache(phys_map, mapsize); > + if (!efi.memmap.map) { > pr_err("Failed to remap EFI memory map\n"); > return -ENOMEM; > } > - memmap.map_end = memmap.map + mapsize; > - efi.memmap = &memmap; > + efi.memmap.map_end = efi.memmap.map + mapsize; > > if (!efi_virtmap_init()) { > pr_err("UEFI virtual mapping missing or invalid -- runtime services will not be available\n"); > diff --git a/drivers/firmware/efi/fake_mem.c b/drivers/firmware/efi/fake_mem.c > index f55b75b2e1f4..48430aba13c1 100644 > --- a/drivers/firmware/efi/fake_mem.c > +++ b/drivers/firmware/efi/fake_mem.c > @@ -57,7 +57,7 @@ static int __init cmp_fake_mem(const void *x1, const void *x2) > void __init efi_fake_memmap(void) > { > u64 start, end, m_start, m_end, m_attr; > - int new_nr_map = memmap.nr_map; > + int new_nr_map = efi.memmap.nr_map; > efi_memory_desc_t *md; > phys_addr_t new_memmap_phy; > void *new_memmap; > @@ -94,25 +94,25 @@ void __init efi_fake_memmap(void) > } > > /* allocate memory for new EFI memmap */ > - new_memmap_phy = memblock_alloc(memmap.desc_size * new_nr_map, > + new_memmap_phy = memblock_alloc(efi.memmap.desc_size * new_nr_map, > PAGE_SIZE); > if (!new_memmap_phy) > return; > > /* create new EFI memmap */ > new_memmap = early_memremap(new_memmap_phy, > - memmap.desc_size * new_nr_map); > + efi.memmap.desc_size * new_nr_map); > if (!new_memmap) { > - memblock_free(new_memmap_phy, memmap.desc_size * new_nr_map); > + memblock_free(new_memmap_phy, efi.memmap.desc_size * new_nr_map); > return; > } > > - for (old = memmap.map, new = new_memmap; > - old < memmap.map_end; > - old += memmap.desc_size, new += memmap.desc_size) { > + for (old = efi.memmap.map, new = new_memmap; > + old < efi.memmap.map_end; > + old += efi.memmap.desc_size, new += efi.memmap.desc_size) { > > /* copy original EFI memory descriptor */ > - memcpy(new, old, memmap.desc_size); > + memcpy(new, old, efi.memmap.desc_size); > md = new; > start = md->phys_addr; > end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1; > @@ -133,8 +133,8 @@ void __init efi_fake_memmap(void) > md->num_pages = (m_end - md->phys_addr + 1) >> > EFI_PAGE_SHIFT; > /* latter part */ > - new += memmap.desc_size; > - memcpy(new, old, memmap.desc_size); > + new += efi.memmap.desc_size; > + memcpy(new, old, efi.memmap.desc_size); > md = new; > md->phys_addr = m_end + 1; > md->num_pages = (end - md->phys_addr + 1) >> > @@ -146,16 +146,16 @@ void __init efi_fake_memmap(void) > md->num_pages = (m_start - md->phys_addr) >> > EFI_PAGE_SHIFT; > /* middle part */ > - new += memmap.desc_size; > - memcpy(new, old, memmap.desc_size); > + new += efi.memmap.desc_size; > + memcpy(new, old, efi.memmap.desc_size); > md = new; > md->attribute |= m_attr; > md->phys_addr = m_start; > md->num_pages = (m_end - m_start + 1) >> > EFI_PAGE_SHIFT; > /* last part */ > - new += memmap.desc_size; > - memcpy(new, old, memmap.desc_size); > + new += efi.memmap.desc_size; > + memcpy(new, old, efi.memmap.desc_size); > md = new; > md->phys_addr = m_end + 1; > md->num_pages = (end - m_end) >> > @@ -168,8 +168,8 @@ void __init efi_fake_memmap(void) > md->num_pages = (m_start - md->phys_addr) >> > EFI_PAGE_SHIFT; > /* latter part */ > - new += memmap.desc_size; > - memcpy(new, old, memmap.desc_size); > + new += efi.memmap.desc_size; > + memcpy(new, old, efi.memmap.desc_size); > md = new; > md->phys_addr = m_start; > md->num_pages = (end - md->phys_addr + 1) >> > @@ -181,10 +181,10 @@ void __init efi_fake_memmap(void) > > /* swap into new EFI memmap */ > efi_unmap_memmap(); > - memmap.map = new_memmap; > - memmap.phys_map = new_memmap_phy; > - memmap.nr_map = new_nr_map; > - memmap.map_end = memmap.map + memmap.nr_map * memmap.desc_size; > + efi.memmap.map = new_memmap; > + efi.memmap.phys_map = new_memmap_phy; > + efi.memmap.nr_map = new_nr_map; > + efi.memmap.map_end = efi.memmap.map + efi.memmap.nr_map * efi.memmap.desc_size; > set_bit(EFI_MEMMAP, &efi.flags); > > /* print new EFI memmap */ > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 942b13863790..c2c0da49876e 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -883,7 +883,7 @@ extern struct efi { > efi_get_next_high_mono_count_t *get_next_high_mono_count; > efi_reset_system_t *reset_system; > efi_set_virtual_address_map_t *set_virtual_address_map; > - struct efi_memory_map *memmap; > + struct efi_memory_map memmap; > unsigned long flags; > } efi; > > @@ -945,7 +945,6 @@ extern void efi_initialize_iomem_resources(struct resource *code_resource, > extern void efi_get_time(struct timespec *now); > extern void efi_reserve_boot_services(void); > extern int efi_get_fdt_params(struct efi_fdt_params *params); > -extern struct efi_memory_map memmap; > extern struct kobject *efi_kobj; > > extern int efi_reboot_quirk_mode; > @@ -964,13 +963,13 @@ static inline void efi_fake_memmap(void) { } > (md) = (void *)(md) + (m)->desc_size) > > /** > - * for_each_efi_memory_desc - iterate over descriptors in efi->memmap > + * for_each_efi_memory_desc - iterate over descriptors in efi.memmap > * @md: the efi_memory_desc_t * iterator > * > * Once the loop finishes @md must not be accessed. > */ > #define for_each_efi_memory_desc(md) \ > - for_each_efi_memory_desc_in_map(efi->memmap, md) > + for_each_efi_memory_desc_in_map(&efi.memmap, md) > > /* > * Format an EFI memory descriptor's type and attributes to a user-provided > -- > 2.7.3 >