[PATCH v2 01/11] efi_loader: use pointers in efi_acpi_register()

[Simon Glass]

Hi Heinrich,

On 2026-06-21T08:19:04, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
> efi_loader: use pointers in efi_acpi_register()
>
> Both efi_add_memory_map() and efi_install_configuration_table() expect
> pointers and not virtual sandbox addresses.
>
> We need to convert the virtual sandbox addresses in efi_acpi_register() to
> pointers in memory before using them.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>
> lib/efi_loader/efi_acpi.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)

> diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> @@ -37,39 +38,45 @@ efi_status_t efi_acpi_register(void)
>       } else {
>               /* Mark space used for tables */
> -             start = ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK);
> -             end = ALIGN(gd->arch.table_end, EFI_PAGE_MASK);
> +             start = ALIGN_DOWN((uintptr_t)map_sysmem(gd->arch.table_start,

gd->arch.table_start is an address and then you are converting it to a
pointer, which is fine. The confusing thing here is that
efi_add_memory_map() takes u64 arguments which are actually pointers
(in sandbox terms) and has no comment as to what the arguments mean
(in samdbox terms). But we have already had that discssion and I don't
want to reopen it.

I would prefer that you declare start and end as void * so that it is
clear that they are pointers. We should do alignment before the
pointer conversion (sandbox uses 4K-aligned addresses anyway) so
something like:

void *start, end;

start = map_sysmem(ALIGN_DOWN(gd->arch.table_start, EFI_PAGE_MASK), 0);
end = map_sysmem(ALIGN_DOWN(gd->arch.table_end, EFI_PAGE_MASK), 0);
ret = efi_add_memory_map(nomap_to_sysmem(start), end - start,
EFI_ACPI_RECLAIM_MEMORY);

Reading this, we can see that we align the address, convert it to a
pointer and then pass the pointer as an address without doing the
sandbox unmapping. This makes it clear what is going on - and avoids
casts in the C code.

> +                                                      0),
> +                                EFI_PAGE_SIZE);
> +             end = ALIGN((uintptr_t)map_sysmem(gd->arch.table_end, 0),
> +                         EFI_PAGE_SIZE);

This silently changes the alignment argument from EFI_PAGE_MASK to
EFI_PAGE_SIZE. The old code was wrong, but this should be its own
commit or at least mentioned in the commit message.

> diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> @@ -37,39 +38,45 @@ efi_status_t efi_acpi_register(void)
> +             if (gd->arch.table_start_high) {
> +                     start = ALIGN_DOWN(
> +                             (uintptr_t)map_sysmem(gd->arch.table_start_high,
> +                                                   0),
> +                             EFI_PAGE_SIZE);
> +                     end = ALIGN((uintptr_t)map_sysmem(
> +                                         gd->arch.table_end_high, 0),
> +                                 EFI_PAGE_SIZE);

See above for the pattern to use. The nested map_sysmem() calls inside
ALIGN_DOWN()/ALIGN() are hard to read. A pair of local pointer
variables would make this much clearer and avoid the awkward line
breaks. What do you think?

Regards,
Simon