On Thu, 12 Dec 2024 at 15:44, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:15, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 17:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 07:48, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Some functions are passing addresses instead of pointers to the efi_add_memory_map() function. This confusion is understandable since the function arguments indicate an address.
Make a note of the 8 places where there are problems, which would break usage in sandbox tests.
Future work will resolve these problems.
NAK, this serves no purpose whatsoever. Just send a patch with whatever changes you think are needed
Please see below
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org
A request was made in v4 to drop this patch, but in the same thread, a query was sent about why we need these addresses, so at least until the series is about to be applied, I've left it in, so we can have that discussion in one place and build a shared understanding of why these addresses are needed, instead of peppering the rest of the series with similar questions.
I'm quite happy to drop this patch. But I want to first make sure that you understand that these bugs exist today in the code, albeit they only matter on sandbox.
Thei weird thing is sandbox works perfectly fine in EFI, passes all internal tests, EFI SCT tests, loads binaries etc. Any idea why that works?
From what I can tell, the code mentioned is not tested by sandbox yet. It would just segfault. We do have a lot of self tests, but my bootflow_efi should allow testing of more features over time. I noticed the runtime-code issue when looking at an EFI log.
BTW how are you running that? When I tried it on x86_64 it just crashed. Are you using the sandbox build on an Arm64 machine?
Yes, and it passes all the SCT tests.
Thanks /Ilias
You will notice that my future patches don't change the code mentioned in this file. The bugs are simply resolved by reworking the EFI-loader memory map to use addresses, instead of pointers cast to u64
So can you please confirm this makes sense? (if not, let's discuss it). Then I can drop this patch and the later one.
The ptrs to u64 doesn't make too much sense tbh. I do understand how sandbox works, but EFI deals with physical addresses which are also mapped 1:1
Yes, and that's why (in efi_boottime) the conversion is done in one place (from u64 pointers/addresses to U-Boot addresses). It simplifies the code and avoids the kind of bugs here.
Regards, Simon
Thanks /Ilias
Link: https://lore.kernel.org/u-boot/CAC_iWjLex4Z41CDpx4u07XeCYgjB7H+Ynd8aQYL-wruH...
(no changes since v2)
Changes in v2:
- Add a new patch with comments where incorrect addresses are used
lib/efi_loader/efi_acpi.c | 2 ++ lib/efi_loader/efi_bootmgr.c | 5 +++++ lib/efi_loader/efi_helper.c | 1 + lib/efi_loader/efi_smbios.c | 6 +++++- lib/efi_loader/efi_var_mem.c | 2 ++ 5 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c index 67bd7f8ca24..ffb360d461b 100644 --- a/lib/efi_loader/efi_acpi.c +++ b/lib/efi_loader/efi_acpi.c @@ -28,12 +28,14 @@ efi_status_t efi_acpi_register(void) /* Mark space used for tables */ start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK); end = ALIGN(gd->arch.table_end, EFI_PAGE_MASK);
/* TODO(sjg): This should use (ulong)map_sysmem(start, ...) */ ret = efi_add_memory_map(start, end - start, EFI_ACPI_RECLAIM_MEMORY); if (ret != EFI_SUCCESS) return ret; if (gd->arch.table_start_high) { start = ALIGN_DOWN(gd->arch.table_start_high, EFI_PAGE_MASK); end = ALIGN(gd->arch.table_end_high, EFI_PAGE_MASK);/* TODO(sjg): This should use (ulong)map_sysmem(start, ...) */ ret = efi_add_memory_map(start, end - start, EFI_ACPI_RECLAIM_MEMORY); if (ret != EFI_SUCCESS)diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 8c51a6ef2ed..bb635d77b53 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -365,6 +365,8 @@ static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size, * TODO: expose the ramdisk to OS. * Need to pass the ramdisk information by the architecture-specific * methods such as 'pmem' device-tree node.
** TODO(sjg): This should use (ulong)map_sysmem(addr) */ ret = efi_add_memory_map(addr, size, EFI_RESERVED_MEMORY_TYPE); if (ret != EFI_SUCCESS) {@@ -399,6 +401,7 @@ static efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
/* cleanup for iso or img image */ if (ctx->ramdisk_blk_dev) {
/* TODO(sjg): This should use (ulong)map_sysmem(...) */ ret = efi_add_memory_map(ctx->image_addr, ctx->image_size, EFI_CONVENTIONAL_MEMORY); if (ret != EFI_SUCCESS)@@ -506,6 +509,8 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
source_buffer = NULL; source_size = 0;
/* TODO(sjg): This does not work on sandbox */ } else if (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) { /* * loaded_dp must exist until efi application returns,diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c index bf96f61d3d0..fd89ef6036f 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -486,6 +486,7 @@ static efi_status_t copy_fdt(void **fdtp) log_err("Failed to reserve space for FDT\n"); goto done; }
/* TODO(sjg): This does not work on sandbox */ new_fdt = (void *)(uintptr_t)new_fdt_addr; memcpy(new_fdt, fdt, fdt_totalsize(fdt)); fdt_set_totalsize(new_fdt, fdt_size);diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 8d2ef6deb51..02d4a5dd045 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -40,7 +40,11 @@ efi_status_t efi_smbios_register(void) return EFI_NOT_FOUND; }
/* Mark space used for tables */
/** Mark space used for tables/** TODO(sjg): This should use (ulong)map_sysmem(addr, ...)*/ ret = efi_add_memory_map(addr, TABLE_SIZE, EFI_RUNTIME_SERVICES_DATA); if (ret) return ret;diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c index 139e16aad7c..5c69a1e0f3e 100644 --- a/lib/efi_loader/efi_var_mem.c +++ b/lib/efi_loader/efi_var_mem.c @@ -226,6 +226,8 @@ efi_status_t efi_var_mem_init(void) &memory); if (ret != EFI_SUCCESS) return ret;
/* TODO(sjg): This does not work on sandbox */ efi_var_buf = (struct efi_var_file *)(uintptr_t)memory; memset(efi_var_buf, 0, EFI_VAR_BUF_SIZE); efi_var_buf->magic = EFI_VAR_FILE_MAGIC;-- 2.34.1