On Thu, Jul 13, 2017 at 11:11 AM, Måns Rullgård <m...@mansr.com> wrote:
> Siarhei Siamashka <siarhei.siamas...@gmail.com> writes:
>
>> On Thu, 13 Jul 2017 01:43:37 +0100
>> Måns Rullgård <m...@mansr.com> wrote:
>>
>>> Maxime Ripard <maxime.rip...@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)
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
