[PATCH 07/40] efi: Correct call to write_acpi_tables()

[Simon Glass]

Hi Heinrich,

On Wed, 1 Dec 2021 at 12:57, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> On 12/1/21 20:32, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 1 Dec 2021 at 11:08, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>
> >> On 12/1/21 17:02, Simon Glass wrote:
> >>> This must be passed a ulong, not a u64. Fix it to avoid LTO warnings on
> >>> sandbox.
> >>>
> >>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>> ---
> >>>
> >>>    lib/efi_loader/efi_acpi.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> >>> index a62c34009cc..016bbf6db33 100644
> >>> --- a/lib/efi_loader/efi_acpi.c
> >>> +++ b/lib/efi_loader/efi_acpi.c
> >>> @@ -34,7 +34,7 @@ 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(acpi);
> >>> +     write_acpi_tables((ulong)acpi);
> >>
> >> This is wrong: acpi is not an address in the virtual address space of
> >> the sandbox. You must not pass it to use map_sysmem() to convert it to a
> >> pointer.
> >
> > I think you are referring to the other patch.
> >
> > Here the problem is that a u64 is being passed to a ulong. As you say,
> > this should be a global prototype and I will fix that in v2.
> >
> >>
> >> Please change the parameter of write_acpi_tables() to be a pointer and
> >> move all sandbox specific conversions to the sandbox code.
> >
> > Sorry, no, I don't want to do that as it would be a significant
> > departure from how U-Boot currently works. We use ulong everywhere for
> > addresses.
>
> We typically use pointers to pass references to memory and not ulong.

(again, I think this discussion relates to the other patch)

That is true in some internal code, but more often a ulong is used for
an address, e.g. with the boot flow, commands, ACPI, board init, etc.
etc. The ulong is the standard holder of an address in U-Boot and ACPI
uses addresses. Haven't we been through this before?

> Relative addresses in the virtual address space of the sandbox are only
> relevant for testing on and user interaction with the sandbox.
>
> The location of the memory location for the ACPI table is not determined
> by the user.
>
> Please, have a look at the called function where the sandbox address is
> immediately converted back to a pointer. The parameter value is only
> used in a debug statement. This double conversion makes is a waste of
> memory and CPU cycles. Even for the debug output on the sandbox I would
> go for a pointer because that is the value that I will need to
> understand what is happening in the booted image.

I think you are over-indexing on sandbox. The first thing to explain
is why a u64 is used to hold what I believe to be a void pointer. IMO
the API is not right. Or perhaps just its interaction with sandbox is
wrong. But somehow, we need to fix the bug in the other patch.

The only thing that is useful to see with sandbox is the address. The
pointers are random numbers really, not easily related to the
addresses that they came from. For example, with 'md 0', I want to see
the address '0', not the pointer value which might be 0x33d27120000 or
anything else.

Regards,
Simon