On Wed, 12 Jul 2017 17:48:16 +0200
"Dr. Philipp Tomsich" <philipp.toms...@theobroma-systems.com> wrote:
> Tom & Maxime,
>
> > On 12 Jul 2017, at 16:34, Tom Rini <tr...@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 <tr...@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 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.pdf
--
Best regards,
Siarhei Siamashka
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
