[U-Boot] Data Abort with gcc 7.1

Peter Robinson pbrobinson at gmail.com
Thu Jul 13 10:20:34 UTC 2017


On Thu, Jul 13, 2017 at 11:11 AM, Måns Rullgård <mans at mansr.com> wrote:
> Siarhei Siamashka <siarhei.siamashka at gmail.com> writes:
>
>> On Thu, 13 Jul 2017 01:43:37 +0100
>> Måns Rullgård <mans at mansr.com> wrote:
>>
>>> Maxime Ripard <maxime.ripard at free-electrons.com> writes:
>>>
>>> > 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.
>>>
>>> For reference, this is the relevant code:
>>>
>>> struct ip_udp_hdr {
>>>      u8              ip_hl_v;        /* header length and version    */
>>>      u8              ip_tos;         /* type of service              */
>>>      u16             ip_len;         /* total length                 */
>>>      u16             ip_id;          /* identification               */
>>>      u16             ip_off;         /* fragment offset field        */
>>>      u8              ip_ttl;         /* time to live                 */
>>>      u8              ip_p;           /* protocol                     */
>>>      u16             ip_sum;         /* checksum                     */
>>>      struct in_addr  ip_src;         /* Source IP address            */
>>>      struct in_addr  ip_dst;         /* Destination IP address       */
>>>      u16             udp_src;        /* UDP source port              */
>>>      u16             udp_dst;        /* UDP destination port         */
>>>      u16             udp_len;        /* Length of UDP packet         */
>>>      u16             udp_xsum;       /* Checksum                     */
>>> };
>>>
>>> void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr source)
>>> {
>>>      struct ip_udp_hdr *ip = (struct ip_udp_hdr *)pkt;
>>>
>>>      /*
>>>       *      Construct an IP header.
>>>       */
>>>      /* IP_HDR_SIZE / 4 (not including UDP) */
>>>      ip->ip_hl_v  = 0x45;
>>>      ip->ip_tos   = 0;
>>>      ip->ip_len   = htons(IP_HDR_SIZE);
>>>      ip->ip_id    = htons(net_ip_id++);
>>>      ip->ip_off   = htons(IP_FLAGS_DFRAG);   /* Don't fragment */
>>>      ip->ip_ttl   = 255;
>>>      ip->ip_sum   = 0;
>>>      /* already in network byte order */
>>>      net_copy_ip((void *)&ip->ip_src, &source);
>>>      /* already in network byte order */
>>>      net_copy_ip((void *)&ip->ip_dst, &dest);
>>> }
>>>
>>> What's changed with gcc7 is that it now merges the writes to the first
>>> three fields (occupying 4 bytes) into a single 32-bit store.  There is
>>> nothing wrong with this.
>>>
>>> Now struct ip_udp_hdr includes 32-bit fields, so it's natural alignment
>>> is 4 bytes.  If the 'pkt' argument is not adequately aligned, the cast
>>> in the first line of the function becomes invalid.  Judging by the
>>> register dump and disassembly, the pointer is indeed not thusly
>>> aligned, and this is an error.
>>
>> Yes, your analysis appears to be the exact copy of mine up to this
>> point: https://lists.denx.de/pipermail/u-boot/2017-July/298016.html
>>
>>> The misaligned pointer should still not cause a hardware abort, however,
>>> on ARMv6 or later which permits unaligned addresses with the STR
>>> instruction.  That is unless some idiot has gone and enabled strict
>>> alignment checking again despite this being against all common sense.
>>> There was a lengthy argument about this a few years ago, ultimately
>>> resulting in the proper settings being put in place.
>>
>> The trick is that unaligned memory accesses still need some special
>> setup even on ARMv6 or later hardware. And we can't always count on
>> having it working when running early bootloader code.
>>
>> You can check sections "A3.2.2 Cases where unaligned accesses are
>> UNPREDICTABLE" and "B3.12.4 Alignment faults" in the ARM Architecture
>> Reference Manual.
>>
>> Basically, the MMU has to be enabled. Also unaligned memory accesses
>> can't be done for the memory pages with device or strongly-ordered
>> attribute.
>
> The MMU should already be enabled in order for the caches to work.
>
>> Moreover, not every ARM instruction supports unaligned memory accesses
>> too. For example, LDM/STM instructions don't work with unaligned memory
>> addresses regardless of the SCTLR.A bit. And if the compiler thinks
>> that the pointer is supposed to be properly aligned, then it may
>> use such instructions. A very good testcase is a simple function
>> like this:
>>
>>   int f(int *x)
>>   {
>>       return x[0] + x[1];
>>   }
>>
>> The compiler will rightfully generate the following instructions:
>>
>> 00000000 <f>:
>>    0:   e8900009        ldm     r0, {r0, r3}
>>    4:   e0830000        add     r0, r3, r0
>>    8:   e12fff1e        bx      lr
>>
>> If the pointer is misaligned, then it will surely fail.
>
> I'm well aware of this.  However, in the case at hand, it was an STR
> instruction that failed, and this shouldn't happen on a correctly
> configured ARMv6+.
>
>>> What hardware did this happen on?  If it was on ARMv5, adding the packed
>>> attribute is probably the correct fix.  If it was ARMv6 or later,
>>> something else is broken as well.
>>
>> It does not matter if this was ARMv6+ hardware or not. The current
>> U-Boot code is wrong and we need to fix it.
>
> The question is how many errors there are.  That's why I asked about the
> hardware.

I've seen it on a number of devices but they were all ARMv7+
(AllWinner, Rockchips etc)


More information about the U-Boot mailing list