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

[Heinrich Schuchardt]

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

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.

Best regards

Heinrich

>
>Regards,
>Simon