On Tue, 20 Sept 2022 at 12:41, Joey Gouly <joey.gouly@xxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > ---
> > 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;
>
Yeah that might be better. Thanks for the suggestion.
