[PATCH v4 0/5] Adjust initial EFI memory-allocation to be in the U-Boot region
U-Boot relocates itself to the top of memory and is supposed to avoid using any memory below that region. This allows the maximum amount of memory to be available for loading images.
The lmb subsystem automatically protects U-Boot's code and data from being overwritten. It relies on this region being a contiguous area of memory. This setup has worked well for at least 15 years and has been documented in the README for as long.
The EFI_LOADER subsystem currently uses memory outside the U-Boot region when starting up.
Some of the downstream problems this causes are:
1. It can overwrite images that have been loaded 2. Requires lmb reservations to avoid (1) even if EFI_LOADER is not being used 3. Uses regions which scripts expect to be available, e.g. the existing kernel_addr_r variables 4. Introduces confusion as to what memory U-Boot can use for itself
This series sets up a very small region of memory which EFI can used at start-up, thus avoiding touching memory outside the U-Boot region.
There are still a few tables which are not in the bloblist, but that will be resolved separately.
Other than tables, the amount of API-visible memory allocated by EFI before booting is actually very small: 3 allocations, of a total of about 250 bytes. However, there are some areas where EFI allocations are done unnecessarily. For example efi_disk_add_dev() uses page-allocation to allocate an efi_device_path, then immediately frees it again. In this case it would be better to use malloc(). Since each allocation consumes a page (4KB), 256KB has been chosen for the size here, enough for 64 allocations.
As soon as efi_init_obj_list() is called, we know that the EFI subsystem is to be used, so we can relax the restrictions and allow EFI to use any memory it likes. This is handled using a simple boolean flag in BSS, since it cannot happen before relocation.
Changes in v4: - Expand the commit message - Reword the commit message since this feature is needed - Use a different technique to keep the memory-usage in place - Drop changes to pool allocation - Reorder the patches - Rewrite the cover letter - Make it a debug message for now
Changes in v3: - Add new patch to drop the memset() from efi_alloc() - Drop patch to convert device_path allocation to use malloc()
Changes in v2: - Drop patch 'Show more information in efi index' - Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()' - Add the word 'warning', use log_warning() and show the end address
Simon Glass (5): efi: Drop the memset() from efi_alloc() efi: Show the location of the bounce buffer when debugging event: Add a generic way to reserve memory efi: Reserve some memory for initial use efi: Keep early allocations to the U-Boot region
common/board_f.c | 1 + common/event.c | 1 + include/asm-generic/global_data.h | 6 ++++ include/efi_loader.h | 24 +++++++++++++- include/event.h | 11 +++++++ lib/efi_loader/efi_bootbin.c | 9 ++++++ lib/efi_loader/efi_memory.c | 54 ++++++++++++++++++++++++------- lib/efi_loader/efi_setup.c | 5 +++ test/py/tests/test_event_dump.py | 1 + 9 files changed, 100 insertions(+), 12 deletions(-)
From my inspection none of the users need the memory to be zeroed. It
is somewhat unexpected that it does so, since the name gives no clue to this.
Drop the memset() so that it effectively becomes a wrapper around the normal EFI-pool allocator.
Another option would be to drop this function and call efi_allocate_pool() directly, but that increase code size a little.
Move the function comment to the header file like most other exported functions in U-Boot.
Comments were made in v3 that another project uses memset() when allocating memory, but that is not required by the spec. In any case, as above, from inspection, none of the users need the memory to be zeroed, as they fill the entire region with their own data.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: - Expand the commit message
Changes in v3: - Add new patch to drop the memset() from efi_alloc() - Drop patch to convert device_path allocation to use malloc()
include/efi_loader.h | 11 ++++++++++- lib/efi_loader/efi_memory.c | 9 --------- 2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 511281e150e..08e27e61b06 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -758,8 +758,17 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, * Return: size in pages */ #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT) -/* Allocate boot service data pool memory */ + +/** + * efi_alloc() - allocate boot services data pool memory + * + * Allocate memory from pool with type EFI_BOOT_SERVICES_DATA + * + * @size: number of bytes to allocate + * Return: pointer to allocated memory, or NULL if out of memory + */ void *efi_alloc(size_t len); + /* Allocate pages on the specified alignment */ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align); /* More specific EFI memory allocator, called by EFI payloads */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index c6f1dd09456..50cb2f3898b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -664,14 +664,6 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, return r; }
-/** - * efi_alloc() - allocate boot services data pool memory - * - * Allocate memory from pool and zero it out. - * - * @size: number of bytes to allocate - * Return: pointer to allocated memory or NULL - */ void *efi_alloc(size_t size) { void *buf; @@ -681,7 +673,6 @@ void *efi_alloc(size_t size) log_err("out of memory"); return NULL; } - memset(buf, 0, size);
return buf; }
On Fri, Oct 11, 2024 at 03:21:22PM -0600, Simon Glass wrote:
From my inspection none of the users need the memory to be zeroed. It is somewhat unexpected that it does so, since the name gives no clue to this.
Drop the memset() so that it effectively becomes a wrapper around the normal EFI-pool allocator.
Another option would be to drop this function and call efi_allocate_pool() directly, but that increase code size a little.
Move the function comment to the header file like most other exported functions in U-Boot.
Comments were made in v3 that another project uses memset() when allocating memory, but that is not required by the spec. In any case, as above, from inspection, none of the users need the memory to be zeroed, as they fill the entire region with their own data.
The other comments made in v3 about another project (EDKII) is that we want to remain implementation compatible. This is a case where (a) if needed, we can reach out and get the spec clarified to say zeroed memory and (b) even if it's not, this is a case where real world interoperability is more important than strict spec compliance. If there's an app out there relying on the zeroed behavior and it's fine on EDKII and not fine on us, it'll be a problem for us because we stopped matching behavior. So unless in the end Heinrich and/or Ilias drop their previous objection, I don't think this is a good idea.
Hi Tom,
On Fri, 11 Oct 2024 at 19:20, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 11, 2024 at 03:21:22PM -0600, Simon Glass wrote:
From my inspection none of the users need the memory to be zeroed. It is somewhat unexpected that it does so, since the name gives no clue to this.
Drop the memset() so that it effectively becomes a wrapper around the normal EFI-pool allocator.
Another option would be to drop this function and call efi_allocate_pool() directly, but that increase code size a little.
Move the function comment to the header file like most other exported functions in U-Boot.
Comments were made in v3 that another project uses memset() when allocating memory, but that is not required by the spec. In any case, as above, from inspection, none of the users need the memory to be zeroed, as they fill the entire region with their own data.
The other comments made in v3 about another project (EDKII) is that we want to remain implementation compatible. This is a case where (a) if needed, we can reach out and get the spec clarified to say zeroed memory and (b) even if it's not, this is a case where real world interoperability is more important than strict spec compliance. If there's an app out there relying on the zeroed behavior and it's fine on EDKII and not fine on us, it'll be a problem for us because we stopped matching behavior. So unless in the end Heinrich and/or Ilias drop their previous objection, I don't think this is a good idea.
This is a minor point, so I don't mind, so long as we get the memory-reservation tidied up. But I'll note that the effect of this patch is zero, since every allocated byte is written anyway, by current users.
I certainly don't want to be implementation-compatible with EDKII. Where did that come from? U-Boot is an open-source bootloader with lots of boards supported in mainline. U-Boot has very different goals and users.
Regards, Simon
On Mon, Oct 14, 2024 at 02:20:42PM -0600, Simon Glass wrote:
[snip]
I certainly don't want to be implementation-compatible with EDKII. Where did that come from?
It came from Ilias pointing out that EDKII does this too, and so we shouldn't deviate from that, and then you re-posted the patch with a reworded commit message, rather than dropping it. So I'm just emphasizing goals for the public record.
The EFI_LOADER_BOUNCE_BUFFER feature was added many years ago. It is still needed and 24 boards (lx2160ardb_tfa_stmm, lx2162aqds_tfa_SECURE_BOOT and the like) use it.
This feature uses EFI page allocation to create a 64MB buffer 'in space' without any knowledge of where boards intend to load their images. This may result in image corruption or other problems.
For example, if the feature is enabled on qemu_arm64 it puts the EFI bounce buffer at 1045MB, with the kernel at 1028MB and the ramdisk at 1088MB. The kernel is probably smaller than 27MB but the buffer does overlap the ramdisk.
The solution is probably to use BOUNCE_BUFFER instead, with the EFI version being dropped. For now, show the address of the EFI bounce buffer so people have a better chance to debug any problem.
Signed-off-by: Simon Glass sjg@chromium.org --- In response to questions on how to show this problem:
First, add CONFIG_EFI_LOADER_BOUNCE_BUFFER=y to configs/qemu_arm64_defconfig and build
Then, to run: qemu-system-aarch64 -machine virt -nographic -cpu cortex-a57 \ -bios u-boot.bin
U-Boot 2024.10-00809-gae29e5f76e4a-dirty (Oct 11 2024 - 11:48:46 -0600)
DRAM: 128 MiB Core: 51 devices, 14 uclasses, devicetree: board Flash: 64 MiB Loading Environment from Flash... *** Warning - bad CRC, using default environment
In: serial,usbkbd Out: serial,vidconsole Err: serial,vidconsole No USB controllers found Net: eth0: virtio-net#32
starting USB... No USB controllers found Hit any key to stop autoboot: 0 => bootefi hello Enabling coop memory No EFI system partition No EFI system partition Failed to persist EFI variables Missing TPMv2 device for EFI_TCG_PROTOCOL Missing RNG device for EFI_RNG_PROTOCOL Warning: EFI bounce buffer 000000004157f000-000000004557f000 Not loaded from disk Booting /MemoryMapped(0x0,0x47763730,0x31b0) EFI: Call: efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) EFI: 0 returned by efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) Hello, world! Running on UEFI 2.10 Firmware vendor: Das U-Boot Firmware revision: 20241000 Have device tree Load options: <none> File path: <none> Boot device: /MemoryMapped(0x0,0x47763730,0x31b0) => print kernel_addr_r kernel_addr_r=0x40400000 => print ramdisk_addr_r ramdisk_addr_r=0x44000000 =>
The EFI bounce buffer overlaps with the ramdisk.
Changes in v4: - Reword the commit message since this feature is needed
lib/efi_loader/efi_bootbin.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index a87006b3c0e..eefdd660cee 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,6 +209,15 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) return -1; }
+#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER + /* + * Add a warning about this buffer, since it may conflict with other + * things + */ + log_debug("Warning: EFI bounce buffer %p-%p\n", efi_bounce_buffer, + efi_bounce_buffer + EFI_LOADER_BOUNCE_BUFFER_SIZE); +#endif + ret = efi_install_fdt(fdt); if (ret != EFI_SUCCESS) return ret;
Am 11. Oktober 2024 23:21:23 MESZ schrieb Simon Glass sjg@chromium.org:
The EFI_LOADER_BOUNCE_BUFFER feature was added many years ago. It is still needed and 24 boards (lx2160ardb_tfa_stmm, lx2162aqds_tfa_SECURE_BOOT and the like) use it.
This feature uses EFI page allocation to create a 64MB buffer 'in space' without any knowledge of where boards intend to load their images. This may result in image corruption or other problems.
For example, if the feature is enabled on qemu_arm64 it puts the EFI bounce buffer at 1045MB, with the kernel at 1028MB and the ramdisk at 1088MB. The kernel is probably smaller than 27MB but the buffer does overlap the ramdisk.
The solution is probably to use BOUNCE_BUFFER instead, with the EFI version being dropped. For now, show the address of the EFI bounce buffer so people have a better chance to debug any problem.
Signed-off-by: Simon Glass sjg@chromium.org
In response to questions on how to show this problem:
First, add CONFIG_EFI_LOADER_BOUNCE_BUFFER=y to configs/qemu_arm64_defconfig and build
Then, to run: qemu-system-aarch64 -machine virt -nographic -cpu cortex-a57 \ -bios u-boot.bin
U-Boot 2024.10-00809-gae29e5f76e4a-dirty (Oct 11 2024 - 11:48:46 -0600)
DRAM: 128 MiB Core: 51 devices, 14 uclasses, devicetree: board Flash: 64 MiB Loading Environment from Flash... *** Warning - bad CRC, using default environment
In: serial,usbkbd Out: serial,vidconsole Err: serial,vidconsole No USB controllers found Net: eth0: virtio-net#32
starting USB... No USB controllers found Hit any key to stop autoboot: 0 => bootefi hello Enabling coop memory No EFI system partition No EFI system partition Failed to persist EFI variables Missing TPMv2 device for EFI_TCG_PROTOCOL Missing RNG device for EFI_RNG_PROTOCOL Warning: EFI bounce buffer 000000004157f000-000000004557f000 Not loaded from disk Booting /MemoryMapped(0x0,0x47763730,0x31b0) EFI: Call: efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) EFI: 0 returned by efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) Hello, world! Running on UEFI 2.10 Firmware vendor: Das U-Boot Firmware revision: 20241000 Have device tree Load options: <none> File path: <none> Boot device: /MemoryMapped(0x0,0x47763730,0x31b0) => print kernel_addr_r kernel_addr_r=0x40400000 => print ramdisk_addr_r ramdisk_addr_r=0x44000000 =>
The EFI bounce buffer overlaps with the ramdisk.
Changes in v4:
- Reword the commit message since this feature is needed
lib/efi_loader/efi_bootbin.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index a87006b3c0e..eefdd660cee 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,6 +209,15 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) return -1; }
+#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
- /*
* Add a warning about this buffer, since it may conflict with other* things*/- log_debug("Warning: EFI bounce buffer %p-%p\n", efi_bounce_buffer,
efi_bounce_buffer + EFI_LOADER_BOUNCE_BUFFER_SIZE);+#endif
Adding a debug message here is ok.
But writing "warning" in a debug message which is just printing an address for a configuration selected item looks weird to me.
Best regards
Heinrich
- ret = efi_install_fdt(fdt); if (ret != EFI_SUCCESS) return ret;
Hi Heinrich,
On Fri, 11 Oct 2024 at 16:02, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 11. Oktober 2024 23:21:23 MESZ schrieb Simon Glass sjg@chromium.org:
The EFI_LOADER_BOUNCE_BUFFER feature was added many years ago. It is still needed and 24 boards (lx2160ardb_tfa_stmm, lx2162aqds_tfa_SECURE_BOOT and the like) use it.
This feature uses EFI page allocation to create a 64MB buffer 'in space' without any knowledge of where boards intend to load their images. This may result in image corruption or other problems.
For example, if the feature is enabled on qemu_arm64 it puts the EFI bounce buffer at 1045MB, with the kernel at 1028MB and the ramdisk at 1088MB. The kernel is probably smaller than 27MB but the buffer does overlap the ramdisk.
The solution is probably to use BOUNCE_BUFFER instead, with the EFI version being dropped. For now, show the address of the EFI bounce buffer so people have a better chance to debug any problem.
Signed-off-by: Simon Glass sjg@chromium.org
In response to questions on how to show this problem:
First, add CONFIG_EFI_LOADER_BOUNCE_BUFFER=y to configs/qemu_arm64_defconfig and build
Then, to run: qemu-system-aarch64 -machine virt -nographic -cpu cortex-a57 \ -bios u-boot.bin
U-Boot 2024.10-00809-gae29e5f76e4a-dirty (Oct 11 2024 - 11:48:46 -0600)
DRAM: 128 MiB Core: 51 devices, 14 uclasses, devicetree: board Flash: 64 MiB Loading Environment from Flash... *** Warning - bad CRC, using default environment
In: serial,usbkbd Out: serial,vidconsole Err: serial,vidconsole No USB controllers found Net: eth0: virtio-net#32
starting USB... No USB controllers found Hit any key to stop autoboot: 0 => bootefi hello Enabling coop memory No EFI system partition No EFI system partition Failed to persist EFI variables Missing TPMv2 device for EFI_TCG_PROTOCOL Missing RNG device for EFI_RNG_PROTOCOL Warning: EFI bounce buffer 000000004157f000-000000004557f000 Not loaded from disk Booting /MemoryMapped(0x0,0x47763730,0x31b0) EFI: Call: efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) EFI: 0 returned by efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) Hello, world! Running on UEFI 2.10 Firmware vendor: Das U-Boot Firmware revision: 20241000 Have device tree Load options: <none> File path: <none> Boot device: /MemoryMapped(0x0,0x47763730,0x31b0) => print kernel_addr_r kernel_addr_r=0x40400000 => print ramdisk_addr_r ramdisk_addr_r=0x44000000 =>
The EFI bounce buffer overlaps with the ramdisk.
Changes in v4:
- Reword the commit message since this feature is needed
lib/efi_loader/efi_bootbin.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index a87006b3c0e..eefdd660cee 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,6 +209,15 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) return -1; }
+#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
/** Add a warning about this buffer, since it may conflict with other* things*/log_debug("Warning: EFI bounce buffer %p-%p\n", efi_bounce_buffer,efi_bounce_buffer + EFI_LOADER_BOUNCE_BUFFER_SIZE);+#endif
Adding a debug message here is ok.
But writing "warning" in a debug message which is just printing an address for a configuration selected item looks weird to me.
I added that because you said it needed to say it was a warning. I'll remove it and try again.
Regards, Simon
On Mon, Oct 14, 2024 at 02:20:09PM -0600, Simon Glass wrote:
Hi Heinrich,
On Fri, 11 Oct 2024 at 16:02, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
Am 11. Oktober 2024 23:21:23 MESZ schrieb Simon Glass sjg@chromium.org:
The EFI_LOADER_BOUNCE_BUFFER feature was added many years ago. It is still needed and 24 boards (lx2160ardb_tfa_stmm, lx2162aqds_tfa_SECURE_BOOT and the like) use it.
This feature uses EFI page allocation to create a 64MB buffer 'in space' without any knowledge of where boards intend to load their images. This may result in image corruption or other problems.
For example, if the feature is enabled on qemu_arm64 it puts the EFI bounce buffer at 1045MB, with the kernel at 1028MB and the ramdisk at 1088MB. The kernel is probably smaller than 27MB but the buffer does overlap the ramdisk.
The solution is probably to use BOUNCE_BUFFER instead, with the EFI version being dropped. For now, show the address of the EFI bounce buffer so people have a better chance to debug any problem.
Signed-off-by: Simon Glass sjg@chromium.org
In response to questions on how to show this problem:
First, add CONFIG_EFI_LOADER_BOUNCE_BUFFER=y to configs/qemu_arm64_defconfig and build
Then, to run: qemu-system-aarch64 -machine virt -nographic -cpu cortex-a57 \ -bios u-boot.bin
U-Boot 2024.10-00809-gae29e5f76e4a-dirty (Oct 11 2024 - 11:48:46 -0600)
DRAM: 128 MiB Core: 51 devices, 14 uclasses, devicetree: board Flash: 64 MiB Loading Environment from Flash... *** Warning - bad CRC, using default environment
In: serial,usbkbd Out: serial,vidconsole Err: serial,vidconsole No USB controllers found Net: eth0: virtio-net#32
starting USB... No USB controllers found Hit any key to stop autoboot: 0 => bootefi hello Enabling coop memory No EFI system partition No EFI system partition Failed to persist EFI variables Missing TPMv2 device for EFI_TCG_PROTOCOL Missing RNG device for EFI_RNG_PROTOCOL Warning: EFI bounce buffer 000000004157f000-000000004557f000 Not loaded from disk Booting /MemoryMapped(0x0,0x47763730,0x31b0) EFI: Call: efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) EFI: 0 returned by efi_load_image(false, efi_root, file_path, source_buffer, source_size, &handle) Hello, world! Running on UEFI 2.10 Firmware vendor: Das U-Boot Firmware revision: 20241000 Have device tree Load options: <none> File path: <none> Boot device: /MemoryMapped(0x0,0x47763730,0x31b0) => print kernel_addr_r kernel_addr_r=0x40400000 => print ramdisk_addr_r ramdisk_addr_r=0x44000000 =>
The EFI bounce buffer overlaps with the ramdisk.
Changes in v4:
- Reword the commit message since this feature is needed
lib/efi_loader/efi_bootbin.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index a87006b3c0e..eefdd660cee 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -209,6 +209,15 @@ efi_status_t efi_binary_run(void *image, size_t size, void *fdt) return -1; }
+#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER
/** Add a warning about this buffer, since it may conflict with other* things*/log_debug("Warning: EFI bounce buffer %p-%p\n", efi_bounce_buffer,efi_bounce_buffer + EFI_LOADER_BOUNCE_BUFFER_SIZE);+#endif
Adding a debug message here is ok.
But writing "warning" in a debug message which is just printing an address for a configuration selected item looks weird to me.
I added that because you said it needed to say it was a warning. I'll remove it and try again.
Since we talked about it on IRC, the most useful option here would be instead to switch this to the regular CONFIG_BOUNCE_BUFFER code so that it can go away entirely. The second most useful option is yes, we can put a debug message for when someone enables "make a hole in memory for me" when they do not need that hole made in memory.
Add an event which allows memory to be reserved, for use after relocation. This can be used by subsystems which need their own memory.
This could be used to clean up reserve_arch() but that is left for a separate series.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
common/board_f.c | 1 + common/event.c | 1 + include/event.h | 11 +++++++++++ 3 files changed, 13 insertions(+)
diff --git a/common/board_f.c b/common/board_f.c index 154675d0e40..9f13bc9c374 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -976,6 +976,7 @@ static const init_fnc_t init_sequence_f[] = { reserve_bootstage, reserve_bloblist, reserve_arch, + INITCALL_EVENT(EVT_RESERVE), reserve_stacks, dram_init_banksize, show_dram_config, diff --git a/common/event.c b/common/event.c index dda569d4478..b7da027094b 100644 --- a/common/event.c +++ b/common/event.c @@ -37,6 +37,7 @@ const char *const type_name[] = { /* init hooks */ "misc_init_f", "fsp_init_r", + "reserve", "settings_r", "last_stage_init",
diff --git a/include/event.h b/include/event.h index 75141a192a4..56c05e8e7a4 100644 --- a/include/event.h +++ b/include/event.h @@ -104,6 +104,17 @@ enum event_t { */ EVT_FSP_INIT_F,
+ /** + * @EVT_RESERVE: + * This event is trigged after all other reservations are done, just + * before the stack is placed. It can be used to reserve memory for any + * other purpose. The memory thus reserved is available immediately and + * will remain valid after relocation. + * + * To reserve memory, subtract the required amount of bytes from + * gd->start_addr_sp + */ + EVT_RESERVE, /** * @EVT_SETTINGS_R: * This event is triggered post-relocation and before console init.
The 'point of cooperation' is where U-Boot starts allowing EFI to use memory outside of the U-Boot region. Until that point, it is desirable to keep more below U-Boot free for loading images.
Reserve a small region for this purpose.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
include/asm-generic/global_data.h | 6 ++++++ lib/efi_loader/efi_memory.c | 21 +++++++++++++++++++++ test/py/tests/test_event_dump.py | 1 + 3 files changed, 28 insertions(+)
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index d6c15e2c406..20705be14dd 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -443,6 +443,12 @@ struct global_data { */ struct upl *upl; #endif +#if CONFIG_IS_ENABLED(EFI_LOADER) + /** + * @efi_region: Start of EFI's early-memory region + */ + ulong efi_region; +#endif }; #ifndef DO_DEPS_ONLY static_assert(sizeof(struct global_data) == GD_SIZE); diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 50cb2f3898b..9cc33397371 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -24,6 +24,12 @@ DECLARE_GLOBAL_DATA_PTR; /* Magic number identifying memory allocated from pool */ #define EFI_ALLOC_POOL_MAGIC 0x1fe67ddf6491caa2
+/* + * Amount of memory to reserve for EFI before relocation. This must be a + * multiple of EFI_PAGE_SIZE + */ +#define EFI_EARLY_REGION_SIZE SZ_256K + efi_uintn_t efi_memory_map_key;
struct efi_mem_list { @@ -944,3 +950,18 @@ int efi_memory_init(void)
return 0; } + +static int reserve_efi_region(void) +{ + /* + * Reserve some memory for EFI. Since pool allocations consume 4KB each + * and there are three allocations, allow 16KB of memory, enough for + * four. This can be increased as needed. + */ + gd->efi_region = ALIGN_DOWN(gd->start_addr_sp - EFI_EARLY_REGION_SIZE, + SZ_4K); + gd->start_addr_sp = gd->efi_region; + + return 0; +} +EVENT_SPY_SIMPLE(EVT_RESERVE, reserve_efi_region); diff --git a/test/py/tests/test_event_dump.py b/test/py/tests/test_event_dump.py index e282c67335c..e36fc6a1586 100644 --- a/test/py/tests/test_event_dump.py +++ b/test/py/tests/test_event_dump.py @@ -20,5 +20,6 @@ EVT_FT_FIXUP bootmeth_vbe_ft_fixup .*boot/vbe_request.c:.* EVT_FT_FIXUP bootmeth_vbe_simple_ft_fixup .*boot/vbe_simple_os.c:.* EVT_LAST_STAGE_INIT install_smbios_table .*lib/efi_loader/efi_smbios.c:.* EVT_MISC_INIT_F sandbox_early_getopt_check .*arch/sandbox/cpu/start.c:.* +EVT_RESERVE reserve_efi_region .*lib/efi_loader/efi_memory.c:.* EVT_TEST h_adder_simple .*test/common/event.c:''' assert re.match(expect, out, re.MULTILINE) is not None
Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass sjg@chromium.org:
The 'point of cooperation' is where U-Boot starts allowing EFI to use memory outside of the U-Boot region. Until that point, it is desirable to keep more below U-Boot free for loading images.
Reserve a small region for this purpose.
Your commit message provides no clue why this should be needed.
If we ensure thst LMB is up before EFI initialization, the EFI subsystem will be able to allocate memory from there.
Best regards
Heinrich
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
include/asm-generic/global_data.h | 6 ++++++ lib/efi_loader/efi_memory.c | 21 +++++++++++++++++++++ test/py/tests/test_event_dump.py | 1 + 3 files changed, 28 insertions(+)
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index d6c15e2c406..20705be14dd 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -443,6 +443,12 @@ struct global_data { */ struct upl *upl; #endif +#if CONFIG_IS_ENABLED(EFI_LOADER)
- /**
* @efi_region: Start of EFI's early-memory region*/- ulong efi_region;
+#endif }; #ifndef DO_DEPS_ONLY static_assert(sizeof(struct global_data) == GD_SIZE); diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 50cb2f3898b..9cc33397371 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -24,6 +24,12 @@ DECLARE_GLOBAL_DATA_PTR; /* Magic number identifying memory allocated from pool */ #define EFI_ALLOC_POOL_MAGIC 0x1fe67ddf6491caa2
+/*
- Amount of memory to reserve for EFI before relocation. This must be a
- multiple of EFI_PAGE_SIZE
- */
+#define EFI_EARLY_REGION_SIZE SZ_256K
efi_uintn_t efi_memory_map_key;
struct efi_mem_list { @@ -944,3 +950,18 @@ int efi_memory_init(void)
return 0; }
+static int reserve_efi_region(void) +{
- /*
* Reserve some memory for EFI. Since pool allocations consume 4KB each* and there are three allocations, allow 16KB of memory, enough for* four. This can be increased as needed.*/- gd->efi_region = ALIGN_DOWN(gd->start_addr_sp - EFI_EARLY_REGION_SIZE,
SZ_4K);- gd->start_addr_sp = gd->efi_region;
- return 0;
+} +EVENT_SPY_SIMPLE(EVT_RESERVE, reserve_efi_region); diff --git a/test/py/tests/test_event_dump.py b/test/py/tests/test_event_dump.py index e282c67335c..e36fc6a1586 100644 --- a/test/py/tests/test_event_dump.py +++ b/test/py/tests/test_event_dump.py @@ -20,5 +20,6 @@ EVT_FT_FIXUP bootmeth_vbe_ft_fixup .*boot/vbe_request.c:.* EVT_FT_FIXUP bootmeth_vbe_simple_ft_fixup .*boot/vbe_simple_os.c:.* EVT_LAST_STAGE_INIT install_smbios_table .*lib/efi_loader/efi_smbios.c:.* EVT_MISC_INIT_F sandbox_early_getopt_check .*arch/sandbox/cpu/start.c:.* +EVT_RESERVE reserve_efi_region .*lib/efi_loader/efi_memory.c:.* EVT_TEST h_adder_simple .*test/common/event.c:''' assert re.match(expect, out, re.MULTILINE) is not None
On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote:
Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass sjg@chromium.org:
The 'point of cooperation' is where U-Boot starts allowing EFI to use memory outside of the U-Boot region. Until that point, it is desirable to keep more below U-Boot free for loading images.
Reserve a small region for this purpose.
Your commit message provides no clue why this should be needed.
The alternative here would be a patch to doc/develop/memory.rst saying something to the effect of:
EFI pool allocations start at ... U-Boot stack pointer + CONFIG_STACK_SIZE and grow downwards from there. These are done by ... a brief description of how the pool allocations work ... Nominally, this will extend for only a few pages worth of memory but depending on requests made by EFI applications can grow larger. All of this is covered by the LMB that covers U-Boot itself and is considered part of the U-Boot memory map. It may be broken down in to further detail to the memory map provided to the OS, in some cases.
Hi Tom
On Sat, 12 Oct 2024 at 04:27, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote:
Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass sjg@chromium.org:
The 'point of cooperation' is where U-Boot starts allowing EFI to use memory outside of the U-Boot region. Until that point, it is desirable to keep more below U-Boot free for loading images.
Reserve a small region for this purpose.
Your commit message provides no clue why this should be needed.
The alternative here would be a patch to doc/develop/memory.rst saying something to the effect of:
EFI pool allocations start at ... U-Boot stack pointer + CONFIG_STACK_SIZE and grow downwards from there. These are done by ... a brief description of how the pool allocations work ... Nominally, this will extend for only a few pages worth of memory but depending on requests made by EFI applications can grow larger. All of this is covered by the LMB that covers U-Boot itself and is considered part of the U-Boot memory map. It may be broken down in to further detail to the memory map provided to the OS, in some cases.
I'll send a patch to update that next week. Perhaps even fold it in the LMB series
Thanks /Ilias
-- Tom
Hi,
On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Tom
On Sat, 12 Oct 2024 at 04:27, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote:
Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass sjg@chromium.org:
The 'point of cooperation' is where U-Boot starts allowing EFI to use memory outside of the U-Boot region. Until that point, it is desirable to keep more below U-Boot free for loading images.
Reserve a small region for this purpose.
Your commit message provides no clue why this should be needed.
Yes, I hadn't realised that people didn't understand what I was getting at. Tom asked the same question on irc.
It allows us to separate the lmb allocations from the EFI allocations, so we don't need to have them both in sync. We use lmb for loading images, then EFI takes over and does what it likes, respecting the existing lmb reservations.
The alternative here would be a patch to doc/develop/memory.rst saying something to the effect of:
EFI pool allocations start at ... U-Boot stack pointer + CONFIG_STACK_SIZE and grow downwards from there. These are done by ... a brief description of how the pool allocations work ... Nominally, this will extend for only a few pages worth of memory but depending on requests made by EFI applications can grow larger. All of this is covered by the LMB that covers U-Boot itself and is considered part of the U-Boot memory map. It may be broken down in to further detail to the memory map provided to the OS, in some cases.
I'll send a patch to update that next week. Perhaps even fold it in the LMB series
Before we know we are booting an EFI app? No, thank you. EFI should not be growing downwards like a stack while U-Boot is running...
Regards, SImon
On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote:
Hi,
On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Tom
On Sat, 12 Oct 2024 at 04:27, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote:
Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass sjg@chromium.org:
The 'point of cooperation' is where U-Boot starts allowing EFI to use memory outside of the U-Boot region. Until that point, it is desirable to keep more below U-Boot free for loading images.
Reserve a small region for this purpose.
Your commit message provides no clue why this should be needed.
Yes, I hadn't realised that people didn't understand what I was getting at. Tom asked the same question on irc.
It allows us to separate the lmb allocations from the EFI allocations, so we don't need to have them both in sync. We use lmb for loading images, then EFI takes over and does what it likes, respecting the existing lmb reservations.
But, again, LMB is not for loading of images. It's for dealing with memory reservations of various types.
Hi Tom,
On Mon, 14 Oct 2024 at 15:42, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote:
Hi,
On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Tom
On Sat, 12 Oct 2024 at 04:27, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote:
Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass sjg@chromium.org:
The 'point of cooperation' is where U-Boot starts allowing EFI to use memory outside of the U-Boot region. Until that point, it is desirable to keep more below U-Boot free for loading images.
Reserve a small region for this purpose.
Your commit message provides no clue why this should be needed.
Yes, I hadn't realised that people didn't understand what I was getting at. Tom asked the same question on irc.
It allows us to separate the lmb allocations from the EFI allocations, so we don't need to have them both in sync. We use lmb for loading images, then EFI takes over and does what it likes, respecting the existing lmb reservations.
But, again, LMB is not for loading of images. It's for dealing with memory reservations of various types.
Up until recently, I believe, my statement was true, but in any case, this isn't gemaine to the issue here.
The point is, this is a useful distinction, allowing us to avoid the complexity of keeping them in sync, and avoid putting this pain on people who are not using EFI. We are either in U-Boot code, in which case lmb rules, or we are going into an app, in which case EFI rules (but must read in the lmb info).
We are going to end up with people turning off EFI_LOADER because it behaves so badly.
Regards, Simon
On Mon, Oct 14, 2024 at 03:50:51PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 14 Oct 2024 at 15:42, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote:
Hi,
On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Tom
On Sat, 12 Oct 2024 at 04:27, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote:
Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass sjg@chromium.org: >The 'point of cooperation' is where U-Boot starts allowing EFI to use >memory outside of the U-Boot region. Until that point, it is desirable >to keep more below U-Boot free for loading images. > >Reserve a small region for this purpose.
Your commit message provides no clue why this should be needed.
Yes, I hadn't realised that people didn't understand what I was getting at. Tom asked the same question on irc.
It allows us to separate the lmb allocations from the EFI allocations, so we don't need to have them both in sync. We use lmb for loading images, then EFI takes over and does what it likes, respecting the existing lmb reservations.
But, again, LMB is not for loading of images. It's for dealing with memory reservations of various types.
Up until recently, I believe, my statement was true, but in any case, this isn't gemaine to the issue here.
The point is, this is a useful distinction, allowing us to avoid the complexity of keeping them in sync, and avoid putting this pain on people who are not using EFI. We are either in U-Boot code, in which case lmb rules, or we are going into an app, in which case EFI rules (but must read in the lmb info).
We are going to end up with people turning off EFI_LOADER because it behaves so badly.
Frankly, in hind sight I should not have agreed to split the LMB rework between "everything else" and "EFI" in hopes that we would then be able to get the "EFI" part of this agreed upon. It's "behaving badly" right now because we merged half of the changes in the hopes that we could get your agreement on the rest of them fairly quickly. This is not happening, however.
Hi Tom,
On Mon, 14 Oct 2024 at 15:55, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 14, 2024 at 03:50:51PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 14 Oct 2024 at 15:42, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote:
Hi,
On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Tom
On Sat, 12 Oct 2024 at 04:27, Tom Rini trini@konsulko.com wrote:
On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass sjg@chromium.org: > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > >memory outside of the U-Boot region. Until that point, it is desirable > >to keep more below U-Boot free for loading images. > > > >Reserve a small region for this purpose. > > Your commit message provides no clue why this should be needed.
Yes, I hadn't realised that people didn't understand what I was getting at. Tom asked the same question on irc.
It allows us to separate the lmb allocations from the EFI allocations, so we don't need to have them both in sync. We use lmb for loading images, then EFI takes over and does what it likes, respecting the existing lmb reservations.
But, again, LMB is not for loading of images. It's for dealing with memory reservations of various types.
Up until recently, I believe, my statement was true, but in any case, this isn't gemaine to the issue here.
The point is, this is a useful distinction, allowing us to avoid the complexity of keeping them in sync, and avoid putting this pain on people who are not using EFI. We are either in U-Boot code, in which case lmb rules, or we are going into an app, in which case EFI rules (but must read in the lmb info).
We are going to end up with people turning off EFI_LOADER because it behaves so badly.
Frankly, in hind sight I should not have agreed to split the LMB rework between "everything else" and "EFI" in hopes that we would then be able to get the "EFI" part of this agreed upon. It's "behaving badly" right now because we merged half of the changes in the hopes that we could get your agreement on the rest of them fairly quickly. This is not happening, however.
Well, another option would be to revert the problem patch (or two) and do this work with careful consideration of the impacts, taking account of my architectural objections. My main concern is using U-Boot's memory like a stack. Suddenly this is 'OK' and we apparently want to update U-Boot's docs to say so. It is just not a good idea!
A good part of the reason that U-Boot solves the problems it does is that it mostly has a good design. The EFI_LOADER can be improved and it does not need to implement things in a particular way...it just needs to follow the spec. We are being far too rigid in saying that every memory allocation at every stage has to be handled the same way, etc.
That said, I did not expect the first series to cause problems. If my EFI-test series had landed then we could perhaps have added a test for this case of the app requesting a chunk of memory. My hope is for that test to grow and cover some of these areas, I hope this lmb/efi thing doesn't cause further problems and I will be looking to ensure that this code does not affect non-EFI booting negatively.
My series to solve this same problem (EFI's use of memory) have never been more than a few small patches. Please take a look at the current series[1], particularly patch 5.
Regards, Simon
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=427729
On Tue, Oct 15, 2024 at 07:12:21AM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 14 Oct 2024 at 15:55, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 14, 2024 at 03:50:51PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 14 Oct 2024 at 15:42, Tom Rini trini@konsulko.com wrote:
On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote:
Hi,
On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Tom
On Sat, 12 Oct 2024 at 04:27, Tom Rini trini@konsulko.com wrote: > > On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass sjg@chromium.org: > > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > > >memory outside of the U-Boot region. Until that point, it is desirable > > >to keep more below U-Boot free for loading images. > > > > > >Reserve a small region for this purpose. > > > > Your commit message provides no clue why this should be needed.
Yes, I hadn't realised that people didn't understand what I was getting at. Tom asked the same question on irc.
It allows us to separate the lmb allocations from the EFI allocations, so we don't need to have them both in sync. We use lmb for loading images, then EFI takes over and does what it likes, respecting the existing lmb reservations.
But, again, LMB is not for loading of images. It's for dealing with memory reservations of various types.
Up until recently, I believe, my statement was true, but in any case, this isn't gemaine to the issue here.
The point is, this is a useful distinction, allowing us to avoid the complexity of keeping them in sync, and avoid putting this pain on people who are not using EFI. We are either in U-Boot code, in which case lmb rules, or we are going into an app, in which case EFI rules (but must read in the lmb info).
We are going to end up with people turning off EFI_LOADER because it behaves so badly.
Frankly, in hind sight I should not have agreed to split the LMB rework between "everything else" and "EFI" in hopes that we would then be able to get the "EFI" part of this agreed upon. It's "behaving badly" right now because we merged half of the changes in the hopes that we could get your agreement on the rest of them fairly quickly. This is not happening, however.
Well, another option would be to revert the problem patch (or two) and do this work with careful consideration of the impacts, taking account of my architectural objections.
Well, no. You have objections that no one else has agreed with and most of the active developers in the areas in question have objections to your designs. So no, I repeat myself. My mistake here was taking half of the work in, rather than all of the work in.
Adjust EFI_LOADER to use the provided region for early memory allocations, to avoid going outside the U-Boot region.
Expand the available memory when efi_init_obj_list() is called.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: - Use a different technique to keep the memory-usage in place - Drop changes to pool allocation - Reorder the patches - Rewrite the cover letter - Make it a debug message for now
Changes in v2: - Drop patch 'Show more information in efi index' - Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()' - Add the word 'warning', use log_warning() and show the end address
include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_memory.c | 24 ++++++++++++++++++++++-- lib/efi_loader/efi_setup.c | 5 +++++ 3 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 08e27e61b06..aa464e9d754 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -805,6 +805,19 @@ int efi_disk_probe(void *ctx, struct event *event); int efi_disk_remove(void *ctx, struct event *event); /* Called by board init to initialize the EFI memory map */ int efi_memory_init(void); + +/** + * efi_memory_coop() - Allow EFI to use all available memory + * + * Up until this function is called, only a small portion of memory is available + * for use by the EFI memory-allocator. This function is called at the + * 'point of cooperation', before jumping into an EFI app, which needs to be + * able to make use of all the memory in the machine + * + * Return: efi_status_t (EFI_SUCCESS on success) + */ +int efi_memory_coop(void); + /* Adds new or overrides configuration table entry to the system table */ efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void *table); /* Sets up a loaded image */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 9cc33397371..2cd0b00e9eb 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -30,6 +30,9 @@ DECLARE_GLOBAL_DATA_PTR; */ #define EFI_EARLY_REGION_SIZE SZ_256K
+/* Set when all memory has been added for use by EFI */ +static bool efi_full_memory; + efi_uintn_t efi_memory_map_key;
struct efi_mem_list { @@ -932,8 +935,25 @@ static void add_u_boot_and_runtime(void)
int efi_memory_init(void) { - efi_add_known_memory(); + efi_status_t ret; + + /* restrict EFI to its own region until efi_memory_coop() is called */ + ret = efi_add_memory_map_pg((ulong)map_sysmem(gd->efi_region, 0), + EFI_EARLY_REGION_SIZE >> EFI_PAGE_SHIFT, + EFI_CONVENTIONAL_MEMORY, false); + + return ret; +}
+int efi_memory_coop(void) +{ + if (efi_full_memory) + return 0; + log_debug("Enabling coop memory\n"); + + efi_full_memory = true; + + efi_add_known_memory(); add_u_boot_and_runtime();
#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER @@ -943,7 +963,7 @@ int efi_memory_init(void) if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA, (64 * 1024 * 1024) >> EFI_PAGE_SHIFT, &efi_bounce_buffer_addr) != EFI_SUCCESS) - return -1; + return -ENOMEM;
efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr; #endif diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index a610e032d2f..40e23ca104c 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -228,6 +228,11 @@ efi_status_t efi_init_obj_list(void) if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) return efi_obj_list_initialized;
+ /* Allow EFI to use all memory */ + ret = efi_memory_coop(); + if (ret != EFI_SUCCESS) + goto out; + /* Set up console modes */ efi_setup_console_size();
Hi Simon,
On Sat, 12 Oct 2024 at 00:23, Simon Glass sjg@chromium.org wrote:
Adjust EFI_LOADER to use the provided region for early memory allocations, to avoid going outside the U-Boot region.
Expand the available memory when efi_init_obj_list() is called.
efi_init_obj_list() is not called when an EFI app is called. It's called when the EFI subsystem comes up.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4:
- Use a different technique to keep the memory-usage in place
- Drop changes to pool allocation
- Reorder the patches
- Rewrite the cover letter
- Make it a debug message for now
Changes in v2:
- Drop patch 'Show more information in efi index'
- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
- Add the word 'warning', use log_warning() and show the end address
include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_memory.c | 24 ++++++++++++++++++++++-- lib/efi_loader/efi_setup.c | 5 +++++ 3 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 08e27e61b06..aa464e9d754 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -805,6 +805,19 @@ int efi_disk_probe(void *ctx, struct event *event); int efi_disk_remove(void *ctx, struct event *event); /* Called by board init to initialize the EFI memory map */ int efi_memory_init(void);
+/**
- efi_memory_coop() - Allow EFI to use all available memory
- Up until this function is called, only a small portion of memory is available
- for use by the EFI memory-allocator. This function is called at the
- 'point of cooperation', before jumping into an EFI app, which needs to be
- able to make use of all the memory in the machine
As above, this is called when the EFI subsystem comes up. Not when an application is called. For example doing a 'printenv -e' to print EFI variables would trigger this. The funtion call you are removing just adds all memory all memory as EFI_CONVENTIONAL_MEMORY, which basically means free memory. I can't understand what this patch is supposed to fix
- Return: efi_status_t (EFI_SUCCESS on success)
- */
+int efi_memory_coop(void);
/* Adds new or overrides configuration table entry to the system table */ efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void *table); /* Sets up a loaded image */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 9cc33397371..2cd0b00e9eb 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -30,6 +30,9 @@ DECLARE_GLOBAL_DATA_PTR; */ #define EFI_EARLY_REGION_SIZE SZ_256K
+/* Set when all memory has been added for use by EFI */ +static bool efi_full_memory;
efi_uintn_t efi_memory_map_key;
struct efi_mem_list { @@ -932,8 +935,25 @@ static void add_u_boot_and_runtime(void)
int efi_memory_init(void) {
efi_add_known_memory();
efi_status_t ret;/* restrict EFI to its own region until efi_memory_coop() is called */ret = efi_add_memory_map_pg((ulong)map_sysmem(gd->efi_region, 0),EFI_EARLY_REGION_SIZE >> EFI_PAGE_SHIFT,EFI_CONVENTIONAL_MEMORY, false);return ret;+}
+int efi_memory_coop(void) +{
if (efi_full_memory)return 0;log_debug("Enabling coop memory\n");efi_full_memory = true;efi_add_known_memory(); add_u_boot_and_runtime();#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER @@ -943,7 +963,7 @@ int efi_memory_init(void) if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA, (64 * 1024 * 1024) >> EFI_PAGE_SHIFT, &efi_bounce_buffer_addr) != EFI_SUCCESS)
return -1;
return -ENOMEM; efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr;#endif diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index a610e032d2f..40e23ca104c 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -228,6 +228,11 @@ efi_status_t efi_init_obj_list(void) if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) return efi_obj_list_initialized;
/* Allow EFI to use all memory */ret = efi_memory_coop();if (ret != EFI_SUCCESS)goto out;/* Set up console modes */ efi_setup_console_size();-- 2.34.1
Thanks /Ilias
Hi Ilias,
On Mon, 14 Oct 2024 at 22:26, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Sat, 12 Oct 2024 at 00:23, Simon Glass sjg@chromium.org wrote:
Adjust EFI_LOADER to use the provided region for early memory allocations, to avoid going outside the U-Boot region.
Expand the available memory when efi_init_obj_list() is called.
efi_init_obj_list() is not called when an EFI app is called. It's called when the EFI subsystem comes up.
Indeed.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4:
- Use a different technique to keep the memory-usage in place
- Drop changes to pool allocation
- Reorder the patches
- Rewrite the cover letter
- Make it a debug message for now
Changes in v2:
- Drop patch 'Show more information in efi index'
- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
- Add the word 'warning', use log_warning() and show the end address
include/efi_loader.h | 13 +++++++++++++ lib/efi_loader/efi_memory.c | 24 ++++++++++++++++++++++-- lib/efi_loader/efi_setup.c | 5 +++++ 3 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 08e27e61b06..aa464e9d754 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -805,6 +805,19 @@ int efi_disk_probe(void *ctx, struct event *event); int efi_disk_remove(void *ctx, struct event *event); /* Called by board init to initialize the EFI memory map */ int efi_memory_init(void);
+/**
- efi_memory_coop() - Allow EFI to use all available memory
- Up until this function is called, only a small portion of memory is available
- for use by the EFI memory-allocator. This function is called at the
- 'point of cooperation', before jumping into an EFI app, which needs to be
- able to make use of all the memory in the machine
As above, this is called when the EFI subsystem comes up. Not when an application is called. For example doing a 'printenv -e' to print EFI variables would trigger this.
Yes. At that point we know we are using an EFI feature. We can certainly extend the intent of this patch to cover that, but I am trying to take baby steps here.
The funtion call you are removing just adds all memory all memory as EFI_CONVENTIONAL_MEMORY, which basically means free memory. I can't understand what this patch is supposed to fix
Basically it feeds a small amount of memory to EFI initially, within the U-Boot region. That is enough to deal with the few allocations that are done before efi_init_obj_list() is called.
So if you don't use any EFI features, EFI's memory usage is limited to the U-Boot region.
- Return: efi_status_t (EFI_SUCCESS on success)
- */
+int efi_memory_coop(void);
/* Adds new or overrides configuration table entry to the system table */ efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void *table); /* Sets up a loaded image */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 9cc33397371..2cd0b00e9eb 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -30,6 +30,9 @@ DECLARE_GLOBAL_DATA_PTR; */ #define EFI_EARLY_REGION_SIZE SZ_256K
+/* Set when all memory has been added for use by EFI */ +static bool efi_full_memory;
efi_uintn_t efi_memory_map_key;
struct efi_mem_list { @@ -932,8 +935,25 @@ static void add_u_boot_and_runtime(void)
int efi_memory_init(void) {
efi_add_known_memory();
efi_status_t ret;/* restrict EFI to its own region until efi_memory_coop() is called */ret = efi_add_memory_map_pg((ulong)map_sysmem(gd->efi_region, 0),EFI_EARLY_REGION_SIZE >> EFI_PAGE_SHIFT,EFI_CONVENTIONAL_MEMORY, false);return ret;+}
+int efi_memory_coop(void) +{
if (efi_full_memory)return 0;log_debug("Enabling coop memory\n");efi_full_memory = true;efi_add_known_memory(); add_u_boot_and_runtime();#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER @@ -943,7 +963,7 @@ int efi_memory_init(void) if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA, (64 * 1024 * 1024) >> EFI_PAGE_SHIFT, &efi_bounce_buffer_addr) != EFI_SUCCESS)
return -1;
return -ENOMEM; efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr;#endif diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index a610e032d2f..40e23ca104c 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -228,6 +228,11 @@ efi_status_t efi_init_obj_list(void) if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) return efi_obj_list_initialized;
/* Allow EFI to use all memory */ret = efi_memory_coop();if (ret != EFI_SUCCESS)goto out;/* Set up console modes */ efi_setup_console_size();-- 2.34.1
Thanks /Ilias
Regards, SImon
[...]
+/**
- efi_memory_coop() - Allow EFI to use all available memory
- Up until this function is called, only a small portion of memory is available
- for use by the EFI memory-allocator. This function is called at the
- 'point of cooperation', before jumping into an EFI app, which needs to be
- able to make use of all the memory in the machine
As above, this is called when the EFI subsystem comes up. Not when an application is called. For example doing a 'printenv -e' to print EFI variables would trigger this.
Yes. At that point we know we are using an EFI feature. We can certainly extend the intent of this patch to cover that, but I am trying to take baby steps here.
The funtion call you are removing just adds all memory all memory as EFI_CONVENTIONAL_MEMORY, which basically means free memory. I can't understand what this patch is supposed to fix
Basically it feeds a small amount of memory to EFI initially, within the U-Boot region. That is enough to deal with the few allocations that are done before efi_init_obj_list() is called.
So if you don't use any EFI features, EFI's memory usage is limited to the U-Boot region.
Yes, but what does it fix?
- Return: efi_status_t (EFI_SUCCESS on success)
- */
+int efi_memory_coop(void);
/* Adds new or overrides configuration table entry to the system table */ efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void *table); /* Sets up a loaded image */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 9cc33397371..2cd0b00e9eb 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -30,6 +30,9 @@ DECLARE_GLOBAL_DATA_PTR; */ #define EFI_EARLY_REGION_SIZE SZ_256K
+/* Set when all memory has been added for use by EFI */ +static bool efi_full_memory;
efi_uintn_t efi_memory_map_key;
struct efi_mem_list { @@ -932,8 +935,25 @@ static void add_u_boot_and_runtime(void)
int efi_memory_init(void) {
efi_add_known_memory();
efi_status_t ret;/* restrict EFI to its own region until efi_memory_coop() is called */ret = efi_add_memory_map_pg((ulong)map_sysmem(gd->efi_region, 0),EFI_EARLY_REGION_SIZE >> EFI_PAGE_SHIFT,EFI_CONVENTIONAL_MEMORY, false);return ret;+}
+int efi_memory_coop(void) +{
if (efi_full_memory)return 0;log_debug("Enabling coop memory\n");efi_full_memory = true;efi_add_known_memory(); add_u_boot_and_runtime();#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER @@ -943,7 +963,7 @@ int efi_memory_init(void) if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA, (64 * 1024 * 1024) >> EFI_PAGE_SHIFT, &efi_bounce_buffer_addr) != EFI_SUCCESS)
return -1;
return -ENOMEM; efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr;#endif diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index a610e032d2f..40e23ca104c 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -228,6 +228,11 @@ efi_status_t efi_init_obj_list(void) if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) return efi_obj_list_initialized;
/* Allow EFI to use all memory */ret = efi_memory_coop();if (ret != EFI_SUCCESS)goto out;
I am not sure that will work. I'll have to check if not adding the runtime regions from the linker scripts during bringup is safe. But, instead of adding all that, including a small gd region for efi_region and having to explicitly make that region bigger every time something new gets added on efi allocations, why don't we simply move efi_memory_init() out of init_sequence_r()? We can protect the runtime services etc via LMB now and initialize the memory map in efi_init_obj_list().
I haven't gone through all the code, but it sounds doable
Thanks /Ilias
/* Set up console modes */ efi_setup_console_size();-- 2.34.1
Thanks /Ilias
Regards, SImon
Hi Ilias,
On Wed, 16 Oct 2024 at 08:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
+/**
- efi_memory_coop() - Allow EFI to use all available memory
- Up until this function is called, only a small portion of memory is available
- for use by the EFI memory-allocator. This function is called at the
- 'point of cooperation', before jumping into an EFI app, which needs to be
- able to make use of all the memory in the machine
As above, this is called when the EFI subsystem comes up. Not when an application is called. For example doing a 'printenv -e' to print EFI variables would trigger this.
Yes. At that point we know we are using an EFI feature. We can certainly extend the intent of this patch to cover that, but I am trying to take baby steps here.
The funtion call you are removing just adds all memory all memory as EFI_CONVENTIONAL_MEMORY, which basically means free memory. I can't understand what this patch is supposed to fix
Basically it feeds a small amount of memory to EFI initially, within the U-Boot region. That is enough to deal with the few allocations that are done before efi_init_obj_list() is called.
So if you don't use any EFI features, EFI's memory usage is limited to the U-Boot region.
Yes, but what does it fix?
Just that. U-Boot should not use memory below its own region, that's all.
- Return: efi_status_t (EFI_SUCCESS on success)
- */
+int efi_memory_coop(void);
/* Adds new or overrides configuration table entry to the system table */ efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void *table); /* Sets up a loaded image */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 9cc33397371..2cd0b00e9eb 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -30,6 +30,9 @@ DECLARE_GLOBAL_DATA_PTR; */ #define EFI_EARLY_REGION_SIZE SZ_256K
+/* Set when all memory has been added for use by EFI */ +static bool efi_full_memory;
efi_uintn_t efi_memory_map_key;
struct efi_mem_list { @@ -932,8 +935,25 @@ static void add_u_boot_and_runtime(void)
int efi_memory_init(void) {
efi_add_known_memory();
efi_status_t ret;/* restrict EFI to its own region until efi_memory_coop() is called */ret = efi_add_memory_map_pg((ulong)map_sysmem(gd->efi_region, 0),EFI_EARLY_REGION_SIZE >> EFI_PAGE_SHIFT,EFI_CONVENTIONAL_MEMORY, false);return ret;+}
+int efi_memory_coop(void) +{
if (efi_full_memory)return 0;log_debug("Enabling coop memory\n");efi_full_memory = true;efi_add_known_memory(); add_u_boot_and_runtime();#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER @@ -943,7 +963,7 @@ int efi_memory_init(void) if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA, (64 * 1024 * 1024) >> EFI_PAGE_SHIFT, &efi_bounce_buffer_addr) != EFI_SUCCESS)
return -1;
return -ENOMEM; efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr;#endif diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index a610e032d2f..40e23ca104c 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -228,6 +228,11 @@ efi_status_t efi_init_obj_list(void) if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) return efi_obj_list_initialized;
/* Allow EFI to use all memory */ret = efi_memory_coop();if (ret != EFI_SUCCESS)goto out;I am not sure that will work. I'll have to check if not adding the runtime regions from the linker scripts during bringup is safe. But, instead of adding all that, including a small gd region for efi_region and having to explicitly make that region bigger every time something new gets added on efi allocations, why don't we simply move efi_memory_init() out of init_sequence_r()? We can protect the runtime services etc via LMB now and initialize the memory map in efi_init_obj_list().
I haven't gone through all the code, but it sounds doable
Yes that sounds like a good idea...but there is a catch. There are few allocations that happen when U-Boot is running - e.g. partitions. I think we need to work towards that bit by bit.
Regards, Simon
Thanks /Ilias
/* Set up console modes */ efi_setup_console_size();-- 2.34.1
Thanks /Ilias
Regards, SImon
Hi Simon
On Sat, 19 Oct 2024 at 21:07, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 16 Oct 2024 at 08:02, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
[...]
+/**
- efi_memory_coop() - Allow EFI to use all available memory
- Up until this function is called, only a small portion of memory is available
- for use by the EFI memory-allocator. This function is called at the
- 'point of cooperation', before jumping into an EFI app, which needs to be
- able to make use of all the memory in the machine
As above, this is called when the EFI subsystem comes up. Not when an application is called. For example doing a 'printenv -e' to print EFI variables would trigger this.
Yes. At that point we know we are using an EFI feature. We can certainly extend the intent of this patch to cover that, but I am trying to take baby steps here.
The funtion call you are removing just adds all memory all memory as EFI_CONVENTIONAL_MEMORY, which basically means free memory. I can't understand what this patch is supposed to fix
Basically it feeds a small amount of memory to EFI initially, within the U-Boot region. That is enough to deal with the few allocations that are done before efi_init_obj_list() is called.
So if you don't use any EFI features, EFI's memory usage is limited to the U-Boot region.
Yes, but what does it fix?
Just that. U-Boot should not use memory below its own region, that's all.
That's not a problem though. Before the subsystem bringup the only thing that gets initialized is the memory map
- Return: efi_status_t (EFI_SUCCESS on success)
- */
+int efi_memory_coop(void);
/* Adds new or overrides configuration table entry to the system table */ efi_status_t efi_install_configuration_table(const efi_guid_t *guid, void *table); /* Sets up a loaded image */ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 9cc33397371..2cd0b00e9eb 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -30,6 +30,9 @@ DECLARE_GLOBAL_DATA_PTR; */ #define EFI_EARLY_REGION_SIZE SZ_256K
+/* Set when all memory has been added for use by EFI */ +static bool efi_full_memory;
efi_uintn_t efi_memory_map_key;
struct efi_mem_list { @@ -932,8 +935,25 @@ static void add_u_boot_and_runtime(void)
int efi_memory_init(void) {
efi_add_known_memory();
efi_status_t ret;/* restrict EFI to its own region until efi_memory_coop() is called */ret = efi_add_memory_map_pg((ulong)map_sysmem(gd->efi_region, 0),EFI_EARLY_REGION_SIZE >> EFI_PAGE_SHIFT,EFI_CONVENTIONAL_MEMORY, false);return ret;+}
+int efi_memory_coop(void) +{
if (efi_full_memory)return 0;log_debug("Enabling coop memory\n");efi_full_memory = true;efi_add_known_memory(); add_u_boot_and_runtime();#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER @@ -943,7 +963,7 @@ int efi_memory_init(void) if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA, (64 * 1024 * 1024) >> EFI_PAGE_SHIFT, &efi_bounce_buffer_addr) != EFI_SUCCESS)
return -1;
return -ENOMEM; efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr;#endif diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index a610e032d2f..40e23ca104c 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -228,6 +228,11 @@ efi_status_t efi_init_obj_list(void) if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED) return efi_obj_list_initialized;
/* Allow EFI to use all memory */ret = efi_memory_coop();if (ret != EFI_SUCCESS)goto out;I am not sure that will work. I'll have to check if not adding the runtime regions from the linker scripts during bringup is safe. But, instead of adding all that, including a small gd region for efi_region and having to explicitly make that region bigger every time something new gets added on efi allocations, why don't we simply move efi_memory_init() out of init_sequence_r()? We can protect the runtime services etc via LMB now and initialize the memory map in efi_init_obj_list().
I haven't gone through all the code, but it sounds doable
Yes that sounds like a good idea...but there is a catch. There are few allocations that happen when U-Boot is running - e.g. partitions. I think we need to work towards that bit by bit.
There's also the early capsule updates that might need this -- hence the memory init really early. I'll have a look at this instead of splitting the memory inits
Thanks /Ilias
Regards, Simon
Thanks /Ilias
/* Set up console modes */ efi_setup_console_size();-- 2.34.1
Thanks /Ilias
Regards, SImon
Hi Simon,
On Sat, 12 Oct 2024 at 00:23, Simon Glass sjg@chromium.org wrote:
U-Boot relocates itself to the top of memory and is supposed to avoid using any memory below that region. This allows the maximum amount of memory to be available for loading images.
The lmb subsystem automatically protects U-Boot's code and data from being overwritten. It relies on this region being a contiguous area of memory. This setup has worked well for at least 15 years and has been documented in the README for as long.
The EFI_LOADER subsystem currently uses memory outside the U-Boot region when starting up.
Some of the downstream problems this causes are:
- It can overwrite images that have been loaded
So can the load command
- Requires lmb reservations to avoid (1) even if EFI_LOADER is not being used
- Uses regions which scripts expect to be available, e.g. the existing kernel_addr_r variables
- Introduces confusion as to what memory U-Boot can use for itself
This series sets up a very small region of memory which EFI can used at start-up, thus avoiding touching memory outside the U-Boot region.
EFI *is* U-Boot memory region. It lies outside the U-Boot binary, because some services are instantiated on boot. It also *is* U-Boot executable code. We should start treating it as such
Thanks /Ilias
There are still a few tables which are not in the bloblist, but that will be resolved separately.
Other than tables, the amount of API-visible memory allocated by EFI before booting is actually very small: 3 allocations, of a total of about 250 bytes. However, there are some areas where EFI allocations are done unnecessarily. For example efi_disk_add_dev() uses page-allocation to allocate an efi_device_path, then immediately frees it again. In this case it would be better to use malloc(). Since each allocation consumes a page (4KB), 256KB has been chosen for the size here, enough for 64 allocations.
As soon as efi_init_obj_list() is called, we know that the EFI subsystem is to be used, so we can relax the restrictions and allow EFI to use any memory it likes. This is handled using a simple boolean flag in BSS, since it cannot happen before relocation.
Changes in v4:
- Expand the commit message
- Reword the commit message since this feature is needed
- Use a different technique to keep the memory-usage in place
- Drop changes to pool allocation
- Reorder the patches
- Rewrite the cover letter
- Make it a debug message for now
Changes in v3:
- Add new patch to drop the memset() from efi_alloc()
- Drop patch to convert device_path allocation to use malloc()
Changes in v2:
- Drop patch 'Show more information in efi index'
- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
- Add the word 'warning', use log_warning() and show the end address
Simon Glass (5): efi: Drop the memset() from efi_alloc() efi: Show the location of the bounce buffer when debugging event: Add a generic way to reserve memory efi: Reserve some memory for initial use efi: Keep early allocations to the U-Boot region
common/board_f.c | 1 + common/event.c | 1 + include/asm-generic/global_data.h | 6 ++++ include/efi_loader.h | 24 +++++++++++++- include/event.h | 11 +++++++ lib/efi_loader/efi_bootbin.c | 9 ++++++ lib/efi_loader/efi_memory.c | 54 ++++++++++++++++++++++++------- lib/efi_loader/efi_setup.c | 5 +++ test/py/tests/test_event_dump.py | 1 + 9 files changed, 100 insertions(+), 12 deletions(-)
-- 2.34.1
Hi Ilias,
On Mon, 14 Oct 2024 at 22:28, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Sat, 12 Oct 2024 at 00:23, Simon Glass sjg@chromium.org wrote:
U-Boot relocates itself to the top of memory and is supposed to avoid using any memory below that region. This allows the maximum amount of memory to be available for loading images.
The lmb subsystem automatically protects U-Boot's code and data from being overwritten. It relies on this region being a contiguous area of memory. This setup has worked well for at least 15 years and has been documented in the README for as long.
The EFI_LOADER subsystem currently uses memory outside the U-Boot region when starting up.
Some of the downstream problems this causes are:
- It can overwrite images that have been loaded
So can the load command
Can you point to where? If lmb is enabled, it should be using that, so not overwrite anything.
- Requires lmb reservations to avoid (1) even if EFI_LOADER is not being used
- Uses regions which scripts expect to be available, e.g. the existing kernel_addr_r variables
- Introduces confusion as to what memory U-Boot can use for itself
This series sets up a very small region of memory which EFI can used at start-up, thus avoiding touching memory outside the U-Boot region.
EFI *is* U-Boot memory region. It lies outside the U-Boot binary, because some services are instantiated on boot. It also *is* U-Boot executable code. We should start treating it as such
This seems like semantics to me. What do you think of this patch?
Regards, Simon
Thanks /Ilias
There are still a few tables which are not in the bloblist, but that will be resolved separately.
Other than tables, the amount of API-visible memory allocated by EFI before booting is actually very small: 3 allocations, of a total of about 250 bytes. However, there are some areas where EFI allocations are done unnecessarily. For example efi_disk_add_dev() uses page-allocation to allocate an efi_device_path, then immediately frees it again. In this case it would be better to use malloc(). Since each allocation consumes a page (4KB), 256KB has been chosen for the size here, enough for 64 allocations.
As soon as efi_init_obj_list() is called, we know that the EFI subsystem is to be used, so we can relax the restrictions and allow EFI to use any memory it likes. This is handled using a simple boolean flag in BSS, since it cannot happen before relocation.
Changes in v4:
- Expand the commit message
- Reword the commit message since this feature is needed
- Use a different technique to keep the memory-usage in place
- Drop changes to pool allocation
- Reorder the patches
- Rewrite the cover letter
- Make it a debug message for now
Changes in v3:
- Add new patch to drop the memset() from efi_alloc()
- Drop patch to convert device_path allocation to use malloc()
Changes in v2:
- Drop patch 'Show more information in efi index'
- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
- Add the word 'warning', use log_warning() and show the end address
Simon Glass (5): efi: Drop the memset() from efi_alloc() efi: Show the location of the bounce buffer when debugging event: Add a generic way to reserve memory efi: Reserve some memory for initial use efi: Keep early allocations to the U-Boot region
common/board_f.c | 1 + common/event.c | 1 + include/asm-generic/global_data.h | 6 ++++ include/efi_loader.h | 24 +++++++++++++- include/event.h | 11 +++++++ lib/efi_loader/efi_bootbin.c | 9 ++++++ lib/efi_loader/efi_memory.c | 54 ++++++++++++++++++++++++------- lib/efi_loader/efi_setup.c | 5 +++ test/py/tests/test_event_dump.py | 1 + 9 files changed, 100 insertions(+), 12 deletions(-)
-- 2.34.1
On Tue, 15 Oct 2024 at 16:26, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Mon, 14 Oct 2024 at 22:28, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Sat, 12 Oct 2024 at 00:23, Simon Glass sjg@chromium.org wrote:
U-Boot relocates itself to the top of memory and is supposed to avoid using any memory below that region. This allows the maximum amount of memory to be available for loading images.
The lmb subsystem automatically protects U-Boot's code and data from being overwritten. It relies on this region being a contiguous area of memory. This setup has worked well for at least 15 years and has been documented in the README for as long.
The EFI_LOADER subsystem currently uses memory outside the U-Boot region when starting up.
Some of the downstream problems this causes are:
- It can overwrite images that have been loaded
So can the load command
Can you point to where? If lmb is enabled, it should be using that, so not overwrite anything.
If you load the initrd in memory X and make the kernel ovelap you will overwrite parts of the initrd. Let's try once again, the ''''problem''' that was discussed in IRC, is that the initramfs overwrites memory that was allocated by the EFI subsystem *because* we haven't merged the LMB followup patches yet.
- Requires lmb reservations to avoid (1) even if EFI_LOADER is not being used
- Uses regions which scripts expect to be available, e.g. the existing kernel_addr_r variables
- Introduces confusion as to what memory U-Boot can use for itself
This series sets up a very small region of memory which EFI can used at start-up, thus avoiding touching memory outside the U-Boot region.
EFI *is* U-Boot memory region. It lies outside the U-Boot binary, because some services are instantiated on boot. It also *is* U-Boot executable code. We should start treating it as such
This seems like semantics to me. What do you think of this patch?
I don't see any reason for doing this. I've asked for a reproducer of the problem you are trying to solve quite a few times and I still don't have an email with one
Thanks /Ilias
Regards, Simon
Thanks /Ilias
There are still a few tables which are not in the bloblist, but that will be resolved separately.
Other than tables, the amount of API-visible memory allocated by EFI before booting is actually very small: 3 allocations, of a total of about 250 bytes. However, there are some areas where EFI allocations are done unnecessarily. For example efi_disk_add_dev() uses page-allocation to allocate an efi_device_path, then immediately frees it again. In this case it would be better to use malloc(). Since each allocation consumes a page (4KB), 256KB has been chosen for the size here, enough for 64 allocations.
As soon as efi_init_obj_list() is called, we know that the EFI subsystem is to be used, so we can relax the restrictions and allow EFI to use any memory it likes. This is handled using a simple boolean flag in BSS, since it cannot happen before relocation.
Changes in v4:
- Expand the commit message
- Reword the commit message since this feature is needed
- Use a different technique to keep the memory-usage in place
- Drop changes to pool allocation
- Reorder the patches
- Rewrite the cover letter
- Make it a debug message for now
Changes in v3:
- Add new patch to drop the memset() from efi_alloc()
- Drop patch to convert device_path allocation to use malloc()
Changes in v2:
- Drop patch 'Show more information in efi index'
- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
- Add the word 'warning', use log_warning() and show the end address
Simon Glass (5): efi: Drop the memset() from efi_alloc() efi: Show the location of the bounce buffer when debugging event: Add a generic way to reserve memory efi: Reserve some memory for initial use efi: Keep early allocations to the U-Boot region
common/board_f.c | 1 + common/event.c | 1 + include/asm-generic/global_data.h | 6 ++++ include/efi_loader.h | 24 +++++++++++++- include/event.h | 11 +++++++ lib/efi_loader/efi_bootbin.c | 9 ++++++ lib/efi_loader/efi_memory.c | 54 ++++++++++++++++++++++++------- lib/efi_loader/efi_setup.c | 5 +++ test/py/tests/test_event_dump.py | 1 + 9 files changed, 100 insertions(+), 12 deletions(-)
-- 2.34.1
Hi Ilias,
On Tue, 15 Oct 2024 at 07:32, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Tue, 15 Oct 2024 at 16:26, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Mon, 14 Oct 2024 at 22:28, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Sat, 12 Oct 2024 at 00:23, Simon Glass sjg@chromium.org wrote:
U-Boot relocates itself to the top of memory and is supposed to avoid using any memory below that region. This allows the maximum amount of memory to be available for loading images.
The lmb subsystem automatically protects U-Boot's code and data from being overwritten. It relies on this region being a contiguous area of memory. This setup has worked well for at least 15 years and has been documented in the README for as long.
The EFI_LOADER subsystem currently uses memory outside the U-Boot region when starting up.
Some of the downstream problems this causes are:
- It can overwrite images that have been loaded
So can the load command
Can you point to where? If lmb is enabled, it should be using that, so not overwrite anything.
If you load the initrd in memory X and make the kernel ovelap you will overwrite parts of the initrd.
But lmb stops that. It is actually what it is for.
Let's try once again, the ''''problem''' that was discussed in IRC, is that the initramfs overwrites memory that was allocated by the EFI subsystem *because* we haven't merged the LMB followup patches yet.
OK, my problem was booting Ubuntu where the bootflow was unbound by exit-boot-services, resulting in freeing memory (and re-using it) which was still in use. I don't actually remember what was in that memory, though. This was over a year ago, when I sent my first version of the test[1]
- Requires lmb reservations to avoid (1) even if EFI_LOADER is not being used
- Uses regions which scripts expect to be available, e.g. the existing kernel_addr_r variables
- Introduces confusion as to what memory U-Boot can use for itself
This series sets up a very small region of memory which EFI can used at start-up, thus avoiding touching memory outside the U-Boot region.
EFI *is* U-Boot memory region. It lies outside the U-Boot binary, because some services are instantiated on boot. It also *is* U-Boot executable code. We should start treating it as such
This seems like semantics to me. What do you think of this patch?
I don't see any reason for doing this. I've asked for a reproducer of the problem you are trying to solve quite a few times and I still don't have an email with one
Yes, I have explained this many times and I still don't have an acknowledgement of my concerns.
EFI_LOADER should only use memory in the U-Boot area, until (at least) an EFI feature is used. Even after that, I would prefer that it keeps to the U-Boot region, if possible, e.g. for small allocations. I fully understand that you have no interest in this, if you think it is possible at all.
Regards, Simon
Thanks /Ilias
Regards, Simon
Thanks /Ilias
There are still a few tables which are not in the bloblist, but that will be resolved separately.
Other than tables, the amount of API-visible memory allocated by EFI before booting is actually very small: 3 allocations, of a total of about 250 bytes. However, there are some areas where EFI allocations are done unnecessarily. For example efi_disk_add_dev() uses page-allocation to allocate an efi_device_path, then immediately frees it again. In this case it would be better to use malloc(). Since each allocation consumes a page (4KB), 256KB has been chosen for the size here, enough for 64 allocations.
As soon as efi_init_obj_list() is called, we know that the EFI subsystem is to be used, so we can relax the restrictions and allow EFI to use any memory it likes. This is handled using a simple boolean flag in BSS, since it cannot happen before relocation.
Changes in v4:
- Expand the commit message
- Reword the commit message since this feature is needed
- Use a different technique to keep the memory-usage in place
- Drop changes to pool allocation
- Reorder the patches
- Rewrite the cover letter
- Make it a debug message for now
Changes in v3:
- Add new patch to drop the memset() from efi_alloc()
- Drop patch to convert device_path allocation to use malloc()
Changes in v2:
- Drop patch 'Show more information in efi index'
- Drop patch 'Avoid pool allocation in efi_get_memory_map_alloc()'
- Add the word 'warning', use log_warning() and show the end address
Simon Glass (5): efi: Drop the memset() from efi_alloc() efi: Show the location of the bounce buffer when debugging event: Add a generic way to reserve memory efi: Reserve some memory for initial use efi: Keep early allocations to the U-Boot region
common/board_f.c | 1 + common/event.c | 1 + include/asm-generic/global_data.h | 6 ++++ include/efi_loader.h | 24 +++++++++++++- include/event.h | 11 +++++++ lib/efi_loader/efi_bootbin.c | 9 ++++++ lib/efi_loader/efi_memory.c | 54 ++++++++++++++++++++++++------- lib/efi_loader/efi_setup.c | 5 +++ test/py/tests/test_event_dump.py | 1 + 9 files changed, 100 insertions(+), 12 deletions(-)
-- 2.34.1
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=418212&state=*
On Thu, Oct 17, 2024 at 05:22:52PM -0600, Simon Glass wrote:
Hi Ilias,
On Tue, 15 Oct 2024 at 07:32, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Tue, 15 Oct 2024 at 16:26, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Mon, 14 Oct 2024 at 22:28, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Sat, 12 Oct 2024 at 00:23, Simon Glass sjg@chromium.org wrote:
U-Boot relocates itself to the top of memory and is supposed to avoid using any memory below that region. This allows the maximum amount of memory to be available for loading images.
The lmb subsystem automatically protects U-Boot's code and data from being overwritten. It relies on this region being a contiguous area of memory. This setup has worked well for at least 15 years and has been documented in the README for as long.
The EFI_LOADER subsystem currently uses memory outside the U-Boot region when starting up.
Some of the downstream problems this causes are:
- It can overwrite images that have been loaded
So can the load command
Can you point to where? If lmb is enabled, it should be using that, so not overwrite anything.
If you load the initrd in memory X and make the kernel ovelap you will overwrite parts of the initrd.
But lmb stops that. It is actually what it is for.
Let's try once again, the ''''problem''' that was discussed in IRC, is that the initramfs overwrites memory that was allocated by the EFI subsystem *because* we haven't merged the LMB followup patches yet.
OK, my problem was booting Ubuntu where the bootflow was unbound by exit-boot-services, resulting in freeing memory (and re-using it) which was still in use. I don't actually remember what was in that memory, though. This was over a year ago, when I sent my first version of the test[1]
This was on x86 as well, yes? I believe that's what you said on the call and part of where people get confused I believe.
Hi Tom,
On Thu, 17 Oct 2024 at 17:35, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 17, 2024 at 05:22:52PM -0600, Simon Glass wrote:
Hi Ilias,
On Tue, 15 Oct 2024 at 07:32, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Tue, 15 Oct 2024 at 16:26, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Mon, 14 Oct 2024 at 22:28, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Sat, 12 Oct 2024 at 00:23, Simon Glass sjg@chromium.org wrote:
U-Boot relocates itself to the top of memory and is supposed to avoid using any memory below that region. This allows the maximum amount of memory to be available for loading images.
The lmb subsystem automatically protects U-Boot's code and data from being overwritten. It relies on this region being a contiguous area of memory. This setup has worked well for at least 15 years and has been documented in the README for as long.
The EFI_LOADER subsystem currently uses memory outside the U-Boot region when starting up.
Some of the downstream problems this causes are:
- It can overwrite images that have been loaded
So can the load command
Can you point to where? If lmb is enabled, it should be using that, so not overwrite anything.
If you load the initrd in memory X and make the kernel ovelap you will overwrite parts of the initrd.
But lmb stops that. It is actually what it is for.
Let's try once again, the ''''problem''' that was discussed in IRC, is that the initramfs overwrites memory that was allocated by the EFI subsystem *because* we haven't merged the LMB followup patches yet.
OK, my problem was booting Ubuntu where the bootflow was unbound by exit-boot-services, resulting in freeing memory (and re-using it) which was still in use. I don't actually remember what was in that memory, though. This was over a year ago, when I sent my first version of the test[1]
This was on x86 as well, yes? I believe that's what you said on the call and part of where people get confused I believe.
Yes, x86
Regards, Simon
On Sat, Oct 19, 2024 at 05:51:03AM -0600, Simon Glass wrote:
Hi Tom,
On Thu, 17 Oct 2024 at 17:35, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 17, 2024 at 05:22:52PM -0600, Simon Glass wrote:
Hi Ilias,
On Tue, 15 Oct 2024 at 07:32, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Tue, 15 Oct 2024 at 16:26, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Mon, 14 Oct 2024 at 22:28, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Sat, 12 Oct 2024 at 00:23, Simon Glass sjg@chromium.org wrote: > > U-Boot relocates itself to the top of memory and is supposed to avoid > using any memory below that region. This allows the maximum amount of > memory to be available for loading images. > > The lmb subsystem automatically protects U-Boot's code and data from > being overwritten. It relies on this region being a contiguous area of > memory. This setup has worked well for at least 15 years and has been > documented in the README for as long. > > The EFI_LOADER subsystem currently uses memory outside the U-Boot region > when starting up. > > Some of the downstream problems this causes are: > > 1. It can overwrite images that have been loaded
So can the load command
Can you point to where? If lmb is enabled, it should be using that, so not overwrite anything.
If you load the initrd in memory X and make the kernel ovelap you will overwrite parts of the initrd.
But lmb stops that. It is actually what it is for.
Let's try once again, the ''''problem''' that was discussed in IRC, is that the initramfs overwrites memory that was allocated by the EFI subsystem *because* we haven't merged the LMB followup patches yet.
OK, my problem was booting Ubuntu where the bootflow was unbound by exit-boot-services, resulting in freeing memory (and re-using it) which was still in use. I don't actually remember what was in that memory, though. This was over a year ago, when I sent my first version of the test[1]
This was on x86 as well, yes? I believe that's what you said on the call and part of where people get confused I believe.
Yes, x86
I just want to emphasize that this is a rather critical detail that was unintentionally omitted and lead to much of the confusion. This is a heavily tested path on aarch64 and I strongly suspect riscv and then I'm not sure who aside from you is testing this path on x86.
participants (4)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Simon Glass -
Tom Rini