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

[Simon Glass]

Hi Heinrich,

On Thu, 28 Nov 2024 at 10:31, Heinrich Schuchardt <xypron.glpk at 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 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;

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

[..]

Regards,
Simon