| Message ID | 20220918213544.2176249-10-ardb@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: disentangle the generic EFI stub from FDT | expand |
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
On Tue, 20 Sept 2022 at 12:41, Joey Gouly <joey.gouly@arm.com> 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@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; > Yeah that might be better. Thanks for the suggestion.
diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c index e9d516ad5f97..df05c53baa23 100644 --- a/drivers/firmware/efi/libstub/arm64-stub.c +++ b/drivers/firmware/efi/libstub/arm64-stub.c @@ -55,7 +55,7 @@ static bool check_image_region(u64 base, u64 size) bool ret = false; int map_offset; - status = efi_get_memory_map(&map); + status = efi_get_memory_map(&map, false); if (status != EFI_SUCCESS) return false; diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index 742ecd9e0df3..f1f7a823be17 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -438,7 +438,7 @@ efi_status_t efi_exit_boot_services(void *handle, void *priv, struct efi_boot_memmap *map; efi_status_t status; - status = efi_get_memory_map(&map); + status = efi_get_memory_map(&map, true); if (status != EFI_SUCCESS) goto fail; diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index f65a8a3844b8..b4fb6d4e8a59 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -897,7 +897,8 @@ void efi_apply_loadoptions_quirk(const void **load_options, u32 *load_options_si char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len); -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); efi_status_t efi_allocate_pages(unsigned long size, unsigned long *addr, unsigned long max); 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); } diff --git a/drivers/firmware/efi/libstub/randomalloc.c b/drivers/firmware/efi/libstub/randomalloc.c index 5d6000c717cc..9fb5869896be 100644 --- a/drivers/firmware/efi/libstub/randomalloc.c +++ b/drivers/firmware/efi/libstub/randomalloc.c @@ -61,7 +61,7 @@ efi_status_t efi_random_alloc(unsigned long size, efi_status_t status; int map_offset; - status = efi_get_memory_map(&map); + status = efi_get_memory_map(&map, false); if (status != EFI_SUCCESS) return status; diff --git a/drivers/firmware/efi/libstub/relocate.c b/drivers/firmware/efi/libstub/relocate.c index cd80db33ab1e..bf6fbd5d22a1 100644 --- a/drivers/firmware/efi/libstub/relocate.c +++ b/drivers/firmware/efi/libstub/relocate.c @@ -28,7 +28,7 @@ efi_status_t efi_low_alloc_above(unsigned long size, unsigned long align, unsigned long nr_pages; int i; - status = efi_get_memory_map(&map); + status = efi_get_memory_map(&map, false); if (status != EFI_SUCCESS) goto fail; diff --git a/include/linux/efi.h b/include/linux/efi.h index 8668d9769bb0..89f16ec3ebab 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -414,6 +414,7 @@ void efi_native_runtime_setup(void); #define LINUX_EFI_ZBOOT_MEDIA_GUID EFI_GUID(0xe565a30d, 0x47da, 0x4dbd, 0xb3, 0x54, 0x9b, 0xb5, 0xc8, 0x4f, 0x8b, 0xe2) #define LINUX_EFI_MOK_VARIABLE_TABLE_GUID EFI_GUID(0xc451ed2b, 0x9694, 0x45d3, 0xba, 0xba, 0xed, 0x9f, 0x89, 0x88, 0xa3, 0x89) #define LINUX_EFI_COCO_SECRET_AREA_GUID EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47) +#define LINUX_EFI_BOOT_MEMMAP_GUID EFI_GUID(0x800f683f, 0xd08b, 0x423a, 0xa2, 0x93, 0x96, 0x5c, 0x3c, 0x6f, 0xe2, 0xb4) #define RISCV_EFI_BOOT_PROTOCOL_GUID EFI_GUID(0xccd15fec, 0x6f73, 0x4eec, 0x83, 0x95, 0x3e, 0x69, 0xe4, 0xb9, 0x40, 0xbf)
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(-)