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

[Ilias Apalodimas]

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;

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
>