[U-Boot] [PATCH v8 12/30] sandbox: Try to start the RAM buffer at a particular address

[Simon Glass]

Hi Alex,

On 22 June 2018 at 06:10, Alexander Graf <agraf at suse.de> wrote:
> On 06/21/2018 09:45 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 21 June 2018 at 03:58, Alexander Graf <agraf at suse.de> wrote:
>>>
>>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 20 June 2018 at 02:51, Alexander Graf <agraf at suse.de> wrote:
>>>>>
>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote:
>>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> On 18 June 2018 at 08:45, Alexander Graf <agraf at suse.de> wrote:
>>>>>>>
>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> Use a starting address of 256MB which should be available. This
>>>>>>>> helps
>>>>>>>> to
>>>>>>>> make sandbox RAM buffers pointers more recognisable.
>>>>>>>>
>>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>>>
>>>>>>>
>>>>>>> Nak, this has a non-0 chance of failing, in case something else is
>>>>>>> already
>>>>>>> mapped at that address. You don't want to have your CI blow up 1% of
>>>>>>> the
>>>>>>> time due to this.
>>>>>>
>>>>>> It's just a hint though. Everything will still work if it doesn't get
>>>>>> this exact address.
>>>>>
>>>>>
>>>>> I don't see what it buys us then.
>>>>
>>>> These are my thoughts:
>>>>
>>>> 1. We get an address before 4GB which is needed for grub (so far as I
>>>> can
>>>> tell)
>>>
>>>
>>> We only need that in the memory map which you want virtual (U-Boot
>>> address
>>> space) anyway. So there's no need to also have the Linux address be <4GB.
>>
>> Grub cannot work without <4GB, right? I don't mind either way, but I
>> think it that if we are picking an address, picking a smaller one is
>> better.
>
>
> Only if you expose host pointers as memory addresses. We don't anymore, and
> grub doesn't care whether a pointer is >4G.

We have to provide grub with pointers to memory with sandbox. These
will be pointers into the sandbox RAM buffer. If this buffer is >=4GB
then grub will not work, from my testing.

>
>>
>>>> 2. It will normally be a nice, easy-on-the-eyes address
>>>
>>>
>>> I'd rather say it's going to confuse people, because now it's easy to
>>> mistake for a U-Boot address.
>>
>> That's possibly true, although at present it is outside the range of
>> valid U-Boot addresses. What do you suggest?
>
>
> I see little reason for change. Also if you want to change it, it's out of
> scope of the efi_loader enablement ;).

Yes it can be dealt with separately.

>
>>
>>>> 3. If it isn't, it will hopefully still be an address below 4GB
>>>> 4. Even if not (e.g. on some very strange system) it will still allow
>>>> sandbox to start up
>>>>
>>>> But anyway, as I said, this will never fail, it is just a hint. So I
>>>> think your original comment is incorrect.
>>>
>>>
>>> It will not fail, it will just potentially not map at exactly the spot
>>> you
>>> wanted the map at, which IMHO would cause even more confusion.
>>>
>>> Imagine people now start to assume that %p output is stable between
>>> invocations - because it is 99% of the cases. They start to integrate
>>> that
>>> into their CI suites and suddenly things fail 1% of the time because the
>>> linker figured today's a great day to map glibc at that address.
>>
>> Remember these are internal sandbox addresses. They should not be
>> visible to most of the code and it is very hard for me to imagine a
>> test that could fail because the address is mapped in different
>> places. How would you access the address from a test? Are you thinking
>> of something that would assume the ram buffer address is something in
>> particular and then call map_to_sysmem() on the pointer? I am really
>> not sure that we should worry about that too much.
>
>
> No, I'm mostly worried about people assuming that 2 invocations of a U-Boot
> binary give them the same console output, even when there are printf()s that
> contain a %p.

We should NEVER printf a %p in code that might be used with sandbox.
That is another point I should have made. U-Boot (including sandbox)
uses addresses and we should print addresses, not pointers.

Regards,
Simon