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. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
