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

Hi Heinrich,

On Thu, 28 Nov 2024 at 10:31, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

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@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;

OK, so this is actually a pointer, by the sound of it. I'll update the comment.

[..]

Regards, Simon