Re: [PATCH 08/40] efi: Correct address handling with ACPI tables

From: Simon Glass <sjg@chromium.org>
To: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: "U-Boot Mailing List" <u-boot@lists.denx.de>,
	"Tom Rini" <trini@konsulko.com>, "Bin Meng" <bmeng.cn@gmail.com>,
	"Marek Behún" <marek.behun@nic.cz>,
	"Alexander Graf" <agraf@csgraf.de>
Subject: Re: [PATCH 08/40] efi: Correct address handling with ACPI tables
Date: Wed, 1 Dec 2021 12:32:54 -0700	[thread overview]
Message-ID: <CAPnjgZ2xPxpfQFWcNHSQnU-ymwqoFs2sh7HfPgj7Mp0CtpMhhw@mail.gmail.com> (raw)
In-Reply-To: <2f4eb944-45ab-a8bd-7f8d-1380f5692410@gmx.de>

Hi Heinrich,

On Wed, 1 Dec 2021 at 11:13, Heinrich Schuchardt <xypron.glpk@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 <sjg@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