[PATCH v5 03/23] efi_loader: Add comments where incorrect addresses are used

[Simon Glass]

Hi Ilias,

On Wed, 11 Dec 2024 at 07:48, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Wed, 11 Dec 2024 at 15:54, Simon Glass <sjg at chromium.org> wrote:
> >
> > Some functions are passing addresses instead of pointers to the
> > efi_add_memory_map() function. This confusion is understandable since
> > the function arguments indicate an address.
> >
> > Make a note of the 8 places where there are problems, which would break
> > usage in sandbox tests.
> >
> > Future work will resolve these problems.
>
> NAK, this serves no purpose whatsoever. Just send a patch with
> whatever changes you think are needed

Please see below

>
> Thanks
> /Ilias
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> > A request was made in v4 to drop this patch, but in the same thread, a
> > query was sent about why we need these addresses, so at least until the
> > series is about to be applied, I've left it in, so we can have that
> > discussion in one place and build a shared understanding of why these
> > addresses are needed, instead of peppering the rest of the series with
> > similar questions.

I'm quite happy to drop this patch. But I want to first make sure that
you understand that these bugs exist today in the code, albeit they
only matter on sandbox. You will notice that my future patches don't
change the code mentioned in this file. The bugs are simply resolved
by reworking the EFI-loader memory map to use addresses, instead of
pointers cast to u64

So can you please confirm this makes sense? (if not, let's discuss
it). Then I can drop this patch and the later one.

> >
> > Link: https://lore.kernel.org/u-boot/CAC_iWjLex4Z41CDpx4u07XeCYgjB7H+Ynd8aQYL-wruH7yRmQA@mail.gmail.com/
> >
> > (no changes since v2)
> >
> > Changes in v2:
> > - Add a new patch with comments where incorrect addresses are used
> >
> >  lib/efi_loader/efi_acpi.c    | 2 ++
> >  lib/efi_loader/efi_bootmgr.c | 5 +++++
> >  lib/efi_loader/efi_helper.c  | 1 +
> >  lib/efi_loader/efi_smbios.c  | 6 +++++-
> >  lib/efi_loader/efi_var_mem.c | 2 ++
> >  5 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> > index 67bd7f8ca24..ffb360d461b 100644
> > --- a/lib/efi_loader/efi_acpi.c
> > +++ b/lib/efi_loader/efi_acpi.c
> > @@ -28,12 +28,14 @@ efi_status_t efi_acpi_register(void)
> >         /* Mark space used for tables */
> >         start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK);
> >         end = ALIGN(gd->arch.table_end, EFI_PAGE_MASK);
> > +       /* TODO(sjg): This should use (ulong)map_sysmem(start, ...) */
> >         ret = efi_add_memory_map(start, end - start, EFI_ACPI_RECLAIM_MEMORY);
> >         if (ret != EFI_SUCCESS)
> >                 return ret;
> >         if (gd->arch.table_start_high) {
> >                 start = ALIGN_DOWN(gd->arch.table_start_high, EFI_PAGE_MASK);
> >                 end = ALIGN(gd->arch.table_end_high, EFI_PAGE_MASK);
> > +               /* TODO(sjg): This should use (ulong)map_sysmem(start, ...) */
> >                 ret = efi_add_memory_map(start, end - start,
> >                                          EFI_ACPI_RECLAIM_MEMORY);
> >                 if (ret != EFI_SUCCESS)
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 8c51a6ef2ed..bb635d77b53 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -365,6 +365,8 @@ static efi_status_t prepare_loaded_image(u16 *label, ulong addr, ulong size,
> >          * TODO: expose the ramdisk to OS.
> >          * Need to pass the ramdisk information by the architecture-specific
> >          * methods such as 'pmem' device-tree node.
> > +        *
> > +        * TODO(sjg): This should use (ulong)map_sysmem(addr)
> >          */
> >         ret = efi_add_memory_map(addr, size, EFI_RESERVED_MEMORY_TYPE);
> >         if (ret != EFI_SUCCESS) {
> > @@ -399,6 +401,7 @@ static efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
> >
> >         /* cleanup for iso or img image */
> >         if (ctx->ramdisk_blk_dev) {
> > +               /* TODO(sjg): This should use (ulong)map_sysmem(...) */
> >                 ret = efi_add_memory_map(ctx->image_addr, ctx->image_size,
> >                                          EFI_CONVENTIONAL_MEMORY);
> >                 if (ret != EFI_SUCCESS)
> > @@ -506,6 +509,8 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> >
> >                 source_buffer = NULL;
> >                 source_size = 0;
> > +
> > +       /* TODO(sjg): This does not work on sandbox */
> >         } else if (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
> >                 /*
> >                  * loaded_dp must exist until efi application returns,
> > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > index bf96f61d3d0..fd89ef6036f 100644
> > --- a/lib/efi_loader/efi_helper.c
> > +++ b/lib/efi_loader/efi_helper.c
> > @@ -486,6 +486,7 @@ static efi_status_t copy_fdt(void **fdtp)
> >                 log_err("Failed to reserve space for FDT\n");
> >                 goto done;
> >         }
> > +       /* TODO(sjg): This does not work on sandbox */
> >         new_fdt = (void *)(uintptr_t)new_fdt_addr;
> >         memcpy(new_fdt, fdt, fdt_totalsize(fdt));
> >         fdt_set_totalsize(new_fdt, fdt_size);
> > diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
> > index 8d2ef6deb51..02d4a5dd045 100644
> > --- a/lib/efi_loader/efi_smbios.c
> > +++ b/lib/efi_loader/efi_smbios.c
> > @@ -40,7 +40,11 @@ efi_status_t efi_smbios_register(void)
> >                 return EFI_NOT_FOUND;
> >         }
> >
> > -       /* Mark space used for tables */
> > +       /*
> > +        * Mark space used for tables/
> > +        *
> > +        * TODO(sjg): This should use (ulong)map_sysmem(addr, ...)
> > +        */
> >         ret = efi_add_memory_map(addr, TABLE_SIZE, EFI_RUNTIME_SERVICES_DATA);
> >         if (ret)
> >                 return ret;
> > diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> > index 139e16aad7c..5c69a1e0f3e 100644
> > --- a/lib/efi_loader/efi_var_mem.c
> > +++ b/lib/efi_loader/efi_var_mem.c
> > @@ -226,6 +226,8 @@ efi_status_t efi_var_mem_init(void)
> >                                  &memory);
> >         if (ret != EFI_SUCCESS)
> >                 return ret;
> > +
> > +       /* TODO(sjg): This does not work on sandbox */
> >         efi_var_buf = (struct efi_var_file *)(uintptr_t)memory;
> >         memset(efi_var_buf, 0, EFI_VAR_BUF_SIZE);
> >         efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
> > --
> > 2.34.1
> >