[PATCH 08/13] efi_loader: Tidy up use of addresses
Heinrich Schuchardt
xypron.glpk at gmx.de
Tue Nov 26 11:16:54 CET 2024
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 at 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;
> 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 -
More information about the U-Boot
mailing list