Hi Heinrich, On Wed, 1 Dec 2021 at 11:13, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > On 12/1/21 17:02, Simon Glass wrote: > > The current EFI implementation confuses pointers and addresses. Normally > > we can get away with this but in the case of sandbox it causes failures. > > > > Despite the fact that efi_allocate_pages() returns a u64, it is actually > > a pointer, not an address. Add special handling to avoid a crash when > > running 'bootefi hello'. > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > --- > > > > lib/efi_loader/efi_acpi.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c > > index 016bbf6db33..9d101aa843e 100644 > > --- a/lib/efi_loader/efi_acpi.c > > +++ b/lib/efi_loader/efi_acpi.c > > @@ -8,6 +8,7 @@ > > #include <common.h> > > #include <efi_loader.h> > > #include <log.h> > > +#include <mapmem.h> > > #include <acpi/acpi_table.h> > > > > static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID; > > @@ -22,6 +23,7 @@ efi_status_t efi_acpi_register(void) > > /* Map within the low 32 bits, to allow for 32bit ACPI tables */ > > u64 acpi = U32_MAX; > > efi_status_t ret; > > + ulong addr; > > > > /* Reserve 64kiB page for ACPI */ > > ret = efi_allocate_pages(EFI_ALLOCATE_MAX_ADDRESS, > > @@ -34,7 +36,8 @@ efi_status_t efi_acpi_register(void) > > * a 4k-aligned address, so it is safe to assume that > > * write_acpi_tables() will write the table at that address. > > */ > > - write_acpi_tables((ulong)acpi); > > + addr = map_to_sysmem((void *)(ulong)acpi); > > Please, don't pollute general code with sandbox specific stuff where > this can be avoided. > > write_acpi_tables() anyway converts to a pointer. We should not convert > twice. Correct the parameter of write_acpi_tables() instead to expect a > pointer.
I don't want to use pointers to pass addresses. It is not the common approach in U-Boot. We use ulong to pass addresses. Re the need for this change, I beg to differ, and the crash shows that I am right :-) Don't you think it is odd that the EFI allocation routine returns a u64 instead of a void *, like malloc()? If that were fixed, then this code would make more sense. I think we talked about it before. Regards, Simon