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

[Heinrich Schuchardt]

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.

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