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

[Alexander Graf]

On 14.06.18 23:35, Simon Glass wrote:
> 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.

I tend to disagree with this. Most code in U-Boot actually consumes
pointers rather than addresses.

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

Ok, circling back to square 1. The main issue we're facing here is that
most code in U-Boot does not really understand the difference between
virtual and physical addresses - it just simply assumes a 1:1 map.

With sandbox, we do not have a 1:1 map anymore - any address is
somewhere else than its pointer.

We have a few options:

  1) Deal with pointers at random addresses throughout the code

I can see why you don't want this. I find ASLR generated addresses quite
cumbersome to read as well.

  2) Explicitly map RAM at VA 0x10000000, then do 1:1 map

This would be the best of all worlds still IMHO. That way we will have
easily readable addresses (that are identical in 32bit and 64bit), but
can still do a 1:1 map. The only thing we need to do is to introduce a
linker section at 0x10000000.

  3) Keep converting addresses to pointers

I'm afraid if we follow this path, we will always have arguments on
where the correct boundaries are. If you start to enable debugging in
core dm code and print out pointers to your dm objects, you will get
pointer values today as well. Sooner or later we will always end up with
pointers.


Alex