On Fri, 22 May 2026 at 13:31, Heinrich Schuchardt <[email protected]> wrote: > > On 5/22/26 12:01, Ilias Apalodimas wrote: > > Hi Heinrich, > > > > On Fri, 22 May 2026 at 11:51, Heinrich Schuchardt > > <[email protected]> wrote: > >> > >> On 5/22/26 09:50, Ilias Apalodimas wrote: > >>> Hi Heinrich, > >>> > >>> On Thu, 21 May 2026 at 21:50, Heinrich Schuchardt > >>> <[email protected]> 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 <[email protected]> > >>>> --- > >>>> 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 <[email protected]> > > > >> > >>> > >>> 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 > >> >
