[PATCH 1/3] efi_loader: use pointers in efi_acpi_register()

[Ilias Apalodimas]

On Fri, 22 May 2026 at 13:31, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 5/22/26 12:01, Ilias Apalodimas wrote:
> > Hi Heinrich,
> >
> > On Fri, 22 May 2026 at 11:51, Heinrich Schuchardt
> > <heinrich.schuchardt at canonical.com> wrote:
> >>
> >> On 5/22/26 09:50, Ilias Apalodimas wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Thu, 21 May 2026 at 21:50, Heinrich Schuchardt
> >>> <heinrich.schuchardt at canonical.com> wrote:
> >>>>
> >>>> Both efi_add_memory_map() and efi_install_configuration_table() expect
> >>>> pointers and not virtual sandbox addresses.
> >>>
> >>> efi_add_memory_map() expects a PA afaict. EFI is identify mapped so
> >>> that will make no differece, but we can be a bit explicit on the
> >>> commit message
> >>
> >> Virtual sandbox addresses are not virtual addresses in the EFI sense.
> >>
> >> They are offsets in a memory chunked retrieved via mmap().
> >>
> >> On sandbox_defconfig:
> >>
> >> => echo $loadaddr
> >> 0x0
> >>
> >> 0x0 is definitively not the address of the mmap'ed memory chunk. It is
> >> the offset inside the mmap'ed memory chunk.
> >>
> >> We must not use these values in the EFI memory map, ACPI tables, or
> >> anything else an application launched by the sandbox accesses.
> >>
> >>>
> >>>>
> >>>> 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 | 42 ++++++++++++++++++++++++---------------
> >>>>    1 file changed, 26 insertions(+), 16 deletions(-)
> >>>>
> >>>> diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> >>>> index 046986a7518..70cb896ac25 100644
> >>>> --- a/lib/efi_loader/efi_acpi.c
> >>>> +++ b/lib/efi_loader/efi_acpi.c
> >>>> @@ -25,7 +25,7 @@ static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
> >>>>     */
> >>>>    efi_status_t efi_acpi_register(void)
> >>>>    {
> >>>> -       ulong addr, start, end;
> >>>> +       void *addr, *start, *end;
> >>>>           efi_status_t ret;
> >>>>
> >>>>           /*
> >>>> @@ -37,39 +37,49 @@ efi_status_t efi_acpi_register(void)
> >>>>            */
> >>>>           if (IS_ENABLED(CONFIG_BLOBLIST_TABLES)) {
> >>>>                   int size;
> >>>> -               void *tab = bloblist_get_blob(BLOBLISTT_ACPI_TABLES, &size);
> >>>>
> >>>> -               if (!tab)
> >>>> +               addr = bloblist_get_blob(BLOBLISTT_ACPI_TABLES, &size);
> >>>> +
> >>>> +               if (!addr)
> >>>>                           return EFI_NOT_FOUND;
> >>>> -               addr = map_to_sysmem(tab);
> >>
> >> In the previous code:
> >>
> >> `tab` is a pointer which you can use to address memory.
> >> `addr` is an offset to the memory that sandbox has claimed via mmap().
> >>
> >>>>
> >>>> -               ret = efi_add_memory_map(addr, size,
> >>
> >> So instead of passing a memory address we were passing here an offset.
> >> This is wrong.
> >
> > Yea that makes sense. I was was remembering map_to_sysmem() wrong.
> >
> > One nit though since you'll send a v2 for selftests. Can you cast
> > (u64)(uintptr_t)addr instead of (uintptr_t)addr;
>
> C converts function parameters to the expected integer type. (u64) for
> an uintptr_t is a widening conversion on 32bit and a nop on 64bit
> systems. An explicit conversion to (u64) has no extra effect.
>
> Same for assignments.

Ok if it's zero-extended properly, then that's ok

Thanks
/Ilias
>
> This is different on C++, where 32bit uintptr_t needs to be explicitly
> converted to u64.
>
> Best regards
>
> Heinrich
>
> >
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> >
> >>
> >>>
> >>> Why was this wrong? Isn't addr after map_to_sysmem() going to return a
> >>> PA in normal hardware and the whatever sandbox has mapped? I think
> >>> this will break the sandbox case for both cases.
> >>
> >> No, map_to_sysmem() returns an offset to an mmap'ed memory chunk.
> >> But in the EFI memory map we need the addresses that can be used as
> >> pointers.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>> +               ret = efi_add_memory_map((uintptr_t)addr, size,
> >>>>                                            EFI_ACPI_RECLAIM_MEMORY);
> >>>>                   if (ret != EFI_SUCCESS)
> >>>>                           return ret;
> >>>>           } 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);
> >>>> -               ret = efi_add_memory_map(start, end - start,
> >>>> +               start = (void *)ALIGN_DOWN(
> >>>> +                       (uintptr_t)map_sysmem(gd->arch.table_start, 0),
> >>>> +                       (uintptr_t)EFI_PAGE_MASK);
> >>>> +               end = (void *)ALIGN((uintptr_t)map_sysmem(gd->arch.table_end,
> >>>> +                                                         0),
> >>>> +                                   (uintptr_t)EFI_PAGE_MASK);
> >>>> +               ret = efi_add_memory_map((uintptr_t)start,
> >>>> +                                        (uintptr_t)end - (uintptr_t)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);
> >>>> -                       ret = efi_add_memory_map(start, end - start,
> >>>> +                       start = (void *)ALIGN_DOWN(
> >>>> +                               (uintptr_t)map_sysmem(gd->arch.table_start_high,
> >>>> +                                                     0),
> >>>> +                               (uintptr_t)EFI_PAGE_MASK);
> >>>> +                       end = (void *)ALIGN((uintptr_t)map_sysmem(
> >>>> +                                                   gd->arch.table_end_high, 0),
> >>>> +                                           EFI_PAGE_MASK);
> >>>> +                       ret = efi_add_memory_map((uintptr_t)start
> >>>
> >>> [...]
> >>>
> >>> Thanks
> >>> /Ilias
> >>
>