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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox