From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Matt Fleming <matt@codeblueprint.co.uk>
Cc: "linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Tony Luck <tony.luck@gmail.com>,
"Luck, Tony" <tony.luck@intel.com>
Subject: Re: [PATCH 2/2] efi: Remove global 'memmap'
Date: Mon, 11 Apr 2016 15:17:55 +0200 [thread overview]
Message-ID: <CAKv+Gu-xUzbxzpTa_E_HjZcSXW+DdhasFDP1n-w-AzuDjvp6FA@mail.gmail.com> (raw)
In-Reply-To: <1460379809-18490-3-git-send-email-matt@codeblueprint.co.uk>
On 11 April 2016 at 15:03, Matt Fleming <matt@codeblueprint.co.uk> 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 <ard.biesheuvel@linaro.org>
> Cc: "Luck, Tony" <tony.luck@intel.com>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
> 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 <asm/efi.h>
>
> -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
>
next prev parent reply other threads:[~2016-04-11 13:17 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-11 13:03 [PATCH 0/2] efi: Delete global 'memmap' variable Matt Fleming
2016-04-11 13:03 ` Matt Fleming
2016-04-11 13:03 ` [PATCH 1/2] efi: Iterate over efi->memmap in for_each_efi_memory_desc Matt Fleming
2016-04-11 13:03 ` Matt Fleming
2016-04-11 13:24 ` kbuild test robot
2016-04-11 13:24 ` kbuild test robot
2016-04-11 13:27 ` kbuild test robot
2016-04-11 13:27 ` kbuild test robot
2016-04-11 13:03 ` [PATCH 2/2] efi: Remove global 'memmap' Matt Fleming
2016-04-11 13:03 ` Matt Fleming
2016-04-11 13:17 ` Ard Biesheuvel [this message]
2016-04-12 20:01 ` Matt Fleming
2016-04-12 20:01 ` Matt Fleming
2016-04-13 8:12 ` Ard Biesheuvel
2016-04-13 8:12 ` Ard Biesheuvel
2016-04-13 10:24 ` Matt Fleming
2016-04-13 10:24 ` Matt Fleming
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu-xUzbxzpTa_E_HjZcSXW+DdhasFDP1n-w-AzuDjvp6FA@mail.gmail.com \
--to=ard.biesheuvel@linaro.org \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matt@codeblueprint.co.uk \
--cc=tony.luck@gmail.com \
--cc=tony.luck@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.