[PATCH v5 0/9] 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() but that will be dealt with in future work. For now, 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 v5: - Use kerneldoc style for macros - Drop the word 'warning' - Rebase on top of efil and efik series - Reword cover letter a little
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 (9): kernel: Document ALIGN macros efi: Disable secure boot in bootflow_efi() test board_f: Correct stack reservation board_f: Rename reserve_stack_aligned() 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 | 15 +++++----- common/event.c | 1 + include/asm-generic/global_data.h | 6 ++++ include/efi_loader.h | 27 +++++++++++++++++- include/event.h | 11 ++++++++ include/linux/kernel.h | 31 +++++++++++++++++++++ lib/efi_loader/efi_bootbin.c | 9 ++++++ lib/efi_loader/efi_memory.c | 46 +++++++++++++++++++++++++++++-- lib/efi_loader/efi_setup.c | 5 ++++ lib/efi_loader/efi_var_common.c | 5 ++++ test/boot/bootflow.c | 1 + test/py/tests/test_event_dump.py | 1 + 12 files changed, 147 insertions(+), 11 deletions(-)
Add comments for these macros, so it is clear what they do.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v5: - Use kerneldoc style for macros
include/linux/kernel.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 9467edd65ab..08388f922be 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -35,9 +35,40 @@
#define REPEAT_BYTE(x) ((~0ul / 0xff) * (x))
+/** + * ALIGN() - Rounds a value up to a given alignment + * + * @x: value to be rounded up + * @a: alignment factor. @a must be a power of two + * Return: lowest value >= @x which is a multiple of @a + * + * For example ALIGN(0x123, 8) == 0x128 + */ #define ALIGN(x,a) __ALIGN_MASK((x),(typeof(x))(a)-1) + +/** + * ALIGN_DOWN() - Rounds a value down to a given alignment + * + * @x: value to be rounded down + * @a: alignment factor. @a must be a power of two + * Return: highest value <= @x which is a multiple of @a + * + * For example ALIGN_DOWN(0x123, 8) == 0x120 + * ALIGN_DOWN(0x120, 16) == 0x120 + */ #define ALIGN_DOWN(x, a) ALIGN((x) - ((a) - 1), (a)) + +/** + * __ALIGN_MASK() - Rounds a value up to a given alignment-mask + * + * @x: value to be rounded up + * @mask: mask to apply, must be one less than a power of two + * Return: lowest value >= @x where the bits set in @mask are 0 + * + * For example __ALIGN_MASK(0x123, 0xf) == 0x130 + */ #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask)) + #define PTR_ALIGN(p, a) ((typeof(p))ALIGN((unsigned long)(p), (a))) #define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0)
This does not support secure boot so far, but if a previous test has enabled it, it will remain enabled, thus causing this test to fail with:
efi_load_pe() Image not authenticated
Fix this by providing a way to disable secure boot in tests.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
include/efi_loader.h | 12 ++++++++++++ lib/efi_loader/efi_var_common.c | 5 +++++ test/boot/bootflow.c | 1 + 3 files changed, 18 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 1269907fa3c..a67d3827812 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -1117,6 +1117,18 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name);
bool efi_secure_boot_enabled(void);
+/** + * efi_set_secure_boot_enabled - set whether secure boot is enabled or not + * + * This should be only be used in tests. + * + * TODO(sjg@chromium.org): Consider how we can reinit the EFI state without + * restarting U-Boot + * + * @enable: true to enable, false to disable + */ +void efi_set_secure_boot_enabled(bool enable); + bool efi_capsule_auth_enabled(void);
void *efi_prepare_aligned_image(void *efi, u64 *efi_size); diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index ea8d2a4cf98..0f0e6e8d792 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -362,6 +362,11 @@ bool efi_secure_boot_enabled(void) return efi_secure_boot; }
+void efi_set_secure_boot_enabled(bool enable) +{ + efi_secure_boot = enable; +} + enum efi_auth_var_type efi_auth_var_get_type(const u16 *name, const efi_guid_t *guid) { diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index da713d8ed72..f64d91e0d64 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -1230,6 +1230,7 @@ static int bootflow_efi(struct unit_test_state *uts) struct udevice *bootstd; const char **old_order;
+ efi_set_secure_boot_enabled(false); ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, &bootstd)); std = dev_get_priv(bootstd); old_order = std->bootdev_order;
The reserve_stack_aligned() function already ensures that the resulting address is aligned to a 16-byte boundary. The comment seems to suggest that 16 is passed reserve_stack_aligned() to make it aligned.
Change the value to 0, since the stack can start at the current address, if it is suitably aligned already.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
common/board_f.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/board_f.c b/common/board_f.c index 98dc2591e1d..677e37d93c0 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -601,7 +601,7 @@ __weak int arch_reserve_stacks(void) static int reserve_stacks(void) { /* make stack pointer 16-byte aligned */ - gd->start_addr_sp = reserve_stack_aligned(16); + gd->start_addr_sp = reserve_stack_aligned(0);
/* * let the architecture-specific code tailor gd->start_addr_sp and
On Sun, Dec 01, 2024 at 08:28:05AM -0700, Simon Glass wrote:
The reserve_stack_aligned() function already ensures that the resulting address is aligned to a 16-byte boundary. The comment seems to suggest that 16 is passed reserve_stack_aligned() to make it aligned.
Change the value to 0, since the stack can start at the current address, if it is suitably aligned already.
Signed-off-by: Simon Glass sjg@chromium.org
Er: /* * reserve after start_addr_sp the requested size and make the stack pointer * 16-byte aligned, this alignment is needed for cast on the reserved memory * ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes * = ARMv8 Instruction Set Overview: quad word, 16 bytes */ static unsigned long reserve_stack_aligned(size_t size) { return ALIGN_DOWN(gd->start_addr_sp - size, 16); }
is what the code is today.
(no changes since v1)
common/board_f.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/board_f.c b/common/board_f.c index 98dc2591e1d..677e37d93c0 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -601,7 +601,7 @@ __weak int arch_reserve_stacks(void) static int reserve_stacks(void) { /* make stack pointer 16-byte aligned */
- gd->start_addr_sp = reserve_stack_aligned(16);
gd->start_addr_sp = reserve_stack_aligned(0);
/*
- let the architecture-specific code tailor gd->start_addr_sp and
And this is the last call to reserve_stack_aligned(). So, this is going to save us between 16 and 0 bytes of memory. If we're going to make a change here we need to comment in the code more what we're up to at this point, and the commit message should be more authoritatively worded too.
Hi Tom,
On Mon, 2 Dec 2024 at 14:59, Tom Rini trini@konsulko.com wrote:
On Sun, Dec 01, 2024 at 08:28:05AM -0700, Simon Glass wrote:
The reserve_stack_aligned() function already ensures that the resulting address is aligned to a 16-byte boundary. The comment seems to suggest that 16 is passed reserve_stack_aligned() to make it aligned.
Change the value to 0, since the stack can start at the current address, if it is suitably aligned already.
Signed-off-by: Simon Glass sjg@chromium.org
Er: /*
- reserve after start_addr_sp the requested size and make the stack pointer
- 16-byte aligned, this alignment is needed for cast on the reserved memory
- ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
= ARMv8 Instruction Set Overview: quad word, 16 bytes*/ static unsigned long reserve_stack_aligned(size_t size) { return ALIGN_DOWN(gd->start_addr_sp - size, 16); }
is what the code is today.
(no changes since v1)
common/board_f.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/board_f.c b/common/board_f.c index 98dc2591e1d..677e37d93c0 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -601,7 +601,7 @@ __weak int arch_reserve_stacks(void) static int reserve_stacks(void) { /* make stack pointer 16-byte aligned */
gd->start_addr_sp = reserve_stack_aligned(16);
gd->start_addr_sp = reserve_stack_aligned(0); /* * let the architecture-specific code tailor gd->start_addr_sp andAnd this is the last call to reserve_stack_aligned(). So, this is going to save us between 16 and 0 bytes of memory. If we're going to make a change here we need to comment in the code more what we're up to at this point, and the commit message should be more authoritatively worded too.
So, other than that, what do you think? Should I respin it? Note that this is part of a series which I doubt will be applied to your tree.
It would be nice if other archs could weigh in, but I see that I have not copied them. Still, the current code is confusing (in that the comment doesn't match the code), so we can likely figure this out in -next
Heinrich expressed some concerns with RISC-V, but there doesn't seem to be any breakage in CI. meminfo reports that the devicetree is above the stack and it semes intact:
$ qemu-system-riscv32 -nographic -machine virt -bios /tmp/b/qemu-riscv32/u-boot.bin
U-Boot 2025.01-rc3-00068-g6f392929ea11-dirty (Dec 03 2024 - 06:51:44 -0700)
CPU: riscv Model: riscv-virtio,qemu DRAM: 128 MiB Core: 26 devices, 13 uclasses, devicetree: board Flash: 32 MiB Loading Environment from nowhere... OK In: serial,usbkbd Out: serial,vidconsole Err: serial,vidconsole No USB controllers found Net: No ethernet found.
Hit any key to stop autoboot: 0 => meminfo DRAM: 128 MiB
Region Base Size End Gap ------------------------------------------------ video 87800000 800000 88000000 code 87740000 bf2d8 877ff2d8 d28 malloc 86f20000 820000 87740000 0 board_info 86f1ffc0 40 86f20000 0 global_data 86f1fe50 170 86f1ffc0 0 devicetree 86f1ef30 f02 86f1fe32 1e stack 85ede000 1000000 86ede000 40f30 lmb 85ede000 0 85ede000 0 lmb 85edb000 3000 85ede000 0 free 80000000 85edb000 5edb000 80000000 => md 86f1ef30 86f1ef30: edfe0dd0 020f0000 38000000 980d0000 ...........8.... 86f1ef40: 28000000 11000000 10000000 00000000 ...(............ 86f1ef50: 6a010000 600d0000 00000000 00000000 ...j...`........
Regards, Simon
On Tue, Dec 03, 2024 at 06:53:55AM -0700, Simon Glass wrote:
Hi Tom,
On Mon, 2 Dec 2024 at 14:59, Tom Rini trini@konsulko.com wrote:
On Sun, Dec 01, 2024 at 08:28:05AM -0700, Simon Glass wrote:
The reserve_stack_aligned() function already ensures that the resulting address is aligned to a 16-byte boundary. The comment seems to suggest that 16 is passed reserve_stack_aligned() to make it aligned.
Change the value to 0, since the stack can start at the current address, if it is suitably aligned already.
Signed-off-by: Simon Glass sjg@chromium.org
Er: /*
- reserve after start_addr_sp the requested size and make the stack pointer
- 16-byte aligned, this alignment is needed for cast on the reserved memory
- ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
= ARMv8 Instruction Set Overview: quad word, 16 bytes*/ static unsigned long reserve_stack_aligned(size_t size) { return ALIGN_DOWN(gd->start_addr_sp - size, 16); }
is what the code is today.
(no changes since v1)
common/board_f.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/board_f.c b/common/board_f.c index 98dc2591e1d..677e37d93c0 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -601,7 +601,7 @@ __weak int arch_reserve_stacks(void) static int reserve_stacks(void) { /* make stack pointer 16-byte aligned */
gd->start_addr_sp = reserve_stack_aligned(16);
gd->start_addr_sp = reserve_stack_aligned(0); /* * let the architecture-specific code tailor gd->start_addr_sp andAnd this is the last call to reserve_stack_aligned(). So, this is going to save us between 16 and 0 bytes of memory. If we're going to make a change here we need to comment in the code more what we're up to at this point, and the commit message should be more authoritatively worded too.
So, other than that, what do you think? Should I respin it? Note that this is part of a series which I doubt will be applied to your tree.
I mean, yes, I've given you review comments. If you want this applied to mainline someday then you need to address them.
It would be nice if other archs could weigh in, but I see that I have not copied them. Still, the current code is confusing (in that the comment doesn't match the code), so we can likely figure this out in -next
Yes, my comment was that you need to make the comment match the code and expand upon the common since I expect it's been that way for a decade+ at this point.
Heinrich expressed some concerns with RISC-V, but there doesn't seem to be any breakage in CI. meminfo reports that the devicetree is above the stack and it semes intact:
Yes, I too was concerned about why exactly we're changing something here with a commit message that sounds unsure if it's correct to make these changes, only probably correct.
Hi Tom,
On Tue, 3 Dec 2024 at 07:00, Tom Rini trini@konsulko.com wrote:
On Tue, Dec 03, 2024 at 06:53:55AM -0700, Simon Glass wrote:
Hi Tom,
On Mon, 2 Dec 2024 at 14:59, Tom Rini trini@konsulko.com wrote:
On Sun, Dec 01, 2024 at 08:28:05AM -0700, Simon Glass wrote:
The reserve_stack_aligned() function already ensures that the resulting address is aligned to a 16-byte boundary. The comment seems to suggest that 16 is passed reserve_stack_aligned() to make it aligned.
Change the value to 0, since the stack can start at the current address, if it is suitably aligned already.
Signed-off-by: Simon Glass sjg@chromium.org
Er: /*
- reserve after start_addr_sp the requested size and make the stack pointer
- 16-byte aligned, this alignment is needed for cast on the reserved memory
- ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes
= ARMv8 Instruction Set Overview: quad word, 16 bytes*/ static unsigned long reserve_stack_aligned(size_t size) { return ALIGN_DOWN(gd->start_addr_sp - size, 16); }
is what the code is today.
(no changes since v1)
common/board_f.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/board_f.c b/common/board_f.c index 98dc2591e1d..677e37d93c0 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -601,7 +601,7 @@ __weak int arch_reserve_stacks(void) static int reserve_stacks(void) { /* make stack pointer 16-byte aligned */
gd->start_addr_sp = reserve_stack_aligned(16);
gd->start_addr_sp = reserve_stack_aligned(0); /* * let the architecture-specific code tailor gd->start_addr_sp andAnd this is the last call to reserve_stack_aligned(). So, this is going to save us between 16 and 0 bytes of memory. If we're going to make a change here we need to comment in the code more what we're up to at this point, and the commit message should be more authoritatively worded too.
So, other than that, what do you think? Should I respin it? Note that this is part of a series which I doubt will be applied to your tree.
I mean, yes, I've given you review comments. If you want this applied to mainline someday then you need to address them.
It would be nice if other archs could weigh in, but I see that I have not copied them. Still, the current code is confusing (in that the comment doesn't match the code), so we can likely figure this out in -next
Yes, my comment was that you need to make the comment match the code and expand upon the common since I expect it's been that way for a decade+ at this point.
Heinrich expressed some concerns with RISC-V, but there doesn't seem to be any breakage in CI. meminfo reports that the devicetree is above the stack and it semes intact:
Yes, I too was concerned about why exactly we're changing something here with a commit message that sounds unsure if it's correct to make these changes, only probably correct.
OK, it is true I was hoping for some comments. Unfortunately the code has been there forever. But I'll reword it to be more assertive.
Regards, Simon
Most of the calls to this function have nothing to do with the stack. Rename the function to avoid confusion.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
common/board_f.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/common/board_f.c b/common/board_f.c index 677e37d93c0..3532ffec46f 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -492,7 +492,7 @@ static int reserve_uboot(void) * ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes * = ARMv8 Instruction Set Overview: quad word, 16 bytes */ -static unsigned long reserve_stack_aligned(size_t size) +static unsigned long reserve_aligned(size_t size) { return ALIGN_DOWN(gd->start_addr_sp - size, 16); } @@ -522,7 +522,7 @@ static int reserve_noncached(void) /* reserve memory for malloc() area */ static int reserve_malloc(void) { - gd->start_addr_sp = reserve_stack_aligned(TOTAL_MALLOC_LEN); + gd->start_addr_sp = reserve_aligned(TOTAL_MALLOC_LEN); debug("Reserving %dk for malloc() at: %08lx\n", TOTAL_MALLOC_LEN >> 10, gd->start_addr_sp); #ifdef CONFIG_SYS_NONCACHED_MEMORY @@ -536,7 +536,7 @@ static int reserve_malloc(void) static int reserve_board(void) { if (!gd->bd) { - gd->start_addr_sp = reserve_stack_aligned(sizeof(struct bd_info)); + gd->start_addr_sp = reserve_aligned(sizeof(struct bd_info)); gd->bd = (struct bd_info *)map_sysmem(gd->start_addr_sp, sizeof(struct bd_info)); memset(gd->bd, '\0', sizeof(struct bd_info)); @@ -548,7 +548,7 @@ static int reserve_board(void)
static int reserve_global_data(void) { - gd->start_addr_sp = reserve_stack_aligned(sizeof(gd_t)); + gd->start_addr_sp = reserve_aligned(sizeof(gd_t)); gd->new_gd = (gd_t *)map_sysmem(gd->start_addr_sp, sizeof(gd_t)); debug("Reserving %zu Bytes for Global Data at: %08lx\n", sizeof(gd_t), gd->start_addr_sp); @@ -567,7 +567,7 @@ static int reserve_fdt(void) gd->boardf->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob), 32);
- gd->start_addr_sp = reserve_stack_aligned( + gd->start_addr_sp = reserve_aligned( gd->boardf->fdt_size); gd->boardf->new_fdt = map_sysmem(gd->start_addr_sp, gd->boardf->fdt_size); @@ -584,7 +584,7 @@ static int reserve_bootstage(void) #ifdef CONFIG_BOOTSTAGE int size = bootstage_get_size(true);
- gd->start_addr_sp = reserve_stack_aligned(size); + gd->start_addr_sp = reserve_aligned(size); gd->boardf->new_bootstage = map_sysmem(gd->start_addr_sp, size); debug("Reserving %#x Bytes for bootstage at: %08lx\n", size, gd->start_addr_sp); @@ -601,7 +601,7 @@ __weak int arch_reserve_stacks(void) static int reserve_stacks(void) { /* make stack pointer 16-byte aligned */ - gd->start_addr_sp = reserve_stack_aligned(0); + gd->start_addr_sp = reserve_aligned(0);
/* * let the architecture-specific code tailor gd->start_addr_sp and
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 ---
(no changes since v4)
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 | 2 +- lib/efi_loader/efi_memory.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index a67d3827812..a0af0db6262 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -762,7 +762,7 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, /** * efi_alloc() - allocate boot-services-data pool-memory * - * Allocate memory from pool and zero it out. + * Allocate memory from pool with type EFI_BOOT_SERVICES_DATA * * @len: number of bytes to allocate * Return: pointer to allocated memory or NULL diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index d77243e54b2..76cf908c2de 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -639,7 +639,6 @@ void *efi_alloc(size_t size) log_err("out of memory\n"); return NULL; } - memset(buf, 0, size);
return buf; }
On Sun, Dec 01, 2024 at 08:28:07AM -0700, 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.
Signed-off-by: Simon Glass sjg@chromium.org
I thought you agreed to drop this because Heinrich or Ilias was saying they'd get the spec clarified to say it should be zeroed?
Hi Tom,
On Mon, 2 Dec 2024 at 15:01, Tom Rini trini@konsulko.com wrote:
On Sun, Dec 01, 2024 at 08:28:07AM -0700, 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.
Signed-off-by: Simon Glass sjg@chromium.org
I thought you agreed to drop this because Heinrich or Ilias was saying they'd get the spec clarified to say it should be zeroed?
The other option is to put a 'z' on the end. Heinrich mentioned that Tianocore zeros memory in an equivalent call, but that doesn't make any sense to me, sorry. At present I suspect we are masking bugs, but it's hard to be sure. I'll figure it out eventually, I suspect....
Regards, Simon
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 v5: - Drop the word 'warning'
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 7e7a6bf31aa..9a880251b47 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -208,6 +208,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("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;
On Sun, 1 Dec 2024 at 20:58, Simon Glass sjg@chromium.org wrote:
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.
Why can't we "allocate" sane values for the env variables in question, similar to what is being done for the apple and qcom boards -- you can refer to arch/arm/mach-apple/board.c:board_late_init(). We won't be facing these issues then. Eventually, there can be a common, platform agnostic function to initialise these values. So, just as a PoC, I added this code to the qemu-arm.c, and no longer observe any overlaps.
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c index 6095cb02b23..bad787f7a65 100644 --- a/board/emulation/qemu-arm/qemu-arm.c +++ b/board/emulation/qemu-arm/qemu-arm.c @@ -109,6 +109,9 @@ int board_init(void)
int board_late_init(void) { + + u32 status = 0; + /* * Make sure virtio bus is enumerated so that peripherals * on the virtio bus can be discovered by their drivers @@ -119,6 +122,14 @@ int board_late_init(void) if (CONFIG_IS_ENABLED(USB_KEYBOARD)) usb_init();
+ status |= env_set_hex("loadaddr", lmb_alloc(SZ_2M, SZ_2M)); + status |= env_set_hex("fdt_addr_r", lmb_alloc(SZ_2M, SZ_2M)); + status |= env_set_hex("kernel_addr_r", lmb_alloc(SZ_2M, SZ_2M)); + status |= env_set_hex("ramdisk_addr_r", lmb_alloc(SZ_8M, SZ_2M)); + + if (status) + log_warning("late_init: Failed to set run time variables\n"); + return 0; }
U-Boot 2025.01-rc3-00042-gacab6e78aca7-dirty (Dec 03 2024 - 17:17:42 +0530)
DRAM: 512 MiB EFI bounce buffer 000000005957f000-000000005d57f000 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#31
starting USB... No USB controllers found Hit any key to stop autoboot: 0
=> bdinfo
...
lmb_dump_all: memory.count = 0x1 memory[0] [0x40000000-0x5fffffff], 0x20000000 bytes, flags: none reserved.count = 0x3 reserved[0] [0x58600000-0x593fffff], 0xe00000 bytes, flags: none reserved[1] [0x5957c000-0x5d57efff], 0x4003000 bytes, flags: no-notify, no-overwrite reserved[2] [0x5d57fde0-0x5fffffff], 0x2a80220 bytes, flags: no-overwrite
...
=> printenv kernel_addr_r kernel_addr_r=58e00000 => printenv ramdisk_addr_r ramdisk_addr_r=58600000 => printenv fdt_addr_r fdt_addr_r=59000000 => printenv loadaddr loadaddr=59200000
-sughosh
Changes in v5:
- Drop the word 'warning'
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 7e7a6bf31aa..9a880251b47 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -208,6 +208,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("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;-- 2.43.0
On 03.12.24 13:06, Sughosh Ganu wrote:
On Sun, 1 Dec 2024 at 20:58, Simon Glass sjg@chromium.org wrote:
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.
Why can't we "allocate" sane values for the env variables in question, similar to what is being done for the apple and qcom boards -- you can refer to arch/arm/mach-apple/board.c:board_late_init(). We won't be facing these issues then. Eventually, there can be a common, platform agnostic function to initialise these values. So, just as a PoC, I added this code to the qemu-arm.c, and no longer observe any overlaps.
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c index 6095cb02b23..bad787f7a65 100644 --- a/board/emulation/qemu-arm/qemu-arm.c +++ b/board/emulation/qemu-arm/qemu-arm.c @@ -109,6 +109,9 @@ int board_init(void)
int board_late_init(void) {
u32 status = 0;/* * Make sure virtio bus is enumerated so that peripherals * on the virtio bus can be discovered by their drivers@@ -119,6 +122,14 @@ int board_late_init(void) if (CONFIG_IS_ENABLED(USB_KEYBOARD)) usb_init();
status |= env_set_hex("loadaddr", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("fdt_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("kernel_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("ramdisk_addr_r", lmb_alloc(SZ_8M, SZ_2M));
Hello Sughosh,
avoiding fixed addresses would make sense for most boards.
Your suggested memory reservations for the kernel and initrd are way too small. On my system I find
decompressed kernel - 67 MiB initrd - 77 MiB
scriptaddr is missing.
ARM have address restrictions (see function efi_get_max_initrd_addr()):
armv7: max_init_rd_addr = round_down(image_addr, SZ_4M) + SZ_512M
armv8: #define VA_BITS_MIN (48) max_init_rd_addr = round_down(image_addr, SZ_1G) + (1UL << (VA_BITS_MIN - 1))
These are not modeled in your suggestion.
I would suggest to make a single <= 512 MiB allocation for both initrd and kernel to keep both together and then assign the addresses in the allocated area, e.g.
kerneladdr = lmb_alloc(SZ_512M, SZ_4M); if (!kerneladdr) goto err; if (env_set_hex("kernel_addr_r", kerneladdr)) goto err; if (env_set_hex("initrd_addr_r", kerneladdr + SZ_256M)) goto err;
Best regards
Heinrich
if (status)log_warning("late_init: Failed to set run time variables\n");return 0;U-Boot 2025.01-rc3-00042-gacab6e78aca7-dirty (Dec 03 2024 - 17:17:42 +0530)
DRAM: 512 MiB EFI bounce buffer 000000005957f000-000000005d57f000 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#31
starting USB... No USB controllers found Hit any key to stop autoboot: 0
=> bdinfo
...
lmb_dump_all: memory.count = 0x1 memory[0] [0x40000000-0x5fffffff], 0x20000000 bytes, flags: none reserved.count = 0x3 reserved[0] [0x58600000-0x593fffff], 0xe00000 bytes, flags: none reserved[1] [0x5957c000-0x5d57efff], 0x4003000 bytes, flags: no-notify, no-overwrite reserved[2] [0x5d57fde0-0x5fffffff], 0x2a80220 bytes, flags: no-overwrite
...
=> printenv kernel_addr_r kernel_addr_r=58e00000 => printenv ramdisk_addr_r ramdisk_addr_r=58600000 => printenv fdt_addr_r fdt_addr_r=59000000 => printenv loadaddr loadaddr=59200000
-sughosh
Changes in v5:
- Drop the word 'warning'
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 7e7a6bf31aa..9a880251b47 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -208,6 +208,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("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;-- 2.43.0
On Tue, 3 Dec 2024 at 18:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 03.12.24 13:06, Sughosh Ganu wrote:
On Sun, 1 Dec 2024 at 20:58, Simon Glass sjg@chromium.org wrote:
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.
Why can't we "allocate" sane values for the env variables in question, similar to what is being done for the apple and qcom boards -- you can refer to arch/arm/mach-apple/board.c:board_late_init(). We won't be facing these issues then. Eventually, there can be a common, platform agnostic function to initialise these values. So, just as a PoC, I added this code to the qemu-arm.c, and no longer observe any overlaps.
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c index 6095cb02b23..bad787f7a65 100644 --- a/board/emulation/qemu-arm/qemu-arm.c +++ b/board/emulation/qemu-arm/qemu-arm.c @@ -109,6 +109,9 @@ int board_init(void)
int board_late_init(void) {
u32 status = 0;/* * Make sure virtio bus is enumerated so that peripherals * on the virtio bus can be discovered by their drivers@@ -119,6 +122,14 @@ int board_late_init(void) if (CONFIG_IS_ENABLED(USB_KEYBOARD)) usb_init();
status |= env_set_hex("loadaddr", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("fdt_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("kernel_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("ramdisk_addr_r", lmb_alloc(SZ_8M, SZ_2M));Hello Sughosh,
avoiding fixed addresses would make sense for most boards.
Your suggested memory reservations for the kernel and initrd are way too small. On my system I find
decompressed kernel - 67 MiB initrd - 77 MiB
scriptaddr is missing.
ARM have address restrictions (see function efi_get_max_initrd_addr()):
armv7: max_init_rd_addr = round_down(image_addr, SZ_4M) + SZ_512M
armv8: #define VA_BITS_MIN (48) max_init_rd_addr = round_down(image_addr, SZ_1G) + (1UL << (VA_BITS_MIN - 1))
These are not modeled in your suggestion.
I would suggest to make a single <= 512 MiB allocation for both initrd and kernel to keep both together and then assign the addresses in the allocated area, e.g.
kerneladdr = lmb_alloc(SZ_512M, SZ_4M); if (!kerneladdr) goto err; if (env_set_hex("kernel_addr_r", kerneladdr)) goto err; if (env_set_hex("initrd_addr_r", kerneladdr + SZ_256M)) goto err;
Yes, I simply wanted to illustrate here that there would not be an issue of overlaps if we used the lmb API's to get memory addresses. In fact, this logic that you have pasted above can be added through the reserve event function that Simon is introducing in this series -- one of the functions can reserve memory for all these env variables. I don't see the need to allocate some separate memory region for EFI though. Thanks.
-sughosh
Best regards
Heinrich
if (status)log_warning("late_init: Failed to set run time variables\n");return 0;U-Boot 2025.01-rc3-00042-gacab6e78aca7-dirty (Dec 03 2024 - 17:17:42 +0530)
DRAM: 512 MiB EFI bounce buffer 000000005957f000-000000005d57f000 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#31
starting USB... No USB controllers found Hit any key to stop autoboot: 0
=> bdinfo
...
lmb_dump_all: memory.count = 0x1 memory[0] [0x40000000-0x5fffffff], 0x20000000 bytes, flags: none reserved.count = 0x3 reserved[0] [0x58600000-0x593fffff], 0xe00000 bytes, flags: none reserved[1] [0x5957c000-0x5d57efff], 0x4003000 bytes, flags: no-notify, no-overwrite reserved[2] [0x5d57fde0-0x5fffffff], 0x2a80220 bytes, flags: no-overwrite
...
=> printenv kernel_addr_r kernel_addr_r=58e00000 => printenv ramdisk_addr_r ramdisk_addr_r=58600000 => printenv fdt_addr_r fdt_addr_r=59000000 => printenv loadaddr loadaddr=59200000
-sughosh
Changes in v5:
- Drop the word 'warning'
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 7e7a6bf31aa..9a880251b47 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -208,6 +208,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("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;-- 2.43.0
On Tue, 3 Dec 2024 at 18:22, Sughosh Ganu sughosh.ganu@linaro.org wrote:
On Tue, 3 Dec 2024 at 18:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 03.12.24 13:06, Sughosh Ganu wrote:
On Sun, 1 Dec 2024 at 20:58, Simon Glass sjg@chromium.org wrote:
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.
Why can't we "allocate" sane values for the env variables in question, similar to what is being done for the apple and qcom boards -- you can refer to arch/arm/mach-apple/board.c:board_late_init(). We won't be facing these issues then. Eventually, there can be a common, platform agnostic function to initialise these values. So, just as a PoC, I added this code to the qemu-arm.c, and no longer observe any overlaps.
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c index 6095cb02b23..bad787f7a65 100644 --- a/board/emulation/qemu-arm/qemu-arm.c +++ b/board/emulation/qemu-arm/qemu-arm.c @@ -109,6 +109,9 @@ int board_init(void)
int board_late_init(void) {
u32 status = 0;/* * Make sure virtio bus is enumerated so that peripherals * on the virtio bus can be discovered by their drivers@@ -119,6 +122,14 @@ int board_late_init(void) if (CONFIG_IS_ENABLED(USB_KEYBOARD)) usb_init();
status |= env_set_hex("loadaddr", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("fdt_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("kernel_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("ramdisk_addr_r", lmb_alloc(SZ_8M, SZ_2M));Hello Sughosh,
avoiding fixed addresses would make sense for most boards.
Your suggested memory reservations for the kernel and initrd are way too small. On my system I find
decompressed kernel - 67 MiB initrd - 77 MiB
scriptaddr is missing.
ARM have address restrictions (see function efi_get_max_initrd_addr()):
armv7: max_init_rd_addr = round_down(image_addr, SZ_4M) + SZ_512M
armv8: #define VA_BITS_MIN (48) max_init_rd_addr = round_down(image_addr, SZ_1G) + (1UL << (VA_BITS_MIN - 1))
These are not modeled in your suggestion.
I would suggest to make a single <= 512 MiB allocation for both initrd and kernel to keep both together and then assign the addresses in the allocated area, e.g.
kerneladdr = lmb_alloc(SZ_512M, SZ_4M); if (!kerneladdr) goto err; if (env_set_hex("kernel_addr_r", kerneladdr)) goto err; if (env_set_hex("initrd_addr_r", kerneladdr + SZ_256M)) goto err;
Yes, I simply wanted to illustrate here that there would not be an issue of overlaps if we used the lmb API's to get memory addresses. In fact, this logic that you have pasted above can be added through the reserve event function that Simon is introducing in this series -- one of the functions can reserve memory for all these env variables. I don't see the need to allocate some separate memory region for EFI though. Thanks.
Although the size of the allocation might have to be controlled through a config symbol, or computed at runtime, based on the size of RAM. Not all platforms might have the same amount of memory.
-sughosh
-sughosh
Best regards
Heinrich
if (status)log_warning("late_init: Failed to set run time variables\n");return 0;U-Boot 2025.01-rc3-00042-gacab6e78aca7-dirty (Dec 03 2024 - 17:17:42 +0530)
DRAM: 512 MiB EFI bounce buffer 000000005957f000-000000005d57f000 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#31
starting USB... No USB controllers found Hit any key to stop autoboot: 0
=> bdinfo
...
lmb_dump_all: memory.count = 0x1 memory[0] [0x40000000-0x5fffffff], 0x20000000 bytes, flags: none reserved.count = 0x3 reserved[0] [0x58600000-0x593fffff], 0xe00000 bytes, flags: none reserved[1] [0x5957c000-0x5d57efff], 0x4003000 bytes, flags: no-notify, no-overwrite reserved[2] [0x5d57fde0-0x5fffffff], 0x2a80220 bytes, flags: no-overwrite
...
=> printenv kernel_addr_r kernel_addr_r=58e00000 => printenv ramdisk_addr_r ramdisk_addr_r=58600000 => printenv fdt_addr_r fdt_addr_r=59000000 => printenv loadaddr loadaddr=59200000
-sughosh
Changes in v5:
- Drop the word 'warning'
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 7e7a6bf31aa..9a880251b47 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -208,6 +208,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("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;-- 2.43.0
Hi Sughosh,
On Tue, 3 Dec 2024 at 06:13, Sughosh Ganu sughosh.ganu@linaro.org wrote:
On Tue, 3 Dec 2024 at 18:22, Sughosh Ganu sughosh.ganu@linaro.org wrote:
On Tue, 3 Dec 2024 at 18:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 03.12.24 13:06, Sughosh Ganu wrote:
On Sun, 1 Dec 2024 at 20:58, Simon Glass sjg@chromium.org wrote:
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.
Why can't we "allocate" sane values for the env variables in question, similar to what is being done for the apple and qcom boards -- you can refer to arch/arm/mach-apple/board.c:board_late_init(). We won't be facing these issues then. Eventually, there can be a common, platform agnostic function to initialise these values. So, just as a PoC, I added this code to the qemu-arm.c, and no longer observe any overlaps.
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c index 6095cb02b23..bad787f7a65 100644 --- a/board/emulation/qemu-arm/qemu-arm.c +++ b/board/emulation/qemu-arm/qemu-arm.c @@ -109,6 +109,9 @@ int board_init(void)
int board_late_init(void) {
u32 status = 0;/* * Make sure virtio bus is enumerated so that peripherals * on the virtio bus can be discovered by their drivers@@ -119,6 +122,14 @@ int board_late_init(void) if (CONFIG_IS_ENABLED(USB_KEYBOARD)) usb_init();
status |= env_set_hex("loadaddr", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("fdt_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("kernel_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("ramdisk_addr_r", lmb_alloc(SZ_8M, SZ_2M));Hello Sughosh,
avoiding fixed addresses would make sense for most boards.
Your suggested memory reservations for the kernel and initrd are way too small. On my system I find
decompressed kernel - 67 MiB initrd - 77 MiB
scriptaddr is missing.
ARM have address restrictions (see function efi_get_max_initrd_addr()):
armv7: max_init_rd_addr = round_down(image_addr, SZ_4M) + SZ_512M
armv8: #define VA_BITS_MIN (48) max_init_rd_addr = round_down(image_addr, SZ_1G) + (1UL << (VA_BITS_MIN - 1))
These are not modeled in your suggestion.
I would suggest to make a single <= 512 MiB allocation for both initrd and kernel to keep both together and then assign the addresses in the allocated area, e.g.
kerneladdr = lmb_alloc(SZ_512M, SZ_4M); if (!kerneladdr) goto err; if (env_set_hex("kernel_addr_r", kerneladdr)) goto err; if (env_set_hex("initrd_addr_r", kerneladdr + SZ_256M)) goto err;
Yes, I simply wanted to illustrate here that there would not be an issue of overlaps if we used the lmb API's to get memory addresses. In fact, this logic that you have pasted above can be added through the reserve event function that Simon is introducing in this series -- one of the functions can reserve memory for all these env variables. I don't see the need to allocate some separate memory region for EFI though. Thanks.
Although the size of the allocation might have to be controlled through a config symbol, or computed at runtime, based on the size of RAM. Not all platforms might have the same amount of memory.
We can use the size of the image we are loading, when known. When not known (e.g. some tftp servers) we can set an address above the others we have used, so it can grow, then adjust the top once we know the size.
But all of this is in the future, since a lot of boards are not using bootstd and we have to respect their env vars. The latest set of boards to move is sunxi, which is blocked in Tom's tree.
Regards, Simon
On Tue, Dec 03, 2024 at 06:45:32AM -0700, Simon Glass wrote:
Hi Sughosh,
On Tue, 3 Dec 2024 at 06:13, Sughosh Ganu sughosh.ganu@linaro.org wrote:
On Tue, 3 Dec 2024 at 18:22, Sughosh Ganu sughosh.ganu@linaro.org wrote:
On Tue, 3 Dec 2024 at 18:11, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 03.12.24 13:06, Sughosh Ganu wrote:
On Sun, 1 Dec 2024 at 20:58, Simon Glass sjg@chromium.org wrote:
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.
Why can't we "allocate" sane values for the env variables in question, similar to what is being done for the apple and qcom boards -- you can refer to arch/arm/mach-apple/board.c:board_late_init(). We won't be facing these issues then. Eventually, there can be a common, platform agnostic function to initialise these values. So, just as a PoC, I added this code to the qemu-arm.c, and no longer observe any overlaps.
diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c index 6095cb02b23..bad787f7a65 100644 --- a/board/emulation/qemu-arm/qemu-arm.c +++ b/board/emulation/qemu-arm/qemu-arm.c @@ -109,6 +109,9 @@ int board_init(void)
int board_late_init(void) {
u32 status = 0;/* * Make sure virtio bus is enumerated so that peripherals * on the virtio bus can be discovered by their drivers@@ -119,6 +122,14 @@ int board_late_init(void) if (CONFIG_IS_ENABLED(USB_KEYBOARD)) usb_init();
status |= env_set_hex("loadaddr", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("fdt_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("kernel_addr_r", lmb_alloc(SZ_2M, SZ_2M));status |= env_set_hex("ramdisk_addr_r", lmb_alloc(SZ_8M, SZ_2M));Hello Sughosh,
avoiding fixed addresses would make sense for most boards.
Your suggested memory reservations for the kernel and initrd are way too small. On my system I find
decompressed kernel - 67 MiB initrd - 77 MiB
scriptaddr is missing.
ARM have address restrictions (see function efi_get_max_initrd_addr()):
armv7: max_init_rd_addr = round_down(image_addr, SZ_4M) + SZ_512M
armv8: #define VA_BITS_MIN (48) max_init_rd_addr = round_down(image_addr, SZ_1G) + (1UL << (VA_BITS_MIN - 1))
These are not modeled in your suggestion.
I would suggest to make a single <= 512 MiB allocation for both initrd and kernel to keep both together and then assign the addresses in the allocated area, e.g.
kerneladdr = lmb_alloc(SZ_512M, SZ_4M); if (!kerneladdr) goto err; if (env_set_hex("kernel_addr_r", kerneladdr)) goto err; if (env_set_hex("initrd_addr_r", kerneladdr + SZ_256M)) goto err;
Yes, I simply wanted to illustrate here that there would not be an issue of overlaps if we used the lmb API's to get memory addresses. In fact, this logic that you have pasted above can be added through the reserve event function that Simon is introducing in this series -- one of the functions can reserve memory for all these env variables. I don't see the need to allocate some separate memory region for EFI though. Thanks.
Although the size of the allocation might have to be controlled through a config symbol, or computed at runtime, based on the size of RAM. Not all platforms might have the same amount of memory.
We can use the size of the image we are loading, when known. When not
Please note that the size of what we're loading alone is not enough. The common case of "execute it right there" means needing to look at the header to see how much space BSS needs too. Happily "Image" formatted images have this information now and we're not quite as stuck as we were in older times.
known (e.g. some tftp servers) we can set an address above the others we have used, so it can grow, then adjust the top once we know the size.
But all of this is in the future, since a lot of boards are not using bootstd and we have to respect their env vars. The latest set of boards to move is sunxi, which is blocked in Tom's tree.
Yes, sunxi moving to bootstd is blocked in mainline because it's not ready yet as it doesn't support all of the use cases sunxi has and no one has implemented what was agreed on as the path forward there.
On Tue, Dec 03, 2024 at 05:36:40PM +0530, Sughosh Ganu wrote:
On Sun, 1 Dec 2024 at 20:58, Simon Glass sjg@chromium.org wrote:
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.
Why can't we "allocate" sane values for the env variables in question, similar to what is being done for the apple and qcom boards -- you can refer to arch/arm/mach-apple/board.c:board_late_init(). We won't be facing these issues then. Eventually, there can be a common, platform agnostic function to initialise these values. So, just as a PoC, I added this code to the qemu-arm.c, and no longer observe any overlaps.
There's two challenges. One challenge is that yes, we should make use of LMB generally to allocate regions in the common case, like mach-apple does. The second challenge is that everyone had agreed (I would swear) that the EFI-specific bounce buffer needs to be removed and just use the generic bounce buffer option. But no one has wanted to do the work, and so Simon keeps posting some variant on this patch.
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 3532ffec46f..0f2ae570174 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 26277b93976..5df3cdd419d 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -442,6 +442,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 76cf908c2de..344656c4eb0 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -26,6 +26,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;
/** @@ -844,3 +850,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 45143c1c7d9..cc802583d3c 100644 --- a/test/py/tests/test_event_dump.py +++ b/test/py/tests/test_event_dump.py @@ -21,5 +21,6 @@ EVT_FT_FIXUP bootmeth_vbe_simple_ft_fixup .*boot/vbe_simple_os.c:.* EVT_LAST_STAGE_INIT alloc_write_acpi_tables .*lib/acpi/acpi_table.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
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 v5: - Rebase on top of efil and efik series - Reword cover letter a little
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 a0af0db6262..600dfc2b48c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -871,6 +871,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 344656c4eb0..88ef111dc70 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -32,6 +32,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;
/** @@ -832,8 +835,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(gd->efi_region, + 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 @@ -843,7 +863,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 468156db813..c68f3ff0748 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();
participants (4)
-
Heinrich Schuchardt -
Simon Glass -
Sughosh Ganu -
Tom Rini