From: "Dr. Philipp Tomsich" philipp.tomsich@theobroma-systems.com Date: Wed, 12 Jul 2017 20:26:13 +0200
On 12 Jul 2017, at 20:07, Siarhei Siamashka siarhei.siamashka@gmail.com wrote:
On Wed, 12 Jul 2017 17:48:16 +0200 "Dr. Philipp Tomsich" philipp.tomsich@theobroma-systems.com wrote:
Tom & Maxime,
On 12 Jul 2017, at 16:34, Tom Rini trini@konsulko.com wrote:
On Wed, Jul 12, 2017 at 04:20:52PM +0200, Maxime Ripard wrote:
On Tue, Jul 11, 2017 at 07:59:21PM +0200, Dr. Philipp Tomsich wrote:
Maxime,
> On 11 Jul 2017, at 18:59, Tom Rini trini@konsulko.com wrote: > > On Tue, Jul 11, 2017 at 06:54:55PM +0200, Maxime Ripard wrote: >> Hi, >> >> I recently got a gcc 7.1 based toolchain, and it seems like it >> generates unaligned code, specifically in the net_set_ip_header >> function in my case. >> >> Whenever some packet is sent, this data abort is triggered: >> >> => setenv ipaddr 10.42.0.1; ping 10.42.0.254 >> using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in >> MAC de:ad:be:ef:00:01 >> HOST MAC de:ad:be:af:00:00 >> RNDIS ready >> musb-hdrc: peripheral reset irq lost! >> high speed config #2: 2 mA, Ethernet Gadget, using RNDIS >> USB RNDIS network up! >> Using usb_ether device >> data abort >> pc : [<7ff9db10>] lr : [<7ff9f00c>] >> reloc pc : [<4a043b10>] lr : [<4a04500c>] >> sp : 7bf37cc8 ip : 00000000 fp : 7ff6236c >> r10: 7ffed2b8 r9 : 7bf39ee8 r8 : 7ffed2b8 >> r7 : 00000001 r6 : 00000000 r5 : 0000002a r4 : 7ffed30e >> r3 : 14000045 r2 : 01002a0a r1 : fe002a0a r0 : 7ffed30e >> Flags: nZCv IRQs off FIQs off Mode SVC_32 >> Resetting CPU ... >> >> >> Running objdump on it gives us this: >> >> 4a043b04 <net_set_ip_header>: >> >> /* >> * Construct an IP header. >> */ >> /* IP_HDR_SIZE / 4 (not including UDP) */ >> ip->ip_hl_v = 0x45; >> 4a043b04: e59f3074 ldr r3, [pc, #116] ; 4a043b80 <net_set_ip_header+0x7c> >> { >> 4a043b08: e92d4013 push {r0, r1, r4, lr} >> 4a043b0c: e1a04000 mov r4, r0 >> ip->ip_hl_v = 0x45; >> 4a043b10: e5803000 str r3, [r0] <---- Abort >> ip->ip_tos = 0; >> ip->ip_len = htons(IP_HDR_SIZE); >> ip->ip_id = htons(net_ip_id++); >> 4a043b14: e59f3068 ldr r3, [pc, #104] ; 4a043b84 <net_set_ip_header+0x80> >> >> It seems like r0 is indeed set to an unaligned address (0x7ffed30e) >> for some reason. >> >> Using a Linaro 6.3 toolchain works on the same commit with the same >> config, so it really seems to be a compiler-related issue. >> >> It generates this code: >> >> 4a043ec4 <net_set_ip_header>: >> >> /* >> * Construct an IP header. >> */ >> /* IP_HDR_SIZE / 4 (not including UDP) */ >> ip->ip_hl_v = 0x45; >> 4a043ec4: e3a03045 mov r3, #69 ; 0x45 >> { >> 4a043ec8: e92d4013 push {r0, r1, r4, lr} >> 4a043ecc: e1a04000 mov r4, r0 >> ip->ip_hl_v = 0x45; >> 4a043ed0: e5c03000 strb r3, [r0] >> ip->ip_tos = 0; >> ip->ip_len = htons(IP_HDR_SIZE); >> 4a043ed4: e3a03b05 mov r3, #5120 ; 0x1400 >> ip->ip_tos = 0; >> 4a043ed8: e3a00000 mov r0, #0 >> ip->ip_len = htons(IP_HDR_SIZE); >> 4a043edc: e1c430b2 strh r3, [r4, #2] >> ip->ip_id = htons(net_ip_id++); >> 4a043ee0: e59f3064 ldr r3, [pc, #100] ; 4a043f4c <net_set_ip_header+0x88> >> >> And it seems like it's using an strb instruction to avoid the >> unaligned access. >> >> As far as I know, we are passing --wno-unaligned-access, so the broken >> situation should not arise, and yet it does, so I'm a bit confused, >> and not really sure what to do from there. > > Can you reduce the code into a testcase? I think the first step is > filing a bug with gcc and seeing where it goes from there as yes, we > should be passing -mno-unaligned-access.
I don’t think that this is a GCC bug, as “-mno-unaligned-access” will change the behaviour for packed data-structures only. Here’s from the GCC docs:
> -munaligned-access > -mno-unaligned-access > > Enables (or disables) reading and writing of 16- and 32- bit > values from addresses that are not 16- or 32- bit aligned. By > default unaligned access is disabled for all pre-ARMv6, all > ARMv6-M and for ARMv8-M Baseline architectures, and enabled for > all other architectures. If unaligned access is not enabled then > words in packed data structures are accessed a byte at a time.
The key word seems to be “in packed data structures”. However, I don’t see an attribute “packed” for the 'struct ip_udp_hdr’ (in include/net.h).
Could you try to verify that the error reproduces with a packed variant of the ‘struct ip_udp_hdr’?
It indeed fixed the issue. There might just have been a subtle change of behaviour in GCC, and this is probably going to bite us in other areas.
I'll send a patch to add the packed attribute.
Please bear in mind that packed should be used carefully. We've had some discussions about this before and have doc/README.unaligned-memory-access.txt which may need a little more updating now as well, depending on what the final resolution here is as I seem to recall some other problem reports with gcc-7.x, but I've not personally been able to hit these just yet. But I need to get on that soon. http://toolchains.free-electrons.com/ is awesome and I eagerly await gcc-7.x toolchains there if I can't get something else spun up in a chroot soon :)
So there’s the remaining question of how to fix this permanently: — with my compiler engineering hat on, I’d recommend to mark this as a packed struct, as that’s what it is: the compiler needs to keep it packed, because that is how it is received/sent on the wire — rereading the doc/README.unaligned-memory-access.txt, the preferred solution in U-Boot would be to use put/get_unaligned to access these fields (although I have concerns with this—see below).
I honestly wonder if the recommendation (to avoid ‘packed’) from the README is appropriate for many of the data structure declarations in U-Boot which deal with the external representation of data (i.e. DMA descriptors, memory-mapped register files and data sent on a wire): the C language does not offer any protection against a compiler adding patting between structure members, as it sees fit. The original wording from the standard is: 14 Each non-bit-field member of a structure or union object is aligned in an implementation-defined manner appropriate to its type. 15 Within a structure object, the non-bit-field members and the units in which bit-fields reside have addresses that increase in the order in which they are declared. A pointer to a structure object, suitably converted, points to its initial member (or if that member is a bit-field, then to the unit in which it resides), and vice versa. There may be unnamed padding within a structure object, but not at its beginning.
In other words: there’s nothing in the standard from stopping a compiler to insert additional padding within structures, unless the ‘packed’ attribute is added.
I would strongly advise against adding the "packed" attribute everywhere unnecessarily. This just makes the code bigger and slower.
The intent was not to add it everywhere, but only to choose this solution when encountering cases like this one: i.e. cases where the source code was risking future trouble.
Clearly we can rely on ABI documents when we have code that targets a specific architecture (e.g the arch/arm), but should not do so in the architecture-independent code.
Well, all ABIs I'm familliar with are "compatible" with some common sense rules about padding and alignment. Basically if one assumes natural alignment for all the types, and avoids holes in the struct, it should be fine to not mark the struct as "packed". However, one must make sure that pointers to such structs are always properly aligned.
The IP header case is notorious. The Ethernet header is 20 bytes. The IP header immediately follows te Ethernet header. So if you receive packets in buffers that are aligned, the IP header will be misaligned. The common solution is to receive packets in deliberately misaligned buffers. Not all hardware supports that though. Which means the packet has to be copied.
It doesn't help that the dominant x86 architecture doesn't really care about alignment. So people keep writing code that doesn't consider proper alignment of packet buffers. Sadly ARM has followed the x86 example and now handles misaligned loads and stores in hardware as well on ARMv7 and ARMv8.
And clearly there’s also the aspect of needing the ‘packed’ to enable GCC to process the unaligned data elements with “-mno-unaligned-accesses”.
The ANSI/ISO C language standard indeed leaves a lot up to the implementation. But we also have ABI documents for each platform, which cover all of these details. We just need to use them.
In the case of 32-bit ARM, it is http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042e/IHI0042E_aapcs.pdf
In the case of 64-bit ARM, it is http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.p...
Given that the common header files should be used across architectures (and also future architectures, once someone decides to port U-Boot there), we shouldn’t assume that the ABI documents for each platform will define this in such a way that we can rely on.
I'm not sure these "alignment" semantics of the "packed" attribute can be relied on. In the past those semantics have been unreliable. The best way to avoid unaligned access is to make sure your packets are properly (mis)aligned.