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

[Simon Glass]

Hi Heinrich,

On Wed, 1 Dec 2021 at 22:00, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am 1. Dezember 2021 22:12:09 MEZ schrieb Simon Glass <sjg at chromium.org>:
> >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.
>
> The UEFI API requires u64 for AllocatePages() and GetMemoryMap(). This is a design choice of the API that I don't expect to be corrected in future API versions.
>
> 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.
>
> In U-Boot except for the sandbox pointers is all we need. Other defconfigs neither need nor use a virtual address space.
>
> The only reason why the sandbox uses a virtual address space is ease of user interaction and ease of imlementing unit tests. This is why offsets to the sandbox memory start are passed around instead of addresses (pointers).

Where did you get that idea? U-Boot has always used addresses rather
than pointers. Sandbox happens to take advantage of that, that is all.

>
> Currently the sandbox requirements have spilled to a lot of different code parts. An alternative design could have strictly restricted the offset-address conversions to console input and output.
>
> I don't expect this design to be reverted. But we should not artificially increase the number of places where we use offsets instead of pointers.
>
> If a called function can not use an offset but needs the pointer, passing an offset as a parameter makes no sense.

It is not an offset, it is an address. You are getting tied up in the
sandbox side of this. Casting an address to a pointer should be done
using mapmem, for sandbox compatibility, that's all.

The use of ulong in U-Boot is actually a very nice feature and very
convenient. I very-much oppose any change there. Sandbox is how we do
our testing and is an important part of the code.

So how do you propose to fix the bug in this EFI code? I am not
prepared to change the ACPI interface to use pointers in its
arguments. Almost all the interfaces in U-Boot work the same way, it
just makes no sense.

Regards,
SImon