| Message ID | 20241209162449.48390-1-hamzamahfooz@linux.microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | efi: make the min and max mmap slack slots configurable | expand |
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 >
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.
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
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); }