[PATCH v2 28/28] efi_loader: Simplify efi_dp_from_mem()

[Heinrich Schuchardt]

On 28.11.24 16:47, Simon Glass wrote:
> This function should take a pointer, not an address. Update it along
> with all users.

This function never was meant to take a sandbox virtual address. It
always expected an EFI_PHYSICAL_ADDRESS which must be convertible to
void * as UEFI uses an identity mapping.

A reasonable commit message would be:

"We can reduce the number of locations where we convert from a pointer
to an integer by passing the address as a pointer instead of an u64."

>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> Changes in v2:
> - Drop patch 'Convert efi_get_memory_map() to return pointers'
> - Drop patch 'efi_loader: Make more use of ulong'
> - Significantly expand and redirect the series
>
>   include/efi_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 4e34e1caede..1269907fa3c 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -919,9 +919,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 98799aead84..e3b8dfb6013 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -515,7 +515,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>   		 * will be freed in return_to_efibootmgr event callback.
>   		 */
>   		loaded_dp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> -					    image_addr, image_size);
> +					    source_buffer, image_size);
>   		ret = efi_install_multiple_protocol_interfaces(
>   			&mem_handle, &efi_guid_device_path, loaded_dp, NULL);
>   		if (ret != EFI_SUCCESS)
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 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)

The parameter used to be an EFI_PHYSICAL_ADDRESS in the UEFI spec terms
which is an UINT64.

> +struct efi_device_path *efi_dp_from_mem(enum efi_memory_type  memory_type,
> +					void *start_ptr, size_t size)

We only can use a pointer here because we do not expect addresses above
4 GiB on 32bit systems.

>   {
>   	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);

An UEFI application loaded by the sandbox does not know anything about
the virtual address space (phys_addr_t) of the sandbox.

The device-tree node must contain addresses that an UEFI application can
use as void *.

map_to_sysmem() is wrong here. You could use

mdp->start_address = (uintptr_t)start_ptr;

Best regards

Heinrich

> +	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")) {