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.
--
Måns Rullgård
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
