[PATCH v2 04/28] efi_loader: Add comments where incorrect addresses are used

[Heinrich Schuchardt]

On 28.11.24 16:47, Simon Glass 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.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> 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)
>   	 */

This cannot work. The EFI memory according to the EFI specification
uses  EFI_PHYSICAL_ADDRESS defined as UINT64.

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

What is not working?

AllocatePages() MUST provide an EFI_PHYSICAL_ADDRESS according to the
UEFI specification.

The UEFI specification further requires an identity mapping. So
EFI_PHYSICAL_ADDRESS variables MUST take a value that can be used as
pointer.


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

No. See above.

> +	 */
>   	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 */

It has been working fine up to now.

Best regards

Heinrich

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