From: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: linux-efi@vger.kernel.org, stable@vger.kernel.org,
"Tyler Hicks" <code@tyhicks.com>,
"Brian Nguyen" <nguyenbrian@microsoft.com>,
"Jacob Pan" <panj@microsoft.com>,
"Allen Pais" <apais@microsoft.com>,
"Jonathan Marek" <jonathan@marek.ca>,
"Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
"Kuppuswamy Sathyanarayanan"
<sathyanarayanan.kuppuswamy@linux.intel.com>,
"Jeremy Linton" <jeremy.linton@arm.com>,
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
"KONDO KAZUMA(近藤 和真)" <kazuma-kondo@nec.com>,
"Kees Cook" <kees@kernel.org>,
"Borislav Petkov (AMD)" <bp@alien8.de>,
"Yuntao Wang" <ytcoode@gmail.com>,
"Aditya Garg" <gargaditya08@live.com>,
"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] efi: make the min and max mmap slack slots configurable
Date: Mon, 9 Dec 2024 12:02:13 -0500 [thread overview]
Message-ID: <380bbf4b-0828-444e-9b93-fa639cc23a1a@linux.microsoft.com> (raw)
In-Reply-To: <CAMj1kXF=f-QAhXJA=U=jbn++Vyzf+e2k+cCS+Jk4Om4p0puD5Q@mail.gmail.com>
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@linux.microsoft.com> 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@vger.kernel.org
>> Cc: Tyler Hicks <code@tyhicks.com>
>> Tested-by: Brian Nguyen <nguyenbrian@microsoft.com>
>> Tested-by: Jacob Pan <panj@microsoft.com>
>> Reviewed-by: Allen Pais <apais@microsoft.com>
>
> 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@linux.microsoft.com>
>> ---
>> 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
>>
next prev parent reply other threads:[~2024-12-09 17:02 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-09 16:24 [PATCH] efi: make the min and max mmap slack slots configurable Hamza Mahfooz
2024-12-09 16:40 ` Ard Biesheuvel
2024-12-09 17:02 ` Hamza Mahfooz [this message]
2024-12-09 18:00 ` Ard Biesheuvel
2024-12-09 18:03 ` Hamza Mahfooz
2024-12-11 23:12 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=380bbf4b-0828-444e-9b93-fa639cc23a1a@linux.microsoft.com \
--to=hamzamahfooz@linux.microsoft.com \
--cc=apais@microsoft.com \
--cc=ardb@kernel.org \
--cc=bp@alien8.de \
--cc=code@tyhicks.com \
--cc=gargaditya08@live.com \
--cc=ilias.apalodimas@linaro.org \
--cc=jeremy.linton@arm.com \
--cc=jonathan@marek.ca \
--cc=kazuma-kondo@nec.com \
--cc=kees@kernel.org \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nguyenbrian@microsoft.com \
--cc=panj@microsoft.com \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=stable@vger.kernel.org \
--cc=ytcoode@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.