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