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