* [PATCH] efi: make the min and max mmap slack slots configurable
@ 2024-12-09 16:24 Hamza Mahfooz
2024-12-09 16:40 ` Ard Biesheuvel
2024-12-11 23:12 ` kernel test robot
0 siblings, 2 replies; 6+ messages in thread
From: Hamza Mahfooz @ 2024-12-09 16:24 UTC (permalink / raw)
To: linux-efi
Cc: Hamza Mahfooz, stable, Tyler Hicks, Brian Nguyen, Jacob Pan,
Allen Pais, Ard Biesheuvel, Jonathan Marek, Ilias Apalodimas,
Kuppuswamy Sathyanarayanan, Jeremy Linton, Kirill A. Shutemov,
KONDO KAZUMA(近藤 和真), Kees Cook,
Borislav Petkov (AMD), Yuntao Wang, Aditya Garg, open list
Recent platforms 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.
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>
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(-)
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
+
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)
{
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);
+
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] efi: make the min and max mmap slack slots configurable
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
2024-12-11 23:12 ` kernel test robot
1 sibling, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2024-12-09 16:40 UTC (permalink / raw)
To: Hamza Mahfooz
Cc: linux-efi, stable, Tyler Hicks, Brian Nguyen, Jacob Pan,
Allen Pais, Jonathan Marek, Ilias Apalodimas,
Kuppuswamy Sathyanarayanan, Jeremy Linton, Kirill A. Shutemov,
KONDO KAZUMA(近藤 和真), Kees Cook,
Borislav Petkov (AMD), Yuntao Wang, Aditya Garg, open list
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?
> 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?
> 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?
> 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?
How did you test this code?
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] efi: make the min and max mmap slack slots configurable
2024-12-09 16:40 ` Ard Biesheuvel
@ 2024-12-09 17:02 ` Hamza Mahfooz
2024-12-09 18:00 ` Ard Biesheuvel
0 siblings, 1 reply; 6+ messages in thread
From: Hamza Mahfooz @ 2024-12-09 17:02 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, stable, Tyler Hicks, Brian Nguyen, Jacob Pan,
Allen Pais, Jonathan Marek, Ilias Apalodimas,
Kuppuswamy Sathyanarayanan, Jeremy Linton, Kirill A. Shutemov,
KONDO KAZUMA(近藤 和真), Kees Cook,
Borislav Petkov (AMD), Yuntao Wang, Aditya Garg, open list
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] efi: make the min and max mmap slack slots configurable
2024-12-09 17:02 ` Hamza Mahfooz
@ 2024-12-09 18:00 ` Ard Biesheuvel
2024-12-09 18:03 ` Hamza Mahfooz
0 siblings, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2024-12-09 18:00 UTC (permalink / raw)
To: Hamza Mahfooz
Cc: linux-efi, stable, Tyler Hicks, Brian Nguyen, Jacob Pan,
Allen Pais, Jonathan Marek, Ilias Apalodimas,
Kuppuswamy Sathyanarayanan, Jeremy Linton, Kirill A. Shutemov,
KONDO KAZUMA(近藤 和真), Kees Cook,
Borislav Petkov (AMD), Yuntao Wang, Aditya Garg, open list
On Mon, 9 Dec 2024 at 18:02, Hamza Mahfooz
<hamzamahfooz@linux.microsoft.com> wrote:
>
> 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.
>
Those are arm64 systems, right?
> >
> >> 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.
>
I think you may have gotten confused here - see below
> >
> >> 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>
> >> ---
...
> >> 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.
>
But allocate_pool() *never* returns EFI_BUFFER_TOO_SMALL. It is
get_memory_map() that may return EFI_BUFFER_TOO_SMALL if the memory
map is larger than the provided buffer. In this case, allocate_pool()
needs to be called again to allocate a buffer of the appropriate size.
So the loop needs to call get_memory_map() again, but given that the
size is returned directly when the first call fails, this iterative
logic seems misguided.
I also think you might be misunderstanding the purpose of the slack
slots. They exist to reduce the likelihood that the memory map grows
more entries than can be accommodated in the buffer in cases where the
first call to ExitBootServices() fails, and GetMemoryMap() needs to be
called again; at that point, memory allocations are no longer possible
(although the UEFI spec was relaxed in this regard between 2.6 and
2.10).
> >
> > 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.
>
allocate_pool() never returns EFI_BUFFER_TOO_SMALL, and so your loop
executes only once. I cannot explain how this happens to fix the boot
for you, but your patch as presented is deeply flawed.
If bumping the number of slots to 32 also solves the problem, I'd
happily consider that instead,
>
> >
> >> if (status != EFI_SUCCESS)
> >> return status;
> >>
> >> + if (n)
> >> + *n = nr;
> >> +
It seems to me that at this point, nr has been doubled after it was
used to perform the allocation, so you are returning a wrong value
here.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] efi: make the min and max mmap slack slots configurable
2024-12-09 18:00 ` Ard Biesheuvel
@ 2024-12-09 18:03 ` Hamza Mahfooz
0 siblings, 0 replies; 6+ messages in thread
From: Hamza Mahfooz @ 2024-12-09 18:03 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-efi, stable, Tyler Hicks, Brian Nguyen, Jacob Pan,
Allen Pais, Jonathan Marek, Ilias Apalodimas,
Kuppuswamy Sathyanarayanan, Jeremy Linton, Kirill A. Shutemov,
KONDO KAZUMA(近藤 和真), Kees Cook,
Borislav Petkov (AMD), Yuntao Wang, Aditya Garg, open list
On 12/9/24 13:00, Ard Biesheuvel wrote:
> On Mon, 9 Dec 2024 at 18:02, Hamza Mahfooz
> <hamzamahfooz@linux.microsoft.com> wrote:
>>
>> 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.
>>
>
> Those are arm64 systems, right?
Yup.
>
>>>
>>>> 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.
>>
>
> I think you may have gotten confused here - see below
>
>>>
>>>> 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>
>>>> ---
> ...
>>>> 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.
>>
>
> But allocate_pool() *never* returns EFI_BUFFER_TOO_SMALL. It is
> get_memory_map() that may return EFI_BUFFER_TOO_SMALL if the memory
> map is larger than the provided buffer. In this case, allocate_pool()
> needs to be called again to allocate a buffer of the appropriate size.
>
> So the loop needs to call get_memory_map() again, but given that the
> size is returned directly when the first call fails, this iterative
> logic seems misguided.
>
> I also think you might be misunderstanding the purpose of the slack
> slots. They exist to reduce the likelihood that the memory map grows
> more entries than can be accommodated in the buffer in cases where the
> first call to ExitBootServices() fails, and GetMemoryMap() needs to be
> called again; at that point, memory allocations are no longer possible
> (although the UEFI spec was relaxed in this regard between 2.6 and
> 2.10).
>
>
>>>
>>> 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.
>>
>
> allocate_pool() never returns EFI_BUFFER_TOO_SMALL, and so your loop
> executes only once. I cannot explain how this happens to fix the boot
> for you, but your patch as presented is deeply flawed.
>
> If bumping the number of slots to 32 also solves the problem, I'd
> happily consider that instead,
Ya, lets go with that, in that case.
>
>
>>
>>>
>>>> if (status != EFI_SUCCESS)
>>>> return status;
>>>>
>>>> + if (n)
>>>> + *n = nr;
>>>> +
>
> It seems to me that at this point, nr has been doubled after it was
> used to perform the allocation, so you are returning a wrong value
> here.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] efi: make the min and max mmap slack slots configurable
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-11 23:12 ` kernel test robot
1 sibling, 0 replies; 6+ messages in thread
From: kernel test robot @ 2024-12-11 23:12 UTC (permalink / raw)
To: Hamza Mahfooz, linux-efi
Cc: llvm, oe-kbuild-all, Hamza Mahfooz, stable, Tyler Hicks,
Brian Nguyen, Jacob Pan, Allen Pais, Ard Biesheuvel,
Jonathan Marek, Ilias Apalodimas, Kuppuswamy Sathyanarayanan,
Jeremy Linton, Kirill A. Shutemov,
KONDO KAZUMA(近藤 和真), Kees Cook,
Borislav Petkov (AMD), Yuntao Wang, Aditya Garg, linux-kernel
Hi Hamza,
kernel test robot noticed the following build warnings:
[auto build test WARNING on efi/next]
[also build test WARNING on linus/master v6.13-rc2 next-20241211]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hamza-Mahfooz/efi-make-the-min-and-max-mmap-slack-slots-configurable/20241210-002724
base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
patch link: https://lore.kernel.org/r/20241209162449.48390-1-hamzamahfooz%40linux.microsoft.com
patch subject: [PATCH] efi: make the min and max mmap slack slots configurable
config: x86_64-buildonly-randconfig-002-20241210 (https://download.01.org/0day-ci/archive/20241212/202412120620.ZY2X03AR-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241212/202412120620.ZY2X03AR-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412120620.ZY2X03AR-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/firmware/efi/libstub/mem.c:23: warning: Function parameter or struct member 'n' not described in 'efi_get_memory_map'
vim +23 drivers/firmware/efi/libstub/mem.c
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 7
1d9b17683547348 Heinrich Schuchardt 2020-02-18 8 /**
1d9b17683547348 Heinrich Schuchardt 2020-02-18 9 * efi_get_memory_map() - get memory map
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 10 * @map: pointer to memory map pointer to which to assign the
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 11 * newly allocated memory map
171539f5a90e3fd Ard Biesheuvel 2022-09-15 12 * @install_cfg_tbl: whether or not to install the boot memory map as a
171539f5a90e3fd Ard Biesheuvel 2022-09-15 13 * configuration table
1d9b17683547348 Heinrich Schuchardt 2020-02-18 14 *
1d9b17683547348 Heinrich Schuchardt 2020-02-18 15 * Retrieve the UEFI memory map. The allocated memory leaves room for
8e602989bc52479 Hamza Mahfooz 2024-12-09 16 * up to CONFIG_EFI_MAX_NR_MMAP_SLACK_SLOTS additional memory map entries.
1d9b17683547348 Heinrich Schuchardt 2020-02-18 17 *
1d9b17683547348 Heinrich Schuchardt 2020-02-18 18 * Return: status code
1d9b17683547348 Heinrich Schuchardt 2020-02-18 19 */
171539f5a90e3fd Ard Biesheuvel 2022-09-15 20 efi_status_t efi_get_memory_map(struct efi_boot_memmap **map,
8e602989bc52479 Hamza Mahfooz 2024-12-09 21 bool install_cfg_tbl,
8e602989bc52479 Hamza Mahfooz 2024-12-09 22 unsigned int *n)
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 @23 {
171539f5a90e3fd Ard Biesheuvel 2022-09-15 24 int memtype = install_cfg_tbl ? EFI_ACPI_RECLAIM_MEMORY
171539f5a90e3fd Ard Biesheuvel 2022-09-15 25 : EFI_LOADER_DATA;
171539f5a90e3fd Ard Biesheuvel 2022-09-15 26 efi_guid_t tbl_guid = LINUX_EFI_BOOT_MEMMAP_GUID;
8e602989bc52479 Hamza Mahfooz 2024-12-09 27 unsigned int nr = CONFIG_EFI_MIN_NR_MMAP_SLACK_SLOTS;
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 28 struct efi_boot_memmap *m, tmp;
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 29 efi_status_t status;
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 30 unsigned long size;
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 31
8e602989bc52479 Hamza Mahfooz 2024-12-09 32 BUILD_BUG_ON(!is_power_of_2(CONFIG_EFI_MIN_NR_MMAP_SLACK_SLOTS) ||
8e602989bc52479 Hamza Mahfooz 2024-12-09 33 !is_power_of_2(CONFIG_EFI_MAX_NR_MMAP_SLACK_SLOTS) ||
8e602989bc52479 Hamza Mahfooz 2024-12-09 34 CONFIG_EFI_MIN_NR_MMAP_SLACK_SLOTS >=
8e602989bc52479 Hamza Mahfooz 2024-12-09 35 CONFIG_EFI_MAX_NR_MMAP_SLACK_SLOTS);
8e602989bc52479 Hamza Mahfooz 2024-12-09 36
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 37 tmp.map_size = 0;
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 38 status = efi_bs_call(get_memory_map, &tmp.map_size, NULL, &tmp.map_key,
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 39 &tmp.desc_size, &tmp.desc_ver);
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 40 if (status != EFI_BUFFER_TOO_SMALL)
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 41 return EFI_LOAD_ERROR;
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 42
8e602989bc52479 Hamza Mahfooz 2024-12-09 43 do {
8e602989bc52479 Hamza Mahfooz 2024-12-09 44 size = tmp.map_size + tmp.desc_size * nr;
171539f5a90e3fd Ard Biesheuvel 2022-09-15 45 status = efi_bs_call(allocate_pool, memtype, sizeof(*m) + size,
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 46 (void **)&m);
8e602989bc52479 Hamza Mahfooz 2024-12-09 47 nr <<= 1;
8e602989bc52479 Hamza Mahfooz 2024-12-09 48 } while (status == EFI_BUFFER_TOO_SMALL &&
8e602989bc52479 Hamza Mahfooz 2024-12-09 49 nr <= CONFIG_EFI_MAX_NR_MMAP_SLACK_SLOTS);
8e602989bc52479 Hamza Mahfooz 2024-12-09 50
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 51 if (status != EFI_SUCCESS)
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 52 return status;
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 53
8e602989bc52479 Hamza Mahfooz 2024-12-09 54 if (n)
8e602989bc52479 Hamza Mahfooz 2024-12-09 55 *n = nr;
8e602989bc52479 Hamza Mahfooz 2024-12-09 56
171539f5a90e3fd Ard Biesheuvel 2022-09-15 57 if (install_cfg_tbl) {
171539f5a90e3fd Ard Biesheuvel 2022-09-15 58 /*
171539f5a90e3fd Ard Biesheuvel 2022-09-15 59 * Installing a configuration table might allocate memory, and
171539f5a90e3fd Ard Biesheuvel 2022-09-15 60 * this may modify the memory map. This means we should install
171539f5a90e3fd Ard Biesheuvel 2022-09-15 61 * the configuration table first, and re-install or delete it
171539f5a90e3fd Ard Biesheuvel 2022-09-15 62 * as needed.
171539f5a90e3fd Ard Biesheuvel 2022-09-15 63 */
171539f5a90e3fd Ard Biesheuvel 2022-09-15 64 status = efi_bs_call(install_configuration_table, &tbl_guid, m);
171539f5a90e3fd Ard Biesheuvel 2022-09-15 65 if (status != EFI_SUCCESS)
171539f5a90e3fd Ard Biesheuvel 2022-09-15 66 goto free_map;
171539f5a90e3fd Ard Biesheuvel 2022-09-15 67 }
171539f5a90e3fd Ard Biesheuvel 2022-09-15 68
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 69 m->buff_size = m->map_size = size;
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 70 status = efi_bs_call(get_memory_map, &m->map_size, m->map, &m->map_key,
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 71 &m->desc_size, &m->desc_ver);
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 72 if (status != EFI_SUCCESS)
171539f5a90e3fd Ard Biesheuvel 2022-09-15 73 goto uninstall_table;
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 74
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 75 *map = m;
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 76 return EFI_SUCCESS;
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 77
171539f5a90e3fd Ard Biesheuvel 2022-09-15 78 uninstall_table:
171539f5a90e3fd Ard Biesheuvel 2022-09-15 79 if (install_cfg_tbl)
171539f5a90e3fd Ard Biesheuvel 2022-09-15 80 efi_bs_call(install_configuration_table, &tbl_guid, NULL);
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 81 free_map:
eab3126571ed1e3 Ard Biesheuvel 2022-06-03 82 efi_bs_call(free_pool, m);
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 83 return status;
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 84 }
f57db62c67c1c9d Ard Biesheuvel 2020-02-10 85
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-11 23:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2024-12-09 18:00 ` Ard Biesheuvel
2024-12-09 18:03 ` Hamza Mahfooz
2024-12-11 23:12 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox