Hi Alex, On 22 June 2018 at 06:10, Alexander Graf <[email protected]> wrote: > On 06/21/2018 09:45 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 21 June 2018 at 03:58, Alexander Graf <[email protected]> wrote: >>> >>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 20 June 2018 at 02:51, Alexander Graf <[email protected]> wrote: >>>>> >>>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>> On 18 June 2018 at 08:45, Alexander Graf <[email protected]> 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 <[email protected]> >>>>>>> >>>>>>> >>>>>>> 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 _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
