[PATCH 00/13] 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.
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Simon Glass (13): efi: Define fields in struct efi_mem_desc efi_loader: Convert efi_get_memory_map() to return pointers 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 efi_loader: Use the enum for memory type efi_loader: Make more use of ulong efi_loader: Tidy up use of addresses lmb: Reduce mapmem contortions in lmb_map_update_notify() efi_loader: Simplify efi_dp_from_mem() efi_loader: Tidy up efi_reserve_memory() efi_loader: Drop extra brackets in efi_mem_carve_out() efi_loader: Don't try to add sandbox runtime code
include/efi.h | 15 ++ include/efi_api.h | 6 +- include/efi_loader.h | 100 +++++-- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 11 +- lib/efi_loader/efi_boottime.c | 51 ++-- lib/efi_loader/efi_device_path.c | 18 +- lib/efi_loader/efi_dt_fixup.c | 8 +- lib/efi_loader/efi_helper.c | 7 +- lib/efi_loader/efi_image_loader.c | 2 +- lib/efi_loader/efi_memory.c | 246 +++++++----------- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 14 files changed, 251 insertions(+), 238 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 addresses, not pointers.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/efi.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/include/efi.h b/include/efi.h index c559fda3004..6f48c6569d5 100644 --- a/include/efi.h +++ b/include/efi.h @@ -273,6 +273,21 @@ 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 + * + * type (enum efi_memory_type): EFI memory-type + * reserved: unused + * @physical_start: Start address of region in physical memory. Note that this + * is an address, not a pointer. Use map_sysmem(physical_start) to convert + * to a pointer + * @virtual_start: Start address of region in physical memory. Note that this + * is an address, not a pointer. Use map_sysmem(virtual_start) to convert + * to a pointer + * @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;
On Mon, 25 Nov 2024 at 22:45, Simon Glass sjg@chromium.org wrote:
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 addresses, not pointers.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/include/efi.h b/include/efi.h index c559fda3004..6f48c6569d5 100644 --- a/include/efi.h +++ b/include/efi.h @@ -273,6 +273,21 @@ 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
- type (enum efi_memory_type): EFI memory-type
- reserved: unused
@type, @reserved
- @physical_start: Start address of region in physical memory. Note that this
is an address, not a pointer. Use map_sysmem(physical_start) to convert
to a pointer
- @virtual_start: Start address of region in physical memory. Note that this
is an address, not a pointer. Use map_sysmem(virtual_start) to convert
to a pointer
- @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; -- 2.43.0
Thanks /Ilias
On 25.11.24 21:44, Simon Glass wrote:
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 addresses, not pointers.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/include/efi.h b/include/efi.h index c559fda3004..6f48c6569d5 100644 --- a/include/efi.h +++ b/include/efi.h @@ -273,6 +273,21 @@ 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
- type (enum efi_memory_type): EFI memory-type
- reserved: unused
- @physical_start: Start address of region in physical memory. Note that this
- is an address, not a pointer. Use map_sysmem(physical_start) to convert
- to a pointer
NAK.
EFI requires identity mapping. The value physical_start must be usable as a pointer (void *). It cannot be a sandbox virtual address (phys_addr_t).
Sandbox virtual addresses have no meaning in UEFI. Restrict them to the CLI.
- @virtual_start: Start address of region in physical memory. Note that this
- is an address, not a pointer. Use map_sysmem(virtual_start) to convert
- to a pointer
Wrong again.
Best regards
Heinrich
- @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;
This function should return pointers, not addresses. Add the conversion.
Signed-off-by: Simon Glass sjg@chromium.org ---
lib/efi_loader/efi_memory.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index e493934c713..8b01821a993 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -748,6 +748,11 @@ 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 = lmem->desc; + /* Convert to pointers, which is what the caller expects */ + memory_map->physical_start = + (ulong)map_sysmem(lmem->desc.physical_start, 0); + memory_map->virtual_start = + (ulong)map_sysmem(lmem->desc.virtual_start, 0); memory_map--; }
On 25.11.24 21:44, Simon Glass wrote:
This function should return pointers, not addresses. Add the conversion.
Signed-off-by: Simon Glass sjg@chromium.org
lib/efi_loader/efi_memory.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index e493934c713..8b01821a993 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -748,6 +748,11 @@ 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 = lmem->desc;
/* Convert to pointers, which is what the caller expects */memory_map->physical_start =(ulong)map_sysmem(lmem->desc.physical_start, 0);memory_map->virtual_start =(ulong)map_sysmem(lmem->desc.virtual_start, 0);
NAK.
physical_start and virtual_addr are not sandbox virtual addresses.
The sandbox virtual address space is irrelevant for UEFI. They should be restricted to the CLI.
Best regards
Heinrich
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:27, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass wrote:
This function should return pointers, not addresses. Add the conversion.
Signed-off-by: Simon Glass sjg@chromium.org
lib/efi_loader/efi_memory.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index e493934c713..8b01821a993 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -748,6 +748,11 @@ 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 = lmem->desc;
/* Convert to pointers, which is what the caller expects */memory_map->physical_start =(ulong)map_sysmem(lmem->desc.physical_start, 0);memory_map->virtual_start =(ulong)map_sysmem(lmem->desc.virtual_start, 0); memory_map--; }NAK.
physical_start and virtual_addr are not sandbox virtual addresses.
The sandbox virtual address space is irrelevant for UEFI. They should be restricted to the CLI.
Well, you can't have it both ways. Either this is a pointer expressed as a u64, or it is an address.
My understanding is that it is a pointer expressed as a u64. The use of u64 is a confusing part of the spec, but it is because they want to specify the size as 64-bit, whereas a void * would not have a fixed size. But you can't run a 32-bit image on a 64-bit machine anyway, so this decision is a mystery to me.
Regards, Simon
Hi again Heinrich,
On Tue, 26 Nov 2024 at 08:38, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:27, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass wrote:
This function should return pointers, not addresses. Add the conversion.
Signed-off-by: Simon Glass sjg@chromium.org
lib/efi_loader/efi_memory.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index e493934c713..8b01821a993 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -748,6 +748,11 @@ 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 = lmem->desc;
/* Convert to pointers, which is what the caller expects */memory_map->physical_start =(ulong)map_sysmem(lmem->desc.physical_start, 0);memory_map->virtual_start =(ulong)map_sysmem(lmem->desc.virtual_start, 0); memory_map--; }NAK.
physical_start and virtual_addr are not sandbox virtual addresses.
The sandbox virtual address space is irrelevant for UEFI. They should be restricted to the CLI.
Well, you can't have it both ways. Either this is a pointer expressed as a u64, or it is an address.
My understanding is that it is a pointer expressed as a u64. The use of u64 is a confusing part of the spec, but it is because they want to specify the size as 64-bit, whereas a void * would not have a fixed size. But you can't run a 32-bit image on a 64-bit machine anyway, so this decision is a mystery to me.
Anyway, I have this around the wrong way, so can drop this patch once I update patch 1 to document what is actually going on...
Regards, Simon
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 ---
lib/efi_loader/efi_boottime.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae3..90437e3e401 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -511,6 +511,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) + log_debug("*buffer = %p\n", *buffer); + return EFI_EXIT(r); }
On 25.11.24 21:44, Simon Glass wrote:
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
lib/efi_loader/efi_boottime.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 080e7f78ae3..90437e3e401 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -511,6 +511,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)
log_debug("*buffer = %p\n", *buffer);
Please, use EFI_PRINT() which will provide indentation.
Best regards
Heinrich
- return EFI_EXIT(r); }
Check for an error returned from the decompress() function, just in case.
Signed-off-by: Simon Glass sjg@chromium.org ---
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; }
/*
On Mon, 25 Nov 2024 at 22:45, Simon Glass sjg@chromium.org wrote:
Check for an error returned from the decompress() function, just in case.
Signed-off-by: Simon Glass sjg@chromium.org
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;}
/*
2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
On 26.11.24 07:29, Ilias Apalodimas wrote:
On Mon, 25 Nov 2024 at 22:45, Simon Glass sjg@chromium.org wrote:
Check for an error returned from the decompress() function, just in case.
Signed-off-by: Simon Glass sjg@chromium.org
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;}
/*
-- 2.43.0
Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
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 ---
include/efi_loader.h | 70 ++++++++++++++++++++++++++++++++----- lib/efi_loader/efi_memory.c | 62 -------------------------------- 2 files changed, 62 insertions(+), 70 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 39809eac1bc..ff7adc1e2a2 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,14 @@ 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 a range into the EFI memory map + * @start: start address, must be a multiple of EFI_PAGE_SIZE + * @size: Size, in bytes + * @memory_type: EFI type of memory added + * Return: status code + */ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
/** @@ -791,7 +845,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); * @start: start address, must be a multiple of * EFI_PAGE_SIZE * @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 8b01821a993..7c8d9485094 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,18 +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 - * @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; @@ -438,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) @@ -554,14 +522,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); @@ -606,14 +566,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; @@ -642,14 +594,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; @@ -664,12 +608,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;
On 25.11.24 21:44, Simon Glass 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
Please, have a look at this line in doc/
doc/api/efi.rst:78: .. kernel-doc:: lib/efi_loader/efi_memory.c
Should we use a separate header efi_memory.h?
Best regards
Heinrich
include/efi_loader.h | 70 ++++++++++++++++++++++++++++++++----- lib/efi_loader/efi_memory.c | 62 -------------------------------- 2 files changed, 62 insertions(+), 70 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 39809eac1bc..ff7adc1e2a2 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,14 @@ 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 a range into the EFI memory map
- @start: start address, must be a multiple of EFI_PAGE_SIZE
- @size: Size, in bytes
- @memory_type: EFI type of memory added
- Return: status code
*/ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
/**
@@ -791,7 +845,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
- @start: start address, must be a multiple of
EFI_PAGE_SIZE- @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 8b01821a993..7c8d9485094 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,18 +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
- @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;
@@ -438,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)
@@ -554,14 +522,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);
@@ -606,14 +566,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;
@@ -642,14 +594,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;
@@ -664,12 +608,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;
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
Please, have a look at this line in doc/
doc/api/efi.rst:78: .. kernel-doc:: lib/efi_loader/efi_memory.c
Hmm, we should not add C files as then we end up with all sorts of internal functions, like checksum(). The help is a bit of a mess on that page IMO and it could use an index at the top or side.
Should we use a separate header efi_memory.h?
Yes.
Best regards
Heinrich
include/efi_loader.h | 70 ++++++++++++++++++++++++++++++++----- lib/efi_loader/efi_memory.c | 62 -------------------------------- 2 files changed, 62 insertions(+), 70 deletions(-)
Regards, Simon
On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
Please, have a look at this line in doc/
doc/api/efi.rst:78: .. kernel-doc:: lib/efi_loader/efi_memory.c
Hmm, we should not add C files as then we end up with all sorts of internal functions, like checksum(). The help is a bit of a mess on that page IMO and it could use an index at the top or side.
Why? Looking at the linux kernel (where we borrow this framework from and lots of developers expect to follow that style) it's extremely common to kernel-doc C files. I don't see why documenting internal functions (which should be clear as being internal) is a problem.
Hi Tom,
On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
Please, have a look at this line in doc/
doc/api/efi.rst:78: .. kernel-doc:: lib/efi_loader/efi_memory.c
Hmm, we should not add C files as then we end up with all sorts of internal functions, like checksum(). The help is a bit of a mess on that page IMO and it could use an index at the top or side.
Why? Looking at the linux kernel (where we borrow this framework from and lots of developers expect to follow that style) it's extremely common to kernel-doc C files. I don't see why documenting internal functions (which should be clear as being internal) is a problem.
It depends what you mean by docs. U-Boot puts the documentation for exported functions in header files.
Putting docs in C files clutters the docs with internal implementations, when most people just want to see the API. The way U-Boot does things, it is easy to see the internal (implementation) docs by looking at the C code and the external functions by looking at the header files.
Regards, Simon
On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
Please, have a look at this line in doc/
doc/api/efi.rst:78: .. kernel-doc:: lib/efi_loader/efi_memory.c
Hmm, we should not add C files as then we end up with all sorts of internal functions, like checksum(). The help is a bit of a mess on that page IMO and it could use an index at the top or side.
Why? Looking at the linux kernel (where we borrow this framework from and lots of developers expect to follow that style) it's extremely common to kernel-doc C files. I don't see why documenting internal functions (which should be clear as being internal) is a problem.
It depends what you mean by docs. U-Boot puts the documentation for exported functions in header files.
I'm sorry, that's a circular definition. And no, there is not a hard and fast rule that we only put kernel-doc style comments in header files (which are consumed by other files and tooling to generate documentation).
Putting docs in C files clutters the docs with internal implementations, when most people just want to see the API. The way U-Boot does things, it is easy to see the internal (implementation) docs by looking at the C code and the external functions by looking at the header files.
Or it makes things harder to understand because the human readable documentation for a given function is separate from the file the implements the function. This is possibly why in the linux kernel, where we borrow "kernel-doc" from, documentation is frequently in the C file which is then included in Documentation files (the type you objected to in the start of this patch) which include additional human-readable explanation of the intention and usage of the API in question.
On 11/26/24 23:33, Tom Rini wrote:
On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
Please, have a look at this line in doc/
doc/api/efi.rst:78: .. kernel-doc:: lib/efi_loader/efi_memory.c
Hmm, we should not add C files as then we end up with all sorts of internal functions, like checksum(). The help is a bit of a mess on that page IMO and it could use an index at the top or side.
Why? Looking at the linux kernel (where we borrow this framework from and lots of developers expect to follow that style) it's extremely common to kernel-doc C files. I don't see why documenting internal functions (which should be clear as being internal) is a problem.
It depends what you mean by docs. U-Boot puts the documentation for exported functions in header files.
I'm sorry, that's a circular definition. And no, there is not a hard and fast rule that we only put kernel-doc style comments in header files (which are consumed by other files and tooling to generate documentation).
Putting docs in C files clutters the docs with internal implementations, when most people just want to see the API. The way U-Boot does things, it is easy to see the internal (implementation) docs by looking at the C code and the external functions by looking at the header files.
Or it makes things harder to understand because the human readable documentation for a given function is separate from the file the implements the function. This is possibly why in the linux kernel, where we borrow "kernel-doc" from, documentation is frequently in the C file which is then included in Documentation files (the type you objected to in the start of this patch) which include additional human-readable explanation of the intention and usage of the API in question.
For static functions we will continue to have function documentation in the C files and not in the headers. I personally never looked up the API documentation in the HTML docs but always in the source.
I am not opposed against moving the EFI function documentation into the header files. But we should update doc/develop/efi.rst accordingly in the same patch series. This is missing here.
Best regards
Heinrich
On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote:
On 11/26/24 23:33, Tom Rini wrote:
On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
Please, have a look at this line in doc/
doc/api/efi.rst:78: .. kernel-doc:: lib/efi_loader/efi_memory.c
Hmm, we should not add C files as then we end up with all sorts of internal functions, like checksum(). The help is a bit of a mess on that page IMO and it could use an index at the top or side.
Why? Looking at the linux kernel (where we borrow this framework from and lots of developers expect to follow that style) it's extremely common to kernel-doc C files. I don't see why documenting internal functions (which should be clear as being internal) is a problem.
It depends what you mean by docs. U-Boot puts the documentation for exported functions in header files.
I'm sorry, that's a circular definition. And no, there is not a hard and fast rule that we only put kernel-doc style comments in header files (which are consumed by other files and tooling to generate documentation).
Putting docs in C files clutters the docs with internal implementations, when most people just want to see the API. The way U-Boot does things, it is easy to see the internal (implementation) docs by looking at the C code and the external functions by looking at the header files.
Or it makes things harder to understand because the human readable documentation for a given function is separate from the file the implements the function. This is possibly why in the linux kernel, where we borrow "kernel-doc" from, documentation is frequently in the C file which is then included in Documentation files (the type you objected to in the start of this patch) which include additional human-readable explanation of the intention and usage of the API in question.
For static functions we will continue to have function documentation in the C files and not in the headers. I personally never looked up the API documentation in the HTML docs but always in the source.
I am not opposed against moving the EFI function documentation into the header files. But we should update doc/develop/efi.rst accordingly in the same patch series. This is missing here.
My point is that the linux kernel is about 60% C files and 40% header files being kernel-doc'd in to documentation files and so "U-Boot doesn't put documentation in to C files" seems like the wrong approach.
Hi,
On Tue, 26 Nov 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote:
On 11/26/24 23:33, Tom Rini wrote:
On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > On 25.11.24 21:44, Simon Glass 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 > > Please, have a look at this line in doc/ > > doc/api/efi.rst:78: > .. kernel-doc:: lib/efi_loader/efi_memory.c
Hmm, we should not add C files as then we end up with all sorts of internal functions, like checksum(). The help is a bit of a mess on that page IMO and it could use an index at the top or side.
Why? Looking at the linux kernel (where we borrow this framework from and lots of developers expect to follow that style) it's extremely common to kernel-doc C files. I don't see why documenting internal functions (which should be clear as being internal) is a problem.
It depends what you mean by docs. U-Boot puts the documentation for exported functions in header files.
I'm sorry, that's a circular definition. And no, there is not a hard and fast rule that we only put kernel-doc style comments in header files (which are consumed by other files and tooling to generate documentation).
Putting docs in C files clutters the docs with internal implementations, when most people just want to see the API. The way U-Boot does things, it is easy to see the internal (implementation) docs by looking at the C code and the external functions by looking at the header files.
Or it makes things harder to understand because the human readable documentation for a given function is separate from the file the implements the function. This is possibly why in the linux kernel, where we borrow "kernel-doc" from, documentation is frequently in the C file which is then included in Documentation files (the type you objected to in the start of this patch) which include additional human-readable explanation of the intention and usage of the API in question.
For static functions we will continue to have function documentation in the C files and not in the headers. I personally never looked up the API documentation in the HTML docs but always in the source.
Same here.
I am not opposed against moving the EFI function documentation into the header files. But we should update doc/develop/efi.rst accordingly in the same patch series. This is missing here.
OK
My point is that the linux kernel is about 60% C files and 40% header files being kernel-doc'd in to documentation files and so "U-Boot doesn't put documentation in to C files" seems like the wrong approach.
U-Boot tries to document functions in header files. At the risk of making a blanket statement with thousands of counter-examples, that is not so much the case in Linux.
To a large extent, people developing features in U-Boot rely on the header files to see what functions they can call. With the documentation there, they often don't need to look at the source. I certainly don't, unless the header-file comment is inadequate.
Regards, SImon
On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote:
Hi,
On Tue, 26 Nov 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote:
On 11/26/24 23:33, Tom Rini wrote:
On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote: > Hi Heinrich, > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > On 25.11.24 21:44, Simon Glass 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 > > > > Please, have a look at this line in doc/ > > > > doc/api/efi.rst:78: > > .. kernel-doc:: lib/efi_loader/efi_memory.c > > Hmm, we should not add C files as then we end up with all sorts of > internal functions, like checksum(). The help is a bit of a mess on > that page IMO and it could use an index at the top or side.
Why? Looking at the linux kernel (where we borrow this framework from and lots of developers expect to follow that style) it's extremely common to kernel-doc C files. I don't see why documenting internal functions (which should be clear as being internal) is a problem.
It depends what you mean by docs. U-Boot puts the documentation for exported functions in header files.
I'm sorry, that's a circular definition. And no, there is not a hard and fast rule that we only put kernel-doc style comments in header files (which are consumed by other files and tooling to generate documentation).
Putting docs in C files clutters the docs with internal implementations, when most people just want to see the API. The way U-Boot does things, it is easy to see the internal (implementation) docs by looking at the C code and the external functions by looking at the header files.
Or it makes things harder to understand because the human readable documentation for a given function is separate from the file the implements the function. This is possibly why in the linux kernel, where we borrow "kernel-doc" from, documentation is frequently in the C file which is then included in Documentation files (the type you objected to in the start of this patch) which include additional human-readable explanation of the intention and usage of the API in question.
For static functions we will continue to have function documentation in the C files and not in the headers. I personally never looked up the API documentation in the HTML docs but always in the source.
Same here.
I am not opposed against moving the EFI function documentation into the header files. But we should update doc/develop/efi.rst accordingly in the same patch series. This is missing here.
OK
My point is that the linux kernel is about 60% C files and 40% header files being kernel-doc'd in to documentation files and so "U-Boot doesn't put documentation in to C files" seems like the wrong approach.
U-Boot tries to document functions in header files. At the risk of making a blanket statement with thousands of counter-examples, that is not so much the case in Linux.
To a large extent, people developing features in U-Boot rely on the header files to see what functions they can call. With the documentation there, they often don't need to look at the source. I certainly don't, unless the header-file comment is inadequate.
I think this is a case of what you do, rather than what everyone else does. We want to be more like the linux kernel here, rather than less like the linux kernel, given the number of kernel people also working here. If Heinrich is fine with re-arranging everything, OK. But I think documentation with the code makes it less likely to get out of date, both in terms of intentional changes and bug fixes.
Hi Tom,
On Wed, 27 Nov 2024 at 07:15, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote:
Hi,
On Tue, 26 Nov 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote:
On 11/26/24 23:33, Tom Rini wrote:
On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote: > > On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote: > > Hi Heinrich, > > > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > On 25.11.24 21:44, Simon Glass 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 > > > > > > Please, have a look at this line in doc/ > > > > > > doc/api/efi.rst:78: > > > .. kernel-doc:: lib/efi_loader/efi_memory.c > > > > Hmm, we should not add C files as then we end up with all sorts of > > internal functions, like checksum(). The help is a bit of a mess on > > that page IMO and it could use an index at the top or side. > > Why? Looking at the linux kernel (where we borrow this framework from > and lots of developers expect to follow that style) it's extremely > common to kernel-doc C files. I don't see why documenting internal > functions (which should be clear as being internal) is a problem.
It depends what you mean by docs. U-Boot puts the documentation for exported functions in header files.
I'm sorry, that's a circular definition. And no, there is not a hard and fast rule that we only put kernel-doc style comments in header files (which are consumed by other files and tooling to generate documentation).
Putting docs in C files clutters the docs with internal implementations, when most people just want to see the API. The way U-Boot does things, it is easy to see the internal (implementation) docs by looking at the C code and the external functions by looking at the header files.
Or it makes things harder to understand because the human readable documentation for a given function is separate from the file the implements the function. This is possibly why in the linux kernel, where we borrow "kernel-doc" from, documentation is frequently in the C file which is then included in Documentation files (the type you objected to in the start of this patch) which include additional human-readable explanation of the intention and usage of the API in question.
For static functions we will continue to have function documentation in the C files and not in the headers. I personally never looked up the API documentation in the HTML docs but always in the source.
Same here.
I am not opposed against moving the EFI function documentation into the header files. But we should update doc/develop/efi.rst accordingly in the same patch series. This is missing here.
OK
My point is that the linux kernel is about 60% C files and 40% header files being kernel-doc'd in to documentation files and so "U-Boot doesn't put documentation in to C files" seems like the wrong approach.
U-Boot tries to document functions in header files. At the risk of making a blanket statement with thousands of counter-examples, that is not so much the case in Linux.
To a large extent, people developing features in U-Boot rely on the header files to see what functions they can call. With the documentation there, they often don't need to look at the source. I certainly don't, unless the header-file comment is inadequate.
I think this is a case of what you do, rather than what everyone else does.
That's not my reading of the source tree.
The file we are talking about has some comments in the header and some in the source file. When you use an lsp client, it doesn't know what to do.
We want to be more like the linux kernel here, rather than less like the linux kernel, given the number of kernel people also working here. If Heinrich is fine with re-arranging everything, OK. But I think documentation with the code makes it less likely to get out of date, both in terms of intentional changes and bug fixes.
I am not suggesting separating documentation from code. I agree that makes no sense. What I have consistently done (and asked for in reviews) is:
- exported functions are documented next to their prototype in the header - static functions are documented next to their implementation
This makes it easy to read the header file and see what the module is doing, without having to dive into the implementation, which could be 1000 lines or more. It also makes for more useful kerneldoc, IMO, since we are showing functions which are exported and therefore more useful for development. Adding the entire C code of U-Boot into kerneldoc seems like a bad use of space.
Regards, Simon
On Wed, Nov 27, 2024 at 07:56:11AM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 07:15, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote:
Hi,
On Tue, 26 Nov 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote:
On 11/26/24 23:33, Tom Rini wrote:
On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote: > Hi Tom, > > On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote: > > > > On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote: > > > Hi Heinrich, > > > > > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > On 25.11.24 21:44, Simon Glass 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 > > > > > > > > Please, have a look at this line in doc/ > > > > > > > > doc/api/efi.rst:78: > > > > .. kernel-doc:: lib/efi_loader/efi_memory.c > > > > > > Hmm, we should not add C files as then we end up with all sorts of > > > internal functions, like checksum(). The help is a bit of a mess on > > > that page IMO and it could use an index at the top or side. > > > > Why? Looking at the linux kernel (where we borrow this framework from > > and lots of developers expect to follow that style) it's extremely > > common to kernel-doc C files. I don't see why documenting internal > > functions (which should be clear as being internal) is a problem. > > It depends what you mean by docs. U-Boot puts the documentation for > exported functions in header files.
I'm sorry, that's a circular definition. And no, there is not a hard and fast rule that we only put kernel-doc style comments in header files (which are consumed by other files and tooling to generate documentation).
> Putting docs in C files clutters the docs with internal > implementations, when most people just want to see the API. The way > U-Boot does things, it is easy to see the internal (implementation) > docs by looking at the C code and the external functions by looking at > the header files.
Or it makes things harder to understand because the human readable documentation for a given function is separate from the file the implements the function. This is possibly why in the linux kernel, where we borrow "kernel-doc" from, documentation is frequently in the C file which is then included in Documentation files (the type you objected to in the start of this patch) which include additional human-readable explanation of the intention and usage of the API in question.
For static functions we will continue to have function documentation in the C files and not in the headers. I personally never looked up the API documentation in the HTML docs but always in the source.
Same here.
I am not opposed against moving the EFI function documentation into the header files. But we should update doc/develop/efi.rst accordingly in the same patch series. This is missing here.
OK
My point is that the linux kernel is about 60% C files and 40% header files being kernel-doc'd in to documentation files and so "U-Boot doesn't put documentation in to C files" seems like the wrong approach.
U-Boot tries to document functions in header files. At the risk of making a blanket statement with thousands of counter-examples, that is not so much the case in Linux.
To a large extent, people developing features in U-Boot rely on the header files to see what functions they can call. With the documentation there, they often don't need to look at the source. I certainly don't, unless the header-file comment is inadequate.
I think this is a case of what you do, rather than what everyone else does.
That's not my reading of the source tree.
So, if we call this point 1 for a moment.
The file we are talking about has some comments in the header and some in the source file. When you use an lsp client, it doesn't know what to do.
Sounds like a bad client? Because again, we should be following the kernel here.
We want to be more like the linux kernel here, rather than less like the linux kernel, given the number of kernel people also working here. If Heinrich is fine with re-arranging everything, OK. But I think documentation with the code makes it less likely to get out of date, both in terms of intentional changes and bug fixes.
I am not suggesting separating documentation from code. I agree that makes no sense. What I have consistently done (and asked for in reviews) is:
- exported functions are documented next to their prototype in the header
- static functions are documented next to their implementation
So going back to point 1 now, yes. This is what I'm saying. Your preference, expressed through reviews and telling people to change when they do it other ways, is where the current situation comes from.
This makes it easy to read the header file and see what the module is doing, without having to dive into the implementation, which could be 1000 lines or more. It also makes for more useful kerneldoc, IMO, since we are showing functions which are exported and therefore more useful for development. Adding the entire C code of U-Boot into kerneldoc seems like a bad use of space.
Um, I don't follow you here. If the code doesn't have kerneldoc comments, it's not included. I just made a silly test with a change to include/clk.h. It's not included in the output. And "the code might be so big it needs refactoring" is not a good excuse.
The kernel is inconsistent here. The first example I went to look for of something that uses both C and header files in a document was Documentation/admin-guide/pstore-blk.rst and both fs/pstore/blk.c and include/linux/pstore_blk.h are kernel-doc'd in and the header describes a public function of the C file while the C file describes other functions too.
Hi Tom,
On Wed, 27 Nov 2024 at 15:11, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 07:56:11AM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 07:15, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote:
Hi,
On Tue, 26 Nov 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote:
On 11/26/24 23:33, Tom Rini wrote: > On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote: > > > > > > On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote: > > > > Hi Heinrich, > > > > > > > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > > > On 25.11.24 21:44, Simon Glass 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 > > > > > > > > > > Please, have a look at this line in doc/ > > > > > > > > > > doc/api/efi.rst:78: > > > > > .. kernel-doc:: lib/efi_loader/efi_memory.c > > > > > > > > Hmm, we should not add C files as then we end up with all sorts of > > > > internal functions, like checksum(). The help is a bit of a mess on > > > > that page IMO and it could use an index at the top or side. > > > > > > Why? Looking at the linux kernel (where we borrow this framework from > > > and lots of developers expect to follow that style) it's extremely > > > common to kernel-doc C files. I don't see why documenting internal > > > functions (which should be clear as being internal) is a problem. > > > > It depends what you mean by docs. U-Boot puts the documentation for > > exported functions in header files. > > I'm sorry, that's a circular definition. And no, there is not a hard and > fast rule that we only put kernel-doc style comments in header files > (which are consumed by other files and tooling to generate > documentation). > > > Putting docs in C files clutters the docs with internal > > implementations, when most people just want to see the API. The way > > U-Boot does things, it is easy to see the internal (implementation) > > docs by looking at the C code and the external functions by looking at > > the header files. > > Or it makes things harder to understand because the human readable > documentation for a given function is separate from the file the > implements the function. This is possibly why in the linux kernel, where > we borrow "kernel-doc" from, documentation is frequently in the C file > which is then included in Documentation files (the type you objected to > in the start of this patch) which include additional human-readable > explanation of the intention and usage of the API in question. >
For static functions we will continue to have function documentation in the C files and not in the headers. I personally never looked up the API documentation in the HTML docs but always in the source.
Same here.
I am not opposed against moving the EFI function documentation into the header files. But we should update doc/develop/efi.rst accordingly in the same patch series. This is missing here.
OK
My point is that the linux kernel is about 60% C files and 40% header files being kernel-doc'd in to documentation files and so "U-Boot doesn't put documentation in to C files" seems like the wrong approach.
U-Boot tries to document functions in header files. At the risk of making a blanket statement with thousands of counter-examples, that is not so much the case in Linux.
To a large extent, people developing features in U-Boot rely on the header files to see what functions they can call. With the documentation there, they often don't need to look at the source. I certainly don't, unless the header-file comment is inadequate.
I think this is a case of what you do, rather than what everyone else does.
That's not my reading of the source tree.
So, if we call this point 1 for a moment.
The file we are talking about has some comments in the header and some in the source file. When you use an lsp client, it doesn't know what to do.
Sounds like a bad client? Because again, we should be following the kernel here.
We want to be more like the linux kernel here, rather than less like the linux kernel, given the number of kernel people also working here. If Heinrich is fine with re-arranging everything, OK. But I think documentation with the code makes it less likely to get out of date, both in terms of intentional changes and bug fixes.
I am not suggesting separating documentation from code. I agree that makes no sense. What I have consistently done (and asked for in reviews) is:
- exported functions are documented next to their prototype in the header
- static functions are documented next to their implementation
So going back to point 1 now, yes. This is what I'm saying. Your preference, expressed through reviews and telling people to change when they do it other ways, is where the current situation comes from.
This makes it easy to read the header file and see what the module is doing, without having to dive into the implementation, which could be 1000 lines or more. It also makes for more useful kerneldoc, IMO, since we are showing functions which are exported and therefore more useful for development. Adding the entire C code of U-Boot into kerneldoc seems like a bad use of space.
Um, I don't follow you here. If the code doesn't have kerneldoc comments, it's not included. I just made a silly test with a change to include/clk.h. It's not included in the output. And "the code might be so big it needs refactoring" is not a good excuse.
Yes, I see that static functions don't appear by default, so adding C files doesn't necessarily result in functions appearing in the doc output.
The kernel is inconsistent here. The first example I went to look for of something that uses both C and header files in a document was Documentation/admin-guide/pstore-blk.rst and both fs/pstore/blk.c and include/linux/pstore_blk.h are kernel-doc'd in and the header describes a public function of the C file while the C file describes other functions too.
Yes, pstore docs seems like a nice way to do things, for docs.
For people working on the code base, I strongly believe that exported functions and structs are the main way to understand how to use a subsystem, so their docs should be in the header file.
Regards, Simon
On Sat, Nov 30, 2024 at 01:24:54PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 15:11, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 07:56:11AM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 07:15, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote:
Hi,
On Tue, 26 Nov 2024 at 16:32, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote: > On 11/26/24 23:33, Tom Rini wrote: > > On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote: > > > Hi Tom, > > > > > > On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote: > > > > > > > > On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote: > > > > > Hi Heinrich, > > > > > > > > > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > > > > > On 25.11.24 21:44, Simon Glass 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 > > > > > > > > > > > > Please, have a look at this line in doc/ > > > > > > > > > > > > doc/api/efi.rst:78: > > > > > > .. kernel-doc:: lib/efi_loader/efi_memory.c > > > > > > > > > > Hmm, we should not add C files as then we end up with all sorts of > > > > > internal functions, like checksum(). The help is a bit of a mess on > > > > > that page IMO and it could use an index at the top or side. > > > > > > > > Why? Looking at the linux kernel (where we borrow this framework from > > > > and lots of developers expect to follow that style) it's extremely > > > > common to kernel-doc C files. I don't see why documenting internal > > > > functions (which should be clear as being internal) is a problem. > > > > > > It depends what you mean by docs. U-Boot puts the documentation for > > > exported functions in header files. > > > > I'm sorry, that's a circular definition. And no, there is not a hard and > > fast rule that we only put kernel-doc style comments in header files > > (which are consumed by other files and tooling to generate > > documentation). > > > > > Putting docs in C files clutters the docs with internal > > > implementations, when most people just want to see the API. The way > > > U-Boot does things, it is easy to see the internal (implementation) > > > docs by looking at the C code and the external functions by looking at > > > the header files. > > > > Or it makes things harder to understand because the human readable > > documentation for a given function is separate from the file the > > implements the function. This is possibly why in the linux kernel, where > > we borrow "kernel-doc" from, documentation is frequently in the C file > > which is then included in Documentation files (the type you objected to > > in the start of this patch) which include additional human-readable > > explanation of the intention and usage of the API in question. > > > > For static functions we will continue to have function documentation in > the C files and not in the headers. I personally never looked up the API > documentation in the HTML docs but always in the source.
Same here.
> > I am not opposed against moving the EFI function documentation into the > header files. But we should update doc/develop/efi.rst accordingly in > the same patch series. This is missing here.
OK
My point is that the linux kernel is about 60% C files and 40% header files being kernel-doc'd in to documentation files and so "U-Boot doesn't put documentation in to C files" seems like the wrong approach.
U-Boot tries to document functions in header files. At the risk of making a blanket statement with thousands of counter-examples, that is not so much the case in Linux.
To a large extent, people developing features in U-Boot rely on the header files to see what functions they can call. With the documentation there, they often don't need to look at the source. I certainly don't, unless the header-file comment is inadequate.
I think this is a case of what you do, rather than what everyone else does.
That's not my reading of the source tree.
So, if we call this point 1 for a moment.
The file we are talking about has some comments in the header and some in the source file. When you use an lsp client, it doesn't know what to do.
Sounds like a bad client? Because again, we should be following the kernel here.
We want to be more like the linux kernel here, rather than less like the linux kernel, given the number of kernel people also working here. If Heinrich is fine with re-arranging everything, OK. But I think documentation with the code makes it less likely to get out of date, both in terms of intentional changes and bug fixes.
I am not suggesting separating documentation from code. I agree that makes no sense. What I have consistently done (and asked for in reviews) is:
- exported functions are documented next to their prototype in the header
- static functions are documented next to their implementation
So going back to point 1 now, yes. This is what I'm saying. Your preference, expressed through reviews and telling people to change when they do it other ways, is where the current situation comes from.
This makes it easy to read the header file and see what the module is doing, without having to dive into the implementation, which could be 1000 lines or more. It also makes for more useful kerneldoc, IMO, since we are showing functions which are exported and therefore more useful for development. Adding the entire C code of U-Boot into kerneldoc seems like a bad use of space.
Um, I don't follow you here. If the code doesn't have kerneldoc comments, it's not included. I just made a silly test with a change to include/clk.h. It's not included in the output. And "the code might be so big it needs refactoring" is not a good excuse.
Yes, I see that static functions don't appear by default, so adding C files doesn't necessarily result in functions appearing in the doc output.
Only properly formatted comments are used in documentation, yes. So static functions could be, if it was important to do so.
The kernel is inconsistent here. The first example I went to look for of something that uses both C and header files in a document was Documentation/admin-guide/pstore-blk.rst and both fs/pstore/blk.c and include/linux/pstore_blk.h are kernel-doc'd in and the header describes a public function of the C file while the C file describes other functions too.
Yes, pstore docs seems like a nice way to do things, for docs.
For people working on the code base, I strongly believe that exported functions and structs are the main way to understand how to use a subsystem, so their docs should be in the header file.
Sure. But there is no project-wide rule. I strongly believe that the API should be documented alongside of the code so that it doesn't drift apart, among other reasons.
Hi Tom,
On Mon, 2 Dec 2024 at 06:35, Tom Rini trini@konsulko.com wrote:
On Sat, Nov 30, 2024 at 01:24:54PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 15:11, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 07:56:11AM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 07:15, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote:
Hi,
On Tue, 26 Nov 2024 at 16:32, Tom Rini trini@konsulko.com wrote: > > On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote: > > On 11/26/24 23:33, Tom Rini wrote: > > > On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote: > > > > > > > > > > On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote: > > > > > > Hi Heinrich, > > > > > > > > > > > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > > > > > > > On 25.11.24 21:44, Simon Glass 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 > > > > > > > > > > > > > > Please, have a look at this line in doc/ > > > > > > > > > > > > > > doc/api/efi.rst:78: > > > > > > > .. kernel-doc:: lib/efi_loader/efi_memory.c > > > > > > > > > > > > Hmm, we should not add C files as then we end up with all sorts of > > > > > > internal functions, like checksum(). The help is a bit of a mess on > > > > > > that page IMO and it could use an index at the top or side. > > > > > > > > > > Why? Looking at the linux kernel (where we borrow this framework from > > > > > and lots of developers expect to follow that style) it's extremely > > > > > common to kernel-doc C files. I don't see why documenting internal > > > > > functions (which should be clear as being internal) is a problem. > > > > > > > > It depends what you mean by docs. U-Boot puts the documentation for > > > > exported functions in header files. > > > > > > I'm sorry, that's a circular definition. And no, there is not a hard and > > > fast rule that we only put kernel-doc style comments in header files > > > (which are consumed by other files and tooling to generate > > > documentation). > > > > > > > Putting docs in C files clutters the docs with internal > > > > implementations, when most people just want to see the API. The way > > > > U-Boot does things, it is easy to see the internal (implementation) > > > > docs by looking at the C code and the external functions by looking at > > > > the header files. > > > > > > Or it makes things harder to understand because the human readable > > > documentation for a given function is separate from the file the > > > implements the function. This is possibly why in the linux kernel, where > > > we borrow "kernel-doc" from, documentation is frequently in the C file > > > which is then included in Documentation files (the type you objected to > > > in the start of this patch) which include additional human-readable > > > explanation of the intention and usage of the API in question. > > > > > > > For static functions we will continue to have function documentation in > > the C files and not in the headers. I personally never looked up the API > > documentation in the HTML docs but always in the source.
Same here.
> > > > I am not opposed against moving the EFI function documentation into the > > header files. But we should update doc/develop/efi.rst accordingly in > > the same patch series. This is missing here.
OK
> > My point is that the linux kernel is about 60% C files and 40% header > files being kernel-doc'd in to documentation files and so "U-Boot > doesn't put documentation in to C files" seems like the wrong approach.
U-Boot tries to document functions in header files. At the risk of making a blanket statement with thousands of counter-examples, that is not so much the case in Linux.
To a large extent, people developing features in U-Boot rely on the header files to see what functions they can call. With the documentation there, they often don't need to look at the source. I certainly don't, unless the header-file comment is inadequate.
I think this is a case of what you do, rather than what everyone else does.
That's not my reading of the source tree.
So, if we call this point 1 for a moment.
The file we are talking about has some comments in the header and some in the source file. When you use an lsp client, it doesn't know what to do.
Sounds like a bad client? Because again, we should be following the kernel here.
We want to be more like the linux kernel here, rather than less like the linux kernel, given the number of kernel people also working here. If Heinrich is fine with re-arranging everything, OK. But I think documentation with the code makes it less likely to get out of date, both in terms of intentional changes and bug fixes.
I am not suggesting separating documentation from code. I agree that makes no sense. What I have consistently done (and asked for in reviews) is:
- exported functions are documented next to their prototype in the header
- static functions are documented next to their implementation
So going back to point 1 now, yes. This is what I'm saying. Your preference, expressed through reviews and telling people to change when they do it other ways, is where the current situation comes from.
This makes it easy to read the header file and see what the module is doing, without having to dive into the implementation, which could be 1000 lines or more. It also makes for more useful kerneldoc, IMO, since we are showing functions which are exported and therefore more useful for development. Adding the entire C code of U-Boot into kerneldoc seems like a bad use of space.
Um, I don't follow you here. If the code doesn't have kerneldoc comments, it's not included. I just made a silly test with a change to include/clk.h. It's not included in the output. And "the code might be so big it needs refactoring" is not a good excuse.
Yes, I see that static functions don't appear by default, so adding C files doesn't necessarily result in functions appearing in the doc output.
Only properly formatted comments are used in documentation, yes. So static functions could be, if it was important to do so.
The kernel is inconsistent here. The first example I went to look for of something that uses both C and header files in a document was Documentation/admin-guide/pstore-blk.rst and both fs/pstore/blk.c and include/linux/pstore_blk.h are kernel-doc'd in and the header describes a public function of the C file while the C file describes other functions too.
Yes, pstore docs seems like a nice way to do things, for docs.
For people working on the code base, I strongly believe that exported functions and structs are the main way to understand how to use a subsystem, so their docs should be in the header file.
Sure. But there is no project-wide rule. I strongly believe that the API should be documented alongside of the code so that it doesn't drift apart, among other reasons.
Again we are reaching the limits of email, I fear, as I am not suggesting that we document the API somewhere other than in the code. It is just that the API should be in the header file, next to the function it relates to. It can't drift apart, since the header-file must match the C function, or it won't compile.
Regards, Simon
On Mon, Dec 02, 2024 at 05:20:31PM -0700, Simon Glass wrote:
Hi Tom,
On Mon, 2 Dec 2024 at 06:35, Tom Rini trini@konsulko.com wrote:
On Sat, Nov 30, 2024 at 01:24:54PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 15:11, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 07:56:11AM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 07:15, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote: > Hi, > > On Tue, 26 Nov 2024 at 16:32, Tom Rini trini@konsulko.com wrote: > > > > On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote: > > > On 11/26/24 23:33, Tom Rini wrote: > > > > On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote: > > > > > Hi Tom, > > > > > > > > > > On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote: > > > > > > > > > > > > On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote: > > > > > > > Hi Heinrich, > > > > > > > > > > > > > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > > > > > > > > > On 25.11.24 21:44, Simon Glass 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 > > > > > > > > > > > > > > > > Please, have a look at this line in doc/ > > > > > > > > > > > > > > > > doc/api/efi.rst:78: > > > > > > > > .. kernel-doc:: lib/efi_loader/efi_memory.c > > > > > > > > > > > > > > Hmm, we should not add C files as then we end up with all sorts of > > > > > > > internal functions, like checksum(). The help is a bit of a mess on > > > > > > > that page IMO and it could use an index at the top or side. > > > > > > > > > > > > Why? Looking at the linux kernel (where we borrow this framework from > > > > > > and lots of developers expect to follow that style) it's extremely > > > > > > common to kernel-doc C files. I don't see why documenting internal > > > > > > functions (which should be clear as being internal) is a problem. > > > > > > > > > > It depends what you mean by docs. U-Boot puts the documentation for > > > > > exported functions in header files. > > > > > > > > I'm sorry, that's a circular definition. And no, there is not a hard and > > > > fast rule that we only put kernel-doc style comments in header files > > > > (which are consumed by other files and tooling to generate > > > > documentation). > > > > > > > > > Putting docs in C files clutters the docs with internal > > > > > implementations, when most people just want to see the API. The way > > > > > U-Boot does things, it is easy to see the internal (implementation) > > > > > docs by looking at the C code and the external functions by looking at > > > > > the header files. > > > > > > > > Or it makes things harder to understand because the human readable > > > > documentation for a given function is separate from the file the > > > > implements the function. This is possibly why in the linux kernel, where > > > > we borrow "kernel-doc" from, documentation is frequently in the C file > > > > which is then included in Documentation files (the type you objected to > > > > in the start of this patch) which include additional human-readable > > > > explanation of the intention and usage of the API in question. > > > > > > > > > > For static functions we will continue to have function documentation in > > > the C files and not in the headers. I personally never looked up the API > > > documentation in the HTML docs but always in the source. > > Same here. > > > > > > > I am not opposed against moving the EFI function documentation into the > > > header files. But we should update doc/develop/efi.rst accordingly in > > > the same patch series. This is missing here. > > OK > > > > > My point is that the linux kernel is about 60% C files and 40% header > > files being kernel-doc'd in to documentation files and so "U-Boot > > doesn't put documentation in to C files" seems like the wrong approach. > > U-Boot tries to document functions in header files. At the risk of > making a blanket statement with thousands of counter-examples, that is > not so much the case in Linux. > > To a large extent, people developing features in U-Boot rely on the > header files to see what functions they can call. With the > documentation there, they often don't need to look at the source. I > certainly don't, unless the header-file comment is inadequate.
I think this is a case of what you do, rather than what everyone else does.
That's not my reading of the source tree.
So, if we call this point 1 for a moment.
The file we are talking about has some comments in the header and some in the source file. When you use an lsp client, it doesn't know what to do.
Sounds like a bad client? Because again, we should be following the kernel here.
We want to be more like the linux kernel here, rather than less like the linux kernel, given the number of kernel people also working here. If Heinrich is fine with re-arranging everything, OK. But I think documentation with the code makes it less likely to get out of date, both in terms of intentional changes and bug fixes.
I am not suggesting separating documentation from code. I agree that makes no sense. What I have consistently done (and asked for in reviews) is:
- exported functions are documented next to their prototype in the header
- static functions are documented next to their implementation
So going back to point 1 now, yes. This is what I'm saying. Your preference, expressed through reviews and telling people to change when they do it other ways, is where the current situation comes from.
This makes it easy to read the header file and see what the module is doing, without having to dive into the implementation, which could be 1000 lines or more. It also makes for more useful kerneldoc, IMO, since we are showing functions which are exported and therefore more useful for development. Adding the entire C code of U-Boot into kerneldoc seems like a bad use of space.
Um, I don't follow you here. If the code doesn't have kerneldoc comments, it's not included. I just made a silly test with a change to include/clk.h. It's not included in the output. And "the code might be so big it needs refactoring" is not a good excuse.
Yes, I see that static functions don't appear by default, so adding C files doesn't necessarily result in functions appearing in the doc output.
Only properly formatted comments are used in documentation, yes. So static functions could be, if it was important to do so.
The kernel is inconsistent here. The first example I went to look for of something that uses both C and header files in a document was Documentation/admin-guide/pstore-blk.rst and both fs/pstore/blk.c and include/linux/pstore_blk.h are kernel-doc'd in and the header describes a public function of the C file while the C file describes other functions too.
Yes, pstore docs seems like a nice way to do things, for docs.
For people working on the code base, I strongly believe that exported functions and structs are the main way to understand how to use a subsystem, so their docs should be in the header file.
Sure. But there is no project-wide rule. I strongly believe that the API should be documented alongside of the code so that it doesn't drift apart, among other reasons.
Again we are reaching the limits of email, I fear, as I am not suggesting that we document the API somewhere other than in the code. It is just that the API should be in the header file, next to the function it relates to. It can't drift apart, since the header-file must match the C function, or it won't compile.
It will certainly drift apart because the comment that documents how the function works is separate from the implementation.
Hi Tom,
On Mon, 2 Dec 2024 at 17:35, Tom Rini trini@konsulko.com wrote:
On Mon, Dec 02, 2024 at 05:20:31PM -0700, Simon Glass wrote:
Hi Tom,
On Mon, 2 Dec 2024 at 06:35, Tom Rini trini@konsulko.com wrote:
On Sat, Nov 30, 2024 at 01:24:54PM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 15:11, Tom Rini trini@konsulko.com wrote:
On Wed, Nov 27, 2024 at 07:56:11AM -0700, Simon Glass wrote:
Hi Tom,
On Wed, 27 Nov 2024 at 07:15, Tom Rini trini@konsulko.com wrote: > > On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote: > > Hi, > > > > On Tue, 26 Nov 2024 at 16:32, Tom Rini trini@konsulko.com wrote: > > > > > > On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote: > > > > On 11/26/24 23:33, Tom Rini wrote: > > > > > On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote: > > > > > > Hi Tom, > > > > > > > > > > > > On Tue, 26 Nov 2024 at 08:55, Tom Rini trini@konsulko.com wrote: > > > > > > > > > > > > > > On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote: > > > > > > > > Hi Heinrich, > > > > > > > > > > > > > > > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > > > > > > > > > > > > > > > > > > On 25.11.24 21:44, Simon Glass 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 > > > > > > > > > > > > > > > > > > Please, have a look at this line in doc/ > > > > > > > > > > > > > > > > > > doc/api/efi.rst:78: > > > > > > > > > .. kernel-doc:: lib/efi_loader/efi_memory.c > > > > > > > > > > > > > > > > Hmm, we should not add C files as then we end up with all sorts of > > > > > > > > internal functions, like checksum(). The help is a bit of a mess on > > > > > > > > that page IMO and it could use an index at the top or side. > > > > > > > > > > > > > > Why? Looking at the linux kernel (where we borrow this framework from > > > > > > > and lots of developers expect to follow that style) it's extremely > > > > > > > common to kernel-doc C files. I don't see why documenting internal > > > > > > > functions (which should be clear as being internal) is a problem. > > > > > > > > > > > > It depends what you mean by docs. U-Boot puts the documentation for > > > > > > exported functions in header files. > > > > > > > > > > I'm sorry, that's a circular definition. And no, there is not a hard and > > > > > fast rule that we only put kernel-doc style comments in header files > > > > > (which are consumed by other files and tooling to generate > > > > > documentation). > > > > > > > > > > > Putting docs in C files clutters the docs with internal > > > > > > implementations, when most people just want to see the API. The way > > > > > > U-Boot does things, it is easy to see the internal (implementation) > > > > > > docs by looking at the C code and the external functions by looking at > > > > > > the header files. > > > > > > > > > > Or it makes things harder to understand because the human readable > > > > > documentation for a given function is separate from the file the > > > > > implements the function. This is possibly why in the linux kernel, where > > > > > we borrow "kernel-doc" from, documentation is frequently in the C file > > > > > which is then included in Documentation files (the type you objected to > > > > > in the start of this patch) which include additional human-readable > > > > > explanation of the intention and usage of the API in question. > > > > > > > > > > > > > For static functions we will continue to have function documentation in > > > > the C files and not in the headers. I personally never looked up the API > > > > documentation in the HTML docs but always in the source. > > > > Same here. > > > > > > > > > > I am not opposed against moving the EFI function documentation into the > > > > header files. But we should update doc/develop/efi.rst accordingly in > > > > the same patch series. This is missing here. > > > > OK > > > > > > > > My point is that the linux kernel is about 60% C files and 40% header > > > files being kernel-doc'd in to documentation files and so "U-Boot > > > doesn't put documentation in to C files" seems like the wrong approach. > > > > U-Boot tries to document functions in header files. At the risk of > > making a blanket statement with thousands of counter-examples, that is > > not so much the case in Linux. > > > > To a large extent, people developing features in U-Boot rely on the > > header files to see what functions they can call. With the > > documentation there, they often don't need to look at the source. I > > certainly don't, unless the header-file comment is inadequate. > > I think this is a case of what you do, rather than what everyone else > does.
That's not my reading of the source tree.
So, if we call this point 1 for a moment.
The file we are talking about has some comments in the header and some in the source file. When you use an lsp client, it doesn't know what to do.
Sounds like a bad client? Because again, we should be following the kernel here.
> We want to be more like the linux kernel here, rather than less > like the linux kernel, given the number of kernel people also working > here. If Heinrich is fine with re-arranging everything, OK. But I think > documentation with the code makes it less likely to get out of date, > both in terms of intentional changes and bug fixes.
I am not suggesting separating documentation from code. I agree that makes no sense. What I have consistently done (and asked for in reviews) is:
- exported functions are documented next to their prototype in the header
- static functions are documented next to their implementation
So going back to point 1 now, yes. This is what I'm saying. Your preference, expressed through reviews and telling people to change when they do it other ways, is where the current situation comes from.
This makes it easy to read the header file and see what the module is doing, without having to dive into the implementation, which could be 1000 lines or more. It also makes for more useful kerneldoc, IMO, since we are showing functions which are exported and therefore more useful for development. Adding the entire C code of U-Boot into kerneldoc seems like a bad use of space.
Um, I don't follow you here. If the code doesn't have kerneldoc comments, it's not included. I just made a silly test with a change to include/clk.h. It's not included in the output. And "the code might be so big it needs refactoring" is not a good excuse.
Yes, I see that static functions don't appear by default, so adding C files doesn't necessarily result in functions appearing in the doc output.
Only properly formatted comments are used in documentation, yes. So static functions could be, if it was important to do so.
The kernel is inconsistent here. The first example I went to look for of something that uses both C and header files in a document was Documentation/admin-guide/pstore-blk.rst and both fs/pstore/blk.c and include/linux/pstore_blk.h are kernel-doc'd in and the header describes a public function of the C file while the C file describes other functions too.
Yes, pstore docs seems like a nice way to do things, for docs.
For people working on the code base, I strongly believe that exported functions and structs are the main way to understand how to use a subsystem, so their docs should be in the header file.
Sure. But there is no project-wide rule. I strongly believe that the API should be documented alongside of the code so that it doesn't drift apart, among other reasons.
Again we are reaching the limits of email, I fear, as I am not suggesting that we document the API somewhere other than in the code. It is just that the API should be in the header file, next to the function it relates to. It can't drift apart, since the header-file must match the C function, or it won't compile.
It will certainly drift apart because the comment that documents how the function works is separate from the implementation.
Updating function comments is a habit that is hard to get into.
Regards, Simon
Rather than an integer, it is better to use the enum provided, when referring to an EFI memory-type. Update existing uses.
Call the value 'mem_type' consistently. Fix up one instance of upper-case hex.
Fix up the calls in struct efi_boot_services so that they use the same enum, adding the missing parameter names and enum efi_allocate_type.
While we are here, rename the 'memory' parameter to 'memoryp' so that it is clear it is a return value.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/efi_api.h | 6 ++++-- include/efi_loader.h | 28 ++++++++++++++------------- lib/efi_loader/efi_boottime.c | 13 +++++++------ lib/efi_loader/efi_device_path.c | 4 ++-- lib/efi_loader/efi_memory.c | 33 ++++++++++++++++---------------- 5 files changed, 45 insertions(+), 39 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index f07d074f93b..1260df0cd58 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -69,8 +69,10 @@ struct efi_boot_services { efi_status_t (EFIAPI *raise_tpl)(efi_uintn_t new_tpl); void (EFIAPI *restore_tpl)(efi_uintn_t old_tpl);
- efi_status_t (EFIAPI *allocate_pages)(int, int, efi_uintn_t, - efi_physical_addr_t *); + efi_status_t (EFIAPI *allocate_pages)(enum efi_allocate_type type, + enum efi_memory_type mem_type, + efi_uintn_t pages, + efi_physical_addr_t *memoryp); efi_status_t (EFIAPI *free_pages)(efi_physical_addr_t, efi_uintn_t); efi_status_t (EFIAPI *get_memory_map)(efi_uintn_t *memory_map_size, struct efi_mem_desc *desc, diff --git a/include/efi_loader.h b/include/efi_loader.h index ff7adc1e2a2..0ef4c6f7dea 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -773,24 +773,25 @@ void *efi_alloc(size_t len); * efi_alloc_aligned_pages() - allocate aligned memory pages * * @len: len in bytes - * @memory_type: usage type of the allocated memory + * @mem_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); +void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, + size_t align);
/** * efi_allocate_pages - allocate memory pages * * @type: type of allocation to be performed - * @memory_type: usage type of the allocated memory + * @mem_type: usage type of the allocated memory * @pages: number of pages to be allocated - * @memory: returns a pointer to the allocated memory + * @memoryp: 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); + enum efi_memory_type mem_type, + efi_uintn_t pages, uint64_t *memoryp);
/** * efi_free_pages() - free memory pages @@ -804,12 +805,12 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages); /** * efi_allocate_pool - allocate memory from pool * - * @pool_type: type of the pool from which memory is to be allocated + * @mem_type: memory 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_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, void **buffer);
/** @@ -834,10 +835,11 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, * efi_add_memory_map() - Add a range into the EFI memory map * @start: start address, must be a multiple of EFI_PAGE_SIZE * @size: Size, in bytes - * @memory_type: EFI type of memory added + * @mem_type: EFI type of memory added * Return: status code */ -efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); +efi_status_t efi_add_memory_map(u64 start, u64 size, + enum efi_memory_type mem_type);
/** * efi_add_memory_map_pg() - add pages to the memory map @@ -845,13 +847,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); * @start: start address, must be a multiple of * EFI_PAGE_SIZE * @pages: number of pages to add - * @memory_type: EFI type of memory added + * @mem_type: EFI 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, + enum efi_memory_type mem_type, bool overlap_conventional);
/* Called by board init to initialize the EFI drivers */ @@ -910,7 +912,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, +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type, uint64_t start_address, size_t size); /* Determine the last device path node that is not the end node. */ diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 90437e3e401..efd9577c9d0 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -414,9 +414,9 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl) /** * efi_allocate_pages_ext() - allocate memory pages * @type: type of allocation to be performed - * @memory_type: usage type of the allocated memory + * @mem_type: usage type of the allocated memory * @pages: number of pages to be allocated - * @memory: allocated memory + * @memoryp: allocated memory * * This function implements the AllocatePages service. * @@ -425,14 +425,15 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl) * * Return: status code */ -static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type, +static efi_status_t EFIAPI efi_allocate_pages_ext(enum efi_allocate_type type, + enum efi_memory_type mem_type, efi_uintn_t pages, - uint64_t *memory) + uint64_t *memoryp) { efi_status_t r;
- EFI_ENTRY("%d, %d, 0x%zx, %p", type, memory_type, pages, memory); - r = efi_allocate_pages(type, memory_type, pages, memory); + EFI_ENTRY("%d, %d, 0x%zx, %p", type, mem_type, pages, memoryp); + r = efi_allocate_pages(type, mem_type, pages, memoryp); return EFI_EXIT(r); }
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index ee387e1dfd4..9c8cd35b97b 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -975,7 +975,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, +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type, uint64_t start_address, size_t size) { @@ -990,7 +990,7 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type, mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE; mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY; mdp->dp.length = sizeof(*mdp); - mdp->memory_type = memory_type; + mdp->memory_type = mem_type; mdp->start_address = start_address; mdp->end_address = start_address + size; buf = &mdp[1]; diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 7c8d9485094..2c46915e5b9 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -258,7 +258,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, }
efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, - int memory_type, + enum efi_memory_type mem_type, bool overlap_conventional) { struct efi_mem_list *lmem; @@ -268,10 +268,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, struct efi_event *evt;
EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, - start, pages, memory_type, overlap_conventional ? + start, pages, mem_type, overlap_conventional ? "yes" : "no");
- if (memory_type >= EFI_MAX_MEMORY_TYPE) + if (mem_type >= EFI_MAX_MEMORY_TYPE) return EFI_INVALID_PARAMETER;
if (!pages) @@ -281,12 +281,12 @@ 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.type = mem_type; newlist->desc.physical_start = start; newlist->desc.virtual_start = start; newlist->desc.num_pages = pages;
- switch (memory_type) { + switch (mem_type) { case EFI_RUNTIME_SERVICES_CODE: case EFI_RUNTIME_SERVICES_DATA: newlist->desc.attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME; @@ -370,14 +370,15 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_SUCCESS; }
-efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type) +efi_status_t efi_add_memory_map(u64 start, u64 size, + enum efi_memory_type mem_type) { u64 pages;
pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK)); start &= ~EFI_PAGE_MASK;
- return efi_add_memory_map_pg(start, pages, memory_type, false); + return efi_add_memory_map_pg(start, pages, mem_type, false); }
/** @@ -416,7 +417,7 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) }
efi_status_t efi_allocate_pages(enum efi_allocate_type type, - enum efi_memory_type memory_type, + enum efi_memory_type mem_type, efi_uintn_t pages, uint64_t *memory) { u64 efi_addr, len; @@ -425,8 +426,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, phys_addr_t addr;
/* Check import parameters */ - if (memory_type >= EFI_PERSISTENT_MEMORY_TYPE && - memory_type <= 0x6FFFFFFF) + if (mem_type >= EFI_PERSISTENT_MEMORY_TYPE && mem_type <= 0x6fffffff) return EFI_INVALID_PARAMETER; if (!memory) return EFI_INVALID_PARAMETER; @@ -469,7 +469,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
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(efi_addr, pages, mem_type, true); if (ret != EFI_SUCCESS) { /* Map would overlap, bail out */ lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags); @@ -522,7 +522,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return ret; }
-void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) +void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, + size_t align) { u64 req_pages = efi_size_in_pages(len); u64 true_pages = req_pages + efi_size_in_pages(align) - 1; @@ -540,12 +541,12 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) return NULL;
if (align < EFI_PAGE_SIZE) { - r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, + r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, req_pages, &mem); return (r == EFI_SUCCESS) ? (void *)(uintptr_t)mem : NULL; }
- r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, + r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, true_pages, &mem); if (r != EFI_SUCCESS) return NULL; @@ -566,7 +567,7 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) return (void *)(uintptr_t)aligned_mem; }
-efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer) +efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, void **buffer) { efi_status_t r; u64 addr; @@ -582,7 +583,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, return EFI_SUCCESS; }
- r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages, + r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, num_pages, &addr); if (r == EFI_SUCCESS) { alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
On 25.11.24 21:44, Simon Glass wrote:
Rather than an integer, it is better to use the enum provided, when referring to an EFI memory-type. Update existing uses.
The C standard provides no definition of the size of the integer used to implement enums. It could use u8, u16, u32, or u64.
As EFI applications may be compiled using a different compiler we need to control the width used for passing parameters in the API interface.
We should have used u32 instead of int here. We could use a typedef for more verbosity though that is frowned upon in U-Boot.
Best regards
Heinrich
Call the value 'mem_type' consistently. Fix up one instance of upper-case hex.
Fix up the calls in struct efi_boot_services so that they use the same enum, adding the missing parameter names and enum efi_allocate_type.
While we are here, rename the 'memory' parameter to 'memoryp' so that it is clear it is a return value.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_api.h | 6 ++++-- include/efi_loader.h | 28 ++++++++++++++------------- lib/efi_loader/efi_boottime.c | 13 +++++++------ lib/efi_loader/efi_device_path.c | 4 ++-- lib/efi_loader/efi_memory.c | 33 ++++++++++++++++---------------- 5 files changed, 45 insertions(+), 39 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index f07d074f93b..1260df0cd58 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -69,8 +69,10 @@ struct efi_boot_services { efi_status_t (EFIAPI *raise_tpl)(efi_uintn_t new_tpl); void (EFIAPI *restore_tpl)(efi_uintn_t old_tpl);
- efi_status_t (EFIAPI *allocate_pages)(int, int, efi_uintn_t,
efi_physical_addr_t *);
- efi_status_t (EFIAPI *allocate_pages)(enum efi_allocate_type type,
enum efi_memory_type mem_type,efi_uintn_t pages,efi_physical_addr_t *memoryp);diff --git a/include/efi_loader.h b/include/efi_loader.h index ff7adc1e2a2..0ef4c6f7dea 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -773,24 +773,25 @@ void *efi_alloc(size_t len);
- efi_alloc_aligned_pages() - allocate aligned memory pages
- @len: len in bytes
- @memory_type: usage type of the allocated memory
*/
- @mem_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); +void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
size_t align);/**
- efi_allocate_pages - allocate memory pages
- @type: type of allocation to be performed
- @memory_type: usage type of the allocated memory
- @mem_type: usage type of the allocated memory
- @pages: number of pages to be allocated
- @memory: returns a pointer to the allocated memory
*/ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
- @memoryp: returns a pointer to the allocated memory
- Return: status code
enum efi_memory_type memory_type,efi_uintn_t pages, uint64_t *memory);
enum efi_memory_type mem_type,efi_uintn_t pages, uint64_t *memoryp);/**
- efi_free_pages() - free memory pages
@@ -804,12 +805,12 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages); /**
- efi_allocate_pool - allocate memory from pool
- @pool_type: type of the pool from which memory is to be allocated
*/
- @mem_type: memory 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_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, void **buffer);
/** @@ -834,10 +835,11 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
- efi_add_memory_map() - Add a range into the EFI memory map
- @start: start address, must be a multiple of EFI_PAGE_SIZE
- @size: Size, in bytes
- @memory_type: EFI type of memory added
*/
- @mem_type: EFI type of memory added
- Return: status code
-efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type); +efi_status_t efi_add_memory_map(u64 start, u64 size,
enum efi_memory_type mem_type);/**
- efi_add_memory_map_pg() - add pages to the memory map
@@ -845,13 +847,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
- @start: start address, must be a multiple of
EFI_PAGE_SIZE- @pages: number of pages to add
- @memory_type: EFI type of memory added
*/ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
- @mem_type: EFI type of memory added
- @overlap_conventional: region may only overlap free(conventional)
memory- Return: status code
int memory_type,
enum efi_memory_type mem_type, bool overlap_conventional);/* Called by board init to initialize the EFI drivers */
@@ -910,7 +912,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, +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type, uint64_t start_address, size_t size); /* Determine the last device path node that is not the end node. */ diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 90437e3e401..efd9577c9d0 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -414,9 +414,9 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl) /**
- efi_allocate_pages_ext() - allocate memory pages
- @type: type of allocation to be performed
- @memory_type: usage type of the allocated memory
- @mem_type: usage type of the allocated memory
- @pages: number of pages to be allocated
- @memory: allocated memory
- @memoryp: allocated memory
- This function implements the AllocatePages service.
@@ -425,14 +425,15 @@ static void EFIAPI efi_restore_tpl(efi_uintn_t old_tpl)
- Return: status code
*/ -static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type, +static efi_status_t EFIAPI efi_allocate_pages_ext(enum efi_allocate_type type,
enum efi_memory_type mem_type, efi_uintn_t pages,
uint64_t *memory)
uint64_t *memoryp)
- EFI_ENTRY("%d, %d, 0x%zx, %p", type, memory_type, pages, memory);
- r = efi_allocate_pages(type, memory_type, pages, memory);
- EFI_ENTRY("%d, %d, 0x%zx, %p", type, mem_type, pages, memoryp);
- r = efi_allocate_pages(type, mem_type, pages, memoryp); return EFI_EXIT(r); }
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index ee387e1dfd4..9c8cd35b97b 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -975,7 +975,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, +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type, uint64_t start_address, size_t size) { @@ -990,7 +990,7 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type, mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE; mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY; mdp->dp.length = sizeof(*mdp);
- mdp->memory_type = memory_type;
- mdp->memory_type = mem_type; mdp->start_address = start_address; mdp->end_address = start_address + size; buf = &mdp[1];
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 7c8d9485094..2c46915e5b9 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -258,7 +258,7 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, }
efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
int memory_type,
enum efi_memory_type mem_type, bool overlap_conventional)@@ -268,10 +268,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, struct efi_event *evt;
EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
start, pages, memory_type, overlap_conventional ?
start, pages, mem_type, overlap_conventional ?
- if (memory_type >= EFI_MAX_MEMORY_TYPE)
if (mem_type >= EFI_MAX_MEMORY_TYPE) return EFI_INVALID_PARAMETER;
if (!pages)
@@ -281,12 +281,12 @@ 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.type = mem_type; newlist->desc.physical_start = start; newlist->desc.virtual_start = start; newlist->desc.num_pages = pages;
- switch (memory_type) {
- switch (mem_type) { case EFI_RUNTIME_SERVICES_CODE: case EFI_RUNTIME_SERVICES_DATA: newlist->desc.attribute = EFI_MEMORY_WB | EFI_MEMORY_RUNTIME;
@@ -370,14 +370,15 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_SUCCESS; }
-efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type) +efi_status_t efi_add_memory_map(u64 start, u64 size,
enum efi_memory_type mem_type){ u64 pages;
pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK)); start &= ~EFI_PAGE_MASK;
- return efi_add_memory_map_pg(start, pages, memory_type, false);
return efi_add_memory_map_pg(start, pages, mem_type, false); }
/**
@@ -416,7 +417,7 @@ static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) }
efi_status_t efi_allocate_pages(enum efi_allocate_type type,
enum efi_memory_type memory_type,
enum efi_memory_type mem_type, efi_uintn_t pages, uint64_t *memory)@@ -425,8 +426,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, phys_addr_t addr;
/* Check import parameters */
- if (memory_type >= EFI_PERSISTENT_MEMORY_TYPE &&
memory_type <= 0x6FFFFFFF)
- if (mem_type >= EFI_PERSISTENT_MEMORY_TYPE && mem_type <= 0x6fffffff) return EFI_INVALID_PARAMETER; if (!memory) return EFI_INVALID_PARAMETER;
@@ -469,7 +469,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
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(efi_addr, pages, mem_type, true); if (ret != EFI_SUCCESS) { /* Map would overlap, bail out */ lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags);
@@ -522,7 +522,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return ret; }
-void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) +void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
size_t align)@@ -540,12 +541,12 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) return NULL;
if (align < EFI_PAGE_SIZE) {
r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type,
r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, req_pages, &mem);
- r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type,
- r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, true_pages, &mem); if (r != EFI_SUCCESS) return NULL;
@@ -566,7 +567,7 @@ void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align) return (void *)(uintptr_t)aligned_mem; }
-efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, void **buffer) +efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, void **buffer) { efi_status_t r; u64 addr; @@ -582,7 +583,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size, return EFI_SUCCESS; }
- r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
- r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, num_pages, &addr); if (r == EFI_SUCCESS) { alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:44, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass wrote:
Rather than an integer, it is better to use the enum provided, when referring to an EFI memory-type. Update existing uses.
The C standard provides no definition of the size of the integer used to implement enums. It could use u8, u16, u32, or u64.
As EFI applications may be compiled using a different compiler we need to control the width used for passing parameters in the API interface.
We should have used u32 instead of int here. We could use a typedef for more verbosity though that is frowned upon in U-Boot.
Yes, the ext function is debatable. The rest seem OK to me, though, as they are internal functions, See also [1].
Best regards
Heinrich
Call the value 'mem_type' consistently. Fix up one instance of upper-case hex.
Fix up the calls in struct efi_boot_services so that they use the same enum, adding the missing parameter names and enum efi_allocate_type.
While we are here, rename the 'memory' parameter to 'memoryp' so that it is clear it is a return value.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_api.h | 6 ++++-- include/efi_loader.h | 28 ++++++++++++++------------- lib/efi_loader/efi_boottime.c | 13 +++++++------ lib/efi_loader/efi_device_path.c | 4 ++-- lib/efi_loader/efi_memory.c | 33 ++++++++++++++++---------------- 5 files changed, 45 insertions(+), 39 deletions(-)
Regards, SImon
[1] https://stackoverflow.com/questions/4879286/specifying-size-of-enum-type-in-...
Hi Heinrich,
On Tue, 26 Nov 2024 at 08:37, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:44, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass wrote:
Rather than an integer, it is better to use the enum provided, when referring to an EFI memory-type. Update existing uses.
The C standard provides no definition of the size of the integer used to implement enums. It could use u8, u16, u32, or u64.
As EFI applications may be compiled using a different compiler we need to control the width used for passing parameters in the API interface.
We should have used u32 instead of int here. We could use a typedef for more verbosity though that is frowned upon in U-Boot.
Yes, the ext function is debatable. The rest seem OK to me, though, as they are internal functions, See also [1].
Hmm, but the spec uses enum, so arguably this patch is more correct?
Best regards
Heinrich
Call the value 'mem_type' consistently. Fix up one instance of upper-case hex.
Fix up the calls in struct efi_boot_services so that they use the same enum, adding the missing parameter names and enum efi_allocate_type.
While we are here, rename the 'memory' parameter to 'memoryp' so that it is clear it is a return value.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_api.h | 6 ++++-- include/efi_loader.h | 28 ++++++++++++++------------- lib/efi_loader/efi_boottime.c | 13 +++++++------ lib/efi_loader/efi_device_path.c | 4 ++-- lib/efi_loader/efi_memory.c | 33 ++++++++++++++++---------------- 5 files changed, 45 insertions(+), 39 deletions(-)
Regards, SImon
[1] https://stackoverflow.com/questions/4879286/specifying-size-of-enum-type-in-...
Regards, Simon
From: Simon Glass sjg@chromium.org Date: Wed, 27 Nov 2024 06:07:05 -0700
Hi Heinrich,
On Tue, 26 Nov 2024 at 08:37, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:44, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass wrote:
Rather than an integer, it is better to use the enum provided, when referring to an EFI memory-type. Update existing uses.
The C standard provides no definition of the size of the integer used to implement enums. It could use u8, u16, u32, or u64.
As EFI applications may be compiled using a different compiler we need to control the width used for passing parameters in the API interface.
We should have used u32 instead of int here. We could use a typedef for more verbosity though that is frowned upon in U-Boot.
Yes, the ext function is debatable. The rest seem OK to me, though, as they are internal functions, See also [1].
Hmm, but the spec uses enum, so arguably this patch is more correct?
Only as a way to define the constants. The actual interfaces that use the constants use UINT32, which would indeed be u32 in Linux land as Heinrich suggests.
So no, the patch isn't correct.
Best regards
Heinrich
Call the value 'mem_type' consistently. Fix up one instance of upper-case hex.
Fix up the calls in struct efi_boot_services so that they use the same enum, adding the missing parameter names and enum efi_allocate_type.
While we are here, rename the 'memory' parameter to 'memoryp' so that it is clear it is a return value.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_api.h | 6 ++++-- include/efi_loader.h | 28 ++++++++++++++------------- lib/efi_loader/efi_boottime.c | 13 +++++++------ lib/efi_loader/efi_device_path.c | 4 ++-- lib/efi_loader/efi_memory.c | 33 ++++++++++++++++---------------- 5 files changed, 45 insertions(+), 39 deletions(-)
Regards, SImon
[1] https://stackoverflow.com/questions/4879286/specifying-size-of-enum-type-in-...
Regards, Simon
Hi Mark,
On Wed, 27 Nov 2024 at 06:23, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 27 Nov 2024 06:07:05 -0700
Hi Heinrich,
On Tue, 26 Nov 2024 at 08:37, Simon Glass sjg@chromium.org wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:44, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass wrote:
Rather than an integer, it is better to use the enum provided, when referring to an EFI memory-type. Update existing uses.
The C standard provides no definition of the size of the integer used to implement enums. It could use u8, u16, u32, or u64.
As EFI applications may be compiled using a different compiler we need to control the width used for passing parameters in the API interface.
We should have used u32 instead of int here. We could use a typedef for more verbosity though that is frowned upon in U-Boot.
Yes, the ext function is debatable. The rest seem OK to me, though, as they are internal functions, See also [1].
Hmm, but the spec uses enum, so arguably this patch is more correct?
Only as a way to define the constants. The actual interfaces that use the constants use UINT32, which would indeed be u32 in Linux land as Heinrich suggests.
So no, the patch isn't correct.
Just to add a bit more detail...
I see the spec says this: "Element of a standard ANSI C enum type declaration. Type INT32.or UINT32. ANSI C does not define the size of sign of an enum so they should never be used in structures. ANSI C integer promotion rules make INT32 or UINT32 interchangeable when passed as an argument to a function."
The compilers we use (at least gcc and clang) use int as the enum type, and int is 32 bits on 32/64-bit machines.
So it looks safe to me. But since the spec goes out of its way to mention this point, it seems best to keep the external interface using int and just use the enum within the implementation code. That's what I did on v2 onwards.
Best regards
Heinrich
Call the value 'mem_type' consistently. Fix up one instance of upper-case hex.
Fix up the calls in struct efi_boot_services so that they use the same enum, adding the missing parameter names and enum efi_allocate_type.
While we are here, rename the 'memory' parameter to 'memoryp' so that it is clear it is a return value.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_api.h | 6 ++++-- include/efi_loader.h | 28 ++++++++++++++------------- lib/efi_loader/efi_boottime.c | 13 +++++++------ lib/efi_loader/efi_device_path.c | 4 ++-- lib/efi_loader/efi_memory.c | 33 ++++++++++++++++---------------- 5 files changed, 45 insertions(+), 39 deletions(-)
Regards, SImon
[1] https://stackoverflow.com/questions/4879286/specifying-size-of-enum-type-in-...
Regards, Simon
One of the confusing parts of the EFI loader is that it uses u64 for addresses, whereas the rest of U-Boot typically uses ulong.
There is a case on 32-bit machines where phys_addr_t can be larger than 32 bits, but this is very much the exception. Also, such machines have mostly faded away and generally don't make use of EFI.
So in the interests of consistency, update the memory functions to use ulong in most cases.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/efi_loader.h | 4 ++-- lib/efi_loader/efi_memory.c | 42 ++++++++++++++++++------------------- 2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 0ef4c6f7dea..cae94fc4661 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -838,7 +838,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, * @mem_type: EFI type of memory added * Return: status code */ -efi_status_t efi_add_memory_map(u64 start, u64 size, +efi_status_t efi_add_memory_map(ulong start, ulong size, enum efi_memory_type mem_type);
/** @@ -852,7 +852,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, * memory * Return: status code */ -efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages, enum efi_memory_type mem_type, bool overlap_conventional);
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 2c46915e5b9..4e9c372b622 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -199,10 +199,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, { struct efi_mem_list *newmap; struct efi_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; - uint64_t carve_end = carve_start + + ulong map_start = map_desc->physical_start; + ulong map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT); + ulong carve_start = carve_desc->physical_start; + ulong carve_end = carve_start + (carve_desc->num_pages << EFI_PAGE_SHIFT);
/* check whether we're overlapping */ @@ -257,17 +257,17 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_LOOP_AGAIN; }
-efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages, enum efi_memory_type mem_type, bool overlap_conventional) { struct efi_mem_list *lmem; struct efi_mem_list *newlist; bool carve_again; - uint64_t carved_pages = 0; + ulong carved_pages = 0; struct efi_event *evt;
- EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, + EFI_PRINT("%s: 0x%lx 0x%lx %d %s\n", __func__, start, pages, mem_type, overlap_conventional ? "yes" : "no");
@@ -370,10 +370,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_SUCCESS; }
-efi_status_t efi_add_memory_map(u64 start, u64 size, +efi_status_t efi_add_memory_map(ulong start, ulong size, enum efi_memory_type mem_type) { - u64 pages; + ulong pages;
pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK)); start &= ~EFI_PAGE_MASK; @@ -396,13 +396,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, * @must_be_allocated: return success if the page is allocated * Return: status code */ -static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) +static efi_status_t efi_check_allocated(ulong 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); + ulong start = item->desc.physical_start; + ulong end = start + (item->desc.num_pages << EFI_PAGE_SHIFT);
if (addr >= start && addr < end) { if (must_be_allocated ^ @@ -420,7 +420,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type mem_type, efi_uintn_t pages, uint64_t *memory) { - u64 efi_addr, len; + ulong efi_addr, len; uint flags; efi_status_t ret; phys_addr_t addr; @@ -430,7 +430,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, return EFI_INVALID_PARAMETER; if (!memory) return EFI_INVALID_PARAMETER; - len = (u64)pages << EFI_PAGE_SHIFT; + len = (ulong)pages << EFI_PAGE_SHIFT; /* Catch possible overflow on 64bit systems */ if (sizeof(efi_uintn_t) == sizeof(u64) && (len >> EFI_PAGE_SHIFT) != (u64)pages) @@ -491,7 +491,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, */ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) { - u64 len; + ulong len; long status; efi_status_t ret;
@@ -506,7 +506,7 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return EFI_INVALID_PARAMETER; }
- len = (u64)pages << EFI_PAGE_SHIFT; + len = (ulong)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 @@ -525,10 +525,10 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, size_t align) { - u64 req_pages = efi_size_in_pages(len); - u64 true_pages = req_pages + efi_size_in_pages(align) - 1; - u64 free_pages; - u64 aligned_mem; + ulong req_pages = efi_size_in_pages(len); + ulong true_pages = req_pages + efi_size_in_pages(align) - 1; + ulong free_pages; + ulong aligned_mem; efi_status_t r; u64 mem;
@@ -572,7 +572,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, efi_status_t r; u64 addr; struct efi_pool_allocation *alloc; - u64 num_pages = efi_size_in_pages(size + + ulong num_pages = efi_size_in_pages(size + sizeof(struct efi_pool_allocation));
if (!buffer)
Hi Simon,
On Mon, 25 Nov 2024 at 22:45, Simon Glass sjg@chromium.org wrote:
One of the confusing parts of the EFI loader is that it uses u64 for addresses, whereas the rest of U-Boot typically uses ulong.
There is a case on 32-bit machines where phys_addr_t can be larger than 32 bits, but this is very much the exception. Also, such machines have mostly faded away and generally don't make use of EFI.
I am not sure how true this statement is with LPAE etc. In any case, I don't want us to convert to ulong, there's no reason to break things for platforms we can't test. So please drop this patch
Thanks /Ilias
So in the interests of consistency, update the memory functions to use ulong in most cases.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_loader.h | 4 ++-- lib/efi_loader/efi_memory.c | 42 ++++++++++++++++++------------------- 2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 0ef4c6f7dea..cae94fc4661 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -838,7 +838,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
- @mem_type: EFI type of memory added
- Return: status code
*/ -efi_status_t efi_add_memory_map(u64 start, u64 size, +efi_status_t efi_add_memory_map(ulong start, ulong size, enum efi_memory_type mem_type);
/** @@ -852,7 +852,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
memory- Return: status code
*/ -efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages, enum efi_memory_type mem_type, bool overlap_conventional);
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 2c46915e5b9..4e9c372b622 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -199,10 +199,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, { struct efi_mem_list *newmap; struct efi_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;uint64_t carve_end = carve_start +
ulong map_start = map_desc->physical_start;ulong map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT);ulong carve_start = carve_desc->physical_start;ulong carve_end = carve_start + (carve_desc->num_pages << EFI_PAGE_SHIFT); /* check whether we're overlapping */@@ -257,17 +257,17 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_LOOP_AGAIN; }
-efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages, enum efi_memory_type mem_type, bool overlap_conventional) { struct efi_mem_list *lmem; struct efi_mem_list *newlist; bool carve_again;
uint64_t carved_pages = 0;
ulong carved_pages = 0; struct efi_event *evt;
EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
EFI_PRINT("%s: 0x%lx 0x%lx %d %s\n", __func__, start, pages, mem_type, overlap_conventional ? "yes" : "no");@@ -370,10 +370,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_SUCCESS; }
-efi_status_t efi_add_memory_map(u64 start, u64 size, +efi_status_t efi_add_memory_map(ulong start, ulong size, enum efi_memory_type mem_type) {
u64 pages;
ulong pages; pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK)); start &= ~EFI_PAGE_MASK;@@ -396,13 +396,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
- @must_be_allocated: return success if the page is allocated
- Return: status code
*/ -static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) +static efi_status_t efi_check_allocated(ulong 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);
ulong start = item->desc.physical_start;ulong end = start + (item->desc.num_pages << EFI_PAGE_SHIFT); if (addr >= start && addr < end) { if (must_be_allocated ^@@ -420,7 +420,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type mem_type, efi_uintn_t pages, uint64_t *memory) {
u64 efi_addr, len;
ulong efi_addr, len; uint flags; efi_status_t ret; phys_addr_t addr;@@ -430,7 +430,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, return EFI_INVALID_PARAMETER; if (!memory) return EFI_INVALID_PARAMETER;
len = (u64)pages << EFI_PAGE_SHIFT;
len = (ulong)pages << EFI_PAGE_SHIFT; /* Catch possible overflow on 64bit systems */ if (sizeof(efi_uintn_t) == sizeof(u64) && (len >> EFI_PAGE_SHIFT) != (u64)pages)@@ -491,7 +491,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, */ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) {
u64 len;
ulong len; long status; efi_status_t ret;@@ -506,7 +506,7 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return EFI_INVALID_PARAMETER; }
len = (u64)pages << EFI_PAGE_SHIFT;
len = (ulong)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@@ -525,10 +525,10 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, size_t align) {
u64 req_pages = efi_size_in_pages(len);u64 true_pages = req_pages + efi_size_in_pages(align) - 1;u64 free_pages;u64 aligned_mem;
ulong req_pages = efi_size_in_pages(len);ulong true_pages = req_pages + efi_size_in_pages(align) - 1;ulong free_pages;ulong aligned_mem; efi_status_t r; u64 mem;@@ -572,7 +572,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, efi_status_t r; u64 addr; struct efi_pool_allocation *alloc;
u64 num_pages = efi_size_in_pages(size +
ulong num_pages = efi_size_in_pages(size + sizeof(struct efi_pool_allocation)); if (!buffer)-- 2.43.0
On 26.11.24 09:00, Ilias Apalodimas wrote:
Hi Simon,
On Mon, 25 Nov 2024 at 22:45, Simon Glass sjg@chromium.org wrote:
One of the confusing parts of the EFI loader is that it uses u64 for addresses, whereas the rest of U-Boot typically uses ulong.
You are confusing sandbox virtual addresses (phys_addr_t) with the physical addresses (EFI_PHYSICAL_ADDRESS) in the UEFI spec.
The EFI specification requires an identity mapping. This means the value of a EFI_PHYSICAL_ADDRESS must be usable as a pointer.
The sandbox virtual addresses are not usable as pointers on the sandbox and hence cannot be used for EFI_PHYSICAL_ADDRESS.
EFI_PHYSICAL_ADDRESS is a 64bit type.
The only useful place for sandbox virtual addresses is in the CLI. We should get rid of them anywhere else to avoid further confusion.
There is a case on 32-bit machines where phys_addr_t can be larger than 32 bits, but this is very much the exception. Also, such machines have mostly faded away and generally don't make use of EFI.
I am not sure how true this statement is with LPAE etc. In any case, I don't want us to convert to ulong, there's no reason to break things for platforms we can't test. So please drop this patch
Thanks /Ilias
So in the interests of consistency, update the memory functions to use ulong in most cases.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_loader.h | 4 ++-- lib/efi_loader/efi_memory.c | 42 ++++++++++++++++++------------------- 2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 0ef4c6f7dea..cae94fc4661 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -838,7 +838,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
- @mem_type: EFI type of memory added
- Return: status code
*/ -efi_status_t efi_add_memory_map(u64 start, u64 size, +efi_status_t efi_add_memory_map(ulong start, ulong size, enum efi_memory_type mem_type);
/** @@ -852,7 +852,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
memory- Return: status code
*/ -efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages, enum efi_memory_type mem_type, bool overlap_conventional);
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 2c46915e5b9..4e9c372b622 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -199,10 +199,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, { struct efi_mem_list *newmap; struct efi_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;uint64_t carve_end = carve_start +
ulong map_start = map_desc->physical_start;ulong map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT);ulong carve_start = carve_desc->physical_start;ulong carve_end = carve_start + (carve_desc->num_pages << EFI_PAGE_SHIFT); /* check whether we're overlapping */@@ -257,17 +257,17 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_LOOP_AGAIN; }
-efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages, enum efi_memory_type mem_type, bool overlap_conventional) { struct efi_mem_list *lmem; struct efi_mem_list *newlist; bool carve_again;
uint64_t carved_pages = 0;
ulong carved_pages = 0; struct efi_event *evt;
EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
EFI_PRINT("%s: 0x%lx 0x%lx %d %s\n", __func__, start, pages, mem_type, overlap_conventional ? "yes" : "no");@@ -370,10 +370,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_SUCCESS; }
-efi_status_t efi_add_memory_map(u64 start, u64 size, +efi_status_t efi_add_memory_map(ulong start, ulong size, enum efi_memory_type mem_type) {
u64 pages;
ulong pages;
The result of efi_size_in_pages() is u64.
pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK));
Maybe we should check here that pages <= SIZE_T_MAX.
start &= ~EFI_PAGE_MASK;@@ -396,13 +396,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
- @must_be_allocated: return success if the page is allocated
- Return: status code
*/ -static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) +static efi_status_t efi_check_allocated(ulong 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);
ulong start = item->desc.physical_start;ulong end = start + (item->desc.num_pages << EFI_PAGE_SHIFT);
Physical start is u64 as defined in the EFI standard. ulong makes no sense here.
if (addr >= start && addr < end) { if (must_be_allocated ^@@ -420,7 +420,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type mem_type, efi_uintn_t pages, uint64_t *memory) {
u64 efi_addr, len;
ulong efi_addr, len;
efi_allocate_pages must return u64. Using ulong for efi_addr does not match.
uint flags; efi_status_t ret; phys_addr_t addr;@@ -430,7 +430,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, return EFI_INVALID_PARAMETER; if (!memory) return EFI_INVALID_PARAMETER;
len = (u64)pages << EFI_PAGE_SHIFT;
len = (ulong)pages << EFI_PAGE_SHIFT; /* Catch possible overflow on 64bit systems */ if (sizeof(efi_uintn_t) == sizeof(u64) && (len >> EFI_PAGE_SHIFT) != (u64)pages)@@ -491,7 +491,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, */ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) {
u64 len;
ulong len;
pages << EFI_PAGE_SHIFT may be more than ULONG_MAX on 32bit systems.
long status; efi_status_t ret;@@ -506,7 +506,7 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return EFI_INVALID_PARAMETER; }
len = (u64)pages << EFI_PAGE_SHIFT;
len = (ulong)pages << EFI_PAGE_SHIFT;
This might overflow on 32bit systems.
/* * The 'memory' variable for sandbox holds a pointer which has already * been mapped with map_sysmem() from efi_allocate_pages(). Convert@@ -525,10 +525,10 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, size_t align) {
u64 req_pages = efi_size_in_pages(len);u64 true_pages = req_pages + efi_size_in_pages(align) - 1;u64 free_pages;u64 aligned_mem;
ulong req_pages = efi_size_in_pages(len);
Please, use efi_uintn_t as this is the parameter type of AllocatePages().
ulong true_pages = req_pages + efi_size_in_pages(align) - 1;ulong free_pages;ulong aligned_mem;
This does not compile on 32bit. efi_allocate_pages expects *u64 as parameter.
efi_status_t r; u64 mem;@@ -572,7 +572,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, efi_status_t r; u64 addr; struct efi_pool_allocation *alloc;
u64 num_pages = efi_size_in_pages(size +
ulong num_pages = efi_size_in_pages(size + sizeof(struct efi_pool_allocation));
The EFI specification defines the argument parameter Pages of AllocatePages() as being UINTN. So we should use efi_uintn_t here.
Best regards
Heinrich
if (!buffer)-- 2.43.0
Hi Heinrich,
On Tue, 26 Nov 2024 at 03:13, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 26.11.24 09:00, Ilias Apalodimas wrote:
Hi Simon,
On Mon, 25 Nov 2024 at 22:45, Simon Glass sjg@chromium.org wrote:
One of the confusing parts of the EFI loader is that it uses u64 for addresses, whereas the rest of U-Boot typically uses ulong.
You are confusing sandbox virtual addresses (phys_addr_t) with the physical addresses (EFI_PHYSICAL_ADDRESS) in the UEFI spec.
No, I am actually trying to resolve the confusion between ulong and void * in the EFI code. Note that phys_addr_t is unrelated to sandbox.
The EFI specification requires an identity mapping. This means the value of a EFI_PHYSICAL_ADDRESS must be usable as a pointer.
Yes, that's right.
The sandbox virtual addresses are not usable as pointers on the sandbox and hence cannot be used for EFI_PHYSICAL_ADDRESS.
EFI_PHYSICAL_ADDRESS is a 64bit type.
The only useful place for sandbox virtual addresses is in the CLI. We should get rid of them anywhere else to avoid further confusion.
This is not correct and this attitude is making it very hard to understand what the EFI loader is doing with memory.
There is a case on 32-bit machines where phys_addr_t can be larger than 32 bits, but this is very much the exception. Also, such machines have mostly faded away and generally don't make use of EFI.
I am not sure how true this statement is with LPAE etc. In any case, I don't want us to convert to ulong, there's no reason to break things for platforms we can't test. So please drop this patch
Thanks /Ilias
So in the interests of consistency, update the memory functions to use ulong in most cases.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_loader.h | 4 ++-- lib/efi_loader/efi_memory.c | 42 ++++++++++++++++++------------------- 2 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 0ef4c6f7dea..cae94fc4661 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -838,7 +838,7 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
- @mem_type: EFI type of memory added
- Return: status code
*/ -efi_status_t efi_add_memory_map(u64 start, u64 size, +efi_status_t efi_add_memory_map(ulong start, ulong size, enum efi_memory_type mem_type);
/** @@ -852,7 +852,7 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
memory- Return: status code
*/ -efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages, enum efi_memory_type mem_type, bool overlap_conventional);
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 2c46915e5b9..4e9c372b622 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -199,10 +199,10 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, { struct efi_mem_list *newmap; struct efi_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;uint64_t carve_end = carve_start +
ulong map_start = map_desc->physical_start;ulong map_end = map_start + (map_desc->num_pages << EFI_PAGE_SHIFT);ulong carve_start = carve_desc->physical_start;ulong carve_end = carve_start + (carve_desc->num_pages << EFI_PAGE_SHIFT); /* check whether we're overlapping */@@ -257,17 +257,17 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_LOOP_AGAIN; }
-efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, +efi_status_t efi_add_memory_map_pg(ulong start, ulong pages, enum efi_memory_type mem_type, bool overlap_conventional) { struct efi_mem_list *lmem; struct efi_mem_list *newlist; bool carve_again;
uint64_t carved_pages = 0;
ulong carved_pages = 0; struct efi_event *evt;
EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__,
EFI_PRINT("%s: 0x%lx 0x%lx %d %s\n", __func__, start, pages, mem_type, overlap_conventional ? "yes" : "no");@@ -370,10 +370,10 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, return EFI_SUCCESS; }
-efi_status_t efi_add_memory_map(u64 start, u64 size, +efi_status_t efi_add_memory_map(ulong start, ulong size, enum efi_memory_type mem_type) {
u64 pages;
ulong pages;The result of efi_size_in_pages() is u64.
pages = efi_size_in_pages(size + (start & EFI_PAGE_MASK));Maybe we should check here that pages <= SIZE_T_MAX.
start &= ~EFI_PAGE_MASK;@@ -396,13 +396,13 @@ efi_status_t efi_add_memory_map(u64 start, u64 size,
- @must_be_allocated: return success if the page is allocated
- Return: status code
*/ -static efi_status_t efi_check_allocated(u64 addr, bool must_be_allocated) +static efi_status_t efi_check_allocated(ulong 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);
ulong start = item->desc.physical_start;ulong end = start + (item->desc.num_pages << EFI_PAGE_SHIFT);Physical start is u64 as defined in the EFI standard. ulong makes no sense here.
My understanding is that a 32-bit machine cannot access 64-bit addresses in U-Boot, so accounting for them makes no sense?
if (addr >= start && addr < end) { if (must_be_allocated ^@@ -420,7 +420,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type mem_type, efi_uintn_t pages, uint64_t *memory) {
u64 efi_addr, len;
ulong efi_addr, len;efi_allocate_pages must return u64. Using ulong for efi_addr does not match.
uint flags; efi_status_t ret; phys_addr_t addr;@@ -430,7 +430,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, return EFI_INVALID_PARAMETER; if (!memory) return EFI_INVALID_PARAMETER;
len = (u64)pages << EFI_PAGE_SHIFT;
len = (ulong)pages << EFI_PAGE_SHIFT; /* Catch possible overflow on 64bit systems */ if (sizeof(efi_uintn_t) == sizeof(u64) && (len >> EFI_PAGE_SHIFT) != (u64)pages)@@ -491,7 +491,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, */ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) {
u64 len;
ulong len;pages << EFI_PAGE_SHIFT may be more than ULONG_MAX on 32bit systems.
long status; efi_status_t ret;@@ -506,7 +506,7 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) return EFI_INVALID_PARAMETER; }
len = (u64)pages << EFI_PAGE_SHIFT;
len = (ulong)pages << EFI_PAGE_SHIFT;This might overflow on 32bit systems.
/* * The 'memory' variable for sandbox holds a pointer which has already * been mapped with map_sysmem() from efi_allocate_pages(). Convert@@ -525,10 +525,10 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, size_t align) {
u64 req_pages = efi_size_in_pages(len);u64 true_pages = req_pages + efi_size_in_pages(align) - 1;u64 free_pages;u64 aligned_mem;
ulong req_pages = efi_size_in_pages(len);Please, use efi_uintn_t as this is the parameter type of AllocatePages().
ulong true_pages = req_pages + efi_size_in_pages(align) - 1;ulong free_pages;ulong aligned_mem;This does not compile on 32bit. efi_allocate_pages expects *u64 as parameter.
efi_status_t r; u64 mem;@@ -572,7 +572,7 @@ efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, efi_status_t r; u64 addr; struct efi_pool_allocation *alloc;
u64 num_pages = efi_size_in_pages(size +
ulong num_pages = efi_size_in_pages(size + sizeof(struct efi_pool_allocation));The EFI specification defines the argument parameter Pages of AllocatePages() as being UINTN. So we should use efi_uintn_t here.
We normally use ulong for size as well as address, in U-Boot.
Regards, SImon
Hi Ilias,
On Tue, 26 Nov 2024 at 01:01, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Mon, 25 Nov 2024 at 22:45, Simon Glass sjg@chromium.org wrote:
One of the confusing parts of the EFI loader is that it uses u64 for addresses, whereas the rest of U-Boot typically uses ulong.
There is a case on 32-bit machines where phys_addr_t can be larger than 32 bits, but this is very much the exception. Also, such machines have mostly faded away and generally don't make use of EFI.
I am not sure how true this statement is with LPAE etc. In any case, I don't want us to convert to ulong, there's no reason to break things for platforms we can't test. So please drop this patch
Yes, I believe LPAE (only on 32-bit machines) is the exception i am aware of. But U-Boot doesn't make use of the MMU for this, right? So do we need to worry about this?
This patch is not essential to the goal here, but I would like to make use of ulong for addresses, since it removes the sort of confusion that we have with EFI at present.
Thanks /Ilias
So in the interests of consistency, update the memory functions to use ulong in most cases.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_loader.h | 4 ++-- lib/efi_loader/efi_memory.c | 42 ++++++++++++++++++------------------- 2 files changed, 23 insertions(+), 23 deletions(-)
[..]
Regards, SImon
The EFI loader's memory implementation is quite confusing. The EFI spec requires that addresses are used in the calls to allocate_pages(), etc. This is unavoidable.
This usage then filters down to various functions within the implementation. This is unfortunate, as there is no clear separation between addresses and pointers. In some parts of the code things become hopelessly confusing, and several bugs have crept in.
Adjust the internal functions to always use a ulong for an address or a void * for a pointer.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/efi_loader.h | 13 +++-- lib/efi_loader/efi_bootmgr.c | 9 ++-- lib/efi_loader/efi_boottime.c | 37 ++++++++------ lib/efi_loader/efi_helper.c | 7 +-- lib/efi_loader/efi_image_loader.c | 2 +- lib/efi_loader/efi_memory.c | 84 +++++++++++++------------------ lib/efi_loader/efi_var_mem.c | 6 +-- 7 files changed, 76 insertions(+), 82 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index cae94fc4661..c4afcf2b60b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -791,7 +791,7 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, */ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type mem_type, - efi_uintn_t pages, uint64_t *memoryp); + efi_uintn_t pages, void **memoryp);
/** * efi_free_pages() - free memory pages @@ -800,7 +800,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, * @pages: number of pages to be freed * Return: status code */ -efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages); +efi_status_t efi_free_pages(void *memory, efi_uintn_t pages);
/** * efi_allocate_pool - allocate memory from pool @@ -833,7 +833,9 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
/** * efi_add_memory_map() - Add a range into the EFI 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 an address, not a pointer. Use map_sysmem(start) to convert to a + * pointer * @size: Size, in bytes * @mem_type: EFI type of memory added * Return: status code @@ -844,8 +846,9 @@ efi_status_t efi_add_memory_map(ulong start, ulong size, /** * 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 an address, not a pointer. Use map_sysmem(start) to convert to a + * pointer * @pages: number of pages to add * @mem_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 8c51a6ef2ed..9e42fa03921 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> @@ -497,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, @@ -506,7 +508,8 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
source_buffer = NULL; source_size = 0; - } 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. @@ -518,7 +521,6 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp, 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"); @@ -1297,8 +1299,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_size_in_pages(fdt_size)); + efi_free_pages(fdt_distro, efi_size_in_pages(fdt_size)); }
if (ret != EFI_SUCCESS) { diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index efd9577c9d0..5ad80ff49cb 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> @@ -431,9 +432,15 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(enum efi_allocate_type type, uint64_t *memoryp) { efi_status_t r; + void *ptr;
+ /* we should not read this unless type indicates it is being used */ + if (type == EFI_ALLOCATE_MAX_ADDRESS || type == EFI_ALLOCATE_ADDRESS) + ptr = (void *)(ulong)*memoryp; EFI_ENTRY("%d, %d, 0x%zx, %p", type, mem_type, pages, memoryp); - r = efi_allocate_pages(type, mem_type, pages, memoryp); + r = efi_allocate_pages(type, mem_type, pages, &ptr); + *memoryp = (ulong)ptr; + return EFI_EXIT(r); }
@@ -455,7 +462,7 @@ static efi_status_t EFIAPI efi_free_pages_ext(uint64_t memory, efi_status_t r;
EFI_ENTRY("%llx, 0x%zx", memory, pages); - r = efi_free_pages(memory, pages); + r = efi_free_pages((void *)(ulong)memory, pages); return EFI_EXIT(r); }
@@ -1951,8 +1958,8 @@ 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 *ptr;
/* Open file */ f = efi_file_from_path(file_path); @@ -1971,17 +1978,17 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path, */ ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, EFI_BOOT_SERVICES_DATA, - efi_size_in_pages(bs), &addr); + efi_size_in_pages(bs), &ptr); if (ret != EFI_SUCCESS) { ret = EFI_OUT_OF_RESOURCES; goto error; }
/* Read file */ - EFI_CALL(ret = f->read(f, &bs, (void *)(uintptr_t)addr)); + EFI_CALL(ret = f->read(f, &bs, ptr)); if (ret != EFI_SUCCESS) - efi_free_pages(addr, efi_size_in_pages(bs)); - *buffer = (void *)(uintptr_t)addr; + efi_free_pages(ptr, efi_size_in_pages(bs)); + *buffer = ptr; *size = bs; error: EFI_CALL(f->close(f)); @@ -2009,9 +2016,10 @@ efi_status_t efi_load_image_from_path(bool boot_policy, struct efi_device_path *dp, *rem; struct efi_load_file_protocol *load_file_protocol = NULL; efi_uintn_t buffer_size; - uint64_t addr, pages; + ulong pages; const efi_guid_t *guid; struct efi_handler *handler; + void *ptr;
/* In case of failure nothing is returned */ *buffer = NULL; @@ -2045,20 +2053,20 @@ efi_status_t efi_load_image_from_path(bool boot_policy, goto out; pages = efi_size_in_pages(buffer_size); ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, EFI_BOOT_SERVICES_DATA, - pages, &addr); + pages, &ptr); if (ret != EFI_SUCCESS) { ret = EFI_OUT_OF_RESOURCES; goto out; } ret = EFI_CALL(load_file_protocol->load_file( load_file_protocol, rem, boot_policy, - &buffer_size, (void *)(uintptr_t)addr)); + &buffer_size, ptr)); if (ret != EFI_SUCCESS) - efi_free_pages(addr, pages); + efi_free_pages(ptr, pages); out: efi_close_protocol(device, guid, efi_root, NULL); if (ret == EFI_SUCCESS) { - *buffer = (void *)(uintptr_t)addr; + *buffer = ptr; *size = buffer_size; }
@@ -2121,8 +2129,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_size_in_pages(source_size)); + efi_free_pages(dest_buffer, efi_size_in_pages(source_size)); if (ret == EFI_SUCCESS || ret == EFI_SECURITY_VIOLATION) { info->system_table = &systab; info->parent_handle = parent_image; @@ -3322,7 +3329,7 @@ close_next: } }
- efi_free_pages((uintptr_t)loaded_image_protocol->image_base, + efi_free_pages(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_helper.c b/lib/efi_loader/efi_helper.c index bf96f61d3d0..1c264eabff9 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -456,7 +456,6 @@ static efi_status_t copy_fdt(void **fdtp) unsigned long fdt_ram_start = -1L, fdt_pages; efi_status_t ret = 0; void *fdt, *new_fdt; - u64 new_fdt_addr; uint fdt_size; int i;
@@ -480,17 +479,15 @@ static efi_status_t copy_fdt(void **fdtp) fdt_size = fdt_pages << EFI_PAGE_SHIFT;
ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, - EFI_ACPI_RECLAIM_MEMORY, fdt_pages, - &new_fdt_addr); + EFI_ACPI_RECLAIM_MEMORY, fdt_pages, &new_fdt); if (ret != EFI_SUCCESS) { log_err("Failed to reserve space for FDT\n"); goto done; } - new_fdt = (void *)(uintptr_t)new_fdt_addr; 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; } diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 0ddf69a0918..9bd77048adf 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -970,7 +970,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(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 4e9c372b622..f687e49c98e 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -418,9 +418,9 @@ static efi_status_t efi_check_allocated(ulong addr, bool must_be_allocated)
efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type mem_type, - efi_uintn_t pages, uint64_t *memory) + efi_uintn_t pages, void **memoryp) { - ulong efi_addr, len; + ulong len; uint flags; efi_status_t ret; phys_addr_t addr; @@ -428,7 +428,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, /* Check import parameters */ if (mem_type >= EFI_PERSISTENT_MEMORY_TYPE && mem_type <= 0x6fffffff) return EFI_INVALID_PARAMETER; - if (!memory) + if (!memoryp) return EFI_INVALID_PARAMETER; len = (ulong)pages << EFI_PAGE_SHIFT; /* Catch possible overflow on 64bit systems */ @@ -447,18 +447,18 @@ 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 = map_to_sysmem(*memoryp); addr = (u64)lmb_alloc_base_flags(len, EFI_PAGE_SIZE, addr, flags); if (!addr) return EFI_OUT_OF_RESOURCES; break; case EFI_ALLOCATE_ADDRESS: - if (*memory & EFI_PAGE_MASK) + addr = map_to_sysmem(*memoryp); + if (addr & 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 = lmb_alloc_addr_flags(addr, len, flags); if (!addr) return EFI_NOT_FOUND; break; @@ -467,42 +467,33 @@ 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, mem_type, true); + ret = efi_add_memory_map_pg(addr, pages, mem_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; + return EFI_OUT_OF_RESOURCES; }
- *memory = efi_addr; + *memoryp = map_sysmem(addr, len);
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) +efi_status_t efi_free_pages(void *memory, efi_uintn_t pages) { - ulong len; + ulong addr, len; long status; efi_status_t ret;
- ret = efi_check_allocated(memory, true); + addr = map_to_sysmem(memory); + ret = efi_check_allocated(addr, true); if (ret != EFI_SUCCESS) return ret;
/* Sanity check */ - if (!memory || (memory & EFI_PAGE_MASK) || !pages) { - printf("%s: illegal free 0x%llx, 0x%zx\n", __func__, - memory, pages); + if (!addr || (addr & EFI_PAGE_MASK) || !pages) { + printf("%s: illegal free %lx, %zx\n", __func__, addr, pages); return EFI_INVALID_PARAMETER; }
@@ -512,12 +503,11 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) * 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(map_to_sysmem(memory), len, LMB_NOOVERWRITE); if (status) return EFI_NOT_FOUND;
- unmap_sysmem((void *)(uintptr_t)memory); + unmap_sysmem(memory);
return ret; } @@ -528,9 +518,9 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, ulong req_pages = efi_size_in_pages(len); ulong true_pages = req_pages + efi_size_in_pages(align) - 1; ulong free_pages; - ulong aligned_mem; + void *aligned_ptr; efi_status_t r; - u64 mem; + void *ptr;
/* align must be zero or a power of two */ if (align & (align - 1)) @@ -542,35 +532,35 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
if (align < EFI_PAGE_SIZE) { r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, - req_pages, &mem); - return (r == EFI_SUCCESS) ? (void *)(uintptr_t)mem : NULL; + req_pages, &ptr); + return (r == EFI_SUCCESS) ? ptr : NULL; }
r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, - true_pages, &mem); + true_pages, &ptr); if (r != EFI_SUCCESS) return NULL;
- aligned_mem = ALIGN(mem, align); + aligned_ptr = (void *)ALIGN((ulong)ptr, align); /* Free pages before alignment */ - free_pages = efi_size_in_pages(aligned_mem - mem); + free_pages = efi_size_in_pages(aligned_ptr - ptr); if (free_pages) - efi_free_pages(mem, free_pages); + efi_free_pages(ptr, free_pages);
/* Free trailing pages */ free_pages = true_pages - (req_pages + free_pages); if (free_pages) { - mem = aligned_mem + req_pages * EFI_PAGE_SIZE; - efi_free_pages(mem, free_pages); + ptr = aligned_ptr + req_pages * EFI_PAGE_SIZE; + efi_free_pages(ptr, free_pages); }
- return (void *)(uintptr_t)aligned_mem; + return aligned_ptr; }
efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, void **buffer) { efi_status_t r; - u64 addr; + void *ptr; struct efi_pool_allocation *alloc; ulong num_pages = efi_size_in_pages(size + sizeof(struct efi_pool_allocation)); @@ -584,9 +574,9 @@ efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, }
r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, num_pages, - &addr); + &ptr); if (r == EFI_SUCCESS) { - alloc = (struct efi_pool_allocation *)(uintptr_t)addr; + alloc = ptr; alloc->num_pages = num_pages; alloc->checksum = checksum(alloc); *buffer = alloc->data; @@ -617,7 +607,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;
@@ -632,7 +622,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(alloc, alloc->num_pages);
return ret; } @@ -788,14 +778,10 @@ int efi_memory_init(void)
#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER /* Request a 32bit 64MB bounce buffer region */ - uint64_t efi_bounce_buffer_addr = 0xffffffff; - if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA, (64 * 1024 * 1024) >> EFI_PAGE_SHIFT, - &efi_bounce_buffer_addr) != EFI_SUCCESS) + &efi_bounce_buffer) != EFI_SUCCESS) return -1; - - efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr; #endif
return 0; diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c index 139e16aad7c..f3f8a25642d 100644 --- a/lib/efi_loader/efi_var_mem.c +++ b/lib/efi_loader/efi_var_mem.c @@ -216,17 +216,17 @@ efi_var_mem_notify_virtual_address_map(struct efi_event *event, void *context)
efi_status_t efi_var_mem_init(void) { - u64 memory; + void *ptr; efi_status_t ret; struct efi_event *event;
ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(EFI_VAR_BUF_SIZE), - &memory); + &ptr); if (ret != EFI_SUCCESS) return ret; - efi_var_buf = (struct efi_var_file *)(uintptr_t)memory; + efi_var_buf = ptr; 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 -
On 25.11.24 21:44, Simon Glass wrote:
The EFI loader's memory implementation is quite confusing. The EFI spec requires that addresses are used in the calls to allocate_pages(), etc. This is unavoidable.
This usage then filters down to various functions within the implementation. This is unfortunate, as there is no clear separation between addresses and pointers. In some parts of the code things become hopelessly confusing, and several bugs have crept in.
Adjust the internal functions to always use a ulong for an address or a void * for a pointer.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_loader.h | 13 +++-- lib/efi_loader/efi_bootmgr.c | 9 ++-- lib/efi_loader/efi_boottime.c | 37 ++++++++------ lib/efi_loader/efi_helper.c | 7 +-- lib/efi_loader/efi_image_loader.c | 2 +- lib/efi_loader/efi_memory.c | 84 +++++++++++++------------------ lib/efi_loader/efi_var_mem.c | 6 +-- 7 files changed, 76 insertions(+), 82 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index cae94fc4661..c4afcf2b60b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -791,7 +791,7 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, */ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type mem_type,
efi_uintn_t pages, uint64_t *memoryp);
efi_uintn_t pages, void **memoryp);/**
- efi_free_pages() - free memory pages
@@ -800,7 +800,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
- @pages: number of pages to be freed
- Return: status code
*/ -efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages); +efi_status_t efi_free_pages(void *memory, efi_uintn_t pages);
Read the UEFI spec, please.
FreePages() expects EFI_PHYSICAL_ADDRESS which is defined as
typedef UINT64 EFI_PHYSICAL_ADDRESS;
Best regards
Heinrich
/**
- efi_allocate_pool - allocate memory from pool
@@ -833,7 +833,9 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
/**
- efi_add_memory_map() - Add a range into the EFI 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 an address, not a pointer. Use map_sysmem(start) to convert to a
- pointer
- @size: Size, in bytes
- @mem_type: EFI type of memory added
- Return: status code
@@ -844,8 +846,9 @@ efi_status_t efi_add_memory_map(ulong start, ulong size, /**
- 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 an address, not a pointer. Use map_sysmem(start) to convert to a
- pointer
- @pages: number of pages to add
- @mem_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 8c51a6ef2ed..9e42fa03921 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> @@ -497,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,
@@ -506,7 +508,8 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
source_buffer = NULL; source_size = 0;
- } 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.
@@ -518,7 +521,6 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp, if (ret != EFI_SUCCESS) goto err;
source_buffer = (void *)image_addr;@@ -1297,8 +1299,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_size_in_pages(fdt_size));
efi_free_pages(fdt_distro, efi_size_in_pages(fdt_size));}
if (ret != EFI_SUCCESS) {
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index efd9577c9d0..5ad80ff49cb 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> @@ -431,9 +432,15 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(enum efi_allocate_type type, uint64_t *memoryp) { efi_status_t r;
void *ptr;
/* we should not read this unless type indicates it is being used */
if (type == EFI_ALLOCATE_MAX_ADDRESS || type == EFI_ALLOCATE_ADDRESS)
ptr = (void *)(ulong)*memoryp;EFI_ENTRY("%d, %d, 0x%zx, %p", type, mem_type, pages, memoryp);
- r = efi_allocate_pages(type, mem_type, pages, memoryp);
- r = efi_allocate_pages(type, mem_type, pages, &ptr);
- *memoryp = (ulong)ptr;
- return EFI_EXIT(r); }
@@ -455,7 +462,7 @@ static efi_status_t EFIAPI efi_free_pages_ext(uint64_t memory, efi_status_t r;
EFI_ENTRY("%llx, 0x%zx", memory, pages);
- r = efi_free_pages(memory, pages);
- r = efi_free_pages((void *)(ulong)memory, pages); return EFI_EXIT(r); }
@@ -1951,8 +1958,8 @@ 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 *ptr;
/* Open file */ f = efi_file_from_path(file_path);
@@ -1971,17 +1978,17 @@ efi_status_t efi_load_image_from_file(struct efi_device_path *file_path, */ ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, EFI_BOOT_SERVICES_DATA,
efi_size_in_pages(bs), &addr);
efi_size_in_pages(bs), &ptr);if (ret != EFI_SUCCESS) { ret = EFI_OUT_OF_RESOURCES; goto error; }
/* Read file */
- EFI_CALL(ret = f->read(f, &bs, (void *)(uintptr_t)addr));
- EFI_CALL(ret = f->read(f, &bs, ptr)); if (ret != EFI_SUCCESS)
efi_free_pages(addr, efi_size_in_pages(bs));- *buffer = (void *)(uintptr_t)addr;
efi_free_pages(ptr, efi_size_in_pages(bs));- *buffer = ptr; *size = bs; error: EFI_CALL(f->close(f));
@@ -2009,9 +2016,10 @@ efi_status_t efi_load_image_from_path(bool boot_policy, struct efi_device_path *dp, *rem; struct efi_load_file_protocol *load_file_protocol = NULL; efi_uintn_t buffer_size;
- uint64_t addr, pages;
ulong pages; const efi_guid_t *guid; struct efi_handler *handler;
void *ptr;
/* In case of failure nothing is returned */ *buffer = NULL;
@@ -2045,20 +2053,20 @@ efi_status_t efi_load_image_from_path(bool boot_policy, goto out; pages = efi_size_in_pages(buffer_size); ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, EFI_BOOT_SERVICES_DATA,
pages, &addr);
pages, &ptr);
&buffer_size, (void *)(uintptr_t)addr));
&buffer_size, ptr));
efi_free_pages(addr, pages);
efi_free_pages(ptr, pages);
*buffer = (void *)(uintptr_t)addr;
*buffer = ptr;@@ -2121,8 +2129,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_size_in_pages(source_size));
efi_free_pages(dest_buffer, efi_size_in_pages(source_size));@@ -3322,7 +3329,7 @@ close_next: } }
- efi_free_pages((uintptr_t)loaded_image_protocol->image_base,
- efi_free_pages(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_helper.c b/lib/efi_loader/efi_helper.c index bf96f61d3d0..1c264eabff9 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -456,7 +456,6 @@ static efi_status_t copy_fdt(void **fdtp) unsigned long fdt_ram_start = -1L, fdt_pages; efi_status_t ret = 0; void *fdt, *new_fdt;
- u64 new_fdt_addr; uint fdt_size; int i;
@@ -480,17 +479,15 @@ static efi_status_t copy_fdt(void **fdtp) fdt_size = fdt_pages << EFI_PAGE_SHIFT;
ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
EFI_ACPI_RECLAIM_MEMORY, fdt_pages,&new_fdt_addr);
EFI_ACPI_RECLAIM_MEMORY, fdt_pages, &new_fdt);
new_fdt = (void *)(uintptr_t)new_fdt_addr; 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; }
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 0ddf69a0918..9bd77048adf 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -970,7 +970,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(efi_reloc, (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 4e9c372b622..f687e49c98e 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -418,9 +418,9 @@ static efi_status_t efi_check_allocated(ulong addr, bool must_be_allocated)
efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type mem_type,
efi_uintn_t pages, uint64_t *memory)
efi_uintn_t pages, void **memoryp)
- ulong efi_addr, len;
- ulong len; uint flags; efi_status_t ret; phys_addr_t addr;
@@ -428,7 +428,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type, /* Check import parameters */ if (mem_type >= EFI_PERSISTENT_MEMORY_TYPE && mem_type <= 0x6fffffff) return EFI_INVALID_PARAMETER;
- if (!memory)
- if (!memoryp) return EFI_INVALID_PARAMETER; len = (ulong)pages << EFI_PAGE_SHIFT; /* Catch possible overflow on 64bit systems */
@@ -447,18 +447,18 @@ 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 = map_to_sysmem(*memoryp);
if (*memory & EFI_PAGE_MASK)
addr = map_to_sysmem(*memoryp);if (addr & 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 = lmb_alloc_addr_flags(addr, len, flags);@@ -467,42 +467,33 @@ 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, mem_type, true);
- ret = efi_add_memory_map_pg(addr, pages, mem_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;
return EFI_OUT_OF_RESOURCES;
- *memory = efi_addr;
*memoryp = map_sysmem(addr, len);
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) +efi_status_t efi_free_pages(void *memory, efi_uintn_t pages) {
- ulong len;
- ulong addr, len; long status; efi_status_t ret;
- ret = efi_check_allocated(memory, true);
addr = map_to_sysmem(memory);
ret = efi_check_allocated(addr, true); if (ret != EFI_SUCCESS) return ret;
/* Sanity check */
- if (!memory || (memory & EFI_PAGE_MASK) || !pages) {
printf("%s: illegal free 0x%llx, 0x%zx\n", __func__,memory, pages);
- if (!addr || (addr & EFI_PAGE_MASK) || !pages) {
printf("%s: illegal free %lx, %zx\n", __func__, addr, pages);@@ -512,12 +503,11 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages) * 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(map_to_sysmem(memory), len, LMB_NOOVERWRITE); if (status) return EFI_NOT_FOUND;
- unmap_sysmem((void *)(uintptr_t)memory);
unmap_sysmem(memory);
return ret; }
@@ -528,9 +518,9 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, ulong req_pages = efi_size_in_pages(len); ulong true_pages = req_pages + efi_size_in_pages(align) - 1; ulong free_pages;
- ulong aligned_mem;
- void *aligned_ptr; efi_status_t r;
- u64 mem;
void *ptr;
/* align must be zero or a power of two */ if (align & (align - 1))
@@ -542,35 +532,35 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type,
if (align < EFI_PAGE_SIZE) { r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
req_pages, &mem);return (r == EFI_SUCCESS) ? (void *)(uintptr_t)mem : NULL;
req_pages, &ptr);return (r == EFI_SUCCESS) ? ptr : NULL;}
r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type,
true_pages, &mem);
true_pages, &ptr);
- aligned_mem = ALIGN(mem, align);
- aligned_ptr = (void *)ALIGN((ulong)ptr, align); /* Free pages before alignment */
- free_pages = efi_size_in_pages(aligned_mem - mem);
- free_pages = efi_size_in_pages(aligned_ptr - ptr); if (free_pages)
efi_free_pages(mem, free_pages);
efi_free_pages(ptr, free_pages);/* Free trailing pages */ free_pages = true_pages - (req_pages + free_pages); if (free_pages) {
mem = aligned_mem + req_pages * EFI_PAGE_SIZE;efi_free_pages(mem, free_pages);
ptr = aligned_ptr + req_pages * EFI_PAGE_SIZE;efi_free_pages(ptr, free_pages);
- return (void *)(uintptr_t)aligned_mem;
return aligned_ptr; }
efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, void **buffer) { efi_status_t r;
- u64 addr;
- void *ptr; struct efi_pool_allocation *alloc; ulong num_pages = efi_size_in_pages(size + sizeof(struct efi_pool_allocation));
@@ -584,9 +574,9 @@ efi_status_t efi_allocate_pool(enum efi_memory_type mem_type, efi_uintn_t size, }
r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, mem_type, num_pages,
&addr);
&ptr);
alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
alloc = ptr;@@ -617,7 +607,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;
@@ -632,7 +622,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(alloc, alloc->num_pages);
return ret; }
@@ -788,14 +778,10 @@ int efi_memory_init(void)
#ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER /* Request a 32bit 64MB bounce buffer region */
- uint64_t efi_bounce_buffer_addr = 0xffffffff;
- if (efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, EFI_BOOT_SERVICES_DATA, (64 * 1024 * 1024) >> EFI_PAGE_SHIFT,
&efi_bounce_buffer_addr) != EFI_SUCCESS)
&efi_bounce_buffer) != EFI_SUCCESS)
efi_bounce_buffer = (void*)(uintptr_t)efi_bounce_buffer_addr; #endif
return 0;
diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c index 139e16aad7c..f3f8a25642d 100644 --- a/lib/efi_loader/efi_var_mem.c +++ b/lib/efi_loader/efi_var_mem.c @@ -216,17 +216,17 @@ efi_var_mem_notify_virtual_address_map(struct efi_event *event, void *context)
efi_status_t efi_var_mem_init(void) {
- u64 memory;
void *ptr; efi_status_t ret; struct efi_event *event;
ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, EFI_RUNTIME_SERVICES_DATA, efi_size_in_pages(EFI_VAR_BUF_SIZE),
&memory);
&ptr);
- efi_var_buf = (struct efi_var_file *)(uintptr_t)memory;
- efi_var_buf = ptr; 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 -
Hi Heinrich,
On Tue, 26 Nov 2024 at 03:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass wrote:
The EFI loader's memory implementation is quite confusing. The EFI spec requires that addresses are used in the calls to allocate_pages(), etc. This is unavoidable.
This usage then filters down to various functions within the implementation. This is unfortunate, as there is no clear separation between addresses and pointers. In some parts of the code things become hopelessly confusing, and several bugs have crept in.
Adjust the internal functions to always use a ulong for an address or a void * for a pointer.
Signed-off-by: Simon Glass sjg@chromium.org
include/efi_loader.h | 13 +++-- lib/efi_loader/efi_bootmgr.c | 9 ++-- lib/efi_loader/efi_boottime.c | 37 ++++++++------ lib/efi_loader/efi_helper.c | 7 +-- lib/efi_loader/efi_image_loader.c | 2 +- lib/efi_loader/efi_memory.c | 84 +++++++++++++------------------ lib/efi_loader/efi_var_mem.c | 6 +-- 7 files changed, 76 insertions(+), 82 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index cae94fc4661..c4afcf2b60b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -791,7 +791,7 @@ void *efi_alloc_aligned_pages(u64 len, enum efi_memory_type mem_type, */ efi_status_t efi_allocate_pages(enum efi_allocate_type type, enum efi_memory_type mem_type,
efi_uintn_t pages, uint64_t *memoryp);
efi_uintn_t pages, void **memoryp);/**
- efi_free_pages() - free memory pages
@@ -800,7 +800,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
- @pages: number of pages to be freed
- Return: status code
*/ -efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages); +efi_status_t efi_free_pages(void *memory, efi_uintn_t pages);
Read the UEFI spec, please.
Again?
FreePages() expects EFI_PHYSICAL_ADDRESS which is defined as
typedef UINT64 EFI_PHYSICAL_ADDRESS;
How does this comment relate to my patch? We should be using pointers in most cases, as is done with allocate_pool.
[..]
Regards, Simon
Now that the EFI-loader uses addresses, we can simplify the code here.
Signed-off-by: Simon Glass sjg@chromium.org ---
lib/lmb.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/lib/lmb.c b/lib/lmb.c index 14b9b8466ff..588787d2a90 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> @@ -448,7 +447,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;
@@ -460,11 +458,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, @@ -474,7 +471,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 function should take a pointer, not an address. Update it along with all users.
Signed-off-by: Simon Glass sjg@chromium.org ---
include/efi_loader.h | 5 ++--- lib/efi_loader/efi_bootbin.c | 3 +-- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_device_path.c | 20 ++++++++++---------- 4 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h index c4afcf2b60b..7f3854a2d6c 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -915,9 +915,8 @@ 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(enum efi_memory_type mem_type, - uint64_t start_address, - size_t size); +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type 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( const struct efi_device_path *dp); 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 9e42fa03921..f435c7225b9 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, - (uintptr_t)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 9c8cd35b97b..025a17b19fa 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,9 +976,8 @@ 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(enum efi_memory_type mem_type, - uint64_t start_address, - size_t size) +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type memory_type, + void *start_ptr, size_t size) { struct efi_device_path_memory *mdp; void *buf, *start; @@ -990,9 +990,9 @@ struct efi_device_path *efi_dp_from_mem(enum efi_memory_type mem_type, mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE; mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY; mdp->dp.length = sizeof(*mdp); - mdp->memory_type = mem_type; - mdp->start_address = start_address; - mdp->end_address = start_address + size; + mdp->memory_type = memory_type; + mdp->start_address = map_to_sysmem(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")) {
Adjust this function to use addresses rather than pointers, so that it doesn't have to use mapmem.
Signed-off-by: Simon Glass sjg@chromium.org ---
lib/efi_loader/efi_dt_fixup.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c index 0dac94b0c6c..ddb0d1f8fce 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;
@@ -21,14 +20,11 @@ const efi_guid_t efi_guid_dt_fixup_protocol = EFI_DT_FIXUP_PROTOCOL_GUID; * @nomap: indicates that the memory range shall not be accessed by the * UEFI payload */ -static void efi_reserve_memory(u64 addr, u64 size, bool nomap) +static void efi_reserve_memory(ulong addr, ulong 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 @@ -36,7 +32,7 @@ static void efi_reserve_memory(u64 addr, u64 size, bool nomap)
ret = efi_add_memory_map(addr, size, type); if (ret != EFI_SUCCESS) - log_err("Reserved memory mapping failed addr %llx size %llx\n", + log_err("Reserved memory mapping failed addr %lx size %lx\n", addr, size); }
Simplify a few expressions in this function.
Signed-off-by: Simon Glass sjg@chromium.org ---
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 f687e49c98e..32c5143e205 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -206,11 +206,11 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, (carve_desc->num_pages << EFI_PAGE_SHIFT);
/* check whether we're overlapping */ - if ((carve_end <= map_start) || (carve_start >= map_end)) + if (carve_end <= map_start || carve_start >= map_end) return EFI_CARVE_NO_OVERLAP;
/* We're overlapping with non-RAM, warn the caller if desired */ - if (overlap_conventional && (map_desc->type != EFI_CONVENTIONAL_MEMORY)) + if (overlap_conventional && map_desc->type != EFI_CONVENTIONAL_MEMORY) return EFI_CARVE_OVERLAPS_NONRAM;
/* Sanitize carve_start and carve_end to lie within our bounds */
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 ---
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 32c5143e205..30bc9803829 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -758,16 +758,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
Apart from the comments on the other patches, the SCT memory tests crash with this series. Please run it locally before sending a v2.
Thanks /Ilias
On Mon, 25 Nov 2024 at 22:45, 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.
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Simon Glass (13): efi: Define fields in struct efi_mem_desc efi_loader: Convert efi_get_memory_map() to return pointers 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 efi_loader: Use the enum for memory type efi_loader: Make more use of ulong efi_loader: Tidy up use of addresses lmb: Reduce mapmem contortions in lmb_map_update_notify() efi_loader: Simplify efi_dp_from_mem() efi_loader: Tidy up efi_reserve_memory() efi_loader: Drop extra brackets in efi_mem_carve_out() efi_loader: Don't try to add sandbox runtime code
include/efi.h | 15 ++ include/efi_api.h | 6 +- include/efi_loader.h | 100 +++++-- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 11 +- lib/efi_loader/efi_boottime.c | 51 ++-- lib/efi_loader/efi_device_path.c | 18 +- lib/efi_loader/efi_dt_fixup.c | 8 +- lib/efi_loader/efi_helper.c | 7 +- lib/efi_loader/efi_image_loader.c | 2 +- lib/efi_loader/efi_memory.c | 246 +++++++----------- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 14 files changed, 251 insertions(+), 238 deletions(-)
-- 2.43.0
Hi Ilias,
On Tue, 26 Nov 2024 at 01:13, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon
Apart from the comments on the other patches, the SCT memory tests crash with this series. Please run it locally before sending a v2.
I did briefly get it running on my laptop, but not for x86_64. I understand from Heinrich that EDK2 doesn't build for any architecture at present.
Which arch are you using? I'd like to be able to run SCT easily, if possible.
Regards, Simon
Thanks /Ilias
On Mon, 25 Nov 2024 at 22:45, 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.
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Simon Glass (13): efi: Define fields in struct efi_mem_desc efi_loader: Convert efi_get_memory_map() to return pointers 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 efi_loader: Use the enum for memory type efi_loader: Make more use of ulong efi_loader: Tidy up use of addresses lmb: Reduce mapmem contortions in lmb_map_update_notify() efi_loader: Simplify efi_dp_from_mem() efi_loader: Tidy up efi_reserve_memory() efi_loader: Drop extra brackets in efi_mem_carve_out() efi_loader: Don't try to add sandbox runtime code
include/efi.h | 15 ++ include/efi_api.h | 6 +- include/efi_loader.h | 100 +++++-- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 11 +- lib/efi_loader/efi_boottime.c | 51 ++-- lib/efi_loader/efi_device_path.c | 18 +- lib/efi_loader/efi_dt_fixup.c | 8 +- lib/efi_loader/efi_helper.c | 7 +- lib/efi_loader/efi_image_loader.c | 2 +- lib/efi_loader/efi_memory.c | 246 +++++++----------- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 14 files changed, 251 insertions(+), 238 deletions(-)
-- 2.43.0
On 25.11.24 21:44, Simon Glass 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
No. It is phys_addr_t that was introduced to handle the sandbox's virtual addresses.
And we should keep this sandbox stuff out of the EFI code. They are only needed for the command line interface.
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.
According to the C specification size of long may be different to the size of void *. uintptr_t is the type that is guaranteed to match the size of void *.
There is no benefit in spreading the ulong abuse any further.
Best regards
Heinrich
Link: https://lore.kernel.org/u-boot/20240725135629.3505072-1-sjg@chromium.org/
Simon Glass (13): efi: Define fields in struct efi_mem_desc efi_loader: Convert efi_get_memory_map() to return pointers 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 efi_loader: Use the enum for memory type efi_loader: Make more use of ulong efi_loader: Tidy up use of addresses lmb: Reduce mapmem contortions in lmb_map_update_notify() efi_loader: Simplify efi_dp_from_mem() efi_loader: Tidy up efi_reserve_memory() efi_loader: Drop extra brackets in efi_mem_carve_out() efi_loader: Don't try to add sandbox runtime code
include/efi.h | 15 ++ include/efi_api.h | 6 +- include/efi_loader.h | 100 +++++-- lib/efi_loader/efi_bootbin.c | 3 +- lib/efi_loader/efi_bootmgr.c | 11 +- lib/efi_loader/efi_boottime.c | 51 ++-- lib/efi_loader/efi_device_path.c | 18 +- lib/efi_loader/efi_dt_fixup.c | 8 +- lib/efi_loader/efi_helper.c | 7 +- lib/efi_loader/efi_image_loader.c | 2 +- lib/efi_loader/efi_memory.c | 246 +++++++----------- lib/efi_loader/efi_var_mem.c | 6 +- .../efi_selftest_startimage_exit.c | 6 +- lib/lmb.c | 10 +- 14 files changed, 251 insertions(+), 238 deletions(-)
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:10, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
No. It is phys_addr_t that was introduced to handle the sandbox's virtual addresses.
And we should keep this sandbox stuff out of the EFI code. They are only needed for the command line interface.
No. My statement is correct and long predates sandbox. U-Boot has always used ulong. Using an address is generally much easier than a pointer, so I believe this design decision was (and remains) a good one. I certainly would not support changing it. The phys_addr_t type has nothing to do with sandbox.
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.
According to the C specification size of long may be different to the size of void *. uintptr_t is the type that is guaranteed to match the size of void *.
There is no benefit in spreading the ulong abuse any further.
U-Boot specifically uses compilers where ulong can hold a pointer. Same with Linux.
Regards, Simon
On Tue, Nov 26, 2024 at 08:39:05AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:10, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
No. It is phys_addr_t that was introduced to handle the sandbox's virtual addresses.
And we should keep this sandbox stuff out of the EFI code. They are only needed for the command line interface.
No. My statement is correct and long predates sandbox. U-Boot has always used ulong. Using an address is generally much easier than a pointer, so I believe this design decision was (and remains) a good one. I certainly would not support changing it. The phys_addr_t type has nothing to do with sandbox.
At the high level, I think parts of the problem are that when U-Boot went from 32bit architectures to 36bit architectures (assorted PowerPC Freescale parts) to 64bit architectures that no, we didn't make some concious and consistent statement that "ulong is for address (except when it's not). If you look at the linux kernel, phys_addr_t is physical addresses, ulong is virtual addresses.
And so phys_addr_t was introduced, iirc, for PowerPC where we had a 32bit architecture with 36bit addresses and had to deal with that.
Hi Tom,
On Tue, 26 Nov 2024 at 09:08, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 08:39:05AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:10, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
No. It is phys_addr_t that was introduced to handle the sandbox's virtual addresses.
And we should keep this sandbox stuff out of the EFI code. They are only needed for the command line interface.
No. My statement is correct and long predates sandbox. U-Boot has always used ulong. Using an address is generally much easier than a pointer, so I believe this design decision was (and remains) a good one. I certainly would not support changing it. The phys_addr_t type has nothing to do with sandbox.
At the high level, I think parts of the problem are that when U-Boot went from 32bit architectures to 36bit architectures (assorted PowerPC Freescale parts) to 64bit architectures that no, we didn't make some concious and consistent statement that "ulong is for address (except when it's not). If you look at the linux kernel, phys_addr_t is physical addresses, ulong is virtual addresses.
And so phys_addr_t was introduced, iirc, for PowerPC where we had a 32bit architecture with 36bit addresses and had to deal with that.
OK, thanks.
I'm a bit unsure what to do here...but am leaning towards having a separate struct for the memory areas in struct efi_mem_list, rather than using the same efi_mem_desc. Then I can have one use addresses and the other pointers.
Regards, Simon
From: Simon Glass sjg@chromium.org Date: Wed, 27 Nov 2024 06:08:10 -0700
Hi Simon,
Hi Tom,
On Tue, 26 Nov 2024 at 09:08, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 08:39:05AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:10, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
No. It is phys_addr_t that was introduced to handle the sandbox's virtual addresses.
And we should keep this sandbox stuff out of the EFI code. They are only needed for the command line interface.
No. My statement is correct and long predates sandbox. U-Boot has always used ulong. Using an address is generally much easier than a pointer, so I believe this design decision was (and remains) a good one. I certainly would not support changing it. The phys_addr_t type has nothing to do with sandbox.
At the high level, I think parts of the problem are that when U-Boot went from 32bit architectures to 36bit architectures (assorted PowerPC Freescale parts) to 64bit architectures that no, we didn't make some concious and consistent statement that "ulong is for address (except when it's not). If you look at the linux kernel, phys_addr_t is physical addresses, ulong is virtual addresses.
And so phys_addr_t was introduced, iirc, for PowerPC where we had a 32bit architecture with 36bit addresses and had to deal with that.
OK, thanks.
I'm a bit unsure what to do here...but am leaning towards having a separate struct for the memory areas in struct efi_mem_list, rather than using the same efi_mem_desc. Then I can have one use addresses and the other pointers.
That does not make sense to me. Why would you want to use pointers to manage what is effectively a memory address map?
Hi Mark,
On Wed, 27 Nov 2024 at 06:52, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Wed, 27 Nov 2024 06:08:10 -0700
Hi Simon,
Hi Tom,
On Tue, 26 Nov 2024 at 09:08, Tom Rini trini@konsulko.com wrote:
On Tue, Nov 26, 2024 at 08:39:05AM -0700, Simon Glass wrote:
Hi Heinrich,
On Tue, 26 Nov 2024 at 02:10, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.11.24 21:44, Simon Glass 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
No. It is phys_addr_t that was introduced to handle the sandbox's virtual addresses.
And we should keep this sandbox stuff out of the EFI code. They are only needed for the command line interface.
No. My statement is correct and long predates sandbox. U-Boot has always used ulong. Using an address is generally much easier than a pointer, so I believe this design decision was (and remains) a good one. I certainly would not support changing it. The phys_addr_t type has nothing to do with sandbox.
At the high level, I think parts of the problem are that when U-Boot went from 32bit architectures to 36bit architectures (assorted PowerPC Freescale parts) to 64bit architectures that no, we didn't make some concious and consistent statement that "ulong is for address (except when it's not). If you look at the linux kernel, phys_addr_t is physical addresses, ulong is virtual addresses.
And so phys_addr_t was introduced, iirc, for PowerPC where we had a 32bit architecture with 36bit addresses and had to deal with that.
OK, thanks.
I'm a bit unsure what to do here...but am leaning towards having a separate struct for the memory areas in struct efi_mem_list, rather than using the same efi_mem_desc. Then I can have one use addresses and the other pointers.
That does not make sense to me. Why would you want to use pointers to manage what is effectively a memory address map?
I sent a series with the idea. Basically it is to use addresses within the efi_memory.c implementation, doing the pointer <-> address conversion in efi_boottime.c
So efi_memory.c has a table with real addresses. The address-pretending-to-be-pointer happens only in efi_boottime.c and doesn't spread any further.
Regards, Simon
participants (5)
-
Heinrich Schuchardt -
Ilias Apalodimas -
Mark Kettenis -
Simon Glass -
Tom Rini