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

[Ilias Apalodimas]

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

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.
>
> 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
>