[U-Boot] [PATCH v2 10/11] efi_loader: Pass address to fs_read()

[Simon Glass]

Hi Alex,

On 14 June 2018 at 13:51, Alexander Graf <agraf at suse.de> wrote:
>
>
> On 14.06.18 21:01, Simon Glass wrote:
>> On 14 June 2018 at 12:22, Alexander Graf <agraf at suse.de> wrote:
>>> The fs_read() function wants to get an address rather than the
>>> pointer to a buffer.
>>>
>>> So let's convert the passed buffer from pointer back a the address
>>> to make efi_loader on sandbox happier.
>>>
>>> Signed-off-by: Alexander Graf <agraf at suse.de>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>>   - Clarify address vs pointer
>>>   - include mapmem.h
>>> ---
>>>  lib/efi_loader/efi_file.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>
> I actually think that this patch tackles the problem the wrong way
> around. I've cooked up another one that converts fs_read() and
> fs_write() to instead take a pointer - which really is what most users
> of the API want in the first place:
>
>
> https://github.com/agraf/u-boot/commit/eb89f036a42cea8d7aaa6d83b8ecd9d202814b0f

Actually this is a pretty good exposition of why we don't want to use
pointers everywhere. U-Boot uses addresses all over the place. Even a
search for something as specific as "ulong addr" gets over 200
matches. If we go down this path we will end up changing a pretty
fundamental part of U-Boot, and I don't think it is a good idea to do
that. Addresses are easy to understand, can be added/subtracted
without pointer arithmetic, can be easily converted to pointers as
needed, can be 32-bits long on sandbox, etc.

So I think we should step back from the abyss here and stick with the
way sandbox currently deals with addresses. It works well, is pretty
easy to understand, allows debugging which is just as easy on sandbox
as other archs (since they both uses addresses until basically the
final access), the addresses are typically small values that start at
0 which much is easier to read than (e.g.) 00007f1b58c8b008.

Here for example is the output from starting U-Boot with debugging in
board_f.c (something I have turned on a lot when refactoring and
debugging the init sequence):

U-Boot 2018.07-rc1-00142-g134ea86c7f-dirty (Jun 14 2018 - 15:04:04 -0600)

DRAM:  Monitor len: 00395AB0
Ram size: 08000000
Ram top: 08000000
Reserving 3670k for U-Boot at: 07c6a000
Reserving 32776k for malloc() at: 05c68000
Reserving 120 Bytes for Board Info at: 05c67f88
Reserving 472 Bytes for Global Data at: 05c67db0
Reserving 4352 Bytes for FDT at: 05c66cb0
Reserving 0x3c8 Bytes for bootstage at: 05c668e8

RAM Configuration:
Bank #0: 0 128 MiB

DRAM:  128 MiB
New Stack Pointer is: 05c668d0
Copying bootstage from 00007fdb0056e038 to 00007fdb061c48f0, size 3c8
Relocation Offset is: 07c6a000
Relocating to 07c6a000, new gd at 05c67db0, sp at 05c668d0


If we use pointers we get things like this:

Reserving 3670k for U-Boot at: 00007f1b58c8b008

I just don't want to deal with 64-bit addresses when debugging things,
and there really is no point. The map_sysmem() function provides a
nice and easy way to cast an address to a pointer, and it works on all
architectures.

Regards,
Simon