Re: [PATCH 1/6] efi: Introduce efi_memmap_free() to free memory allocated by efi_memmap_alloc()

[Ard Biesheuvel]

On 3 July 2018 at 03:50, Sai Praneeth Prakhya
<sai.praneeth.prakhya@xxxxxxxxx> wrote:
> From: Sai Praneeth <sai.praneeth.prakhya@xxxxxxxxx>
>
> efi_memmap_alloc() allocates memory depending on whether mm_init() has
> already been invoked or not. Apart from memblock_alloc() memory and
> alloc_pages() memory, efi memory map could also have a third variant of
> memory allocation and that is memblock_reserved. This happens only for
> the memory map passed to kernel by firmware and thus can happen only
> once during boot process.
>
> In order to identify these three different types of allocations and thus
> to call the appropriate free() variant, introduce an enum named
> efi_memmap_type and also introduce a efi memmap API named
> efi_memmap_free() to free memory allocated by efi_memmap_alloc().
>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx>
> Suggested-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> Cc: Lee Chun-Yi <jlee@xxxxxxxx>
> Cc: Dave Young <dyoung@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Laszlo Ersek <lersek@xxxxxxxxxx>
> Cc: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
> Cc: Bhupesh Sharma <bhsharma@xxxxxxxxxx>
> Cc: Nicolai Stange <nicstange@xxxxxxxxx>
> Cc: Naresh Bhat <naresh.bhat@xxxxxxxxxx>
> Cc: Ricardo Neri <ricardo.neri@xxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Taku Izumi <izumi.taku@xxxxxxxxxxxxxx>
> Cc: Ravi Shankar <ravi.v.shankar@xxxxxxxxx>
> Cc: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>

Do you really think you should cc all these people?

> ---
>  drivers/firmware/efi/memmap.c | 28 ++++++++++++++++++++++++++++
>  include/linux/efi.h           |  8 ++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> index 5fc70520e04c..0686e063c644 100644
> --- a/drivers/firmware/efi/memmap.c
> +++ b/drivers/firmware/efi/memmap.c
> @@ -12,6 +12,7 @@
>  #include <asm/early_ioremap.h>
>  #include <linux/memblock.h>
>  #include <linux/slab.h>
> +#include <linux/bootmem.h>
>
>  static phys_addr_t __init __efi_memmap_alloc_early(unsigned long size)
>  {
> @@ -50,6 +51,33 @@ phys_addr_t __init efi_memmap_alloc(unsigned int num_entries)
>  }
>
>  /**
> + * efi_memmap_free - Free memory allocated by efi_memmap_alloc()
> + * @mem: Physical address allocated by efi_memmap_alloc().
> + * @num_entries: Number of entries in the allocated map.
> + * @alloc_type: What type of allocation did efi_memmap_alloc() perform?
> + *
> + * Use this function to free memory allocated by efi_memmap_alloc().
> + * efi_memmap_alloc() allocates memory depending on whether mm_init()
> + * has already been invoked or not. It uses either memblock or "normal"
> + * page allocation, similarly, we free it in two different ways. Also
> + * note that there is a third type of memory used by memmap which is
> + * memblock_reserved() and is passed by EFI stub to kernel.
> + */
> +void __init efi_memmap_free(phys_addr_t mem, unsigned int num_entries,
> +                           enum efi_memmap_type alloc_type)
> +{
> +       unsigned long size = num_entries * efi.memmap.desc_size;
> +       unsigned int order = get_order(size);
> +
> +       if (alloc_type == BUDDY_ALLOCATOR)
> +               __free_pages(pfn_to_page(PHYS_PFN(mem)), order);
> +       else if (alloc_type == MEMBLOCK)
> +               memblock_free(mem, size);
> +       else
> +               free_bootmem(mem, size);
> +}
> +
> +/**
>   * __efi_memmap_init - Common code for mapping the EFI memory map
>   * @data: EFI memory map data
>   * @late: Use early or late mapping function?
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 56add823f190..455875c01ed1 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -765,6 +765,12 @@ struct efi_memory_map_data {
>         unsigned long desc_size;
>  };
>
> +enum efi_memmap_type {
> +       EFI_STUB,
> +       MEMBLOCK,
> +       BUDDY_ALLOCATOR,
> +};
> +
>  struct efi_memory_map {
>         phys_addr_t phys_map;
>         void *map;
> @@ -1016,6 +1022,8 @@ extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
>                                          struct range *range);
>  extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
>                                      void *buf, struct efi_mem_range *mem);
> +extern void __init efi_memmap_free(phys_addr_t mem, unsigned int num_entries,
> +                                  enum efi_memmap_type alloc_type);
>
>  extern int efi_config_init(efi_config_table_type_t *arch_tables);
>  #ifdef CONFIG_EFI_ESRT
> --
> 2.7.4
>

Sai,

Thanks a lot for respinning all of this.

I am now thinking we should perhaps hide the allocation type from the
callers rather than passing around implementation details of the
allocator, and have explicit alloc/free operations.

Could we instead have a single 'efi_realloc_memmap()' function that
takes care of all of this? We'd still need a free routine for error
handling, I suppose, but it would get rid of the need for obscure
enums or bools that the callers have to pass around.