[PATCH v5 00/23] efi: Tidy up confusion between pointers and addresses
The EFI-loader implementation converts things back and forth between addresses and pointers, with not much consistency in how this is done.
Within most of U-Boot a pointer is a void * and an address is a ulong
This convention is very helpful, since it is obvious in common code as to whether you need to call map_sysmem() and friends, or not.
As part of cleaning up the EFI memory-management, I found it almost impossible to know in some cases whether something is an address or a pointer. I decided to give up on that and come back to it when this is resolved.
This series starts applying the normal ulong/void * convention to the EFI_loader code, so making things easier to follow. For now, u64 is often used instead of ulong, but the effect is the same.
The main changes are: - Rather than using the external struct efi_mem_desc data-structure for internal bookkeeping, create a new one, so it can have different semantics - Within efi_memory.c addresses are used, rather than addresses masquerading as pointers. The conversions are done in efi_boottime
Unforunately my foray into attempting to use enum for the memory type failed. The problem is not just that the external interface cannot use an enum. In fact the enum in the spec is really just confusing, since values not mentioned in the enum are valid. While we could handle this by declaring a few more values in enum efi_memory_type, it doesn't seem worth it. For example, if we declare 0x6fffffff and -1 as valid values, we get the correct range, but then we need to be careful about conversion in efi_boottime. If we declare 0xffffffff as valid, then the enum ends up being 64-bits wide! Yes, that would work, I suppose it wouldn't matter, but at that point I believe using an enum is actually more confusing than not. It also made passing SCT tricky, since invalid values are passed in...
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Changes in v5: - Drop the enum / mem_type patch as it is not needed - Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v4: - Add new patch to show the address for pool allocations - Combine the various pointer-to-address patches into one
Changes in v3: - Adjust comments - Show the returned address rather than the pointer - Put the header-file in its own section - Add comment to struct efi_device_path_memory - Use a pointer for the values in struct efi_device_path_memory
Changes in v2: - Fix missing @ - Note that this holds pointers, not addresses - Add a new patch with comments where incorrect addresses are used - Use EFI_PRINT() instead of log_debug() - Rebase on early patch - Add new patch to add the EFI-loader API documentation - Add new patch to use a separate stuct for memory nodes - Drop patch 'Convert efi_get_memory_map() to return pointers' - Drop patch 'efi_loader: Make more use of ulong' - Significantly expand and redirect the series
Simon Glass (23): efi: Define fields in struct efi_mem_desc efi_loader: Fix typos in enum efi_allocate_type efi_loader: Add comments where incorrect addresses are used efi_loader: Show the resulting memory address from an alloc efi_loader: Update startimage_exit self-test to check error efi_loader: Move some memory-function comments to header doc: efi: Add the EFI-loader API documentation efi_loader: Use a separate struct for memory nodes efi_loader: Drop virtual_start from priv_mem_desc efi_loader: Drop reserved from priv_mem_desc efi_loader: Use the enum for the memory type in priv_mem_desc efi_loader: Avoid assigning desc in efi_mem_carve_out() efi_loader: Move struct efi_mem_list fields together efi_loader: Rename struct efi_mem_list to mem_node efi_loader: Rename physical_start to base efi_loader: Use correct type in efi_add_runtime_mmio() efi_loader: Show the address for pool allocations efi_loader: Don't try to add sandbox runtime code efi_loader: Update to use addresses internally efi_loader: Correct address-usage in copy_fdt() efi_loader: Drop comments about incorrect addresses efi_bootmgr: Avoid casts in try_load_from_uri_path() efi_loader: Simplify efi_dp_from_mem()
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- doc/api/efi.rst | 6 + include/efi.h | 23 +- include/efi_api.h | 10 + include/efi_loader.h | 87 +++++- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 10 +- lib/efi_loader/efi_boottime.c | 43 ++- lib/efi_loader/efi_device_path.c | 16 +- lib/efi_loader/efi_dt_fixup.c | 4 - lib/efi_loader/efi_helper.c | 4 +- lib/efi_loader/efi_image_loader.c | 3 +- lib/efi_loader/efi_memory.c | 261 +++++++----------- lib/efi_loader/efi_runtime.c | 7 +- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 18 files changed, 278 insertions(+), 225 deletions(-)
There is quite a bit of confusion in the EFI code as to whether a field contains an address or a pointer. As a first step towards resolving this, document the memory-descriptor struct, indicating that it holds pointers, not addresses.
Dro the same for efi_add_memory_map() as it is widely used, as well as efi_add_memory_map_pg() which is only used by lmb
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v3)
Changes in v3: - Adjust comments
Changes in v2: - Fix missing @ - Note that this holds pointers, not addresses
include/efi.h | 21 +++++++++++++++++++++ include/efi_loader.h | 6 ++++-- lib/efi_loader/efi_memory.c | 4 +++- 3 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/include/efi.h b/include/efi.h index ebd7e11d6e3..58838e3ff2c 100644 --- a/include/efi.h +++ b/include/efi.h @@ -276,6 +276,27 @@ enum efi_memory_type { #define EFI_PAGE_SIZE (1ULL << EFI_PAGE_SHIFT) #define EFI_PAGE_MASK (EFI_PAGE_SIZE - 1)
+/** + * struct efi_mem_desc - defines an EFI memory record + * + * This field implements the EFI_MEMORY_DESCRIPTOR type of the UEFI + * specification. + * + * Note that this struct is for use outside U-Boot, so the two 'start' fields + * are pointers, not addresses. Use map_to_sysmem() to convert to an address, so + * that sandbox operates correctly. + * + * @type (enum efi_memory_type): EFI memory-type + * @reserved: unused + * @physical_start: Start address of region in physical memory + * @virtual_start: Start address of region in virtual memory, which will be the + * same as @physical_start before where both addresses will always be the + * same before SetVirtualMemoryMap() is called as the UEFI specification + * requires identity mapping. + * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE + * bytes) + * @attribute: Memory attributes (see EFI_MEMORY_...) + */ struct efi_mem_desc { u32 type; u32 reserved; diff --git a/include/efi_loader.h b/include/efi_loader.h index 39809eac1bc..ee0cdd36500 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -788,8 +788,10 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); /** * efi_add_memory_map_pg() - add pages to the memory map * - * @start: start address, must be a multiple of - * EFI_PAGE_SIZE + * @start: start address, must be a multiple of EFI_PAGE_SIZE. Note that this + * is actually a pointer provided as an integer. To pass in an address, pass + * in (ulong)map_to_sysmem(addr) + * * @pages: number of pages to add * @memory_type: type of memory added * @overlap_conventional: region may only overlap free(conventional) diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index e493934c713..f1154f73e05 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -384,7 +384,9 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, /** * efi_add_memory_map() - add memory area to the memory map * - * @start: start address of the memory area + * @start: start address of the memory area. Note that this is + * actually a pointer provided as an integer. To pass in + * an address, pass in (ulong)map_to_sysmem(addr) * @size: length in bytes of the memory area * @memory_type: type of memory added *
Hi Simon,
[...]
u32 reserved;diff --git a/include/efi_loader.h b/include/efi_loader.h index 39809eac1bc..ee0cdd36500 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -788,8 +788,10 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); /**
- efi_add_memory_map_pg() - add pages to the memory map
- @start: start address, must be a multiple of
EFI_PAGE_SIZE
- @start: start address, must be a multiple of EFI_PAGE_SIZE. Note that this
- is actually a pointer provided as an integer. To pass in an address, pass
- in (ulong)map_to_sysmem(addr)
Why is this a pointer? This is the physical address of the hardware.
Thanks /Ilias
- @pages: number of pages to add
- @memory_type: type of memory added
- @overlap_conventional: region may only overlap free(conventional)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index e493934c713..f1154f73e05 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -384,7 +384,9 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, /**
- efi_add_memory_map() - add memory area to the memory map
- @start: start address of the memory area
- @start: start address of the memory area. Note that this is
actually a pointer provided as an integer. To pass in
an address, pass in (ulong)map_to_sysmem(addr)- @size: length in bytes of the memory area
- @memory_type: type of memory added
-- 2.34.1
Hi Ilias,
On Wed, 11 Dec 2024 at 07:45, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
[...]
u32 reserved;diff --git a/include/efi_loader.h b/include/efi_loader.h index 39809eac1bc..ee0cdd36500 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -788,8 +788,10 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); /**
- efi_add_memory_map_pg() - add pages to the memory map
- @start: start address, must be a multiple of
EFI_PAGE_SIZE
- @start: start address, must be a multiple of EFI_PAGE_SIZE. Note that this
- is actually a pointer provided as an integer. To pass in an address, pass
- in (ulong)map_to_sysmem(addr)
Why is this a pointer? This is the physical address of the hardware.
The distinction between pointer and address is to make it clear whether you need to map_sysmem() or not. That is the purpose of this series - to clean up the confusion about translating back and forth. There definitely is quite a bit of confusion in the EFI_LOADER code.
Generally in U-Boot ulong is used for an address and void * for a pointer. In the case of EFI_LOADER, the original author may not have been aware of that. Incidentally, it was also written without sandbox support (which I added) and tests[1] (which Heinrich added).
If you like we could arrange a call to go over how sandbox works with memory.
Regards, Simon
[1] https://lore.kernel.org/u-boot/7fbaabab-5d82-4670-eab6-edb9d27f0bb9@suse.de/
Hi Ilias,
On Wed, 11 Dec 2024 at 07:45, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
[...]
u32 reserved;
Applied to sjg/master, thanks!
Fix 'indicatged' and 'adress' typos.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Tom Rini trini@konsulko.com ---
(no changes since v1)
include/efi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/efi.h b/include/efi.h index 58838e3ff2c..cf5c4784b61 100644 --- a/include/efi.h +++ b/include/efi.h @@ -178,7 +178,7 @@ enum efi_allocate_type { EFI_ALLOCATE_MAX_ADDRESS, /** * @EFI_ALLOCATE_ADDRESS: - * Allocate a memory block starting at the indicatged adress. + * Allocate a memory block starting at the indicated address. */ EFI_ALLOCATE_ADDRESS, /**
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Fix 'indicatged' and 'adress' typos.
I don't really like patches that changes typos. Personally it only makes my life bisecting harder. Since Tom and Heinrich already reviewed it, I don't mind, but this really has no place in this patchset.
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Tom Rini trini@konsulko.com
(no changes since v1)
include/efi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/efi.h b/include/efi.h index 58838e3ff2c..cf5c4784b61 100644 --- a/include/efi.h +++ b/include/efi.h @@ -178,7 +178,7 @@ enum efi_allocate_type { EFI_ALLOCATE_MAX_ADDRESS, /** * @EFI_ALLOCATE_ADDRESS:
* Allocate a memory block starting at the indicatged adress.
* Allocate a memory block starting at the indicated address. */ EFI_ALLOCATE_ADDRESS, /**-- 2.34.1
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Fix 'indicatged' and 'adress' typos.
I don't really like patches that changes typos. Personally it only makes my life bisecting harder. Since Tom and Heinrich already reviewed it, I don't mind, but this really has no place in this patchset.
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Tom Rini trini@konsulko.com
(no changes since v1)
include/efi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to sjg/master, thanks!
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.
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.
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;
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
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.
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
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. 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.
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
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?
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
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
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?
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
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
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(-)
Applied to sjg/master, thanks!
Update efi_allocate_pool_ext() to log the pointer returned from this call, which can be helpful when debugging.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v3)
Changes in v3: - Show the returned address rather than the pointer
Changes in v2: - Use EFI_PRINT() instead of log_debug()
lib/efi_loader/efi_boottime.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae3..f27b3827ed2 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -13,6 +13,7 @@ #include <irq_func.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <pe.h> #include <time.h> #include <u-boot/crc.h> @@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer); r = efi_allocate_pool(pool_type, size, buffer); + if (r == EFI_SUCCESS) + EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer)); + return EFI_EXIT(r); }
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Update efi_allocate_pool_ext() to log the pointer returned from this call, which can be helpful when debugging.
Can you explain how logging the success is helping? I think its pretty pointless, why not log a failure?
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v3)
Changes in v3:
- Show the returned address rather than the pointer
Changes in v2:
- Use EFI_PRINT() instead of log_debug()
lib/efi_loader/efi_boottime.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae3..f27b3827ed2 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -13,6 +13,7 @@ #include <irq_func.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <pe.h> #include <time.h> #include <u-boot/crc.h> @@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer); r = efi_allocate_pool(pool_type, size, buffer);
if (r == EFI_SUCCESS)EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer));
buffer can be null if size == 0
return EFI_EXIT(r);}
-- 2.34.1
Hi Ilias,
On Wed, 11 Dec 2024 at 07:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Update efi_allocate_pool_ext() to log the pointer returned from this call, which can be helpful when debugging.
Can you explain how logging the success is helping? I think its pretty pointless, why not log a failure?
It shows the memory address that was allocated.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v3)
Changes in v3:
- Show the returned address rather than the pointer
Changes in v2:
- Use EFI_PRINT() instead of log_debug()
lib/efi_loader/efi_boottime.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae3..f27b3827ed2 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -13,6 +13,7 @@ #include <irq_func.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <pe.h> #include <time.h> #include <u-boot/crc.h> @@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer); r = efi_allocate_pool(pool_type, size, buffer);
if (r == EFI_SUCCESS)EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer));buffer can be null if size == 0
No, buffer cannot be NULL, I believe you are thinking of *buffer
That's one reason why I prefer bufferp to buffer. It avoids this sort of confusion.
return EFI_EXIT(r);}
-- 2.34.1
Regards, SImon
On Wed, 11 Dec 2024 at 17:53, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 07:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Update efi_allocate_pool_ext() to log the pointer returned from this call, which can be helpful when debugging.
Can you explain how logging the success is helping? I think its pretty pointless, why not log a failure?
It shows the memory address that was allocated.
Sure, but that's not what I asked.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v3)
Changes in v3:
- Show the returned address rather than the pointer
Changes in v2:
- Use EFI_PRINT() instead of log_debug()
lib/efi_loader/efi_boottime.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae3..f27b3827ed2 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -13,6 +13,7 @@ #include <irq_func.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <pe.h> #include <time.h> #include <u-boot/crc.h> @@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer); r = efi_allocate_pool(pool_type, size, buffer);
if (r == EFI_SUCCESS)EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer));buffer can be null if size == 0
No, buffer cannot be NULL, I believe you are thinking of *buffer
That's one reason why I prefer bufferp to buffer. It avoids this sort of confusion.
I personally don't find any of this confusing. Yes, I am talking about *buffer, but I am pretty sure this is obvious in the context of the if check. It can be NULL
Thanks /Ilias
return EFI_EXIT(r);}
-- 2.34.1
Regards, SImon
Hi Ilias,
On Wed, 11 Dec 2024 at 23:22, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 17:53, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 07:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Update efi_allocate_pool_ext() to log the pointer returned from this call, which can be helpful when debugging.
Can you explain how logging the success is helping? I think its pretty pointless, why not log a failure?
It shows the memory address that was allocated.
Sure, but that's not what I asked.
On failure, the address is undefined, so logging it just adds confusion. It will likely be a random number.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v3)
Changes in v3:
- Show the returned address rather than the pointer
Changes in v2:
- Use EFI_PRINT() instead of log_debug()
lib/efi_loader/efi_boottime.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae3..f27b3827ed2 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -13,6 +13,7 @@ #include <irq_func.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <pe.h> #include <time.h> #include <u-boot/crc.h> @@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer); r = efi_allocate_pool(pool_type, size, buffer);
if (r == EFI_SUCCESS)EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer));buffer can be null if size == 0
No, buffer cannot be NULL, I believe you are thinking of *buffer
That's one reason why I prefer bufferp to buffer. It avoids this sort of confusion.
I personally don't find any of this confusing. Yes, I am talking about *buffer, but I am pretty sure this is obvious in the context of the if check. It can be NULL
OK, then I am missing your point. Could you expand on what you mean here?
Regards, Simon
On Thu, 12 Dec 2024 at 15:44, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:22, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 17:53, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 07:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Update efi_allocate_pool_ext() to log the pointer returned from this call, which can be helpful when debugging.
Can you explain how logging the success is helping? I think its pretty pointless, why not log a failure?
It shows the memory address that was allocated.
Sure, but that's not what I asked.
On failure, the address is undefined, so logging it just adds confusion. It will likely be a random number.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v3)
Changes in v3:
- Show the returned address rather than the pointer
Changes in v2:
- Use EFI_PRINT() instead of log_debug()
lib/efi_loader/efi_boottime.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae3..f27b3827ed2 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -13,6 +13,7 @@ #include <irq_func.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <pe.h> #include <time.h> #include <u-boot/crc.h> @@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer); r = efi_allocate_pool(pool_type, size, buffer);
if (r == EFI_SUCCESS)EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer));buffer can be null if size == 0
No, buffer cannot be NULL, I believe you are thinking of *buffer
That's one reason why I prefer bufferp to buffer. It avoids this sort of confusion.
I personally don't find any of this confusing. Yes, I am talking about *buffer, but I am pretty sure this is obvious in the context of the if check. It can be NULL
OK, then I am missing your point. Could you expand on what you mean here?
EFI is require identity mapping. You have a physical address but if you want to dereference the ptr it it's *(virt addr). Since it's 1:1 *(phys addr) would also work, but we shouldn't do the latter.
Thanks /Ilias
Regards, Simon
On Thu, 12 Dec 2024 at 17:46, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:44, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:22, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 17:53, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 07:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Update efi_allocate_pool_ext() to log the pointer returned from this call, which can be helpful when debugging.
Can you explain how logging the success is helping? I think its pretty pointless, why not log a failure?
It shows the memory address that was allocated.
Sure, but that's not what I asked.
On failure, the address is undefined, so logging it just adds confusion. It will likely be a random number.
I mean just report a failure to update. What use can the allocated address be of?
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v3)
Changes in v3:
- Show the returned address rather than the pointer
Changes in v2:
- Use EFI_PRINT() instead of log_debug()
lib/efi_loader/efi_boottime.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae3..f27b3827ed2 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -13,6 +13,7 @@ #include <irq_func.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <pe.h> #include <time.h> #include <u-boot/crc.h> @@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer); r = efi_allocate_pool(pool_type, size, buffer);
if (r == EFI_SUCCESS)EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer));buffer can be null if size == 0
No, buffer cannot be NULL, I believe you are thinking of *buffer
That's one reason why I prefer bufferp to buffer. It avoids this sort of confusion.
I personally don't find any of this confusing. Yes, I am talking about *buffer, but I am pretty sure this is obvious in the context of the if check. It can be NULL
OK, then I am missing your point. Could you expand on what you mean here?
EFI is require identity mapping. You have a physical address but if you want to dereference the ptr it it's *(virt addr). Since it's 1:1 *(phys addr) would also work, but we shouldn't do the latter.
Thanks /Ilias
Regards, Simon
Hi Ilias,
On Thu, 12 Dec 2024 at 08:48, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 17:46, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:44, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:22, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 17:53, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 07:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote: > > Update efi_allocate_pool_ext() to log the pointer returned from this > call, which can be helpful when debugging.
Can you explain how logging the success is helping? I think its pretty pointless, why not log a failure?
It shows the memory address that was allocated.
Sure, but that's not what I asked.
On failure, the address is undefined, so logging it just adds confusion. It will likely be a random number.
I mean just report a failure to update. What use can the allocated address be of?
Can you please rewrite the patch so I can understand what you are getting at?
Thanks /Ilias
> > Signed-off-by: Simon Glass sjg@chromium.org > --- > > (no changes since v3) > > Changes in v3: > - Show the returned address rather than the pointer > > Changes in v2: > - Use EFI_PRINT() instead of log_debug() > > lib/efi_loader/efi_boottime.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > index 080e7f78ae3..f27b3827ed2 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -13,6 +13,7 @@ > #include <irq_func.h> > #include <log.h> > #include <malloc.h> > +#include <mapmem.h> > #include <pe.h> > #include <time.h> > #include <u-boot/crc.h> > @@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type, > > EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer); > r = efi_allocate_pool(pool_type, size, buffer); > + if (r == EFI_SUCCESS) > + EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer));
buffer can be null if size == 0
No, buffer cannot be NULL, I believe you are thinking of *buffer
That's one reason why I prefer bufferp to buffer. It avoids this sort of confusion.
I personally don't find any of this confusing. Yes, I am talking about *buffer, but I am pretty sure this is obvious in the context of the if check. It can be NULL
OK, then I am missing your point. Could you expand on what you mean here?
EFI is require identity mapping. You have a physical address but if you want to dereference the ptr it it's *(virt addr). Since it's 1:1 *(phys addr) would also work, but we shouldn't do the latter.
We need to talk about sandbox...
Regards, Simon
Hi Ilias,
On Thu, 12 Dec 2024 at 08:48, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 17:46, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:44, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:22, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 17:53, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 07:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote: > > Update efi_allocate_pool_ext() to log the pointer returned from this > call, which can be helpful when debugging.
Can you explain how logging the success is helping? I think its pretty pointless, why not log a failure?
It shows the memory address that was allocated.
Sure, but that's not what I asked.
On failure, the address is undefined, so logging it just adds confusion. It will likely be a random number.
I mean just report a failure to update. What use can the allocated address be of?
Can you please rewrite the patch so I can understand what you are getting at?
Thanks /Ilias
> > Signed-off-by: Simon Glass sjg@chromium.org > --- > > (no changes since v3) > > Changes in v3: > - Show the returned address rather than the pointer > > Changes in v2: > - Use EFI_PRINT() instead of log_debug() > > lib/efi_loader/efi_boottime.c | 4 ++++ > 1 file changed, 4 insertions(+) >
Applied to sjg/master, thanks!
Check for an error returned from the decompress() function, just in case.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org ---
(no changes since v1)
lib/efi_selftest/efi_selftest_startimage_exit.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lib/efi_selftest/efi_selftest_startimage_exit.c b/lib/efi_selftest/efi_selftest_startimage_exit.c index b65a10b7a4b..8d119f054c5 100644 --- a/lib/efi_selftest/efi_selftest_startimage_exit.c +++ b/lib/efi_selftest/efi_selftest_startimage_exit.c @@ -84,13 +84,15 @@ static efi_status_t decompress(u8 **image) static int setup(const efi_handle_t handle, const struct efi_system_table *systable) { + efi_status_t ret; + image_handle = handle; boottime = systable->boottime;
/* Load the application image into memory */ - decompress(&image); + ret = decompress(&image);
- return EFI_ST_SUCCESS; + return ret; }
/*
Check for an error returned from the decompress() function, just in case.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org ---
(no changes since v1)
lib/efi_selftest/efi_selftest_startimage_exit.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Applied to sjg/master, thanks!
Exported functions should be documented in the header file, not the implementation. We tend to make such updates on a piecemeal basis to avoid a 'flag day'. Move some comments related to memory allocation to follow the convention.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - Rebase on early patch
include/efi_loader.h | 77 +++++++++++++++++++++++++++++++---- lib/efi_loader/efi_memory.c | 81 ------------------------------------- 2 files changed, 69 insertions(+), 89 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index ee0cdd36500..00a1259c006 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -758,21 +758,68 @@ 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 and zero it out. + * + * @size: number of bytes to allocate + * Return: pointer to allocated memory or NULL + */ void *efi_alloc(size_t len); -/* Allocate pages on the specified alignment */ + +/** + * efi_alloc_aligned_pages() - allocate aligned memory pages + * + * @len: len in bytes + * @memory_type: usage type of the allocated memory + * @align: alignment in bytes + * Return: aligned memory or NULL + */ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align); -/* More specific EFI memory allocator, called by EFI payloads */ + +/** + * efi_allocate_pages - allocate memory pages + * + * @type: type of allocation to be performed + * @memory_type: usage type of the allocated memory + * @pages: number of pages to be allocated + * @memory: returns a pointer to the allocated memory + * Return: status code + */ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type memory_type, efi_uintn_t pages, uint64_t *memory); -/* EFI memory free function. */ + +/** + * efi_free_pages() - free memory pages + * + * @memory: start of the memory area to be freed + * @pages: number of pages to be freed + * Return: status code + */ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages); -/* EFI memory allocator for small allocations */ + +/** + * efi_allocate_pool - allocate memory from pool + * + * @pool_type: type of the pool from which memory is to be allocated + * @size: number of bytes to be allocated + * @buffer: allocated memory + * Return: status code + */ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer); -/* EFI pool memory free function. */ + +/** + * efi_free_pool() - free memory from pool + * + * @buffer: start of memory to be freed + * Return: status code + */ efi_status_t efi_free_pool(void *buffer); + /* Allocate and retrieve EFI memory map */ efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size, struct efi_mem_desc **memory_map); @@ -782,7 +829,21 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, efi_uintn_t *map_key, efi_uintn_t *descriptor_size, uint32_t *descriptor_version); -/* Adds a range into the EFI memory map */ + +/** + * efi_add_memory_map() - add memory area to the memory map + * + * @start: start address of the memory area. Note that this is + * actually a pointer provided as an integer. To pass in + * an address, pass in (ulong)map_to_sysmem(addr) + * @size: length in bytes of the memory area + * @memory_type: type of memory added + * + * Return: status code + * + * This function automatically aligns the start and size of the memory area + * to EFI_PAGE_SIZE. + */ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
/** @@ -793,7 +854,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); * in (ulong)map_to_sysmem(addr) * * @pages: number of pages to add - * @memory_type: type of memory added + * @memory_type: EFI type of memory added * @overlap_conventional: region may only overlap free(conventional) * memory * Return: status code diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index f1154f73e05..4bc6d12a1fe 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -257,17 +257,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_LOOP_AGAIN; }
-/** - * efi_add_memory_map_pg() - add pages to the memory map - * - * @start: start address, must be a multiple of - * EFI_PAGE_SIZE - * @pages: number of pages to add - * @memory_type: type of memory added - * @overlap_conventional: region may only overlap free(conventional) - * memory - * Return: status code - */ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, int memory_type, bool overlap_conventional) @@ -381,20 +370,6 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_SUCCESS; }
-/** - * efi_add_memory_map() - add memory area to the memory map - * - * @start: start address of the memory area. Note that this is - * actually a pointer provided as an integer. To pass in - * an address, pass in (ulong)map_to_sysmem(addr) - * @size: length in bytes of the memory area - * @memory_type: type of memory added - * - * Return: status code - * - * This function automatically aligns the start and size of the memory area - * to EFI_PAGE_SIZE. - */ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type) { u64 pages; @@ -440,15 +415,6 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) return EFI_NOT_FOUND; }
-/** - * efi_allocate_pages - allocate memory pages - * - * @type: type of allocation to be performed - * @memory_type: usage type of the allocated memory - * @pages: number of pages to be allocated - * @memory: allocated memory - * Return: status code - */ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type memory_type, efi_uintn_t pages, uint64_t *memory) @@ -516,13 +482,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, return EFI_SUCCESS; }
-/** - * efi_free_pages() - free memory pages - * - * @memory: start of the memory area to be freed - * @pages: number of pages to be freed - * Return: status code - */ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) { u64 len; @@ -556,14 +515,6 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return ret; }
-/** - * efi_alloc_aligned_pages() - allocate aligned memory pages - * - * @len: len in bytes - * @memory_type: usage type of the allocated memory - * @align: alignment in bytes - * Return: aligned memory or NULL - */ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) { u64 req_pages = efi_size_in_pages(len); @@ -608,14 +559,6 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) return (void *)(uintptr_t)aligned_mem; }
-/** - * efi_allocate_pool - allocate memory from pool - * - * @pool_type: type of the pool from which memory is to be allocated - * @size: number of bytes to be allocated - * @buffer: allocated memory - * Return: status code - */ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer) { efi_status_t r; @@ -644,14 +587,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; @@ -666,12 +601,6 @@ void *efi_alloc(size_t size) return buf; }
-/** - * efi_free_pool() - free memory from pool - * - * @buffer: start of memory to be freed - * Return: status code - */ efi_status_t efi_free_pool(void *buffer) { efi_status_t ret; @@ -793,16 +722,6 @@ efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size, return ret; }
-/** - * efi_add_known_memory() - add memory types to the EFI memory map - * - * This function is to be used to add different memory types other - * than EFI_CONVENTIONAL_MEMORY to the EFI memory map. The conventional - * memory is handled by the LMB module and gets added to the memory - * map through the LMB module. - * - * This function may be overridden for architectures specific purposes. - */ __weak void efi_add_known_memory(void) { }
There was discussion about this and I am not sure what people prefer. What I do care about is that this has no place in this patchset. Please send it separately.
Thanks /Ilias
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Exported functions should be documented in the header file, not the implementation. We tend to make such updates on a piecemeal basis to avoid a 'flag day'. Move some comments related to memory allocation to follow the convention.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Rebase on early patch
include/efi_loader.h | 77 +++++++++++++++++++++++++++++++---- lib/efi_loader/efi_memory.c | 81 ------------------------------------- 2 files changed, 69 insertions(+), 89 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index ee0cdd36500..00a1259c006 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -758,21 +758,68 @@ 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 and zero it out.
- @size: number of bytes to allocate
- Return: pointer to allocated memory or NULL
- */
void *efi_alloc(size_t len); -/* Allocate pages on the specified alignment */
+/**
- efi_alloc_aligned_pages() - allocate aligned memory pages
- @len: len in bytes
- @memory_type: usage type of the allocated memory
- @align: alignment in bytes
- Return: aligned memory or NULL
- */
void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align); -/* More specific EFI memory allocator, called by EFI payloads */
+/**
- efi_allocate_pages - allocate memory pages
- @type: type of allocation to be performed
- @memory_type: usage type of the allocated memory
- @pages: number of pages to be allocated
- @memory: returns a pointer to the allocated memory
- Return: status code
- */
efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type memory_type, efi_uintn_t pages, uint64_t *memory); -/* EFI memory free function. */
+/**
- efi_free_pages() - free memory pages
- @memory: start of the memory area to be freed
- @pages: number of pages to be freed
- Return: status code
- */
efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages); -/* EFI memory allocator for small allocations */
+/**
- efi_allocate_pool - allocate memory from pool
- @pool_type: type of the pool from which memory is to be allocated
- @size: number of bytes to be allocated
- @buffer: allocated memory
- Return: status code
- */
efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer); -/* EFI pool memory free function. */
+/**
- efi_free_pool() - free memory from pool
- @buffer: start of memory to be freed
- Return: status code
- */
efi_status_t efi_free_pool(void *buffer);
/* Allocate and retrieve EFI memory map */ efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size, struct efi_mem_desc **memory_map); @@ -782,7 +829,21 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, efi_uintn_t *map_key, efi_uintn_t *descriptor_size, uint32_t *descriptor_version); -/* Adds a range into the EFI memory map */
+/**
- efi_add_memory_map() - add memory area to the memory map
- @start: start address of the memory area. Note that this is
actually a pointer provided as an integer. To pass in
an address, pass in (ulong)map_to_sysmem(addr)
- @size: length in bytes of the memory area
- @memory_type: type of memory added
- Return: status code
- This function automatically aligns the start and size of the memory area
- to EFI_PAGE_SIZE.
- */
efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
/** @@ -793,7 +854,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
- in (ulong)map_to_sysmem(addr)
- @pages: number of pages to add
- @memory_type: type of memory added
- @memory_type: EFI type of memory added
- @overlap_conventional: region may only overlap free(conventional)
memory- Return: status code
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index f1154f73e05..4bc6d12a1fe 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -257,17 +257,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_LOOP_AGAIN; }
-/**
- efi_add_memory_map_pg() - add pages to the memory map
- @start: start address, must be a multiple of
EFI_PAGE_SIZE
- @pages: number of pages to add
- @memory_type: type of memory added
- @overlap_conventional: region may only overlap free(conventional)
memory
- Return: status code
- */
efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, int memory_type, bool overlap_conventional) @@ -381,20 +370,6 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_SUCCESS; }
-/**
- efi_add_memory_map() - add memory area to the memory map
- @start: start address of the memory area. Note that this is
actually a pointer provided as an integer. To pass in
an address, pass in (ulong)map_to_sysmem(addr)
- @size: length in bytes of the memory area
- @memory_type: type of memory added
- Return: status code
- This function automatically aligns the start and size of the memory area
- to EFI_PAGE_SIZE.
- */
efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type) { u64 pages; @@ -440,15 +415,6 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) return EFI_NOT_FOUND; }
-/**
- efi_allocate_pages - allocate memory pages
- @type: type of allocation to be performed
- @memory_type: usage type of the allocated memory
- @pages: number of pages to be allocated
- @memory: allocated memory
- Return: status code
- */
efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type memory_type, efi_uintn_t pages, uint64_t *memory) @@ -516,13 +482,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, return EFI_SUCCESS; }
-/**
- efi_free_pages() - free memory pages
- @memory: start of the memory area to be freed
- @pages: number of pages to be freed
- Return: status code
- */
efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) { u64 len; @@ -556,14 +515,6 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return ret; }
-/**
- efi_alloc_aligned_pages() - allocate aligned memory pages
- @len: len in bytes
- @memory_type: usage type of the allocated memory
- @align: alignment in bytes
- Return: aligned memory or NULL
- */
void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) { u64 req_pages = efi_size_in_pages(len); @@ -608,14 +559,6 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) return (void *)(uintptr_t)aligned_mem; }
-/**
- efi_allocate_pool - allocate memory from pool
- @pool_type: type of the pool from which memory is to be allocated
- @size: number of bytes to be allocated
- @buffer: allocated memory
- Return: status code
- */
efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer) { efi_status_t r; @@ -644,14 +587,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; @@ -666,12 +601,6 @@ void *efi_alloc(size_t size) return buf; }
-/**
- efi_free_pool() - free memory from pool
- @buffer: start of memory to be freed
- Return: status code
- */
efi_status_t efi_free_pool(void *buffer) { efi_status_t ret; @@ -793,16 +722,6 @@ efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size, return ret; }
-/**
- efi_add_known_memory() - add memory types to the EFI memory map
- This function is to be used to add different memory types other
- than EFI_CONVENTIONAL_MEMORY to the EFI memory map. The conventional
- memory is handled by the LMB module and gets added to the memory
- map through the LMB module.
- This function may be overridden for architectures specific purposes.
- */
__weak void efi_add_known_memory(void) { } -- 2.34.1
There was discussion about this and I am not sure what people prefer. What I do care about is that this has no place in this patchset. Please send it separately.
Thanks /Ilias
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Exported functions should be documented in the header file, not the implementation. We tend to make such updates on a piecemeal basis to avoid a 'flag day'. Move some comments related to memory allocation to follow the convention.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Rebase on early patch
include/efi_loader.h | 77 +++++++++++++++++++++++++++++++---- lib/efi_loader/efi_memory.c | 81 ------------------------------------- 2 files changed, 69 insertions(+), 89 deletions(-)
Applied to sjg/master, thanks!
Bring in the documentation from the efi_loader.h header file, so we can see the API defined there.
Fix efi_alloc() to avoid a warning.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de ---
(no changes since v3)
Changes in v3: - Put the header-file in its own section
Changes in v2: - Add new patch to add the EFI-loader API documentation
doc/api/efi.rst | 6 ++++++ include/efi_loader.h | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/doc/api/efi.rst b/doc/api/efi.rst index 43d6f936fb0..49814bcf5ea 100644 --- a/doc/api/efi.rst +++ b/doc/api/efi.rst @@ -54,6 +54,12 @@ drivers, handles, loaded images, and the memory map). .. kernel-doc:: cmd/efidebug.c :internal:
+Overall API +----------- + +.. kernel-doc:: include/efi_loader.h + :internal: + Initialization of the UEFI sub-system -------------------------------------
diff --git a/include/efi_loader.h b/include/efi_loader.h index 00a1259c006..3f75f2efcb6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -764,7 +764,7 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf, * * Allocate memory from pool and zero it out. * - * @size: number of bytes to allocate + * @len: number of bytes to allocate * Return: pointer to allocated memory or NULL */ void *efi_alloc(size_t len);
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Bring in the documentation from the efi_loader.h header file, so we can see the API defined there.
Fix efi_alloc() to avoid a warning.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de
(no changes since v3)
Changes in v3:
- Put the header-file in its own section
Changes in v2:
- Add new patch to add the EFI-loader API documentation
doc/api/efi.rst | 6 ++++++ include/efi_loader.h | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/doc/api/efi.rst b/doc/api/efi.rst index 43d6f936fb0..49814bcf5ea 100644 --- a/doc/api/efi.rst +++ b/doc/api/efi.rst @@ -54,6 +54,12 @@ drivers, handles, loaded images, and the memory map). .. kernel-doc:: cmd/efidebug.c :internal:
+Overall API +-----------
+.. kernel-doc:: include/efi_loader.h
- :internal:
Initialization of the UEFI sub-system
diff --git a/include/efi_loader.h b/include/efi_loader.h index 00a1259c006..3f75f2efcb6 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -764,7 +764,7 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
- Allocate memory from pool and zero it out.
- @size: number of bytes to allocate
*/
- @len: number of bytes to allocate
- Return: pointer to allocated memory or NULL
void *efi_alloc(size_t len);
2.34.1
If that gets placed on a different patchset Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Bring in the documentation from the efi_loader.h header file, so we can see the API defined there.
Fix efi_alloc() to avoid a warning.
Signed-off-by: Simon Glass sjg@chromium.org Suggested-by: Heinrich Schuchardt xypron.glpk@gmx.de
(no changes since v3)
Changes in v3:
- Put the header-file in its own section
Changes in v2:
- Add new patch to add the EFI-loader API documentation
doc/api/efi.rst | 6 ++++++ include/efi_loader.h | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)
Applied to sjg/master, thanks!
At present efi_memory uses struct efi_mem_desc for each node of its memory list. This is convenient but is not ideal, since:
- it means that the fields are under a 'desc' sub-structure - it includes an unused 'reserved' field - it includes virtual_start which is confusing as it is always the same as physical_start - we must use u64 to store pointers, since that is how they are returned by calls to efi_get_memory_map()
As a first step to tidying this up, create a new, private struct to hold these fields.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v2)
Changes in v2: - Add new patch to use a separate stuct for memory nodes
lib/efi_loader/efi_memory.c | 49 +++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 4bc6d12a1fe..db40aee8353 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -27,9 +27,39 @@ DECLARE_GLOBAL_DATA_PTR;
efi_uintn_t efi_memory_map_key;
+/** + * struct priv_mem_desc - defines an memory region + * + * Note that this struct is for use inside U-Boot and is not visible to the + * EFI application, other than through calls to efi_get_memory_map(), where this + * internal format is converted to the external struct efi_mem_desc format. + * + * @type (enum efi_memory_type): EFI memory-type + * @reserved: unused + * @physical_start: Start address of region in physical memory + * @virtual_start: Start address of region in physical memory + * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE + * bytes) + * @attribute: Memory attributes (see EFI_MEMORY...) + */ +struct priv_mem_desc { + u32 type; + u32 reserved; + efi_physical_addr_t physical_start; + efi_virtual_addr_t virtual_start; + u64 num_pages; + u64 attribute; +}; + +/** + * struct efi_mem_list - defines an EFI memory record + * + * @link: Link to prev/next node in list + * @desc: Memory information about this node + */ struct efi_mem_list { struct list_head link; - struct efi_mem_desc desc; + struct priv_mem_desc desc; };
#define EFI_CARVE_NO_OVERLAP -1 @@ -116,7 +146,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b) * @desc: memory descriptor * Return: end address + 1 */ -static uint64_t desc_get_end(struct efi_mem_desc *desc) +static uint64_t desc_get_end(struct priv_mem_desc *desc) { return desc->physical_start + (desc->num_pages << EFI_PAGE_SHIFT); } @@ -138,8 +168,8 @@ static void efi_mem_sort(void) while (merge_again) { merge_again = false; list_for_each_entry(lmem, &efi_mem, link) { - struct efi_mem_desc *prev; - struct efi_mem_desc *cur; + struct priv_mem_desc *prev; + struct priv_mem_desc *cur; uint64_t pages;
if (!prevmem) { @@ -194,11 +224,11 @@ static void efi_mem_sort(void) * to re-add the already carved out pages to the mapping. */ static s64 efi_mem_carve_out(struct efi_mem_list *map, - struct efi_mem_desc *carve_desc, + struct priv_mem_desc *carve_desc, bool overlap_conventional) { struct efi_mem_list *newmap; - struct efi_mem_desc *map_desc = &map->desc; + struct priv_mem_desc *map_desc = &map->desc; uint64_t map_start = map_desc->physical_start; uint64_t map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT); uint64_t carve_start = carve_desc->physical_start; @@ -678,7 +708,12 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, /* Return the list in ascending order */ memory_map = &memory_map[map_entries - 1]; list_for_each_entry(lmem, &efi_mem, link) { - *memory_map = lmem->desc; + memory_map->type = lmem->desc.type; + memory_map->reserved = lmem->desc.reserved; + memory_map->physical_start = lmem->desc.physical_start; + memory_map->virtual_start = lmem->desc.virtual_start; + memory_map->num_pages = lmem->desc.num_pages; + memory_map->attribute = lmem->desc.attribute; memory_map--; }
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
At present efi_memory uses struct efi_mem_desc for each node of its memory list. This is convenient but is not ideal, since:
- it means that the fields are under a 'desc' sub-structure
- it includes an unused 'reserved' field
That's interesting. The EFI spec does not define that and I looked as back as 2.5. Heinrich, this predates my involvement with EFI, do you remember why it was added?
Because it's defined in
- it includes virtual_start which is confusing as it is always the same as physical_start
- we must use u64 to store pointers, since that is how they are returned by calls to efi_get_memory_map()
I don't understand the 'pointers' again here. It's a physical address.
As a first step to tidying this up, create a new, private struct to hold these fields.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to use a separate stuct for memory nodes
lib/efi_loader/efi_memory.c | 49 +++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 4bc6d12a1fe..db40aee8353 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -27,9 +27,39 @@ DECLARE_GLOBAL_DATA_PTR;
efi_uintn_t efi_memory_map_key;
+/**
- struct priv_mem_desc - defines an memory region
- Note that this struct is for use inside U-Boot and is not visible to the
- EFI application, other than through calls to efi_get_memory_map(), where this
- internal format is converted to the external struct efi_mem_desc format.
- @type (enum efi_memory_type): EFI memory-type
- @reserved: unused
- @physical_start: Start address of region in physical memory
- @virtual_start: Start address of region in physical memory
- @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
bytes)
- @attribute: Memory attributes (see EFI_MEMORY...)
- */
+struct priv_mem_desc {
u32 type;u32 reserved;efi_physical_addr_t physical_start;efi_virtual_addr_t virtual_start;u64 num_pages;u64 attribute;+};
This is not private, it's described by the EFI spec.
+/**
- struct efi_mem_list - defines an EFI memory record
- @link: Link to prev/next node in list
- @desc: Memory information about this node
- */
struct efi_mem_list { struct list_head link;
struct efi_mem_desc desc;
struct priv_mem_desc desc;};
#define EFI_CARVE_NO_OVERLAP -1 @@ -116,7 +146,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
- @desc: memory descriptor
- Return: end address + 1
*/ -static uint64_t desc_get_end(struct efi_mem_desc *desc) +static uint64_t desc_get_end(struct priv_mem_desc *desc) { return desc->physical_start + (desc->num_pages << EFI_PAGE_SHIFT); } @@ -138,8 +168,8 @@ static void efi_mem_sort(void) while (merge_again) { merge_again = false; list_for_each_entry(lmem, &efi_mem, link) {
struct efi_mem_desc *prev;struct efi_mem_desc *cur;
struct priv_mem_desc *prev;struct priv_mem_desc *cur; uint64_t pages; if (!prevmem) {@@ -194,11 +224,11 @@ static void efi_mem_sort(void)
- to re-add the already carved out pages to the mapping.
*/ static s64 efi_mem_carve_out(struct efi_mem_list *map,
struct efi_mem_desc *carve_desc,
struct priv_mem_desc *carve_desc, bool overlap_conventional){ struct efi_mem_list *newmap;
struct efi_mem_desc *map_desc = &map->desc;
struct priv_mem_desc *map_desc = &map->desc; uint64_t map_start = map_desc->physical_start; uint64_t map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT); uint64_t carve_start = carve_desc->physical_start;@@ -678,7 +708,12 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, /* Return the list in ascending order */ memory_map = &memory_map[map_entries - 1]; list_for_each_entry(lmem, &efi_mem, link) {
*memory_map = lmem->desc;
What's wrong with that ?
memory_map->type = lmem->desc.type;memory_map->reserved = lmem->desc.reserved;memory_map->physical_start = lmem->desc.physical_start;memory_map->virtual_start = lmem->desc.virtual_start;memory_map->num_pages = lmem->desc.num_pages;memory_map->attribute = lmem->desc.attribute; memory_map--; }-- 2.34.1
The only problem I see here is the reserved field. I think this patch should just remove that
Thanks /Ilias
Hi Ilias,
On Wed, 11 Dec 2024 at 08:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
At present efi_memory uses struct efi_mem_desc for each node of its memory list. This is convenient but is not ideal, since:
- it means that the fields are under a 'desc' sub-structure
- it includes an unused 'reserved' field
That's interesting. The EFI spec does not define that and I looked as back as 2.5. Heinrich, this predates my involvement with EFI, do you remember why it was added?
Because it's defined in
- it includes virtual_start which is confusing as it is always the same as physical_start
- we must use u64 to store pointers, since that is how they are returned by calls to efi_get_memory_map()
I don't understand the 'pointers' again here. It's a physical address.
Let's have a call so I can explain this...
As a first step to tidying this up, create a new, private struct to hold these fields.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to use a separate stuct for memory nodes
lib/efi_loader/efi_memory.c | 49 +++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 4bc6d12a1fe..db40aee8353 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -27,9 +27,39 @@ DECLARE_GLOBAL_DATA_PTR;
efi_uintn_t efi_memory_map_key;
+/**
- struct priv_mem_desc - defines an memory region
- Note that this struct is for use inside U-Boot and is not visible to the
- EFI application, other than through calls to efi_get_memory_map(), where this
- internal format is converted to the external struct efi_mem_desc format.
- @type (enum efi_memory_type): EFI memory-type
- @reserved: unused
- @physical_start: Start address of region in physical memory
- @virtual_start: Start address of region in physical memory
- @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
bytes)
- @attribute: Memory attributes (see EFI_MEMORY...)
- */
+struct priv_mem_desc {
u32 type;u32 reserved;efi_physical_addr_t physical_start;efi_virtual_addr_t virtual_start;u64 num_pages;u64 attribute;+};
This is not private, it's described by the EFI spec.
It is private to this module, and future patches change it to be used only in this file, so I have commented and named it that way.
+/**
- struct efi_mem_list - defines an EFI memory record
- @link: Link to prev/next node in list
- @desc: Memory information about this node
- */
struct efi_mem_list { struct list_head link;
struct efi_mem_desc desc;
struct priv_mem_desc desc;};
#define EFI_CARVE_NO_OVERLAP -1 @@ -116,7 +146,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b)
- @desc: memory descriptor
- Return: end address + 1
*/ -static uint64_t desc_get_end(struct efi_mem_desc *desc) +static uint64_t desc_get_end(struct priv_mem_desc *desc) { return desc->physical_start + (desc->num_pages << EFI_PAGE_SHIFT); } @@ -138,8 +168,8 @@ static void efi_mem_sort(void) while (merge_again) { merge_again = false; list_for_each_entry(lmem, &efi_mem, link) {
struct efi_mem_desc *prev;struct efi_mem_desc *cur;
struct priv_mem_desc *prev;struct priv_mem_desc *cur; uint64_t pages; if (!prevmem) {@@ -194,11 +224,11 @@ static void efi_mem_sort(void)
- to re-add the already carved out pages to the mapping.
*/ static s64 efi_mem_carve_out(struct efi_mem_list *map,
struct efi_mem_desc *carve_desc,
struct priv_mem_desc *carve_desc, bool overlap_conventional){ struct efi_mem_list *newmap;
struct efi_mem_desc *map_desc = &map->desc;
struct priv_mem_desc *map_desc = &map->desc; uint64_t map_start = map_desc->physical_start; uint64_t map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT); uint64_t carve_start = carve_desc->physical_start;@@ -678,7 +708,12 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, /* Return the list in ascending order */ memory_map = &memory_map[map_entries - 1]; list_for_each_entry(lmem, &efi_mem, link) {
*memory_map = lmem->desc;What's wrong with that ?
There are two different structs now
memory_map->type = lmem->desc.type;memory_map->reserved = lmem->desc.reserved;memory_map->physical_start = lmem->desc.physical_start;memory_map->virtual_start = lmem->desc.virtual_start;memory_map->num_pages = lmem->desc.num_pages;memory_map->attribute = lmem->desc.attribute; memory_map--; }-- 2.34.1
The only problem I see here is the reserved field. I think this patch should just remove that
OK
Regards, Simon
Hi Ilias,
On Wed, 11 Dec 2024 at 08:08, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
At present efi_memory uses struct efi_mem_desc for each node of its memory list. This is convenient but is not ideal, since:
- it means that the fields are under a 'desc' sub-structure
- it includes an unused 'reserved' field
That's interesting. The EFI spec does not define that and I looked as back as 2.5. Heinrich, this predates my involvement with EFI, do you remember why it was added?
Because it's defined in
- it includes virtual_start which is confusing as it is always the same as physical_start
- we must use u64 to store pointers, since that is how they are returned by calls to efi_get_memory_map()
I don't understand the 'pointers' again here. It's a physical address.
Let's have a call so I can explain this...
As a first step to tidying this up, create a new, private struct to hold these fields.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v2)
Changes in v2:
- Add new patch to use a separate stuct for memory nodes
lib/efi_loader/efi_memory.c | 49 +++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-)
Applied to sjg/master, thanks!
This field is always the same as physical_start, so keeping track of it separately is unnecessary and confusing. Drop it.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index db40aee8353..8211600f7f6 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -37,7 +37,6 @@ efi_uintn_t efi_memory_map_key; * @type (enum efi_memory_type): EFI memory-type * @reserved: unused * @physical_start: Start address of region in physical memory - * @virtual_start: Start address of region in physical memory * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE * bytes) * @attribute: Memory attributes (see EFI_MEMORY...) @@ -46,7 +45,6 @@ struct priv_mem_desc { u32 type; u32 reserved; efi_physical_addr_t physical_start; - efi_virtual_addr_t virtual_start; u64 num_pages; u64 attribute; }; @@ -187,7 +185,6 @@ static void efi_mem_sort(void) pages = cur->num_pages; prev->num_pages += pages; prev->physical_start -= pages << EFI_PAGE_SHIFT; - prev->virtual_start -= pages << EFI_PAGE_SHIFT; list_del(&lmem->link); free(lmem);
@@ -255,7 +252,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, free(map); } else { map->desc.physical_start = carve_end; - map->desc.virtual_start = carve_end; map->desc.num_pages = (map_end - carve_end) >> EFI_PAGE_SHIFT; } @@ -276,7 +272,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_OUT_OF_RESOURCES; newmap->desc = map->desc; newmap->desc.physical_start = carve_start; - newmap->desc.virtual_start = carve_start; newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; /* Insert before current entry (descending address order) */ list_add_tail(&newmap->link, &map->link); @@ -313,7 +308,6 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_OUT_OF_RESOURCES; newlist->desc.type = memory_type; newlist->desc.physical_start = start; - newlist->desc.virtual_start = start; newlist->desc.num_pages = pages;
switch (memory_type) { @@ -711,7 +705,9 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, memory_map->type = lmem->desc.type; memory_map->reserved = lmem->desc.reserved; memory_map->physical_start = lmem->desc.physical_start; - memory_map->virtual_start = lmem->desc.virtual_start; + + /* virtual and physical are always the same */ + memory_map->virtual_start = lmem->desc.physical_start; memory_map->num_pages = lmem->desc.num_pages; memory_map->attribute = lmem->desc.attribute; memory_map--;
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This field is always the same as physical_start, so keeping track of it separately is unnecessary and confusing. Drop it.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index db40aee8353..8211600f7f6 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -37,7 +37,6 @@ efi_uintn_t efi_memory_map_key;
- @type (enum efi_memory_type): EFI memory-type
- @reserved: unused
- @physical_start: Start address of region in physical memory
- @virtual_start: Start address of region in physical memory
- @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
bytes)- @attribute: Memory attributes (see EFI_MEMORY...)
@@ -46,7 +45,6 @@ struct priv_mem_desc { u32 type; u32 reserved; efi_physical_addr_t physical_start;
efi_virtual_addr_t virtual_start; u64 num_pages; u64 attribute;}; @@ -187,7 +185,6 @@ static void efi_mem_sort(void) pages = cur->num_pages; prev->num_pages += pages; prev->physical_start -= pages << EFI_PAGE_SHIFT;
prev->virtual_start -= pages << EFI_PAGE_SHIFT; list_del(&lmem->link); free(lmem);@@ -255,7 +252,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, free(map); } else { map->desc.physical_start = carve_end;
map->desc.virtual_start = carve_end; map->desc.num_pages = (map_end - carve_end) >> EFI_PAGE_SHIFT; }@@ -276,7 +272,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_OUT_OF_RESOURCES; newmap->desc = map->desc; newmap->desc.physical_start = carve_start;
newmap->desc.virtual_start = carve_start; newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; /* Insert before current entry (descending address order) */ list_add_tail(&newmap->link, &map->link);@@ -313,7 +308,6 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_OUT_OF_RESOURCES; newlist->desc.type = memory_type; newlist->desc.physical_start = start;
newlist->desc.virtual_start = start; newlist->desc.num_pages = pages; switch (memory_type) {@@ -711,7 +705,9 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, memory_map->type = lmem->desc.type; memory_map->reserved = lmem->desc.reserved; memory_map->physical_start = lmem->desc.physical_start;
memory_map->virtual_start = lmem->desc.virtual_start;
/* virtual and physical are always the same */memory_map->virtual_start = lmem->desc.physical_start; memory_map->num_pages = lmem->desc.num_pages; memory_map->attribute = lmem->desc.attribute; memory_map--;-- 2.34.1
I don't like duplicating efi_mem_desc and have a priv without the virtual address. I don't understand what you find confusing, but we have follows the spec (minus that reserved member) and I prefer it
Thanks /Ilias
Hi Ilias,
On Wed, 11 Dec 2024 at 08:09, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This field is always the same as physical_start, so keeping track of it separately is unnecessary and confusing. Drop it.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index db40aee8353..8211600f7f6 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -37,7 +37,6 @@ efi_uintn_t efi_memory_map_key;
- @type (enum efi_memory_type): EFI memory-type
- @reserved: unused
- @physical_start: Start address of region in physical memory
- @virtual_start: Start address of region in physical memory
- @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
bytes)- @attribute: Memory attributes (see EFI_MEMORY...)
@@ -46,7 +45,6 @@ struct priv_mem_desc { u32 type; u32 reserved; efi_physical_addr_t physical_start;
efi_virtual_addr_t virtual_start; u64 num_pages; u64 attribute;}; @@ -187,7 +185,6 @@ static void efi_mem_sort(void) pages = cur->num_pages; prev->num_pages += pages; prev->physical_start -= pages << EFI_PAGE_SHIFT;
prev->virtual_start -= pages << EFI_PAGE_SHIFT; list_del(&lmem->link); free(lmem);@@ -255,7 +252,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, free(map); } else { map->desc.physical_start = carve_end;
map->desc.virtual_start = carve_end; map->desc.num_pages = (map_end - carve_end) >> EFI_PAGE_SHIFT; }@@ -276,7 +272,6 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_OUT_OF_RESOURCES; newmap->desc = map->desc; newmap->desc.physical_start = carve_start;
newmap->desc.virtual_start = carve_start; newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; /* Insert before current entry (descending address order) */ list_add_tail(&newmap->link, &map->link);@@ -313,7 +308,6 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_OUT_OF_RESOURCES; newlist->desc.type = memory_type; newlist->desc.physical_start = start;
newlist->desc.virtual_start = start; newlist->desc.num_pages = pages; switch (memory_type) {@@ -711,7 +705,9 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, memory_map->type = lmem->desc.type; memory_map->reserved = lmem->desc.reserved; memory_map->physical_start = lmem->desc.physical_start;
memory_map->virtual_start = lmem->desc.virtual_start;
/* virtual and physical are always the same */memory_map->virtual_start = lmem->desc.physical_start; memory_map->num_pages = lmem->desc.num_pages; memory_map->attribute = lmem->desc.attribute; memory_map--;-- 2.34.1
I don't like duplicating efi_mem_desc and have a priv without the virtual address. I don't understand what you find confusing, but we have follows the spec (minus that reserved member) and I prefer it
We are still stuck on this address/pointer thing. As you will see at the end of this series, I am changing the internal table to use addresses, like lmb, so it is easier to understand what is going on...
Regards.
Simon
Hi Ilias,
On Wed, 11 Dec 2024 at 08:09, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This field is always the same as physical_start, so keeping track of it separately is unnecessary and confusing. Drop it.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
Applied to sjg/master, thanks!
This field is not used. Drop it and set the value to 0 when the memory-map is requested.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 8211600f7f6..024361483a0 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -35,7 +35,6 @@ efi_uintn_t efi_memory_map_key; * internal format is converted to the external struct efi_mem_desc format. * * @type (enum efi_memory_type): EFI memory-type - * @reserved: unused * @physical_start: Start address of region in physical memory * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE * bytes) @@ -43,7 +42,6 @@ efi_uintn_t efi_memory_map_key; */ struct priv_mem_desc { u32 type; - u32 reserved; efi_physical_addr_t physical_start; u64 num_pages; u64 attribute; @@ -703,7 +701,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, memory_map = &memory_map[map_entries - 1]; list_for_each_entry(lmem, &efi_mem, link) { memory_map->type = lmem->desc.type; - memory_map->reserved = lmem->desc.reserved; + memory_map->reserved = 0; memory_map->physical_start = lmem->desc.physical_start;
/* virtual and physical are always the same */
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This field is not used. Drop it and set the value to 0 when the memory-map is requested.
Looking at the spec the reserved field is wrong. Instead you should just remove it from struct efi_mem_desc
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 8211600f7f6..024361483a0 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -35,7 +35,6 @@ efi_uintn_t efi_memory_map_key;
- internal format is converted to the external struct efi_mem_desc format.
- @type (enum efi_memory_type): EFI memory-type
- @reserved: unused
- @physical_start: Start address of region in physical memory
- @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
bytes)@@ -43,7 +42,6 @@ efi_uintn_t efi_memory_map_key; */ struct priv_mem_desc { u32 type;
u32 reserved; efi_physical_addr_t physical_start; u64 num_pages; u64 attribute;@@ -703,7 +701,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, memory_map = &memory_map[map_entries - 1]; list_for_each_entry(lmem, &efi_mem, link) { memory_map->type = lmem->desc.type;
memory_map->reserved = lmem->desc.reserved;
memory_map->reserved = 0; memory_map->physical_start = lmem->desc.physical_start; /* virtual and physical are always the same */-- 2.34.1
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This field is not used. Drop it and set the value to 0 when the memory-map is requested.
Looking at the spec the reserved field is wrong. Instead you should just remove it from struct efi_mem_desc
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Applied to sjg/master, thanks!
We can make use of the enum now, since this struct is not exported to the EFI app.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 024361483a0..4c0fd0c5dca 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -34,14 +34,14 @@ efi_uintn_t efi_memory_map_key; * EFI application, other than through calls to efi_get_memory_map(), where this * internal format is converted to the external struct efi_mem_desc format. * - * @type (enum efi_memory_type): EFI memory-type + * @type: EFI memory-type * @physical_start: Start address of region in physical memory * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE * bytes) * @attribute: Memory attributes (see EFI_MEMORY...) */ struct priv_mem_desc { - u32 type; + enum efi_memory_type type; efi_physical_addr_t physical_start; u64 num_pages; u64 attribute;
We can make use of the enum now, since this struct is not exported to the EFI app.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Applied to sjg/master, thanks!
Rather than assigning desc and then changing two fields, assign all four fields, for clarity.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 4c0fd0c5dca..f3aaa0e9288 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -268,9 +268,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, newmap = calloc(1, sizeof(*newmap)); if (!newmap) return EFI_CARVE_OUT_OF_RESOURCES; - newmap->desc = map->desc; + newmap->desc.type = map->desc.type; newmap->desc.physical_start = carve_start; newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; + newmap->desc.attribute = map->desc.attribute; /* Insert before current entry (descending address order) */ list_add_tail(&newmap->link, &map->link);
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Rather than assigning desc and then changing two fields, assign all four fields, for clarity.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 4c0fd0c5dca..f3aaa0e9288 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -268,9 +268,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, newmap = calloc(1, sizeof(*newmap)); if (!newmap) return EFI_CARVE_OUT_OF_RESOURCES;
newmap->desc = map->desc;
newmap->desc.type = map->desc.type; newmap->desc.physical_start = carve_start;
virtual start needs to be assigned if you want to do that as well. But what we have is fine really
Thanks /Ilias
newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT;
newmap->desc.attribute = map->desc.attribute; /* Insert before current entry (descending address order) */ list_add_tail(&newmap->link, &map->link);-- 2.34.1
Hi Ilias,
On Wed, 11 Dec 2024 at 08:15, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Rather than assigning desc and then changing two fields, assign all four fields, for clarity.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 4c0fd0c5dca..f3aaa0e9288 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -268,9 +268,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, newmap = calloc(1, sizeof(*newmap)); if (!newmap) return EFI_CARVE_OUT_OF_RESOURCES;
newmap->desc = map->desc;
newmap->desc.type = map->desc.type; newmap->desc.physical_start = carve_start;virtual start needs to be assigned if you want to do that as well. But what we have is fine really
At this point in the series, there is no virtual_start - see: efi_loader: Drop virtual_start from priv_mem_desc
Thanks /Ilias
newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT;
newmap->desc.attribute = map->desc.attribute; /* Insert before current entry (descending address order) */ list_add_tail(&newmap->link, &map->link);-- 2.34.1
Regards, SImon
Hi Ilias,
On Wed, 11 Dec 2024 at 08:15, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
Rather than assigning desc and then changing two fields, assign all four fields, for clarity.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Applied to sjg/master, thanks!
We don't need a separate struct for the values in this node. Move everything in together, so that there is just one struct to consider.
Add a comment about physical_start, so it is clear that it is really a pointer.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 85 +++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 46 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index f3aaa0e9288..89e001fa753 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -28,36 +28,29 @@ DECLARE_GLOBAL_DATA_PTR; efi_uintn_t efi_memory_map_key;
/** - * struct priv_mem_desc - defines an memory region + * struct efi_mem_list - defines an EFI memory record * * Note that this struct is for use inside U-Boot and is not visible to the * EFI application, other than through calls to efi_get_memory_map(), where this * internal format is converted to the external struct efi_mem_desc format. * + * @link: Link to prev/next node in list * @type: EFI memory-type - * @physical_start: Start address of region in physical memory + * @physical_start: Start address of region in physical memory. Note that this + * is really a pointer stored as an address, so use map_to_sysmem() to + * convert it to an address if needed * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE * bytes) * @attribute: Memory attributes (see EFI_MEMORY...) */ -struct priv_mem_desc { +struct efi_mem_list { + struct list_head link; enum efi_memory_type type; efi_physical_addr_t physical_start; u64 num_pages; u64 attribute; };
-/** - * struct efi_mem_list - defines an EFI memory record - * - * @link: Link to prev/next node in list - * @desc: Memory information about this node - */ -struct efi_mem_list { - struct list_head link; - struct priv_mem_desc desc; -}; - #define EFI_CARVE_NO_OVERLAP -1 #define EFI_CARVE_LOOP_AGAIN -2 #define EFI_CARVE_OVERLAPS_NONRAM -3 @@ -128,9 +121,9 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b) struct efi_mem_list *mema = list_entry(a, struct efi_mem_list, link); struct efi_mem_list *memb = list_entry(b, struct efi_mem_list, link);
- if (mema->desc.physical_start == memb->desc.physical_start) + if (mema->physical_start == memb->physical_start) return 0; - else if (mema->desc.physical_start < memb->desc.physical_start) + else if (mema->physical_start < memb->physical_start) return 1; else return -1; @@ -139,12 +132,12 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b) /** * desc_get_end() - get end address of memory area * - * @desc: memory descriptor + * @node: memory node * Return: end address + 1 */ -static uint64_t desc_get_end(struct priv_mem_desc *desc) +static uint64_t desc_get_end(struct efi_mem_list *node) { - return desc->physical_start + (desc->num_pages << EFI_PAGE_SHIFT); + return node->physical_start + (node->num_pages << EFI_PAGE_SHIFT); }
/** @@ -164,8 +157,8 @@ static void efi_mem_sort(void) while (merge_again) { merge_again = false; list_for_each_entry(lmem, &efi_mem, link) { - struct priv_mem_desc *prev; - struct priv_mem_desc *cur; + struct efi_mem_list *prev; + struct efi_mem_list *cur; uint64_t pages;
if (!prevmem) { @@ -173,8 +166,8 @@ static void efi_mem_sort(void) continue; }
- cur = &lmem->desc; - prev = &prevmem->desc; + cur = lmem; + prev = prevmem;
if ((desc_get_end(cur) == prev->physical_start) && (prev->type == cur->type) && @@ -219,11 +212,11 @@ static void efi_mem_sort(void) * to re-add the already carved out pages to the mapping. */ static s64 efi_mem_carve_out(struct efi_mem_list *map, - struct priv_mem_desc *carve_desc, + struct efi_mem_list *carve_desc, bool overlap_conventional) { struct efi_mem_list *newmap; - struct priv_mem_desc *map_desc = &map->desc; + struct efi_mem_list *map_desc = map; uint64_t map_start = map_desc->physical_start; uint64_t map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT); uint64_t carve_start = carve_desc->physical_start; @@ -249,8 +242,8 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, list_del(&map->link); free(map); } else { - map->desc.physical_start = carve_end; - map->desc.num_pages = (map_end - carve_end) + map->physical_start = carve_end; + map->num_pages = (map_end - carve_end) >> EFI_PAGE_SHIFT; }
@@ -268,10 +261,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, newmap = calloc(1, sizeof(*newmap)); if (!newmap) return EFI_CARVE_OUT_OF_RESOURCES; - newmap->desc.type = map->desc.type; - newmap->desc.physical_start = carve_start; - newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; - newmap->desc.attribute = map->desc.attribute; + newmap->type = map->type; + newmap->physical_start = carve_start; + newmap->num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; + newmap->attribute = map->attribute; /* Insert before current entry (descending address order) */ list_add_tail(&newmap->link, &map->link);
@@ -305,20 +298,20 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, newlist = calloc(1, sizeof(*newlist)); if (!newlist) return EFI_OUT_OF_RESOURCES; - newlist->desc.type = memory_type; - newlist->desc.physical_start = start; - newlist->desc.num_pages = pages; + newlist->type = memory_type; + newlist->physical_start = start; + newlist->num_pages = pages;
switch (memory_type) { case EFI_RUNTIME_SERVICES_CODE: case EFI_RUNTIME_SERVICES_DATA: - newlist->desc.attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME; + newlist->attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME; break; case EFI_MMAP_IO: - newlist->desc.attribute = EFI_MEMORY_RUNTIME; + newlist->attribute = EFI_MEMORY_RUNTIME; break; default: - newlist->desc.attribute = EFI_MEMORY_WB; + newlist->attribute = EFI_MEMORY_WB; break; }
@@ -328,7 +321,7 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, list_for_each_entry(lmem, &efi_mem, link) { s64 r;
- r = efi_mem_carve_out(lmem, &newlist->desc, + r = efi_mem_carve_out(lmem, newlist, overlap_conventional); switch (r) { case EFI_CARVE_OUT_OF_RESOURCES: @@ -423,12 +416,12 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) struct efi_mem_list *item;
list_for_each_entry(item, &efi_mem, link) { - u64 start = item->desc.physical_start; - u64 end = start + (item->desc.num_pages << EFI_PAGE_SHIFT); + u64 start = item->physical_start; + u64 end = start + (item->num_pages << EFI_PAGE_SHIFT);
if (addr >= start && addr < end) { if (must_be_allocated ^ - (item->desc.type == EFI_CONVENTIONAL_MEMORY)) + (item->type == EFI_CONVENTIONAL_MEMORY)) return EFI_SUCCESS; else return EFI_NOT_FOUND; @@ -701,14 +694,14 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, /* Return the list in ascending order */ memory_map = &memory_map[map_entries - 1]; list_for_each_entry(lmem, &efi_mem, link) { - memory_map->type = lmem->desc.type; + memory_map->type = lmem->type; memory_map->reserved = 0; - memory_map->physical_start = lmem->desc.physical_start; + memory_map->physical_start = lmem->physical_start;
/* virtual and physical are always the same */ - memory_map->virtual_start = lmem->desc.physical_start; - memory_map->num_pages = lmem->desc.num_pages; - memory_map->attribute = lmem->desc.attribute; + memory_map->virtual_start = lmem->physical_start; + memory_map->num_pages = lmem->num_pages; + memory_map->attribute = lmem->attribute; memory_map--; }
We don't need a separate struct for the values in this node. Move everything in together, so that there is just one struct to consider.
Add a comment about physical_start, so it is clear that it is really a pointer.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 85 +++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 46 deletions(-)
Applied to sjg/master, thanks!
This struct is really a node in the list, not a list itself. Also it doesn't need an efi_ prefix. Rename it.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 89e001fa753..42319ceb16f 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -28,7 +28,7 @@ DECLARE_GLOBAL_DATA_PTR; efi_uintn_t efi_memory_map_key;
/** - * struct efi_mem_list - defines an EFI memory record + * struct mem_node - defines an EFI memory record * * Note that this struct is for use inside U-Boot and is not visible to the * EFI application, other than through calls to efi_get_memory_map(), where this @@ -43,7 +43,7 @@ efi_uintn_t efi_memory_map_key; * bytes) * @attribute: Memory attributes (see EFI_MEMORY...) */ -struct efi_mem_list { +struct mem_node { struct list_head link; enum efi_memory_type type; efi_physical_addr_t physical_start; @@ -118,8 +118,8 @@ static u64 checksum(struct efi_pool_allocation *alloc) */ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b) { - struct efi_mem_list *mema = list_entry(a, struct efi_mem_list, link); - struct efi_mem_list *memb = list_entry(b, struct efi_mem_list, link); + struct mem_node *mema = list_entry(a, struct mem_node, link); + struct mem_node *memb = list_entry(b, struct mem_node, link);
if (mema->physical_start == memb->physical_start) return 0; @@ -135,7 +135,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b) * @node: memory node * Return: end address + 1 */ -static uint64_t desc_get_end(struct efi_mem_list *node) +static uint64_t desc_get_end(struct mem_node *node) { return node->physical_start + (node->num_pages << EFI_PAGE_SHIFT); } @@ -147,8 +147,8 @@ static uint64_t desc_get_end(struct efi_mem_list *node) */ static void efi_mem_sort(void) { - struct efi_mem_list *lmem; - struct efi_mem_list *prevmem = NULL; + struct mem_node *lmem; + struct mem_node *prevmem = NULL; bool merge_again = true;
list_sort(NULL, &efi_mem, efi_mem_cmp); @@ -157,8 +157,8 @@ static void efi_mem_sort(void) while (merge_again) { merge_again = false; list_for_each_entry(lmem, &efi_mem, link) { - struct efi_mem_list *prev; - struct efi_mem_list *cur; + struct mem_node *prev; + struct mem_node *cur; uint64_t pages;
if (!prevmem) { @@ -211,12 +211,11 @@ static void efi_mem_sort(void) * In case of EFI_CARVE_OVERLAPS_NONRAM it is the callers responsibility * to re-add the already carved out pages to the mapping. */ -static s64 efi_mem_carve_out(struct efi_mem_list *map, - struct efi_mem_list *carve_desc, +static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc, bool overlap_conventional) { - struct efi_mem_list *newmap; - struct efi_mem_list *map_desc = map; + struct mem_node *newmap; + struct mem_node *map_desc = map; uint64_t map_start = map_desc->physical_start; uint64_t map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT); uint64_t carve_start = carve_desc->physical_start; @@ -278,8 +277,8 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, int memory_type, bool overlap_conventional) { - struct efi_mem_list *lmem; - struct efi_mem_list *newlist; + struct mem_node *lmem; + struct mem_node *newlist; bool carve_again; uint64_t carved_pages = 0; struct efi_event *evt; @@ -413,7 +412,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type) */ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) { - struct efi_mem_list *item; + struct mem_node *item;
list_for_each_entry(item, &efi_mem, link) { u64 start = item->physical_start; @@ -664,7 +663,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, { size_t map_entries; efi_uintn_t map_size = 0; - struct efi_mem_list *lmem; + struct mem_node *lmem; efi_uintn_t provided_map_size;
if (!memory_map_size)
This struct is really a node in the list, not a list itself. Also it doesn't need an efi_ prefix. Rename it.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)
Applied to sjg/master, thanks!
The word 'physical' suggests that there is an associated virtual address but there is not. Use 'base' since this is the base address of a region.
Change some uint64_t and remove some brackets to keep checkpatch happy.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 38 ++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 42319ceb16f..0f149f99c7d 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -36,7 +36,7 @@ efi_uintn_t efi_memory_map_key; * * @link: Link to prev/next node in list * @type: EFI memory-type - * @physical_start: Start address of region in physical memory. Note that this + * @base: Start address of region in physical memory. Note that this * is really a pointer stored as an address, so use map_to_sysmem() to * convert it to an address if needed * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE @@ -46,7 +46,7 @@ efi_uintn_t efi_memory_map_key; struct mem_node { struct list_head link; enum efi_memory_type type; - efi_physical_addr_t physical_start; + efi_physical_addr_t base; u64 num_pages; u64 attribute; }; @@ -121,9 +121,9 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b) struct mem_node *mema = list_entry(a, struct mem_node, link); struct mem_node *memb = list_entry(b, struct mem_node, link);
- if (mema->physical_start == memb->physical_start) + if (mema->base == memb->base) return 0; - else if (mema->physical_start < memb->physical_start) + else if (mema->base < memb->base) return 1; else return -1; @@ -137,7 +137,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b) */ static uint64_t desc_get_end(struct mem_node *node) { - return node->physical_start + (node->num_pages << EFI_PAGE_SHIFT); + return node->base + (node->num_pages << EFI_PAGE_SHIFT); }
/** @@ -169,13 +169,13 @@ static void efi_mem_sort(void) cur = lmem; prev = prevmem;
- if ((desc_get_end(cur) == prev->physical_start) && - (prev->type == cur->type) && - (prev->attribute == cur->attribute)) { + if (desc_get_end(cur) == prev->base && + prev->type == cur->type && + prev->attribute == cur->attribute) { /* There is an existing map before, reuse it */ pages = cur->num_pages; prev->num_pages += pages; - prev->physical_start -= pages << EFI_PAGE_SHIFT; + prev->base -= pages << EFI_PAGE_SHIFT; list_del(&lmem->link); free(lmem);
@@ -216,10 +216,10 @@ static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc, { struct mem_node *newmap; struct mem_node *map_desc = map; - uint64_t map_start = map_desc->physical_start; - uint64_t map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT); - uint64_t carve_start = carve_desc->physical_start; - uint64_t carve_end = carve_start + + u64 map_start = map_desc->base; + u64 map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT); + u64 carve_start = carve_desc->base; + u64 carve_end = carve_start + (carve_desc->num_pages << EFI_PAGE_SHIFT);
/* check whether we're overlapping */ @@ -241,7 +241,7 @@ static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc, list_del(&map->link); free(map); } else { - map->physical_start = carve_end; + map->base = carve_end; map->num_pages = (map_end - carve_end) >> EFI_PAGE_SHIFT; } @@ -261,7 +261,7 @@ static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc, if (!newmap) return EFI_CARVE_OUT_OF_RESOURCES; newmap->type = map->type; - newmap->physical_start = carve_start; + newmap->base = carve_start; newmap->num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; newmap->attribute = map->attribute; /* Insert before current entry (descending address order) */ @@ -298,7 +298,7 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, if (!newlist) return EFI_OUT_OF_RESOURCES; newlist->type = memory_type; - newlist->physical_start = start; + newlist->base = start; newlist->num_pages = pages;
switch (memory_type) { @@ -415,7 +415,7 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) struct mem_node *item;
list_for_each_entry(item, &efi_mem, link) { - u64 start = item->physical_start; + u64 start = item->base; u64 end = start + (item->num_pages << EFI_PAGE_SHIFT);
if (addr >= start && addr < end) { @@ -695,10 +695,10 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, list_for_each_entry(lmem, &efi_mem, link) { memory_map->type = lmem->type; memory_map->reserved = 0; - memory_map->physical_start = lmem->physical_start; + memory_map->physical_start = lmem->base;
/* virtual and physical are always the same */ - memory_map->virtual_start = lmem->physical_start; + memory_map->virtual_start = lmem->base; memory_map->num_pages = lmem->num_pages; memory_map->attribute = lmem->attribute; memory_map--;
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
The word 'physical' suggests that there is an associated virtual address but there is not. Use 'base' since this is the base address of a region.
Change some uint64_t and remove some brackets to keep checkpatch happy.
Please drop this. physical start is what the spec uses and it's a lot easier to remember when reading the spec
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 38 ++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 42319ceb16f..0f149f99c7d 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -36,7 +36,7 @@ efi_uintn_t efi_memory_map_key;
- @link: Link to prev/next node in list
- @type: EFI memory-type
- @physical_start: Start address of region in physical memory. Note that this
- @base: Start address of region in physical memory. Note that this
is really a pointer stored as an address, so use map_to_sysmem() toconvert it to an address if needed- @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
@@ -46,7 +46,7 @@ efi_uintn_t efi_memory_map_key; struct mem_node { struct list_head link; enum efi_memory_type type;
efi_physical_addr_t physical_start;
efi_physical_addr_t base; u64 num_pages; u64 attribute;}; @@ -121,9 +121,9 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b) struct mem_node *mema = list_entry(a, struct mem_node, link); struct mem_node *memb = list_entry(b, struct mem_node, link);
if (mema->physical_start == memb->physical_start)
if (mema->base == memb->base) return 0;
else if (mema->physical_start < memb->physical_start)
else if (mema->base < memb->base) return 1; else return -1;@@ -137,7 +137,7 @@ static int efi_mem_cmp(void *priv, struct list_head *a, struct list_head *b) */ static uint64_t desc_get_end(struct mem_node *node) {
return node->physical_start + (node->num_pages << EFI_PAGE_SHIFT);
return node->base + (node->num_pages << EFI_PAGE_SHIFT);}
/** @@ -169,13 +169,13 @@ static void efi_mem_sort(void) cur = lmem; prev = prevmem;
if ((desc_get_end(cur) == prev->physical_start) &&(prev->type == cur->type) &&(prev->attribute == cur->attribute)) {
if (desc_get_end(cur) == prev->base &&prev->type == cur->type &&prev->attribute == cur->attribute) { /* There is an existing map before, reuse it */ pages = cur->num_pages; prev->num_pages += pages;
prev->physical_start -= pages << EFI_PAGE_SHIFT;
prev->base -= pages << EFI_PAGE_SHIFT; list_del(&lmem->link); free(lmem);@@ -216,10 +216,10 @@ static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc, { struct mem_node *newmap; struct mem_node *map_desc = map;
uint64_t map_start = map_desc->physical_start;uint64_t map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT);uint64_t carve_start = carve_desc->physical_start;uint64_t carve_end = carve_start +
u64 map_start = map_desc->base;u64 map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT);u64 carve_start = carve_desc->base;u64 carve_end = carve_start + (carve_desc->num_pages << EFI_PAGE_SHIFT); /* check whether we're overlapping */@@ -241,7 +241,7 @@ static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc, list_del(&map->link); free(map); } else {
map->physical_start = carve_end;
map->base = carve_end; map->num_pages = (map_end - carve_end) >> EFI_PAGE_SHIFT; }@@ -261,7 +261,7 @@ static s64 efi_mem_carve_out(struct mem_node *map, struct mem_node *carve_desc, if (!newmap) return EFI_CARVE_OUT_OF_RESOURCES; newmap->type = map->type;
newmap->physical_start = carve_start;
newmap->base = carve_start; newmap->num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; newmap->attribute = map->attribute; /* Insert before current entry (descending address order) */@@ -298,7 +298,7 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, if (!newlist) return EFI_OUT_OF_RESOURCES; newlist->type = memory_type;
newlist->physical_start = start;
newlist->base = start; newlist->num_pages = pages; switch (memory_type) {@@ -415,7 +415,7 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) struct mem_node *item;
list_for_each_entry(item, &efi_mem, link) {
u64 start = item->physical_start;
u64 start = item->base; u64 end = start + (item->num_pages << EFI_PAGE_SHIFT); if (addr >= start && addr < end) {@@ -695,10 +695,10 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, list_for_each_entry(lmem, &efi_mem, link) { memory_map->type = lmem->type; memory_map->reserved = 0;
memory_map->physical_start = lmem->physical_start;
memory_map->physical_start = lmem->base; /* virtual and physical are always the same */
memory_map->virtual_start = lmem->physical_start;
memory_map->virtual_start = lmem->base; memory_map->num_pages = lmem->num_pages; memory_map->attribute = lmem->attribute; memory_map--;-- 2.34.1
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
The word 'physical' suggests that there is an associated virtual address but there is not. Use 'base' since this is the base address of a region.
Change some uint64_t and remove some brackets to keep checkpatch happy.
Please drop this. physical start is what the spec uses and it's a lot easier to remember when reading the spec
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 38 ++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)
Applied to sjg/master, thanks!
This function is passed the address of a void * so update the argument to match. It is better to have casts in the caller than introduce confusion as to what is passed in.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- include/efi_loader.h | 2 +- lib/efi_loader/efi_runtime.c | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c index d2d3e346a36..f9c379e9b06 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c +++ b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c @@ -1272,7 +1272,7 @@ void __efi_runtime EFIAPI efi_reset_system(
efi_status_t efi_reset_system_init(void) { - return efi_add_runtime_mmio(&rstcr, sizeof(*rstcr)); + return efi_add_runtime_mmio((void **)&rstcr, sizeof(*rstcr)); }
#endif diff --git a/arch/arm/mach-bcm283x/reset.c b/arch/arm/mach-bcm283x/reset.c index 1dc7ce50d1d..fd10f01d4f7 100644 --- a/arch/arm/mach-bcm283x/reset.c +++ b/arch/arm/mach-bcm283x/reset.c @@ -85,7 +85,7 @@ void __efi_runtime EFIAPI efi_reset_system( efi_status_t efi_reset_system_init(void) { wdog_regs = (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR; - return efi_add_runtime_mmio(&wdog_regs, sizeof(*wdog_regs)); + return efi_add_runtime_mmio((void **)&wdog_regs, sizeof(*wdog_regs)); }
#endif diff --git a/include/efi_loader.h b/include/efi_loader.h index 3f75f2efcb6..3bfc56ebb5b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -73,7 +73,7 @@ struct jmp_buf_data; * Call this with mmio_ptr as the _pointer_ to a pointer to an MMIO region * to make it available at runtime */ -efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len); +efi_status_t efi_add_runtime_mmio(void **mmio_ptr, u64 len);
/* * Special case handler for error/abort that just tries to dtrt to get diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 05369c47b01..9d3b940afbd 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -927,7 +927,7 @@ out: * @len: size of the memory-mapped IO region * Returns: status code */ -efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len) +efi_status_t efi_add_runtime_mmio(void **mmio_ptr, u64 len) { struct efi_runtime_mmio_list *newmmio; uint64_t addr = *(uintptr_t *)mmio_ptr; @@ -941,7 +941,7 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len) if (!newmmio) return EFI_OUT_OF_RESOURCES; newmmio->ptr = mmio_ptr; - newmmio->paddr = *(uintptr_t *)mmio_ptr; + newmmio->paddr = (uintptr_t)*(void **)mmio_ptr; newmmio->len = len; list_add_tail(&newmmio->link, &efi_runtime_mmio);
This function is passed the address of a void * so update the argument to match. It is better to have casts in the caller than introduce confusion as to what is passed in.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- include/efi_loader.h | 2 +- lib/efi_loader/efi_runtime.c | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-)
Applied to sjg/master, thanks!
When logging pool allocations, show the address of the pointer, not the pointer itself. This makes it easier to debug with sandbox
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v4)
Changes in v4: - Add new patch to show the address for pool allocations
lib/efi_loader/efi_boottime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index f27b3827ed2..47ed22b1d9e 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -533,7 +533,7 @@ static efi_status_t EFIAPI efi_free_pool_ext(void *buffer) { efi_status_t r;
- EFI_ENTRY("%p", buffer); + EFI_ENTRY("%llx", (u64)map_to_sysmem(buffer)); r = efi_free_pool(buffer); return EFI_EXIT(r); }
When logging pool allocations, show the address of the pointer, not the pointer itself. This makes it easier to debug with sandbox
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v4)
Changes in v4: - Add new patch to show the address for pool allocations
lib/efi_loader/efi_boottime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to sjg/master, thanks!
This cannot work since the code is not present in the emulated memory. In any case, sandbox cannot make use of the runtime code.
For now, just drop it from sandbox. We can always adjust things to copy it into memory, if needed.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_memory.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 0f149f99c7d..6475b94f951 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -767,16 +767,22 @@ static void add_u_boot_and_runtime(void) runtime_mask = SZ_64K - 1; #endif
- /* - * Add Runtime Services. We mark surrounding boottime code as runtime as - * well to fulfill the runtime alignment constraints but avoid padding. - */ - runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask; - runtime_end = (uintptr_t)__efi_runtime_stop; - runtime_end = (runtime_end + runtime_mask) & ~runtime_mask; - runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; - efi_add_memory_map_pg(runtime_start, runtime_pages, - EFI_RUNTIME_SERVICES_CODE, false); + if (!IS_ENABLED(CONFIG_SANDBOX)) { + /* + * Add Runtime Services. We mark surrounding boottime code as + * runtime as well to fulfill the runtime alignment constraints + * but avoid padding. + * + * This is not enabled for sandbox, since we cannot map the + * sandbox code into emulated SDRAM + */ + runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask; + runtime_end = (uintptr_t)__efi_runtime_stop; + runtime_end = (runtime_end + runtime_mask) & ~runtime_mask; + runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; + efi_add_memory_map_pg(runtime_start, runtime_pages, + EFI_RUNTIME_SERVICES_CODE, false); + } }
int efi_memory_init(void)
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This cannot work since the code is not present in the emulated memory. In any case, sandbox cannot make use of the runtime code.
For now, just drop it from sandbox. We can always adjust things to copy it into memory, if needed.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 0f149f99c7d..6475b94f951 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -767,16 +767,22 @@ static void add_u_boot_and_runtime(void) runtime_mask = SZ_64K - 1; #endif
/** Add Runtime Services. We mark surrounding boottime code as runtime as* well to fulfill the runtime alignment constraints but avoid padding.*/runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;runtime_end = (uintptr_t)__efi_runtime_stop;runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;efi_add_memory_map_pg(runtime_start, runtime_pages,EFI_RUNTIME_SERVICES_CODE, false);
Can sandbox boot an OS? If not there's no point adding this. But if you insist, I prefer if (IS_ENABLED(CONFIG_SANDBOX) return
at the top of the function
Thanks /Ilias
if (!IS_ENABLED(CONFIG_SANDBOX)) {/** Add Runtime Services. We mark surrounding boottime code as* runtime as well to fulfill the runtime alignment constraints* but avoid padding.** This is not enabled for sandbox, since we cannot map the* sandbox code into emulated SDRAM*/runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;runtime_end = (uintptr_t)__efi_runtime_stop;runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;efi_add_memory_map_pg(runtime_start, runtime_pages,EFI_RUNTIME_SERVICES_CODE, false);}}
int efi_memory_init(void)
2.34.1
Hi Ilias,
On Wed, 11 Dec 2024 at 08:19, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This cannot work since the code is not present in the emulated memory. In any case, sandbox cannot make use of the runtime code.
For now, just drop it from sandbox. We can always adjust things to copy it into memory, if needed.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 0f149f99c7d..6475b94f951 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -767,16 +767,22 @@ static void add_u_boot_and_runtime(void) runtime_mask = SZ_64K - 1; #endif
/** Add Runtime Services. We mark surrounding boottime code as runtime as* well to fulfill the runtime alignment constraints but avoid padding.*/runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;runtime_end = (uintptr_t)__efi_runtime_stop;runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;efi_add_memory_map_pg(runtime_start, runtime_pages,EFI_RUNTIME_SERVICES_CODE, false);Can sandbox boot an OS? If not there's no point adding this. But if you insist, I prefer
No sandbox can't really boot an OS. The primary reason for this change is to avoid strange things appearing in the EFI memory map (and therefore the EFI log). I found it very confusing.
if (IS_ENABLED(CONFIG_SANDBOX) return
at the top of the function
OK
Thanks /Ilias
if (!IS_ENABLED(CONFIG_SANDBOX)) {/** Add Runtime Services. We mark surrounding boottime code as* runtime as well to fulfill the runtime alignment constraints* but avoid padding.** This is not enabled for sandbox, since we cannot map the* sandbox code into emulated SDRAM*/runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;runtime_end = (uintptr_t)__efi_runtime_stop;runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;efi_add_memory_map_pg(runtime_start, runtime_pages,EFI_RUNTIME_SERVICES_CODE, false);}}
int efi_memory_init(void)
2.34.1
Regards, Simon
On Wed, 11 Dec 2024 at 17:54, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 08:19, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This cannot work since the code is not present in the emulated memory. In any case, sandbox cannot make use of the runtime code.
For now, just drop it from sandbox. We can always adjust things to copy it into memory, if needed.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 0f149f99c7d..6475b94f951 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -767,16 +767,22 @@ static void add_u_boot_and_runtime(void) runtime_mask = SZ_64K - 1; #endif
/** Add Runtime Services. We mark surrounding boottime code as runtime as* well to fulfill the runtime alignment constraints but avoid padding.*/runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;runtime_end = (uintptr_t)__efi_runtime_stop;runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;efi_add_memory_map_pg(runtime_start, runtime_pages,EFI_RUNTIME_SERVICES_CODE, false);Can sandbox boot an OS? If not there's no point adding this. But if you insist, I prefer
No sandbox can't really boot an OS. The primary reason for this change is to avoid strange things appearing in the EFI memory map (and therefore the EFI log). I found it very confusing.
if (IS_ENABLED(CONFIG_SANDBOX) return
at the top of the function
OK
But can't we call ExitBootServices from sandbox and test some of those?
Thanks /Ilias
Thanks /Ilias
if (!IS_ENABLED(CONFIG_SANDBOX)) {/** Add Runtime Services. We mark surrounding boottime code as* runtime as well to fulfill the runtime alignment constraints* but avoid padding.** This is not enabled for sandbox, since we cannot map the* sandbox code into emulated SDRAM*/runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;runtime_end = (uintptr_t)__efi_runtime_stop;runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;efi_add_memory_map_pg(runtime_start, runtime_pages,EFI_RUNTIME_SERVICES_CODE, false);}}
int efi_memory_init(void)
2.34.1
Regards, Simon
Hi Ilias,
On Wed, 11 Dec 2024 at 23:16, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 17:54, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 08:19, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This cannot work since the code is not present in the emulated memory. In any case, sandbox cannot make use of the runtime code.
For now, just drop it from sandbox. We can always adjust things to copy it into memory, if needed.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 0f149f99c7d..6475b94f951 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -767,16 +767,22 @@ static void add_u_boot_and_runtime(void) runtime_mask = SZ_64K - 1; #endif
/** Add Runtime Services. We mark surrounding boottime code as runtime as* well to fulfill the runtime alignment constraints but avoid padding.*/runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;runtime_end = (uintptr_t)__efi_runtime_stop;runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;efi_add_memory_map_pg(runtime_start, runtime_pages,EFI_RUNTIME_SERVICES_CODE, false);Can sandbox boot an OS? If not there's no point adding this. But if you insist, I prefer
No sandbox can't really boot an OS. The primary reason for this change is to avoid strange things appearing in the EFI memory map (and therefore the EFI log). I found it very confusing.
if (IS_ENABLED(CONFIG_SANDBOX) return
at the top of the function
OK
But can't we call ExitBootServices from sandbox and test some of those?
Yes I'm pretty sure that could be made to work, although I would need to try it. But we don't have such a test today, so we can worry about it then.
Thanks /Ilias
Thanks /Ilias
if (!IS_ENABLED(CONFIG_SANDBOX)) {/** Add Runtime Services. We mark surrounding boottime code as* runtime as well to fulfill the runtime alignment constraints* but avoid padding.** This is not enabled for sandbox, since we cannot map the* sandbox code into emulated SDRAM*/runtime_start = (uintptr_t)__efi_runtime_start & ~runtime_mask;runtime_end = (uintptr_t)__efi_runtime_stop;runtime_end = (runtime_end + runtime_mask) & ~runtime_mask;runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT;efi_add_memory_map_pg(runtime_start, runtime_pages,EFI_RUNTIME_SERVICES_CODE, false);}}
int efi_memory_init(void)
2.34.1
Regards, SImon
Hi Ilias,
On Wed, 11 Dec 2024 at 23:16, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Wed, 11 Dec 2024 at 17:54, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 08:19, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
This cannot work since the code is not present in the emulated memory. In any case, sandbox cannot make use of the runtime code.
For now, just drop it from sandbox. We can always adjust things to copy it into memory, if needed.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_memory.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
Applied to sjg/master, thanks!
This collects together several v3 patches into one, to avoid any temporary test-breakage which would make future bisecting difficult.
For efi_memory, the efi_allocate_pool() call uses a pointer to refer to memory, but efi_allocate_pages() uses an int. This causes quite a bit of confusion, since sandbox has a distinction between pointers and addresses. Adjust efi_allocate/free_pages_ext() to handle the conversions.
Update efi_add_memory_map() function and its friend to use an address rather than a pointer cast to an integer.
For lmb, now that efi_add_memory_map_pg() uses a address rather than a pointer cast to an int, we can simplify the code here.
For efi_reserve_memory(), use addresses rather than pointers, so that it doesn't have to use mapmem.
In general this simplifies the code, but the main benefit is that casts are no-longer needed in most places, so the compiler can check that we are doing the right thing.
For efi_add_runtime_mmio(), now that efi_add_memory_map() takes an address rather than a pointer, do the mapping in this function.
Update the memset() parameter to be a char while we are here.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v4)
Changes in v4: - Combine the various pointer-to-address patches into one
include/efi_loader.h | 13 +++++------ lib/efi_loader/efi_bootmgr.c | 3 ++- lib/efi_loader/efi_boottime.c | 37 ++++++++++++++++++++++-------- lib/efi_loader/efi_dt_fixup.c | 4 ---- lib/efi_loader/efi_image_loader.c | 3 ++- lib/efi_loader/efi_memory.c | 38 ++++++++++--------------------- lib/efi_loader/efi_runtime.c | 3 ++- lib/efi_loader/efi_var_mem.c | 6 ++--- lib/lmb.c | 10 +++----- 9 files changed, 57 insertions(+), 60 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 3bfc56ebb5b..9143471c1ce 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -795,7 +795,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, /** * efi_free_pages() - free memory pages * - * @memory: start of the memory area to be freed + * @memory: start-address of the memory area to be freed * @pages: number of pages to be freed * Return: status code */ @@ -833,9 +833,9 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, /** * efi_add_memory_map() - add memory area to the memory map * - * @start: start address of the memory area. Note that this is - * actually a pointer provided as an integer. To pass in - * an address, pass in (ulong)map_to_sysmem(addr) + * @start: start address, must be a multiple of EFI_PAGE_SIZE. Note + * that this is an address, not a pointer. Use + * map_to_sysmem(ptr) if you need to pass in a pointer * @size: length in bytes of the memory area * @memory_type: type of memory added * @@ -850,9 +850,8 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); * efi_add_memory_map_pg() - add pages to the memory map * * @start: start address, must be a multiple of EFI_PAGE_SIZE. Note that this - * is actually a pointer provided as an integer. To pass in an address, pass - * in (ulong)map_to_sysmem(addr) - * + * is an address, not a pointer. Use map_to_sysmem(ptr) if you need to pass + * in a pointer * @pages: number of pages to add * @memory_type: EFI type of memory added * @overlap_conventional: region may only overlap free(conventional) diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index bb635d77b53..30d4ce09e7e 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -14,6 +14,7 @@ #include <efi.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <net.h> #include <efi_loader.h> #include <efi_variable.h> @@ -1302,7 +1303,7 @@ efi_status_t efi_bootmgr_run(void *fdt) if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) { free(fdt_lo); if (fdt_distro) - efi_free_pages((uintptr_t)fdt_distro, + efi_free_pages(map_to_sysmem(fdt_distro), efi_size_in_pages(fdt_size)); }
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 47ed22b1d9e..ffe43accd1e 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -431,15 +431,25 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type, uint64_t *memory) { efi_status_t r; + u64 addr;
EFI_ENTRY("%d, %d, 0x%zx, %p", type, memory_type, pages, memory); - r = efi_allocate_pages(type, memory_type, pages, memory); + if (!memory) + return EFI_INVALID_PARAMETER; + + /* we should not read this unless type indicates it is being used */ + if (type == EFI_ALLOCATE_MAX_ADDRESS || type == EFI_ALLOCATE_ADDRESS) + addr = map_to_sysmem((void *)(uintptr_t)*memory); + r = efi_allocate_pages(type, memory_type, pages, &addr); + if (r == EFI_SUCCESS) + *memory = (uintptr_t)map_sysmem(addr, pages * EFI_PAGE_SIZE); + return EFI_EXIT(r); }
/** * efi_free_pages_ext() - Free memory pages. - * @memory: start of the memory area to be freed + * @memory: start of the memory area to be freed (a pointer, cast to u64) * @pages: number of pages to be freed * * This function implements the FreePages service. @@ -453,9 +463,12 @@ static efi_status_t EFIAPI efi_free_pages_ext(uint64_t memory, efi_uintn_t pages) { efi_status_t r; + u64 addr;
EFI_ENTRY("%llx, 0x%zx", memory, pages); - r = efi_free_pages(memory, pages); + addr = map_to_sysmem((void *)(uintptr_t)memory); + r = efi_free_pages(addr, pages); + return EFI_EXIT(r); }
@@ -1951,8 +1964,9 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path, { struct efi_file_handle *f; efi_status_t ret; - u64 addr; efi_uintn_t bs; + void *buf; + u64 addr;
/* Open file */ f = efi_file_from_path(file_path); @@ -1978,10 +1992,11 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path, }
/* Read file */ - EFI_CALL(ret = f->read(f, &bs, (void *)(uintptr_t)addr)); + buf = map_sysmem(addr, bs); + EFI_CALL(ret = f->read(f, &bs, buf)); if (ret != EFI_SUCCESS) efi_free_pages(addr, efi_size_in_pages(bs)); - *buffer = (void *)(uintptr_t)addr; + *buffer = buf; *size = bs; error: EFI_CALL(f->close(f)); @@ -2012,6 +2027,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy, uint64_t addr, pages; const efi_guid_t *guid; struct efi_handler *handler; + void *buf;
/* In case of failure nothing is returned */ *buffer = NULL; @@ -2050,15 +2066,16 @@ efi_status_t efi_load_image_from_path(bool boot_policy, ret = EFI_OUT_OF_RESOURCES; goto out; } + buf = map_sysmem(addr, buffer_size); ret = EFI_CALL(load_file_protocol->load_file( load_file_protocol, rem, boot_policy, - &buffer_size, (void *)(uintptr_t)addr)); + &buffer_size, buf)); if (ret != EFI_SUCCESS) efi_free_pages(addr, pages); out: efi_close_protocol(device, guid, efi_root, NULL); if (ret == EFI_SUCCESS) { - *buffer = (void *)(uintptr_t)addr; + *buffer = buf; *size = buffer_size; }
@@ -2121,7 +2138,7 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, ret = efi_load_pe(*image_obj, dest_buffer, source_size, info); if (!source_buffer) /* Release buffer to which file was loaded */ - efi_free_pages((uintptr_t)dest_buffer, + efi_free_pages(map_to_sysmem(dest_buffer), efi_size_in_pages(source_size)); if (ret == EFI_SUCCESS || ret == EFI_SECURITY_VIOLATION) { info->system_table = &systab; @@ -3322,7 +3339,7 @@ close_next: } }
- efi_free_pages((uintptr_t)loaded_image_protocol->image_base, + efi_free_pages(map_to_sysmem(loaded_image_protocol->image_base), efi_size_in_pages(loaded_image_protocol->image_size)); efi_delete_handle(&image_obj->header);
diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c index 0dac94b0c6c..72d5d432a42 100644 --- a/lib/efi_loader/efi_dt_fixup.c +++ b/lib/efi_loader/efi_dt_fixup.c @@ -9,7 +9,6 @@ #include <efi_loader.h> #include <efi_rng.h> #include <fdtdec.h> -#include <mapmem.h>
const efi_guid_t efi_guid_dt_fixup_protocol = EFI_DT_FIXUP_PROTOCOL_GUID;
@@ -26,9 +25,6 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap) int type; efi_uintn_t ret;
- /* Convert from sandbox address space. */ - addr = (uintptr_t)map_sysmem(addr, 0); - if (nomap) type = EFI_RESERVED_MEMORY_TYPE; else diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 0ddf69a0918..2f2587b32a6 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -13,6 +13,7 @@ #include <efi_loader.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <pe.h> #include <sort.h> #include <crypto/mscode.h> @@ -970,7 +971,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, /* Run through relocations */ if (efi_loader_relocate(rel, rel_size, efi_reloc, (unsigned long)image_base) != EFI_SUCCESS) { - efi_free_pages((uintptr_t) efi_reloc, + efi_free_pages(map_to_sysmem(efi_reloc), (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT); ret = EFI_LOAD_ERROR; goto err; diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 6475b94f951..2995f1b13f5 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -36,9 +36,7 @@ efi_uintn_t efi_memory_map_key; * * @link: Link to prev/next node in list * @type: EFI memory-type - * @base: Start address of region in physical memory. Note that this - * is really a pointer stored as an address, so use map_to_sysmem() to - * convert it to an address if needed + * @base: Start address of region in physical memory * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE * bytes) * @attribute: Memory attributes (see EFI_MEMORY...) @@ -434,7 +432,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type memory_type, efi_uintn_t pages, uint64_t *memory) { - u64 efi_addr, len; + u64 len; uint flags; efi_status_t ret; phys_addr_t addr; @@ -462,8 +460,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, break; case EFI_ALLOCATE_MAX_ADDRESS: /* Max address */ - addr = map_to_sysmem((void *)(uintptr_t)*memory); - addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, addr, + addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, *memory, flags); if (!addr) return EFI_OUT_OF_RESOURCES; @@ -472,8 +469,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, if (*memory & EFI_PAGE_MASK) return EFI_NOT_FOUND;
- addr = map_to_sysmem((void *)(uintptr_t)*memory); - addr = (u64)lmb_alloc_addr_flags(addr, len, flags); + addr = (u64)lmb_alloc_addr_flags(*memory, len, flags); if (!addr) return EFI_NOT_FOUND; break; @@ -482,17 +478,15 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, return EFI_INVALID_PARAMETER; }
- efi_addr = (u64)(uintptr_t)map_sysmem(addr, 0); /* Reserve that map in our memory maps */ - ret = efi_add_memory_map_pg(efi_addr, pages, memory_type, true); + ret = efi_add_memory_map_pg(addr, pages, memory_type, true); if (ret != EFI_SUCCESS) { /* Map would overlap, bail out */ lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags); - unmap_sysmem((void *)(uintptr_t)efi_addr); return EFI_OUT_OF_RESOURCES; }
- *memory = efi_addr; + *memory = addr;
return EFI_SUCCESS; } @@ -515,18 +509,10 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) }
len = (u64)pages << EFI_PAGE_SHIFT; - /* - * The 'memory' variable for sandbox holds a pointer which has already - * been mapped with map_sysmem() from efi_allocate_pages(). Convert - * it back to an address LMB understands - */ - status = lmb_free_flags(map_to_sysmem((void *)(uintptr_t)memory), len, - LMB_NOOVERWRITE); + status = lmb_free_flags(memory, len, LMB_NOOVERWRITE); if (status) return EFI_NOT_FOUND;
- unmap_sysmem((void *)(uintptr_t)memory); - return ret; }
@@ -550,7 +536,7 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) if (align < EFI_PAGE_SIZE) { r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, req_pages, &mem); - return (r == EFI_SUCCESS) ? (void *)(uintptr_t)mem : NULL; + return (r == EFI_SUCCESS) ? map_sysmem(mem, len) : NULL; }
r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, @@ -571,7 +557,7 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) efi_free_pages(mem, free_pages); }
- return (void *)(uintptr_t)aligned_mem; + return map_sysmem(aligned_mem, len); }
efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer) @@ -593,7 +579,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages, &addr); if (r == EFI_SUCCESS) { - alloc = (struct efi_pool_allocation *)(uintptr_t)addr; + alloc = map_sysmem(addr, size); alloc->num_pages = num_pages; alloc->checksum = checksum(alloc); *buffer = alloc->data; @@ -624,7 +610,7 @@ efi_status_t efi_free_pool(void *buffer) if (!buffer) return EFI_INVALID_PARAMETER;
- ret = efi_check_allocated((uintptr_t)buffer, true); + ret = efi_check_allocated(map_to_sysmem(buffer), true); if (ret != EFI_SUCCESS) return ret;
@@ -639,7 +625,7 @@ efi_status_t efi_free_pool(void *buffer) /* Avoid double free */ alloc->checksum = 0;
- ret = efi_free_pages((uintptr_t)alloc, alloc->num_pages); + ret = efi_free_pages(map_to_sysmem(alloc), alloc->num_pages);
return ret; } diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 9d3b940afbd..37ed5963e2b 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -13,6 +13,7 @@ #include <efi_variable.h> #include <log.h> #include <malloc.h> +#include <mapmem.h> #include <rtc.h> #include <asm/global_data.h> #include <u-boot/crc.h> @@ -930,7 +931,7 @@ out: efi_status_t efi_add_runtime_mmio(void **mmio_ptr, u64 len) { struct efi_runtime_mmio_list *newmmio; - uint64_t addr = *(uintptr_t *)mmio_ptr; + u64 addr = map_to_sysmem(*mmio_ptr); efi_status_t ret;
ret = efi_add_memory_map(addr, len, EFI_MMAP_IO); diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c index 5c69a1e0f3e..a96a1805865 100644 --- a/lib/efi_loader/efi_var_mem.c +++ b/lib/efi_loader/efi_var_mem.c @@ -7,6 +7,7 @@
#include <efi_loader.h> #include <efi_variable.h> +#include <mapmem.h> #include <u-boot/crc.h>
/* @@ -227,9 +228,8 @@ efi_status_t efi_var_mem_init(void) 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 = map_sysmem(memory, EFI_VAR_BUF_SIZE); + memset(efi_var_buf, '\0', EFI_VAR_BUF_SIZE); efi_var_buf->magic = EFI_VAR_FILE_MAGIC; efi_var_buf->length = (uintptr_t)efi_var_buf->var - (uintptr_t)efi_var_buf; diff --git a/lib/lmb.c b/lib/lmb.c index 3a765c11bee..73d5aaf191e 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -10,7 +10,6 @@ #include <efi_loader.h> #include <event.h> #include <image.h> -#include <mapmem.h> #include <lmb.h> #include <log.h> #include <malloc.h> @@ -439,7 +438,6 @@ static bool lmb_should_notify(enum lmb_flags flags) static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op, enum lmb_flags flags) { - u64 efi_addr; u64 pages; efi_status_t status;
@@ -451,11 +449,10 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op, if (!lmb_should_notify(flags)) return 0;
- efi_addr = (uintptr_t)map_sysmem(addr, 0); - pages = efi_size_in_pages(size + (efi_addr & EFI_PAGE_MASK)); - efi_addr &= ~EFI_PAGE_MASK; + pages = efi_size_in_pages(size + (addr & EFI_PAGE_MASK)); + addr &= ~EFI_PAGE_MASK;
- status = efi_add_memory_map_pg(efi_addr, pages, + status = efi_add_memory_map_pg(addr, pages, op == MAP_OP_RESERVE ? EFI_BOOT_SERVICES_DATA : EFI_CONVENTIONAL_MEMORY, @@ -465,7 +462,6 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op, status & ~EFI_ERROR_MASK); return -1; } - unmap_sysmem((void *)(uintptr_t)efi_addr);
return 0; }
This collects together several v3 patches into one, to avoid any temporary test-breakage which would make future bisecting difficult.
For efi_memory, the efi_allocate_pool() call uses a pointer to refer to memory, but efi_allocate_pages() uses an int. This causes quite a bit of confusion, since sandbox has a distinction between pointers and addresses. Adjust efi_allocate/free_pages_ext() to handle the conversions.
Update efi_add_memory_map() function and its friend to use an address rather than a pointer cast to an integer.
For lmb, now that efi_add_memory_map_pg() uses a address rather than a pointer cast to an int, we can simplify the code here.
For efi_reserve_memory(), use addresses rather than pointers, so that it doesn't have to use mapmem.
In general this simplifies the code, but the main benefit is that casts are no-longer needed in most places, so the compiler can check that we are doing the right thing.
For efi_add_runtime_mmio(), now that efi_add_memory_map() takes an address rather than a pointer, do the mapping in this function.
Update the memset() parameter to be a char while we are here.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v4)
Changes in v4: - Combine the various pointer-to-address patches into one
include/efi_loader.h | 13 +++++------ lib/efi_loader/efi_bootmgr.c | 3 ++- lib/efi_loader/efi_boottime.c | 37 ++++++++++++++++++++++-------- lib/efi_loader/efi_dt_fixup.c | 4 ---- lib/efi_loader/efi_image_loader.c | 3 ++- lib/efi_loader/efi_memory.c | 38 ++++++++++--------------------- lib/efi_loader/efi_runtime.c | 3 ++- lib/efi_loader/efi_var_mem.c | 6 ++--- lib/lmb.c | 10 +++----- 9 files changed, 57 insertions(+), 60 deletions(-)
Applied to sjg/master, thanks!
Update this function to work correctly with sandbox
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_helper.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c index fd89ef6036f..72902a5ade3 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -486,12 +486,11 @@ 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; + new_fdt = map_sysmem(new_fdt_addr, fdt_size); memcpy(new_fdt, fdt, fdt_totalsize(fdt)); fdt_set_totalsize(new_fdt, fdt_size);
- *fdtp = (void *)(uintptr_t)new_fdt_addr; + *fdtp = new_fdt; done: return ret; }
Update this function to work correctly with sandbox
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_helper.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Applied to sjg/master, thanks!
Now that these problems have been resolved, drop the comments.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_acpi.c | 2 -- lib/efi_loader/efi_bootmgr.c | 3 --- lib/efi_loader/efi_smbios.c | 6 +----- 3 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c index ffb360d461b..67bd7f8ca24 100644 --- a/lib/efi_loader/efi_acpi.c +++ b/lib/efi_loader/efi_acpi.c @@ -28,14 +28,12 @@ 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 30d4ce09e7e..9383adbd958 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -366,8 +366,6 @@ 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) { @@ -402,7 +400,6 @@ 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) diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 02d4a5dd045..8d2ef6deb51 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -40,11 +40,7 @@ efi_status_t efi_smbios_register(void) return EFI_NOT_FOUND; }
- /* - * Mark space used for tables/ - * - * TODO(sjg): This should use (ulong)map_sysmem(addr, ...) - */ + /* Mark space used for tables */ ret = efi_add_memory_map(addr, TABLE_SIZE, EFI_RUNTIME_SERVICES_DATA); if (ret) return ret;
Hi Simon,
On Wed, 11 Dec 2024 at 15:55, Simon Glass sjg@chromium.org wrote:
Now that these problems have been resolved, drop the comments.
Please drop this along with the patch that added it
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_acpi.c | 2 -- lib/efi_loader/efi_bootmgr.c | 3 --- lib/efi_loader/efi_smbios.c | 6 +----- 3 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c index ffb360d461b..67bd7f8ca24 100644 --- a/lib/efi_loader/efi_acpi.c +++ b/lib/efi_loader/efi_acpi.c @@ -28,14 +28,12 @@ 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 30d4ce09e7e..9383adbd958 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -366,8 +366,6 @@ 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) {@@ -402,7 +400,6 @@ 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)diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index 02d4a5dd045..8d2ef6deb51 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -40,11 +40,7 @@ efi_status_t efi_smbios_register(void) return EFI_NOT_FOUND; }
/** Mark space used for tables/** TODO(sjg): This should use (ulong)map_sysmem(addr, ...)*/
/* Mark space used for tables */ ret = efi_add_memory_map(addr, TABLE_SIZE, EFI_RUNTIME_SERVICES_DATA); if (ret) return ret;-- 2.34.1
Hi Simon,
On Wed, 11 Dec 2024 at 15:55, Simon Glass sjg@chromium.org wrote:
Now that these problems have been resolved, drop the comments.
Please drop this along with the patch that added it
Thanks /Ilias
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
lib/efi_loader/efi_acpi.c | 2 -- lib/efi_loader/efi_bootmgr.c | 3 --- lib/efi_loader/efi_smbios.c | 6 +----- 3 files changed, 1 insertion(+), 10 deletions(-)
Applied to sjg/master, thanks!
Update this function to use map_sysmem() so that it can work correctly on sandbox
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_bootmgr.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 9383adbd958..98799aead84 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -498,6 +498,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp, * If the file is PE-COFF image, load the downloaded file. */ uri_len = strlen(uridp->uri); + source_buffer = map_sysmem(image_addr, image_size); if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) || !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) { ret = prepare_loaded_image(lo_label, image_addr, image_size, @@ -507,21 +508,19 @@ 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) { + } else if (efi_check_pe(source_buffer, image_size, NULL) == + EFI_SUCCESS) { /* * loaded_dp must exist until efi application returns, * will be freed in return_to_efibootmgr event callback. */ loaded_dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, - (uintptr_t)image_addr, image_size); + image_addr, image_size); ret = efi_install_multiple_protocol_interfaces( &mem_handle, &efi_guid_device_path, loaded_dp, NULL); if (ret != EFI_SUCCESS) goto err;
- source_buffer = (void *)image_addr; source_size = image_size; } else { log_err("Error: file type is not supported\n");
Update this function to use map_sysmem() so that it can work correctly on sandbox
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
lib/efi_loader/efi_bootmgr.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Applied to sjg/master, thanks!
This function should take a pointer, not an address. Update it along with all users.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v5: - Drop the enum / mem_type patch as it is not needed - Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v3: - Add comment to struct efi_device_path_memory - Use a pointer for the values in struct efi_device_path_memory
Changes in v2: - Drop patch 'Convert efi_get_memory_map() to return pointers' - Drop patch 'efi_loader: Make more use of ulong' - Significantly expand and redirect the series
include/efi_api.h | 10 ++++++++++ include/efi_loader.h | 3 +-- lib/efi_loader/efi_bootbin.c | 3 +-- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_device_path.c | 16 ++++++++-------- 5 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index f07d074f93b..f7da915b0e4 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -573,6 +573,16 @@ struct efi_mac_addr { # define DEVICE_PATH_SUB_TYPE_VENDOR 0x04 # define DEVICE_PATH_SUB_TYPE_CONTROLLER 0x05
+/** + * struct efi_device_path_memory - 'Memory Mapped Device Path' object + * + * @dp: header for device-path protocol + * @memory_type: see enum efi_memory_type + * @start_address: start address of the memory; note that this is provided to + * the EFI application so must be a pointer cast to u64 + * @end_address: end address of the memory; note that this is provided to + * the EFI application so must be a pointer cast to u64 + */ struct efi_device_path_memory { struct efi_device_path dp; u32 memory_type; diff --git a/include/efi_loader.h b/include/efi_loader.h index 9143471c1ce..1e5a067deca 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -918,8 +918,7 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part); struct efi_device_path *efi_dp_from_file(const struct efi_device_path *dp, const char *path); struct efi_device_path *efi_dp_from_eth(void); -struct efi_device_path *efi_dp_from_mem(uint32_t mem_type, - uint64_t start_address, +struct efi_device_path *efi_dp_from_mem(uint32_t mem_type, void *start_ptr, size_t size); /* Determine the last device path node that is not the end node. */ const struct efi_device_path *efi_dp_last_node( diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index a87006b3c0e..7e7a6bf31aa 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -136,8 +136,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) * loaded directly into memory via JTAG, etc: */ file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, - (uintptr_t)source_buffer, - source_size); + source_buffer, source_size); /* * Make sure that device for device_path exist * in load_image(). Otherwise, shell and grub will fail. diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 98799aead84..e3b8dfb6013 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -515,7 +515,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp, * will be freed in return_to_efibootmgr event callback. */ loaded_dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, - image_addr, image_size); + source_buffer, image_size); ret = efi_install_multiple_protocol_interfaces( &mem_handle, &efi_guid_device_path, loaded_dp, NULL); if (ret != EFI_SUCCESS) diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index ee387e1dfd4..75745214501 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -11,6 +11,7 @@ #include <dm.h> #include <dm/root.h> #include <log.h> +#include <mapmem.h> #include <net.h> #include <usb.h> #include <mmc.h> @@ -975,8 +976,7 @@ struct efi_device_path __maybe_unused *efi_dp_from_eth(void) }
/* Construct a device-path for memory-mapped image */ -struct efi_device_path *efi_dp_from_mem(uint32_t memory_type, - uint64_t start_address, +struct efi_device_path *efi_dp_from_mem(uint32_t memory_type, void *start_ptr, size_t size) { struct efi_device_path_memory *mdp; @@ -991,8 +991,8 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type, mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY; mdp->dp.length = sizeof(*mdp); mdp->memory_type = memory_type; - mdp->start_address = start_address; - mdp->end_address = start_address + size; + mdp->start_address = (uintptr_t)start_ptr; + mdp->end_address = mdp->start_address + size; buf = &mdp[1];
*((struct efi_device_path *)buf) = END; @@ -1061,7 +1061,7 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, struct efi_device_path *dp; struct disk_partition fs_partition; size_t image_size; - void *image_addr; + void *image_ptr; int part = 0;
if (path && !file) @@ -1070,10 +1070,10 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr, if (IS_ENABLED(CONFIG_EFI_BINARY_EXEC) && (!strcmp(dev, "Mem") || !strcmp(dev, "hostfs"))) { /* loadm command and semihosting */ - efi_get_image_parameters(&image_addr, &image_size); + efi_get_image_parameters(&image_ptr, &image_size);
- dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, - (uintptr_t)image_addr, image_size); + dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, image_ptr, + image_size); } else if (IS_ENABLED(CONFIG_NETDEVICES) && !strcmp(dev, "Net")) { dp = efi_dp_from_eth(); } else if (!strcmp(dev, "Uart")) {
This function should take a pointer, not an address. Update it along with all users.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v5: - Drop the enum / mem_type patch as it is not needed - Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v3: - Add comment to struct efi_device_path_memory - Use a pointer for the values in struct efi_device_path_memory
Changes in v2: - Drop patch 'Convert efi_get_memory_map() to return pointers' - Drop patch 'efi_loader: Make more use of ulong' - Significantly expand and redirect the series
include/efi_api.h | 10 ++++++++++ include/efi_loader.h | 3 +-- lib/efi_loader/efi_bootbin.c | 3 +-- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_device_path.c | 16 ++++++++-------- 5 files changed, 21 insertions(+), 13 deletions(-)
Applied to sjg/master, thanks!
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
The EFI-loader implementation converts things back and forth between addresses and pointers, with not much consistency in how this is done.
Within most of U-Boot a pointer is a void * and an address is a ulong
This convention is very helpful, since it is obvious in common code as to whether you need to call map_sysmem() and friends, or not.
As part of cleaning up the EFI memory-management, I found it almost impossible to know in some cases whether something is an address or a pointer. I decided to give up on that and come back to it when this is resolved.
This series starts applying the normal ulong/void * convention to the EFI_loader code, so making things easier to follow. For now, u64 is often used instead of ulong, but the effect is the same.
The main changes are:
- Rather than using the external struct efi_mem_desc data-structure for internal bookkeeping, create a new one, so it can have different semantics
- Within efi_memory.c addresses are used, rather than addresses masquerading as pointers. The conversions are done in efi_boottime
Unforunately my foray into attempting to use enum for the memory type failed. The problem is not just that the external interface cannot use an enum. In fact the enum in the spec is really just confusing, since values not mentioned in the enum are valid. While we could handle this by declaring a few more values in enum efi_memory_type, it doesn't seem worth it. For example, if we declare 0x6fffffff and -1 as valid values, we get the correct range, but then we need to be careful about conversion in efi_boottime. If we declare 0xffffffff as valid, then the enum ends up being 64-bits wide! Yes, that would work, I suppose it wouldn't matter, but at that point I believe using an enum is actually more confusing than not. It also made passing SCT tricky, since invalid values are passed in...
So my general feedback on this is - Split it into a number of reviewable patches - Send the typos & doc changes in one patchset that we can easily pick up since it's mostly reviewed - Drop the two patches that add 'sjg' stuff that are removed later - Investigate why there's a reserved member in efi_mem_desc -- which I think is a bug and send the fix as a single patch - Drop the renames and duplication of efi_mem_desc with priv_mem_desc. I don't see any point in duplication that, keeping the memory descriptors in a list as it currently works is fine - Send the map_to_sysmem() places you need in a different patchset with an explanation of *why* this is working currently (or not working)
Hope this helps /Ilias
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Changes in v5:
- Drop the enum / mem_type patch as it is not needed
- Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v4:
- Add new patch to show the address for pool allocations
- Combine the various pointer-to-address patches into one
Changes in v3:
- Adjust comments
- Show the returned address rather than the pointer
- Put the header-file in its own section
- Add comment to struct efi_device_path_memory
- Use a pointer for the values in struct efi_device_path_memory
Changes in v2:
- Fix missing @
- Note that this holds pointers, not addresses
- Add a new patch with comments where incorrect addresses are used
- Use EFI_PRINT() instead of log_debug()
- Rebase on early patch
- Add new patch to add the EFI-loader API documentation
- Add new patch to use a separate stuct for memory nodes
- Drop patch 'Convert efi_get_memory_map() to return pointers'
- Drop patch 'efi_loader: Make more use of ulong'
- Significantly expand and redirect the series
Simon Glass (23): efi: Define fields in struct efi_mem_desc efi_loader: Fix typos in enum efi_allocate_type efi_loader: Add comments where incorrect addresses are used efi_loader: Show the resulting memory address from an alloc efi_loader: Update startimage_exit self-test to check error efi_loader: Move some memory-function comments to header doc: efi: Add the EFI-loader API documentation efi_loader: Use a separate struct for memory nodes efi_loader: Drop virtual_start from priv_mem_desc efi_loader: Drop reserved from priv_mem_desc efi_loader: Use the enum for the memory type in priv_mem_desc efi_loader: Avoid assigning desc in efi_mem_carve_out() efi_loader: Move struct efi_mem_list fields together efi_loader: Rename struct efi_mem_list to mem_node efi_loader: Rename physical_start to base efi_loader: Use correct type in efi_add_runtime_mmio() efi_loader: Show the address for pool allocations efi_loader: Don't try to add sandbox runtime code efi_loader: Update to use addresses internally efi_loader: Correct address-usage in copy_fdt() efi_loader: Drop comments about incorrect addresses efi_bootmgr: Avoid casts in try_load_from_uri_path() efi_loader: Simplify efi_dp_from_mem()
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- doc/api/efi.rst | 6 + include/efi.h | 23 +- include/efi_api.h | 10 + include/efi_loader.h | 87 +++++- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 10 +- lib/efi_loader/efi_boottime.c | 43 ++- lib/efi_loader/efi_device_path.c | 16 +- lib/efi_loader/efi_dt_fixup.c | 4 - lib/efi_loader/efi_helper.c | 4 +- lib/efi_loader/efi_image_loader.c | 3 +- lib/efi_loader/efi_memory.c | 261 +++++++----------- lib/efi_loader/efi_runtime.c | 7 +- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 18 files changed, 278 insertions(+), 225 deletions(-)
-- 2.34.1
Hi Ilias,
On Wed, 11 Dec 2024 at 23:40, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
The EFI-loader implementation converts things back and forth between addresses and pointers, with not much consistency in how this is done.
Within most of U-Boot a pointer is a void * and an address is a ulong
This convention is very helpful, since it is obvious in common code as to whether you need to call map_sysmem() and friends, or not.
As part of cleaning up the EFI memory-management, I found it almost impossible to know in some cases whether something is an address or a pointer. I decided to give up on that and come back to it when this is resolved.
This series starts applying the normal ulong/void * convention to the EFI_loader code, so making things easier to follow. For now, u64 is often used instead of ulong, but the effect is the same.
The main changes are:
- Rather than using the external struct efi_mem_desc data-structure for internal bookkeeping, create a new one, so it can have different semantics
- Within efi_memory.c addresses are used, rather than addresses masquerading as pointers. The conversions are done in efi_boottime
Unforunately my foray into attempting to use enum for the memory type failed. The problem is not just that the external interface cannot use an enum. In fact the enum in the spec is really just confusing, since values not mentioned in the enum are valid. While we could handle this by declaring a few more values in enum efi_memory_type, it doesn't seem worth it. For example, if we declare 0x6fffffff and -1 as valid values, we get the correct range, but then we need to be careful about conversion in efi_boottime. If we declare 0xffffffff as valid, then the enum ends up being 64-bits wide! Yes, that would work, I suppose it wouldn't matter, but at that point I believe using an enum is actually more confusing than not. It also made passing SCT tricky, since invalid values are passed in...
So my general feedback on this is
- Split it into a number of reviewable patches
- Send the typos & doc changes in one patchset that we can easily pick
up since it's mostly reviewed
- Drop the two patches that add 'sjg' stuff that are removed later
- Investigate why there's a reserved member in efi_mem_desc -- which I
think is a bug and send the fix as a single patch
- Drop the renames and duplication of efi_mem_desc with priv_mem_desc.
I don't see any point in duplication that, keeping the memory descriptors in a list as it currently works is fine
All the other points are OK, just more work. However, this one defeats the purpose of the series. As you can see, I am lining up the EFI internal-table more with how U-Boot and lmb do things, with addresses. I cannot use both addresses and pointers in the same struct...I initially did that but it was just far too confusing.
- Send the map_to_sysmem() places you need in a different patchset
with an explanation of *why* this is working currently (or not working)
Hope this helps /Ilias
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Changes in v5:
- Drop the enum / mem_type patch as it is not needed
- Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v4:
- Add new patch to show the address for pool allocations
- Combine the various pointer-to-address patches into one
Changes in v3:
- Adjust comments
- Show the returned address rather than the pointer
- Put the header-file in its own section
- Add comment to struct efi_device_path_memory
- Use a pointer for the values in struct efi_device_path_memory
Changes in v2:
- Fix missing @
- Note that this holds pointers, not addresses
- Add a new patch with comments where incorrect addresses are used
- Use EFI_PRINT() instead of log_debug()
- Rebase on early patch
- Add new patch to add the EFI-loader API documentation
- Add new patch to use a separate stuct for memory nodes
- Drop patch 'Convert efi_get_memory_map() to return pointers'
- Drop patch 'efi_loader: Make more use of ulong'
- Significantly expand and redirect the series
Simon Glass (23): efi: Define fields in struct efi_mem_desc efi_loader: Fix typos in enum efi_allocate_type efi_loader: Add comments where incorrect addresses are used efi_loader: Show the resulting memory address from an alloc efi_loader: Update startimage_exit self-test to check error efi_loader: Move some memory-function comments to header doc: efi: Add the EFI-loader API documentation efi_loader: Use a separate struct for memory nodes efi_loader: Drop virtual_start from priv_mem_desc efi_loader: Drop reserved from priv_mem_desc efi_loader: Use the enum for the memory type in priv_mem_desc efi_loader: Avoid assigning desc in efi_mem_carve_out() efi_loader: Move struct efi_mem_list fields together efi_loader: Rename struct efi_mem_list to mem_node efi_loader: Rename physical_start to base efi_loader: Use correct type in efi_add_runtime_mmio() efi_loader: Show the address for pool allocations efi_loader: Don't try to add sandbox runtime code efi_loader: Update to use addresses internally efi_loader: Correct address-usage in copy_fdt() efi_loader: Drop comments about incorrect addresses efi_bootmgr: Avoid casts in try_load_from_uri_path() efi_loader: Simplify efi_dp_from_mem()
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- doc/api/efi.rst | 6 + include/efi.h | 23 +- include/efi_api.h | 10 + include/efi_loader.h | 87 +++++- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 10 +- lib/efi_loader/efi_boottime.c | 43 ++- lib/efi_loader/efi_device_path.c | 16 +- lib/efi_loader/efi_dt_fixup.c | 4 - lib/efi_loader/efi_helper.c | 4 +- lib/efi_loader/efi_image_loader.c | 3 +- lib/efi_loader/efi_memory.c | 261 +++++++----------- lib/efi_loader/efi_runtime.c | 7 +- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 18 files changed, 278 insertions(+), 225 deletions(-)
-- 2.34.1
Regards, SImon
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:40, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
The EFI-loader implementation converts things back and forth between addresses and pointers, with not much consistency in how this is done.
Within most of U-Boot a pointer is a void * and an address is a ulong
This convention is very helpful, since it is obvious in common code as to whether you need to call map_sysmem() and friends, or not.
As part of cleaning up the EFI memory-management, I found it almost impossible to know in some cases whether something is an address or a pointer. I decided to give up on that and come back to it when this is resolved.
This series starts applying the normal ulong/void * convention to the EFI_loader code, so making things easier to follow. For now, u64 is often used instead of ulong, but the effect is the same.
The main changes are:
- Rather than using the external struct efi_mem_desc data-structure for internal bookkeeping, create a new one, so it can have different semantics
- Within efi_memory.c addresses are used, rather than addresses masquerading as pointers. The conversions are done in efi_boottime
Unforunately my foray into attempting to use enum for the memory type failed. The problem is not just that the external interface cannot use an enum. In fact the enum in the spec is really just confusing, since values not mentioned in the enum are valid. While we could handle this by declaring a few more values in enum efi_memory_type, it doesn't seem worth it. For example, if we declare 0x6fffffff and -1 as valid values, we get the correct range, but then we need to be careful about conversion in efi_boottime. If we declare 0xffffffff as valid, then the enum ends up being 64-bits wide! Yes, that would work, I suppose it wouldn't matter, but at that point I believe using an enum is actually more confusing than not. It also made passing SCT tricky, since invalid values are passed in...
So my general feedback on this is
- Split it into a number of reviewable patches
- Send the typos & doc changes in one patchset that we can easily pick
up since it's mostly reviewed
- Drop the two patches that add 'sjg' stuff that are removed later
- Investigate why there's a reserved member in efi_mem_desc -- which I
think is a bug and send the fix as a single patch
- Drop the renames and duplication of efi_mem_desc with priv_mem_desc.
I don't see any point in duplication that, keeping the memory descriptors in a list as it currently works is fine
All the other points are OK, just more work. However, this one defeats the purpose of the series. As you can see, I am lining up the EFI internal-table more with how U-Boot and lmb do things, with addresses.
I think Heinrich and I have repeated this more than once. EFI works with physical addresses, just like sandbox needs to map_sysmem. It's way easier for us to review and look at code exactly how the spec describes it. Also the reason for duplicating that in the commit message is - the name is confusing, which reallys int - The 'reserved' filed isn't needed, which is probably just a generic bug in the EFI struct definition
Thanks /Ilias
I cannot use both addresses and pointers in the same struct...I initially did that but it was just far too confusing.
- Send the map_to_sysmem() places you need in a different patchset
with an explanation of *why* this is working currently (or not working)
Hope this helps /Ilias
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Changes in v5:
- Drop the enum / mem_type patch as it is not needed
- Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v4:
- Add new patch to show the address for pool allocations
- Combine the various pointer-to-address patches into one
Changes in v3:
- Adjust comments
- Show the returned address rather than the pointer
- Put the header-file in its own section
- Add comment to struct efi_device_path_memory
- Use a pointer for the values in struct efi_device_path_memory
Changes in v2:
- Fix missing @
- Note that this holds pointers, not addresses
- Add a new patch with comments where incorrect addresses are used
- Use EFI_PRINT() instead of log_debug()
- Rebase on early patch
- Add new patch to add the EFI-loader API documentation
- Add new patch to use a separate stuct for memory nodes
- Drop patch 'Convert efi_get_memory_map() to return pointers'
- Drop patch 'efi_loader: Make more use of ulong'
- Significantly expand and redirect the series
Simon Glass (23): efi: Define fields in struct efi_mem_desc efi_loader: Fix typos in enum efi_allocate_type efi_loader: Add comments where incorrect addresses are used efi_loader: Show the resulting memory address from an alloc efi_loader: Update startimage_exit self-test to check error efi_loader: Move some memory-function comments to header doc: efi: Add the EFI-loader API documentation efi_loader: Use a separate struct for memory nodes efi_loader: Drop virtual_start from priv_mem_desc efi_loader: Drop reserved from priv_mem_desc efi_loader: Use the enum for the memory type in priv_mem_desc efi_loader: Avoid assigning desc in efi_mem_carve_out() efi_loader: Move struct efi_mem_list fields together efi_loader: Rename struct efi_mem_list to mem_node efi_loader: Rename physical_start to base efi_loader: Use correct type in efi_add_runtime_mmio() efi_loader: Show the address for pool allocations efi_loader: Don't try to add sandbox runtime code efi_loader: Update to use addresses internally efi_loader: Correct address-usage in copy_fdt() efi_loader: Drop comments about incorrect addresses efi_bootmgr: Avoid casts in try_load_from_uri_path() efi_loader: Simplify efi_dp_from_mem()
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- doc/api/efi.rst | 6 + include/efi.h | 23 +- include/efi_api.h | 10 + include/efi_loader.h | 87 +++++- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 10 +- lib/efi_loader/efi_boottime.c | 43 ++- lib/efi_loader/efi_device_path.c | 16 +- lib/efi_loader/efi_dt_fixup.c | 4 - lib/efi_loader/efi_helper.c | 4 +- lib/efi_loader/efi_image_loader.c | 3 +- lib/efi_loader/efi_memory.c | 261 +++++++----------- lib/efi_loader/efi_runtime.c | 7 +- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 18 files changed, 278 insertions(+), 225 deletions(-)
-- 2.34.1
Regards, SImon
Hi Ilias,
On Thu, 12 Dec 2024 at 08:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:40, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
The EFI-loader implementation converts things back and forth between addresses and pointers, with not much consistency in how this is done.
Within most of U-Boot a pointer is a void * and an address is a ulong
This convention is very helpful, since it is obvious in common code as to whether you need to call map_sysmem() and friends, or not.
As part of cleaning up the EFI memory-management, I found it almost impossible to know in some cases whether something is an address or a pointer. I decided to give up on that and come back to it when this is resolved.
This series starts applying the normal ulong/void * convention to the EFI_loader code, so making things easier to follow. For now, u64 is often used instead of ulong, but the effect is the same.
The main changes are:
- Rather than using the external struct efi_mem_desc data-structure for internal bookkeeping, create a new one, so it can have different semantics
- Within efi_memory.c addresses are used, rather than addresses masquerading as pointers. The conversions are done in efi_boottime
Unforunately my foray into attempting to use enum for the memory type failed. The problem is not just that the external interface cannot use an enum. In fact the enum in the spec is really just confusing, since values not mentioned in the enum are valid. While we could handle this by declaring a few more values in enum efi_memory_type, it doesn't seem worth it. For example, if we declare 0x6fffffff and -1 as valid values, we get the correct range, but then we need to be careful about conversion in efi_boottime. If we declare 0xffffffff as valid, then the enum ends up being 64-bits wide! Yes, that would work, I suppose it wouldn't matter, but at that point I believe using an enum is actually more confusing than not. It also made passing SCT tricky, since invalid values are passed in...
So my general feedback on this is
- Split it into a number of reviewable patches
- Send the typos & doc changes in one patchset that we can easily pick
up since it's mostly reviewed
- Drop the two patches that add 'sjg' stuff that are removed later
- Investigate why there's a reserved member in efi_mem_desc -- which I
think is a bug and send the fix as a single patch
- Drop the renames and duplication of efi_mem_desc with priv_mem_desc.
I don't see any point in duplication that, keeping the memory descriptors in a list as it currently works is fine
All the other points are OK, just more work. However, this one defeats the purpose of the series. As you can see, I am lining up the EFI internal-table more with how U-Boot and lmb do things, with addresses.
I think Heinrich and I have repeated this more than once.
As an aside, I notice that you say this every now and then but I don't think you realise how it comes across to the other person.
EFI works with physical addresses, just like sandbox needs to map_sysmem. It's way easier for us to review and look at code exactly how the spec describes it.
So it looks like this series is not going to fly, because you *want* the internal EFI tables to hold pointers-as-u64, rather than addresses. You don't want to do that translation at the EFI boundary. Is that right?
Also the reason for duplicating that in the commit
message is
- the name is confusing, which reallys int
I am unsure what you are referring to here.
- The 'reserved' filed isn't needed, which is probably just a generic
bug in the EFI struct definition
I was surprised to see that I wrote that code. My guess is that it is there to ensure the alignment is correct. The first field is u32 and the third is u64 so perhaps I was concerned that the compiler would not naturally align the u64? It was over 9 years ago and I don't remember.
struct efi_mem_desc { u32 type; u32 reserved; efi_physical_addr_t physical_start; efi_virtual_addr_t virtual_start; u64 num_pages; u64 attribute; };
The spec says "Unless otherwise specified all data types are naturally aligned. Structures are aligned on boundaries equal to the largest internal datum of the structure and internal data are implicitly padded to achieve natural alignment."
I'm pretty sure any compiler that we use will handle this, so you are right, we don't need the field.
Regards, Simon
Thanks /Ilias
I cannot use both addresses and pointers in the same struct...I initially did that but it was just far too confusing.
- Send the map_to_sysmem() places you need in a different patchset
with an explanation of *why* this is working currently (or not working)
Hope this helps /Ilias
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Changes in v5:
- Drop the enum / mem_type patch as it is not needed
- Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v4:
- Add new patch to show the address for pool allocations
- Combine the various pointer-to-address patches into one
Changes in v3:
- Adjust comments
- Show the returned address rather than the pointer
- Put the header-file in its own section
- Add comment to struct efi_device_path_memory
- Use a pointer for the values in struct efi_device_path_memory
Changes in v2:
- Fix missing @
- Note that this holds pointers, not addresses
- Add a new patch with comments where incorrect addresses are used
- Use EFI_PRINT() instead of log_debug()
- Rebase on early patch
- Add new patch to add the EFI-loader API documentation
- Add new patch to use a separate stuct for memory nodes
- Drop patch 'Convert efi_get_memory_map() to return pointers'
- Drop patch 'efi_loader: Make more use of ulong'
- Significantly expand and redirect the series
Simon Glass (23): efi: Define fields in struct efi_mem_desc efi_loader: Fix typos in enum efi_allocate_type efi_loader: Add comments where incorrect addresses are used efi_loader: Show the resulting memory address from an alloc efi_loader: Update startimage_exit self-test to check error efi_loader: Move some memory-function comments to header doc: efi: Add the EFI-loader API documentation efi_loader: Use a separate struct for memory nodes efi_loader: Drop virtual_start from priv_mem_desc efi_loader: Drop reserved from priv_mem_desc efi_loader: Use the enum for the memory type in priv_mem_desc efi_loader: Avoid assigning desc in efi_mem_carve_out() efi_loader: Move struct efi_mem_list fields together efi_loader: Rename struct efi_mem_list to mem_node efi_loader: Rename physical_start to base efi_loader: Use correct type in efi_add_runtime_mmio() efi_loader: Show the address for pool allocations efi_loader: Don't try to add sandbox runtime code efi_loader: Update to use addresses internally efi_loader: Correct address-usage in copy_fdt() efi_loader: Drop comments about incorrect addresses efi_bootmgr: Avoid casts in try_load_from_uri_path() efi_loader: Simplify efi_dp_from_mem()
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- doc/api/efi.rst | 6 + include/efi.h | 23 +- include/efi_api.h | 10 + include/efi_loader.h | 87 +++++- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 10 +- lib/efi_loader/efi_boottime.c | 43 ++- lib/efi_loader/efi_device_path.c | 16 +- lib/efi_loader/efi_dt_fixup.c | 4 - lib/efi_loader/efi_helper.c | 4 +- lib/efi_loader/efi_image_loader.c | 3 +- lib/efi_loader/efi_memory.c | 261 +++++++----------- lib/efi_loader/efi_runtime.c | 7 +- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 18 files changed, 278 insertions(+), 225 deletions(-)
-- 2.34.1
Regards, SImon
From: Simon Glass sjg@chromium.org Date: Fri, 13 Dec 2024 07:02:42 -0700
Hi Simon,
Hi Ilias,
On Thu, 12 Dec 2024 at 08:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:40, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
The EFI-loader implementation converts things back and forth between addresses and pointers, with not much consistency in how this is done.
Within most of U-Boot a pointer is a void * and an address is a ulong
This convention is very helpful, since it is obvious in common code as to whether you need to call map_sysmem() and friends, or not.
As part of cleaning up the EFI memory-management, I found it almost impossible to know in some cases whether something is an address or a pointer. I decided to give up on that and come back to it when this is resolved.
This series starts applying the normal ulong/void * convention to the EFI_loader code, so making things easier to follow. For now, u64 is often used instead of ulong, but the effect is the same.
The main changes are:
- Rather than using the external struct efi_mem_desc data-structure for internal bookkeeping, create a new one, so it can have different semantics
- Within efi_memory.c addresses are used, rather than addresses masquerading as pointers. The conversions are done in efi_boottime
Unforunately my foray into attempting to use enum for the memory type failed. The problem is not just that the external interface cannot use an enum. In fact the enum in the spec is really just confusing, since values not mentioned in the enum are valid. While we could handle this by declaring a few more values in enum efi_memory_type, it doesn't seem worth it. For example, if we declare 0x6fffffff and -1 as valid values, we get the correct range, but then we need to be careful about conversion in efi_boottime. If we declare 0xffffffff as valid, then the enum ends up being 64-bits wide! Yes, that would work, I suppose it wouldn't matter, but at that point I believe using an enum is actually more confusing than not. It also made passing SCT tricky, since invalid values are passed in...
So my general feedback on this is
- Split it into a number of reviewable patches
- Send the typos & doc changes in one patchset that we can easily pick
up since it's mostly reviewed
- Drop the two patches that add 'sjg' stuff that are removed later
- Investigate why there's a reserved member in efi_mem_desc -- which I
think is a bug and send the fix as a single patch
- Drop the renames and duplication of efi_mem_desc with priv_mem_desc.
I don't see any point in duplication that, keeping the memory descriptors in a list as it currently works is fine
All the other points are OK, just more work. However, this one defeats the purpose of the series. As you can see, I am lining up the EFI internal-table more with how U-Boot and lmb do things, with addresses.
I think Heinrich and I have repeated this more than once.
As an aside, I notice that you say this every now and then but I don't think you realise how it comes across to the other person.
EFI works with physical addresses, just like sandbox needs to map_sysmem. It's way easier for us to review and look at code exactly how the spec describes it.
So it looks like this series is not going to fly, because you *want* the internal EFI tables to hold pointers-as-u64, rather than addresses. You don't want to do that translation at the EFI boundary. Is that right?
Also the reason for duplicating that in the commit
message is
- the name is confusing, which reallys int
I am unsure what you are referring to here.
- The 'reserved' filed isn't needed, which is probably just a generic
bug in the EFI struct definition
I was surprised to see that I wrote that code. My guess is that it is there to ensure the alignment is correct. The first field is u32 and the third is u64 so perhaps I was concerned that the compiler would not naturally align the u64? It was over 9 years ago and I don't remember.
struct efi_mem_desc { u32 type; u32 reserved; efi_physical_addr_t physical_start; efi_virtual_addr_t virtual_start; u64 num_pages; u64 attribute; };
The spec says "Unless otherwise specified all data types are naturally aligned. Structures are aligned on boundaries equal to the largest internal datum of the structure and internal data are implicitly padded to achieve natural alignment."
I'm pretty sure any compiler that we use will handle this, so you are right, we don't need the field.
The i386 SysV ABI uses a maximum alignment of 4 bytes, so without that field, the later memebers of the struct would not be naturally aligned. Now whether a 32-bit x86 U-Boot is something we care about is a different question.
Regards, Simon
Thanks /Ilias
I cannot use both addresses and pointers in the same struct...I initially did that but it was just far too confusing.
- Send the map_to_sysmem() places you need in a different patchset
with an explanation of *why* this is working currently (or not working)
Hope this helps /Ilias
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Changes in v5:
- Drop the enum / mem_type patch as it is not needed
- Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v4:
- Add new patch to show the address for pool allocations
- Combine the various pointer-to-address patches into one
Changes in v3:
- Adjust comments
- Show the returned address rather than the pointer
- Put the header-file in its own section
- Add comment to struct efi_device_path_memory
- Use a pointer for the values in struct efi_device_path_memory
Changes in v2:
- Fix missing @
- Note that this holds pointers, not addresses
- Add a new patch with comments where incorrect addresses are used
- Use EFI_PRINT() instead of log_debug()
- Rebase on early patch
- Add new patch to add the EFI-loader API documentation
- Add new patch to use a separate stuct for memory nodes
- Drop patch 'Convert efi_get_memory_map() to return pointers'
- Drop patch 'efi_loader: Make more use of ulong'
- Significantly expand and redirect the series
Simon Glass (23): efi: Define fields in struct efi_mem_desc efi_loader: Fix typos in enum efi_allocate_type efi_loader: Add comments where incorrect addresses are used efi_loader: Show the resulting memory address from an alloc efi_loader: Update startimage_exit self-test to check error efi_loader: Move some memory-function comments to header doc: efi: Add the EFI-loader API documentation efi_loader: Use a separate struct for memory nodes efi_loader: Drop virtual_start from priv_mem_desc efi_loader: Drop reserved from priv_mem_desc efi_loader: Use the enum for the memory type in priv_mem_desc efi_loader: Avoid assigning desc in efi_mem_carve_out() efi_loader: Move struct efi_mem_list fields together efi_loader: Rename struct efi_mem_list to mem_node efi_loader: Rename physical_start to base efi_loader: Use correct type in efi_add_runtime_mmio() efi_loader: Show the address for pool allocations efi_loader: Don't try to add sandbox runtime code efi_loader: Update to use addresses internally efi_loader: Correct address-usage in copy_fdt() efi_loader: Drop comments about incorrect addresses efi_bootmgr: Avoid casts in try_load_from_uri_path() efi_loader: Simplify efi_dp_from_mem()
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- doc/api/efi.rst | 6 + include/efi.h | 23 +- include/efi_api.h | 10 + include/efi_loader.h | 87 +++++- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 10 +- lib/efi_loader/efi_boottime.c | 43 ++- lib/efi_loader/efi_device_path.c | 16 +- lib/efi_loader/efi_dt_fixup.c | 4 - lib/efi_loader/efi_helper.c | 4 +- lib/efi_loader/efi_image_loader.c | 3 +- lib/efi_loader/efi_memory.c | 261 +++++++----------- lib/efi_loader/efi_runtime.c | 7 +- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 18 files changed, 278 insertions(+), 225 deletions(-)
-- 2.34.1
Regards, SImon
Hi again Ilias,
On Fri, 13 Dec 2024 at 07:02, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 08:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:40, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
The EFI-loader implementation converts things back and forth between addresses and pointers, with not much consistency in how this is done.
Within most of U-Boot a pointer is a void * and an address is a ulong
This convention is very helpful, since it is obvious in common code as to whether you need to call map_sysmem() and friends, or not.
As part of cleaning up the EFI memory-management, I found it almost impossible to know in some cases whether something is an address or a pointer. I decided to give up on that and come back to it when this is resolved.
This series starts applying the normal ulong/void * convention to the EFI_loader code, so making things easier to follow. For now, u64 is often used instead of ulong, but the effect is the same.
The main changes are:
- Rather than using the external struct efi_mem_desc data-structure for internal bookkeeping, create a new one, so it can have different semantics
- Within efi_memory.c addresses are used, rather than addresses masquerading as pointers. The conversions are done in efi_boottime
Unforunately my foray into attempting to use enum for the memory type failed. The problem is not just that the external interface cannot use an enum. In fact the enum in the spec is really just confusing, since values not mentioned in the enum are valid. While we could handle this by declaring a few more values in enum efi_memory_type, it doesn't seem worth it. For example, if we declare 0x6fffffff and -1 as valid values, we get the correct range, but then we need to be careful about conversion in efi_boottime. If we declare 0xffffffff as valid, then the enum ends up being 64-bits wide! Yes, that would work, I suppose it wouldn't matter, but at that point I believe using an enum is actually more confusing than not. It also made passing SCT tricky, since invalid values are passed in...
So my general feedback on this is
- Split it into a number of reviewable patches
- Send the typos & doc changes in one patchset that we can easily pick
up since it's mostly reviewed
- Drop the two patches that add 'sjg' stuff that are removed later
- Investigate why there's a reserved member in efi_mem_desc -- which I
think is a bug and send the fix as a single patch
- Drop the renames and duplication of efi_mem_desc with priv_mem_desc.
I don't see any point in duplication that, keeping the memory descriptors in a list as it currently works is fine
All the other points are OK, just more work. However, this one defeats the purpose of the series. As you can see, I am lining up the EFI internal-table more with how U-Boot and lmb do things, with addresses.
I think Heinrich and I have repeated this more than once.
As an aside, I notice that you say this every now and then but I don't think you realise how it comes across to the other person.
EFI works with physical addresses, just like sandbox needs to map_sysmem. It's way easier for us to review and look at code exactly how the spec describes it.
So it looks like this series is not going to fly, because you *want* the internal EFI tables to hold pointers-as-u64, rather than addresses. You don't want to do that translation at the EFI boundary. Is that right?
Just let me know what you want to do here. From what you said, this is a NAK, but if I have interpreted this incorrectly, and you are OK with the concept and just want some code changes, PLMK.
Also the reason for duplicating that in the commit
message is
- the name is confusing, which reallys int
I am unsure what you are referring to here.
- The 'reserved' filed isn't needed, which is probably just a generic
bug in the EFI struct definition
I was surprised to see that I wrote that code. My guess is that it is there to ensure the alignment is correct. The first field is u32 and the third is u64 so perhaps I was concerned that the compiler would not naturally align the u64? It was over 9 years ago and I don't remember.
struct efi_mem_desc { u32 type; u32 reserved; efi_physical_addr_t physical_start; efi_virtual_addr_t virtual_start; u64 num_pages; u64 attribute; };
The spec says "Unless otherwise specified all data types are naturally aligned. Structures are aligned on boundaries equal to the largest internal datum of the structure and internal data are implicitly padded to achieve natural alignment."
I'm pretty sure any compiler that we use will handle this, so you are right, we don't need the field.
Regards, Simon
Thanks /Ilias
I cannot use both addresses and pointers in the same struct...I initially did that but it was just far too confusing.
- Send the map_to_sysmem() places you need in a different patchset
with an explanation of *why* this is working currently (or not working)
Hope this helps /Ilias
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Changes in v5:
- Drop the enum / mem_type patch as it is not needed
- Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v4:
- Add new patch to show the address for pool allocations
- Combine the various pointer-to-address patches into one
Changes in v3:
- Adjust comments
- Show the returned address rather than the pointer
- Put the header-file in its own section
- Add comment to struct efi_device_path_memory
- Use a pointer for the values in struct efi_device_path_memory
Changes in v2:
- Fix missing @
- Note that this holds pointers, not addresses
- Add a new patch with comments where incorrect addresses are used
- Use EFI_PRINT() instead of log_debug()
- Rebase on early patch
- Add new patch to add the EFI-loader API documentation
- Add new patch to use a separate stuct for memory nodes
- Drop patch 'Convert efi_get_memory_map() to return pointers'
- Drop patch 'efi_loader: Make more use of ulong'
- Significantly expand and redirect the series
Simon Glass (23): efi: Define fields in struct efi_mem_desc efi_loader: Fix typos in enum efi_allocate_type efi_loader: Add comments where incorrect addresses are used efi_loader: Show the resulting memory address from an alloc efi_loader: Update startimage_exit self-test to check error efi_loader: Move some memory-function comments to header doc: efi: Add the EFI-loader API documentation efi_loader: Use a separate struct for memory nodes efi_loader: Drop virtual_start from priv_mem_desc efi_loader: Drop reserved from priv_mem_desc efi_loader: Use the enum for the memory type in priv_mem_desc efi_loader: Avoid assigning desc in efi_mem_carve_out() efi_loader: Move struct efi_mem_list fields together efi_loader: Rename struct efi_mem_list to mem_node efi_loader: Rename physical_start to base efi_loader: Use correct type in efi_add_runtime_mmio() efi_loader: Show the address for pool allocations efi_loader: Don't try to add sandbox runtime code efi_loader: Update to use addresses internally efi_loader: Correct address-usage in copy_fdt() efi_loader: Drop comments about incorrect addresses efi_bootmgr: Avoid casts in try_load_from_uri_path() efi_loader: Simplify efi_dp_from_mem()
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- doc/api/efi.rst | 6 + include/efi.h | 23 +- include/efi_api.h | 10 + include/efi_loader.h | 87 +++++- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 10 +- lib/efi_loader/efi_boottime.c | 43 ++- lib/efi_loader/efi_device_path.c | 16 +- lib/efi_loader/efi_dt_fixup.c | 4 - lib/efi_loader/efi_helper.c | 4 +- lib/efi_loader/efi_image_loader.c | 3 +- lib/efi_loader/efi_memory.c | 261 +++++++----------- lib/efi_loader/efi_runtime.c | 7 +- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 18 files changed, 278 insertions(+), 225 deletions(-)
-- 2.34.1
Rgards, SImon
Hi Simon,
On Mon, 16 Dec 2024 at 17:14, Simon Glass sjg@chromium.org wrote:
Hi again Ilias,
On Fri, 13 Dec 2024 at 07:02, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 08:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:40, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote:
The EFI-loader implementation converts things back and forth between addresses and pointers, with not much consistency in how this is done.
Within most of U-Boot a pointer is a void * and an address is a ulong
This convention is very helpful, since it is obvious in common code as to whether you need to call map_sysmem() and friends, or not.
As part of cleaning up the EFI memory-management, I found it almost impossible to know in some cases whether something is an address or a pointer. I decided to give up on that and come back to it when this is resolved.
This series starts applying the normal ulong/void * convention to the EFI_loader code, so making things easier to follow. For now, u64 is often used instead of ulong, but the effect is the same.
The main changes are:
- Rather than using the external struct efi_mem_desc data-structure for internal bookkeeping, create a new one, so it can have different semantics
- Within efi_memory.c addresses are used, rather than addresses masquerading as pointers. The conversions are done in efi_boottime
Unforunately my foray into attempting to use enum for the memory type failed. The problem is not just that the external interface cannot use an enum. In fact the enum in the spec is really just confusing, since values not mentioned in the enum are valid. While we could handle this by declaring a few more values in enum efi_memory_type, it doesn't seem worth it. For example, if we declare 0x6fffffff and -1 as valid values, we get the correct range, but then we need to be careful about conversion in efi_boottime. If we declare 0xffffffff as valid, then the enum ends up being 64-bits wide! Yes, that would work, I suppose it wouldn't matter, but at that point I believe using an enum is actually more confusing than not. It also made passing SCT tricky, since invalid values are passed in...
So my general feedback on this is
- Split it into a number of reviewable patches
- Send the typos & doc changes in one patchset that we can easily pick
up since it's mostly reviewed
- Drop the two patches that add 'sjg' stuff that are removed later
- Investigate why there's a reserved member in efi_mem_desc -- which I
think is a bug and send the fix as a single patch
- Drop the renames and duplication of efi_mem_desc with priv_mem_desc.
I don't see any point in duplication that, keeping the memory descriptors in a list as it currently works is fine
All the other points are OK, just more work. However, this one defeats the purpose of the series. As you can see, I am lining up the EFI internal-table more with how U-Boot and lmb do things, with addresses.
I think Heinrich and I have repeated this more than once.
As an aside, I notice that you say this every now and then but I don't think you realise how it comes across to the other person.
EFI works with physical addresses, just like sandbox needs to map_sysmem. It's way easier for us to review and look at code exactly how the spec describes it.
So it looks like this series is not going to fly, because you *want* the internal EFI tables to hold pointers-as-u64, rather than addresses. You don't want to do that translation at the EFI boundary. Is that right?
Just let me know what you want to do here. From what you said, this is a NAK, but if I have interpreted this incorrectly, and you are OK with the concept and just want some code changes, PLMK.
It's all summarized here https://lore.kernel.org/u-boot/CAFLszThNvbxZWOmLky0jpFPeUgFhAaFLVkMVVk9UCCP_...
Thanks /Ilias
Also the reason for duplicating that in the commit
message is
- the name is confusing, which reallys int
I am unsure what you are referring to here.
- The 'reserved' filed isn't needed, which is probably just a generic
bug in the EFI struct definition
I was surprised to see that I wrote that code. My guess is that it is there to ensure the alignment is correct. The first field is u32 and the third is u64 so perhaps I was concerned that the compiler would not naturally align the u64? It was over 9 years ago and I don't remember.
struct efi_mem_desc { u32 type; u32 reserved; efi_physical_addr_t physical_start; efi_virtual_addr_t virtual_start; u64 num_pages; u64 attribute; };
The spec says "Unless otherwise specified all data types are naturally aligned. Structures are aligned on boundaries equal to the largest internal datum of the structure and internal data are implicitly padded to achieve natural alignment."
I'm pretty sure any compiler that we use will handle this, so you are right, we don't need the field.
Regards, Simon
Thanks /Ilias
I cannot use both addresses and pointers in the same struct...I initially did that but it was just far too confusing.
- Send the map_to_sysmem() places you need in a different patchset
with an explanation of *why* this is working currently (or not working)
Hope this helps /Ilias
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Changes in v5:
- Drop the enum / mem_type patch as it is not needed
- Drop the patch to remove extra brackets in efi_mem_carve_out()
Changes in v4:
- Add new patch to show the address for pool allocations
- Combine the various pointer-to-address patches into one
Changes in v3:
- Adjust comments
- Show the returned address rather than the pointer
- Put the header-file in its own section
- Add comment to struct efi_device_path_memory
- Use a pointer for the values in struct efi_device_path_memory
Changes in v2:
- Fix missing @
- Note that this holds pointers, not addresses
- Add a new patch with comments where incorrect addresses are used
- Use EFI_PRINT() instead of log_debug()
- Rebase on early patch
- Add new patch to add the EFI-loader API documentation
- Add new patch to use a separate stuct for memory nodes
- Drop patch 'Convert efi_get_memory_map() to return pointers'
- Drop patch 'efi_loader: Make more use of ulong'
- Significantly expand and redirect the series
Simon Glass (23): efi: Define fields in struct efi_mem_desc efi_loader: Fix typos in enum efi_allocate_type efi_loader: Add comments where incorrect addresses are used efi_loader: Show the resulting memory address from an alloc efi_loader: Update startimage_exit self-test to check error efi_loader: Move some memory-function comments to header doc: efi: Add the EFI-loader API documentation efi_loader: Use a separate struct for memory nodes efi_loader: Drop virtual_start from priv_mem_desc efi_loader: Drop reserved from priv_mem_desc efi_loader: Use the enum for the memory type in priv_mem_desc efi_loader: Avoid assigning desc in efi_mem_carve_out() efi_loader: Move struct efi_mem_list fields together efi_loader: Rename struct efi_mem_list to mem_node efi_loader: Rename physical_start to base efi_loader: Use correct type in efi_add_runtime_mmio() efi_loader: Show the address for pool allocations efi_loader: Don't try to add sandbox runtime code efi_loader: Update to use addresses internally efi_loader: Correct address-usage in copy_fdt() efi_loader: Drop comments about incorrect addresses efi_bootmgr: Avoid casts in try_load_from_uri_path() efi_loader: Simplify efi_dp_from_mem()
arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- arch/arm/mach-bcm283x/reset.c | 2 +- doc/api/efi.rst | 6 + include/efi.h | 23 +- include/efi_api.h | 10 + include/efi_loader.h | 87 +++++- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 10 +- lib/efi_loader/efi_boottime.c | 43 ++- lib/efi_loader/efi_device_path.c | 16 +- lib/efi_loader/efi_dt_fixup.c | 4 - lib/efi_loader/efi_helper.c | 4 +- lib/efi_loader/efi_image_loader.c | 3 +- lib/efi_loader/efi_memory.c | 261 +++++++----------- lib/efi_loader/efi_runtime.c | 7 +- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 18 files changed, 278 insertions(+), 225 deletions(-)
-- 2.34.1
Rgards, SImon
Hi Ilias,
On Tue, 17 Dec 2024 at 00:09, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Mon, 16 Dec 2024 at 17:14, Simon Glass sjg@chromium.org wrote:
Hi again Ilias,
On Fri, 13 Dec 2024 at 07:02, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Thu, 12 Dec 2024 at 08:51, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Thu, 12 Dec 2024 at 15:45, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 11 Dec 2024 at 23:40, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Wed, 11 Dec 2024 at 15:54, Simon Glass sjg@chromium.org wrote: > > The EFI-loader implementation converts things back and forth between > addresses and pointers, with not much consistency in how this is done. > > Within most of U-Boot a pointer is a void * and an address is a ulong > > This convention is very helpful, since it is obvious in common code as > to whether you need to call map_sysmem() and friends, or not. > > As part of cleaning up the EFI memory-management, I found it almost > impossible to know in some cases whether something is an address or a > pointer. I decided to give up on that and come back to it when this is > resolved. > > This series starts applying the normal ulong/void * convention to the > EFI_loader code, so making things easier to follow. For now, u64 is > often used instead of ulong, but the effect is the same. > > The main changes are: > - Rather than using the external struct efi_mem_desc data-structure for > internal bookkeeping, create a new one, so it can have different > semantics > - Within efi_memory.c addresses are used, rather than addresses > masquerading as pointers. The conversions are done in efi_boottime > > Unforunately my foray into attempting to use enum for the memory type > failed. The problem is not just that the external interface cannot use > an enum. In fact the enum in the spec is really just confusing, since > values not mentioned in the enum are valid. While we could handle this > by declaring a few more values in enum efi_memory_type, it doesn't seem > worth it. For example, if we declare 0x6fffffff and -1 as valid values, > we get the correct range, but then we need to be careful about > conversion in efi_boottime. If we declare 0xffffffff as valid, then the > enum ends up being 64-bits wide! Yes, that would work, I suppose it > wouldn't matter, but at that point I believe using an enum is actually > more confusing than not. It also made passing SCT tricky, since invalid > values are passed in... >
So my general feedback on this is
- Split it into a number of reviewable patches
- Send the typos & doc changes in one patchset that we can easily pick
up since it's mostly reviewed
- Drop the two patches that add 'sjg' stuff that are removed later
- Investigate why there's a reserved member in efi_mem_desc -- which I
think is a bug and send the fix as a single patch
- Drop the renames and duplication of efi_mem_desc with priv_mem_desc.
I don't see any point in duplication that, keeping the memory descriptors in a list as it currently works is fine
All the other points are OK, just more work. However, this one defeats the purpose of the series. As you can see, I am lining up the EFI internal-table more with how U-Boot and lmb do things, with addresses.
I think Heinrich and I have repeated this more than once.
As an aside, I notice that you say this every now and then but I don't think you realise how it comes across to the other person.
EFI works with physical addresses, just like sandbox needs to map_sysmem. It's way easier for us to review and look at code exactly how the spec describes it.
So it looks like this series is not going to fly, because you *want* the internal EFI tables to hold pointers-as-u64, rather than addresses. You don't want to do that translation at the EFI boundary. Is that right?
Just let me know what you want to do here. From what you said, this is a NAK, but if I have interpreted this incorrectly, and you are OK with the concept and just want some code changes, PLMK.
It's all summarized here https://lore.kernel.org/u-boot/CAFLszThNvbxZWOmLky0jpFPeUgFhAaFLVkMVVk9UCCP_...
OK, I understand that the address/pointer thing does not suit you.
I'll apply this to my tree for now.
Thanks /Ilias
Also the reason for duplicating that in the commit
message is
- the name is confusing, which reallys int
I am unsure what you are referring to here.
- The 'reserved' filed isn't needed, which is probably just a generic
bug in the EFI struct definition
I was surprised to see that I wrote that code. My guess is that it is there to ensure the alignment is correct. The first field is u32 and the third is u64 so perhaps I was concerned that the compiler would not naturally align the u64? It was over 9 years ago and I don't remember.
struct efi_mem_desc { u32 type; u32 reserved; efi_physical_addr_t physical_start; efi_virtual_addr_t virtual_start; u64 num_pages; u64 attribute; };
The spec says "Unless otherwise specified all data types are naturally aligned. Structures are aligned on boundaries equal to the largest internal datum of the structure and internal data are implicitly padded to achieve natural alignment."
I'm pretty sure any compiler that we use will handle this, so you are right, we don't need the field.
Regards, Simon
Thanks /Ilias
I cannot use both addresses and pointers in the same struct...I initially did that but it was just far too confusing.
- Send the map_to_sysmem() places you need in a different patchset
with an explanation of *why* this is working currently (or not working)
Hope this helps /Ilias
> Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/ > > Changes in v5: > - Drop the enum / mem_type patch as it is not needed > - Drop the patch to remove extra brackets in efi_mem_carve_out() > > Changes in v4: > - Add new patch to show the address for pool allocations > - Combine the various pointer-to-address patches into one > > Changes in v3: > - Adjust comments > - Show the returned address rather than the pointer > - Put the header-file in its own section > - Add comment to struct efi_device_path_memory > - Use a pointer for the values in struct efi_device_path_memory > > Changes in v2: > - Fix missing @ > - Note that this holds pointers, not addresses > - Add a new patch with comments where incorrect addresses are used > - Use EFI_PRINT() instead of log_debug() > - Rebase on early patch > - Add new patch to add the EFI-loader API documentation > - Add new patch to use a separate stuct for memory nodes > - Drop patch 'Convert efi_get_memory_map() to return pointers' > - Drop patch 'efi_loader: Make more use of ulong' > - Significantly expand and redirect the series > > Simon Glass (23): > efi: Define fields in struct efi_mem_desc > efi_loader: Fix typos in enum efi_allocate_type > efi_loader: Add comments where incorrect addresses are used > efi_loader: Show the resulting memory address from an alloc > efi_loader: Update startimage_exit self-test to check error > efi_loader: Move some memory-function comments to header > doc: efi: Add the EFI-loader API documentation > efi_loader: Use a separate struct for memory nodes > efi_loader: Drop virtual_start from priv_mem_desc > efi_loader: Drop reserved from priv_mem_desc > efi_loader: Use the enum for the memory type in priv_mem_desc > efi_loader: Avoid assigning desc in efi_mem_carve_out() > efi_loader: Move struct efi_mem_list fields together > efi_loader: Rename struct efi_mem_list to mem_node > efi_loader: Rename physical_start to base > efi_loader: Use correct type in efi_add_runtime_mmio() > efi_loader: Show the address for pool allocations > efi_loader: Don't try to add sandbox runtime code > efi_loader: Update to use addresses internally > efi_loader: Correct address-usage in copy_fdt() > efi_loader: Drop comments about incorrect addresses > efi_bootmgr: Avoid casts in try_load_from_uri_path() > efi_loader: Simplify efi_dp_from_mem() > > arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 2 +- > arch/arm/mach-bcm283x/reset.c | 2 +- > doc/api/efi.rst | 6 + > include/efi.h | 23 +- > include/efi_api.h | 10 + > include/efi_loader.h | 87 +++++- > lib/efi_loader/efi_bootbin.c | 3 +- > lib/efi_loader/efi_bootmgr.c | 10 +- > lib/efi_loader/efi_boottime.c | 43 ++- > lib/efi_loader/efi_device_path.c | 16 +- > lib/efi_loader/efi_dt_fixup.c | 4 - > lib/efi_loader/efi_helper.c | 4 +- > lib/efi_loader/efi_image_loader.c | 3 +- > lib/efi_loader/efi_memory.c | 261 +++++++----------- > lib/efi_loader/efi_runtime.c | 7 +- > lib/efi_loader/efi_var_mem.c | 6 +- > .../efi_selftest_startimage_exit.c | 6 +- > lib/lmb.c | 10 +- > 18 files changed, 278 insertions(+), 225 deletions(-) > > -- > 2.34.1 >
Regards, Simon
participants (3)
-
Ilias Apalodimas -
Mark Kettenis -
Simon Glass