Hi Alex, On 20 June 2018 at 03:31, Alexander Graf <ag...@suse.de> wrote: > On 06/20/2018 12:03 AM, Simon Glass wrote: >> >> Hi Alex, >> >> On 14 June 2018 at 15:55, Alexander Graf <ag...@suse.de> wrote: >>> >>> >>> On 14.06.18 23:35, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 14 June 2018 at 13:51, Alexander Graf <ag...@suse.de> wrote: >>>>> >>>>> >>>>> On 14.06.18 21:01, Simon Glass wrote: >>>>>> >>>>>> On 14 June 2018 at 12:22, Alexander Graf <ag...@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 <ag...@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 <s...@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. >> >> That might be true within individual components, but addresses are the >> common currency in U-Boot. See for example bootm, all the >> image-handling stuff including FIT, commands, device tree address >> properties. Pointers are more often used at the lowest level of code. > > > Yes, but all counterparts of fs_read and APIs on the same level use > pointers. And please take a few minutes to look at its users. 90% of them > are using pointers too.
Do you mean the callers of fs_read()? That's not the point. Of course there will be conversion from addresses to pointers, but I am trying to keep those at the 'edge', away from the core U-Boot code. Your patch is basically designed to avoid a pointer conversion (which you can't support) in the filesystem layer. Next you will be doing this in networking, display, etc. etc. There is just no need for this change. > > As I mentioned elsewhere already, going from address to pointer is something > reasonable. Going from address to pointer usually a sign of bad design ;). > If most users of fs_read() are already using pointers, it means that either > all of its users are wrong or fs_read() is wrong. I think the latter is the > case. By that logic you would have U-Boot not use addresses at all, and have pointers everywhere. It just isn't the way the code base is today. > > >> >>>> 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. >> >> Traditionally that was true, but over the past 5 years quite a bit of >> code has been converted to work correctly with sandbox. Also, almost >> all *tested* code supports sandbox. > > > I think the difference is between infrastructure and device code. Most > device code is still assuming 1:1 maps, but just doesn't get executed in > sandbox. If you mean the drivers, then yes I agree. The drivers typically get an address and map it to a pointer which they use from then on. I do have some ideas about executing driver code in sandbox. We already build a lot of drivers in sandbox, but execute only a very few. But sandbox could absolutely provide a way to test driver code, with suitable plumbing. Of course, we need address mapping...:-) > >> >>> 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. >> >> I'm not sure what this means. > > > Address Space Layout Randomization. It's what causes these weird looking > addresses ;). No, I mean, I am not sure what your comment means. I know what ASLR is, but I just don't understand your comment as a whole. What problem are you trying to solve? > >> >>> 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. >> >> Linker section or mmap() is essentially the same thing. The former >> will presumably just fail to run (bus error?) if the address is not >> available. The latter will select a similar, available address. > > > No, they're inherently different things. When a process starts, its virtual > address space is basically empty (apart from the vdso and a tiny chunk of > ld). The linker is then responsible to ensure that all application defined > memory regions are mapped. Only after that happened, it will map linked > libraries (such as glibc, SDL, whatever) at random places in address space. > > So with a linker section, we're basically guaranteed to always have memory > live at the same spot. With mmap() there is a good chance we're overlapping > with something that happened to get mapped there in between. I don't think so. The only calls to mmap() in U-Boot are in os.c so we can control use of this <4GB address. From my limited testing with 64-bit machines I always get a huge address for things where I don't specify a hint address. > >> >>> 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. >> >> The addresses should generally be converted just before use. Most APIs >> should use addresses rather than pointers, particularly those related >> to images, loaded data in memory, hardware peripheral addresses and >> booting. > > > I don't think that's a reasonable definition for the API requirements to be > honest. I think addresses make a lot of sense as long as you don't want to > touch any data inside. You basically use them like a token. Once you've gone > past that point - so you've run map_sysmem() on that address - we should > stay in pointer land IMHO. Again, by that logic we would never uses addresses in U-Boot. Everything would be a pointer. Addresses have a number of advantages over pointers: - easy to do arithmetic - don't need casts - easy to printf() without getting massive 16-digit hex strings all the time - can be 32-bits wide in sandbox even on a 64-bit machine Probably some others...you yourself said you cannot do pointer arithmetic without casting to a uintptr_t first. I don't think that is true, but it is indicative of the messiness of dealing with pointers. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
