Re: [PATCH 09/12] efi: libstub: install boot-time memory map as config table

[Joey Gouly <joey.gouly@arm.com>]

Hi Ard,

On Sun, Sep 18, 2022 at 11:35:41PM +0200, Ard Biesheuvel wrote:
> Expose the EFI boot time memory map to the kernel via a configuration
> table. This is arch agnostic and enables future changes that remove the
> dependency on DT on architectures that don't otherwise rely on it.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/libstub/arm64-stub.c      |  2 +-
>  drivers/firmware/efi/libstub/efi-stub-helper.c |  2 +-
>  drivers/firmware/efi/libstub/efistub.h         |  3 ++-
>  drivers/firmware/efi/libstub/mem.c             | 26 ++++++++++++++++++--
>  drivers/firmware/efi/libstub/randomalloc.c     |  2 +-
>  drivers/firmware/efi/libstub/relocate.c        |  2 +-
>  include/linux/efi.h                            |  1 +
>  7 files changed, 31 insertions(+), 7 deletions(-)
> 
[..]
> diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
> index 40721573e494..ed4c145afe11 100644
> --- a/drivers/firmware/efi/libstub/mem.c
> +++ b/drivers/firmware/efi/libstub/mem.c
> @@ -9,14 +9,20 @@
>   * efi_get_memory_map() - get memory map
>   * @map:		pointer to memory map pointer to which to assign the
>   *			newly allocated memory map
> + * @install_cfg_tbl:	whether or not to install the boot memory map as a
> + *			configuration table
>   *
>   * Retrieve the UEFI memory map. The allocated memory leaves room for
>   * up to EFI_MMAP_NR_SLACK_SLOTS additional memory map entries.
>   *
>   * Return:	status code
>   */
> -efi_status_t efi_get_memory_map(struct efi_boot_memmap **map)
> +efi_status_t efi_get_memory_map(struct efi_boot_memmap **map,
> +				bool install_cfg_tbl)
>  {
> +	int memtype = install_cfg_tbl ? EFI_ACPI_RECLAIM_MEMORY
> +				      : EFI_LOADER_DATA;
> +	efi_guid_t tbl_guid = LINUX_EFI_BOOT_MEMMAP_GUID;
>  	struct efi_boot_memmap *m, tmp;
>  	efi_status_t status;
>  	unsigned long size;
> @@ -28,11 +34,23 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap **map)
>  		return EFI_LOAD_ERROR;
>  
>  	size = tmp.map_size + tmp.desc_size * EFI_MMAP_NR_SLACK_SLOTS;
> -	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, sizeof(*m) + size,
> +	status = efi_bs_call(allocate_pool, memtype, sizeof(*m) + size,
>  			     (void **)&m);
>  	if (status != EFI_SUCCESS)
>  		return status;
>  
> +	if (install_cfg_tbl) {
> +		/*
> +		 * Installing a configuration table might allocate memory, and
> +		 * this may modify the memory map. This means we should install
> +		 * the configuration table first, and re-install or delete it
> +		 * as needed.
> +		 */
> +		status = efi_bs_call(install_configuration_table, &tbl_guid, m);
> +		if (status != EFI_SUCCESS)
> +			goto free_map;
> +	}
> +
>  	m->buff_size = m->map_size = size;
>  	status = efi_bs_call(get_memory_map, &m->map_size, m->map, &m->map_key,
>  			     &m->desc_size, &m->desc_ver);
> @@ -40,6 +58,10 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap **map)
>  	if (status == EFI_SUCCESS) {
>  		*map = m;
>  	} else {
> +		if (install_cfg_tbl)
> +			efi_bs_call(install_configuration_table, &tbl_guid,
> +				    NULL);
> +free_map:
>  		efi_bs_call(free_pool, m);
>  	}

You have another commit about removing goto kludges, so maybe write this like the following,
rather than a goto into an 'else' statement?

diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
index feef8d4be113..2f22ef7c5232 100644
--- a/drivers/firmware/efi/libstub/mem.c
+++ b/drivers/firmware/efi/libstub/mem.c
@@ -64,10 +64,12 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap *map)
                        *map->key_ptr = key;
                if (map->desc_ver)
                        *map->desc_ver = desc_version;
-       } else {
-               efi_bs_call(free_pool, m);
        }
 
+free_map:
+       if (status != EFI_SUCCESS)
+               efi_bs_call(free_pool, m);
+
 fail:
        *map->map = m;
        return status;

(This is a diff against next-20220920, not your branch, but you get the idea)

Thanks,
Joey