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

[Heinrich Schuchardt]

On 5/23/26 11:38, Simon Glass wrote:
> Hi Heinrich,
> 
> On 2026-05-21T18:50:22, 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>
>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>>
>> 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
>> @@ -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;
> 
> Making start/end pointers doesn't buy anything - they only feed a
> length calculation and efi_add_memory_map(), which takes a u64 - every
> later use then needs a (uintptr_t) cast, plus (void *) casts on the
> ALIGN_DOWN()/ALIGN() results. Please leave start/end as ulong and only
> convert addr, which is the one that genuinely wants pointer type for
> efi_install_configuration_table()

Yes, using uintptr_t for start and end would eliminate conversions.

> 
>> diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
>> @@ -37,39 +37,49 @@ efi_status_t efi_acpi_register(void)
>> +             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);
> 
> Do you need the (uintptr_t) cast on EFI_PAGE_MASK ?

EFI_PAGE_MASK and EFI_PAGE_SIZE are 64bit wide. uintptr_t and void * are 
only 32bit on 32bit systems. Removing the conversion leads to an error.

> 
> BTW the use of EFI_PAGE_MASK as the alignment argument is
> pre-existing, but looks wrong: ALIGN(x, a) treats a as the alignment
> and internally computes a-1 as the mask, so this becomes ALIGN(x,
> EFI_PAGE_SIZE-1). Worth a separate fix?

You are right, we must use EFI_PAGE_SIZE here.

Best regards

Heinrich

> 
> Regards,
> Simon