Hi Ard,
On 12/9/24 11:40, Ard Biesheuvel wrote:
> Hello Hamza,
>
> Thanks for the patch.
>
> On Mon, 9 Dec 2024 at 17:25, Hamza Mahfooz
> <hamzamahfooz@xxxxxxxxxxxxxxxxxxx> wrote:
>>
>> Recent platforms
>
> Which platforms are you referring to here?
Grace Blackwell 200 in particular.
>
>> require more slack slots than the current value of
>> EFI_MMAP_NR_SLACK_SLOTS, otherwise they fail to boot. So, introduce
>> EFI_MIN_NR_MMAP_SLACK_SLOTS and EFI_MAX_NR_MMAP_SLACK_SLOTS
>> and use them to determine a number of slots that the platform
>> is willing to accept.
>>
>
> What does 'acceptance' mean in this case?
Not having allocate_pool() return EFI_BUFFER_TOO_SMALL.
>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Cc: Tyler Hicks <code@xxxxxxxxxxx>
>> Tested-by: Brian Nguyen <nguyenbrian@xxxxxxxxxxxxx>
>> Tested-by: Jacob Pan <panj@xxxxxxxxxxxxx>
>> Reviewed-by: Allen Pais <apais@xxxxxxxxxxxxx>
>
> I appreciate the effort of your colleagues, but if these
> tested/reviewed-bys were not given on an open list, they are
> meaningless, and I am going to drop them unless the people in question
> reply to this thread.
>
>> Signed-off-by: Hamza Mahfooz <hamzamahfooz@xxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/firmware/efi/Kconfig | 23 +++++++++++++++++
>> .../firmware/efi/libstub/efi-stub-helper.c | 2 +-
>> drivers/firmware/efi/libstub/efistub.h | 15 +----------
>> drivers/firmware/efi/libstub/kaslr.c | 2 +-
>> drivers/firmware/efi/libstub/mem.c | 25 +++++++++++++++----
>> drivers/firmware/efi/libstub/randomalloc.c | 2 +-
>> drivers/firmware/efi/libstub/relocate.c | 2 +-
>> drivers/firmware/efi/libstub/x86-stub.c | 8 +++---
>> 8 files changed, 52 insertions(+), 27 deletions(-)
>>
>
> This looks rather intrusive for a patch that is intended as cc:stable.
>
>> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
>> index e312d731f4a3..7fedc271d543 100644
>> --- a/drivers/firmware/efi/Kconfig
>> +++ b/drivers/firmware/efi/Kconfig
>> @@ -155,6 +155,29 @@ config EFI_TEST
>> Say Y here to enable the runtime services support via /dev/efi_test.
>> If unsure, say N.
>>
>> +#
>> +# An efi_boot_memmap is used by efi_get_memory_map() to return the
>> +# EFI memory map in a dynamically allocated buffer.
>> +#
>> +# The buffer allocated for the EFI memory map includes extra room for
>> +# a range of [EFI_MIN_NR_MMAP_SLACK_SLOTS, EFI_MAX_NR_MMAP_SLACK_SLOTS]
>> +# additional EFI memory descriptors. This facilitates the reuse of the
>> +# EFI memory map buffer when a second call to ExitBootServices() is
>> +# needed because of intervening changes to the EFI memory map. Other
>> +# related structures, e.g. x86 e820ext, need to factor in this headroom
>> +# requirement as well.
>> +#
>> +
>> +config EFI_MIN_NR_MMAP_SLACK_SLOTS
>> + int
>> + depends on EFI
>> + default 8
>> +
>> +config EFI_MAX_NR_MMAP_SLACK_SLOTS
>> + int
>> + depends on EFI
>> + default 64
>> +
>
> What would be the reason for not always bumping this to 64?
I personally don't mind always bumping it up, but it seemed to
me like it might regress existing platforms if we did that.
>
>> config EFI_DEV_PATH_PARSER
>> bool
>>
>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> index c0c81ca4237e..adf2b0c0dd34 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -432,7 +432,7 @@ efi_status_t efi_exit_boot_services(void *handle, void *priv,
>> if (efi_disable_pci_dma)
>> efi_pci_disable_bridge_busmaster();
>>
>> - status = efi_get_memory_map(&map, true);
>> + status = efi_get_memory_map(&map, true, NULL);
>> if (status != EFI_SUCCESS)
>> return status;
>>
>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>> index 76e44c185f29..d86c6e13de5f 100644
>> --- a/drivers/firmware/efi/libstub/efistub.h
>> +++ b/drivers/firmware/efi/libstub/efistub.h
>> @@ -160,19 +160,6 @@ void efi_set_u64_split(u64 data, u32 *lo, u32 *hi)
>> */
>> #define EFI_100NSEC_PER_USEC ((u64)10)
>>
>> -/*
>> - * An efi_boot_memmap is used by efi_get_memory_map() to return the
>> - * EFI memory map in a dynamically allocated buffer.
>> - *
>> - * The buffer allocated for the EFI memory map includes extra room for
>> - * a minimum of EFI_MMAP_NR_SLACK_SLOTS additional EFI memory descriptors.
>> - * This facilitates the reuse of the EFI memory map buffer when a second
>> - * call to ExitBootServices() is needed because of intervening changes to
>> - * the EFI memory map. Other related structures, e.g. x86 e820ext, need
>> - * to factor in this headroom requirement as well.
>> - */
>> -#define EFI_MMAP_NR_SLACK_SLOTS 8
>> -
>> typedef struct efi_generic_dev_path efi_device_path_protocol_t;
>>
>> union efi_device_path_to_text_protocol {
>> @@ -1059,7 +1046,7 @@ void efi_apply_loadoptions_quirk(const void **load_options, u32 *load_options_si
>> char *efi_convert_cmdline(efi_loaded_image_t *image);
>>
>> efi_status_t efi_get_memory_map(struct efi_boot_memmap **map,
>> - bool install_cfg_tbl);
>> + bool install_cfg_tbl, unsigned int *n);
>>
>> efi_status_t efi_allocate_pages(unsigned long size, unsigned long *addr,
>> unsigned long max);
>> diff --git a/drivers/firmware/efi/libstub/kaslr.c b/drivers/firmware/efi/libstub/kaslr.c
>> index 6318c40bda38..06e7a1ef34ab 100644
>> --- a/drivers/firmware/efi/libstub/kaslr.c
>> +++ b/drivers/firmware/efi/libstub/kaslr.c
>> @@ -62,7 +62,7 @@ static bool check_image_region(u64 base, u64 size)
>> bool ret = false;
>> int map_offset;
>>
>> - status = efi_get_memory_map(&map, false);
>> + status = efi_get_memory_map(&map, false, NULL);
>> if (status != EFI_SUCCESS)
>> return false;
>>
>> diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
>> index 4f1fa302234d..cab25183b790 100644
>> --- a/drivers/firmware/efi/libstub/mem.c
>> +++ b/drivers/firmware/efi/libstub/mem.c
>> @@ -13,32 +13,47 @@
>> * configuration table
>> *
>> * Retrieve the UEFI memory map. The allocated memory leaves room for
>> - * up to EFI_MMAP_NR_SLACK_SLOTS additional memory map entries.
>> + * up to CONFIG_EFI_MAX_NR_MMAP_SLACK_SLOTS additional memory map entries.
>> *
>> * Return: status code
>> */
>> efi_status_t efi_get_memory_map(struct efi_boot_memmap **map,
>> - bool install_cfg_tbl)
>> + bool install_cfg_tbl,
>> + unsigned int *n)
>
> What is the purpose of 'n'? Having single letter names for function
> parameters is not great for legibility.
>
>> {
>> int memtype = install_cfg_tbl ? EFI_ACPI_RECLAIM_MEMORY
>> : EFI_LOADER_DATA;
>> efi_guid_t tbl_guid = LINUX_EFI_BOOT_MEMMAP_GUID;
>> + unsigned int nr = CONFIG_EFI_MIN_NR_MMAP_SLACK_SLOTS;
>> struct efi_boot_memmap *m, tmp;
>> efi_status_t status;
>> unsigned long size;
>>
>> + BUILD_BUG_ON(!is_power_of_2(CONFIG_EFI_MIN_NR_MMAP_SLACK_SLOTS) ||
>> + !is_power_of_2(CONFIG_EFI_MAX_NR_MMAP_SLACK_SLOTS) ||
>> + CONFIG_EFI_MIN_NR_MMAP_SLACK_SLOTS >=
>> + CONFIG_EFI_MAX_NR_MMAP_SLACK_SLOTS);
>> +
>> tmp.map_size = 0;
>> status = efi_bs_call(get_memory_map, &tmp.map_size, NULL, &tmp.map_key,
>> &tmp.desc_size, &tmp.desc_ver);
>> if (status != EFI_BUFFER_TOO_SMALL)
>> return EFI_LOAD_ERROR;
>>
>> - size = tmp.map_size + tmp.desc_size * EFI_MMAP_NR_SLACK_SLOTS;
>> - status = efi_bs_call(allocate_pool, memtype, sizeof(*m) + size,
>> - (void **)&m);
>> + do {
>> + size = tmp.map_size + tmp.desc_size * nr;
>> + status = efi_bs_call(allocate_pool, memtype, sizeof(*m) + size,
>> + (void **)&m);
>> + nr <<= 1;
>> + } while (status == EFI_BUFFER_TOO_SMALL &&
>> + nr <= CONFIG_EFI_MAX_NR_MMAP_SLACK_SLOTS);
>> +
>
> Under what circumstances would you expect AllocatePool() to return
> EFI_BUFFER_TOO_SMALL? What is the purpose of this loop?
We have observed that allocate_pool() will return EFI_BUFFER_TOO_SMALL
if EFI_MMAP_NR_SLACK_SLOTS is less than 32. The loop is there so only
the minimum number of extra slots are allocated.
>
> How did you test this code?
I was able to successfully boot the platform with this patch applied,
without it we need to append `efi=disable_early_pci_dma` to the kernel's
cmdline be able to boot the system.
>
>> if (status != EFI_SUCCESS)
>> return status;
>>
>> + if (n)
>> + *n = nr;
>> +
>> if (install_cfg_tbl) {
>> /*
>> * Installing a configuration table might allocate memory, and
>> diff --git a/drivers/firmware/efi/libstub/randomalloc.c b/drivers/firmware/efi/libstub/randomalloc.c
>> index c41e7b2091cd..e80a65e7b87a 100644
>> --- a/drivers/firmware/efi/libstub/randomalloc.c
>> +++ b/drivers/firmware/efi/libstub/randomalloc.c
>> @@ -65,7 +65,7 @@ efi_status_t efi_random_alloc(unsigned long size,
>> efi_status_t status;
>> int map_offset;
>>
>> - status = efi_get_memory_map(&map, false);
>> + status = efi_get_memory_map(&map, false, NULL);
>> if (status != EFI_SUCCESS)
>> return status;
>>
>> diff --git a/drivers/firmware/efi/libstub/relocate.c b/drivers/firmware/efi/libstub/relocate.c
>> index d694bcfa1074..b7b0aad95ba4 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, false);
>> + status = efi_get_memory_map(&map, false, NULL);
>> if (status != EFI_SUCCESS)
>> goto fail;
>>
>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>> index 188c8000d245..cb14f0d2a3d9 100644
>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>> @@ -740,15 +740,15 @@ static efi_status_t allocate_e820(struct boot_params *params,
>> struct efi_boot_memmap *map;
>> efi_status_t status;
>> __u32 nr_desc;
>> + __u32 nr;
>>
>> - status = efi_get_memory_map(&map, false);
>> + status = efi_get_memory_map(&map, false, &nr);
>> if (status != EFI_SUCCESS)
>> return status;
>>
>> nr_desc = map->map_size / map->desc_size;
>> - if (nr_desc > ARRAY_SIZE(params->e820_table) - EFI_MMAP_NR_SLACK_SLOTS) {
>> - u32 nr_e820ext = nr_desc - ARRAY_SIZE(params->e820_table) +
>> - EFI_MMAP_NR_SLACK_SLOTS;
>> + if (nr_desc > ARRAY_SIZE(params->e820_table) - nr) {
>> + u32 nr_e820ext = nr_desc - ARRAY_SIZE(params->e820_table) + nr;
>>
>> status = alloc_e820ext(nr_e820ext, e820ext, e820ext_size);
>> }
>> --
>> 2.47.1
>>
