Use more asm-generic headers

From: Rob Herring <[email protected]>

This moves ARM over to the asm-generic/unaligned.h header. This has the benefit of better code generated especially for ARMv7 on gcc 4.7+ compilers.

As Arnd Bergmann, points out: The asm-generic version uses the "struct" version for native-endian unaligned access and the "byteshift" version for the opposite endianess. The current ARM version however uses the "byteshift" implementation for both.

Thanks to Nicolas Pitre for the excellent analysis:

Test case:

int foo (int *x) { return get_unaligned(x); } long long bar (long long *x) { return get_unaligned(x); }

With the current ARM version:

foo:

	ldrb	r3, [r0, #2]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 2B], MEM[(const u8 *)x_1(D) + 2B]
	ldrb	r1, [r0, #1]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 1B], MEM[(const u8 *)x_1(D) + 1B]
	ldrb	r2, [r0, #0]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D)], MEM[(const u8 *)x_1(D)]
	mov	r3, r3, asl #16	@ tmp154, MEM[(const u8 *)x_1(D) + 2B],
	ldrb	r0, [r0, #3]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 3B], MEM[(const u8 *)x_1(D) + 3B]
	orr	r3, r3, r1, asl #8	@, tmp155, tmp154, MEM[(const u8 *)x_1(D) + 1B],
	orr	r3, r3, r2	@ tmp157, tmp155, MEM[(const u8 *)x_1(D)]
	orr	r0, r3, r0, asl #24	@,, tmp157, MEM[(const u8 *)x_1(D) + 3B],
	bx	lr	@

bar:

	stmfd	sp!, {r4, r5, r6, r7}	@,
	mov	r2, #0	@ tmp184,
	ldrb	r5, [r0, #6]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 6B], MEM[(const u8 *)x_1(D) + 6B]
	ldrb	r4, [r0, #5]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 5B], MEM[(const u8 *)x_1(D) + 5B]
	ldrb	ip, [r0, #2]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 2B], MEM[(const u8 *)x_1(D) + 2B]
	ldrb	r1, [r0, #4]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 4B], MEM[(const u8 *)x_1(D) + 4B]
	mov	r5, r5, asl #16	@ tmp175, MEM[(const u8 *)x_1(D) + 6B],
	ldrb	r7, [r0, #1]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 1B], MEM[(const u8 *)x_1(D) + 1B]
	orr	r5, r5, r4, asl #8	@, tmp176, tmp175, MEM[(const u8 *)x_1(D) + 5B],
	ldrb	r6, [r0, #7]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 7B], MEM[(const u8 *)x_1(D) + 7B]
	orr	r5, r5, r1	@ tmp178, tmp176, MEM[(const u8 *)x_1(D) + 4B]
	ldrb	r4, [r0, #0]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D)], MEM[(const u8 *)x_1(D)]
	mov	ip, ip, asl #16	@ tmp188, MEM[(const u8 *)x_1(D) + 2B],
	ldrb	r1, [r0, #3]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 3B], MEM[(const u8 *)x_1(D) + 3B]
	orr	ip, ip, r7, asl #8	@, tmp189, tmp188, MEM[(const u8 *)x_1(D) + 1B],
	orr	r3, r5, r6, asl #24	@,, tmp178, MEM[(const u8 *)x_1(D) + 7B],
	orr	ip, ip, r4	@ tmp191, tmp189, MEM[(const u8 *)x_1(D)]
	orr	ip, ip, r1, asl #24	@, tmp194, tmp191, MEM[(const u8 *)x_1(D) + 3B],
	mov	r1, r3	@,
	orr	r0, r2, ip	@ tmp171, tmp184, tmp194
	ldmfd	sp!, {r4, r5, r6, r7}
	bx	lr

In both cases the code is slightly suboptimal. One may wonder why wasting r2 with the constant 0 in the second case for example. And all the mov's could be folded in subsequent orr's, etc.

Now with the asm-generic version:

foo:

	ldr	r0, [r0, #0]	@ unaligned	@,* x
	bx	lr	@

bar:

	mov	r3, r0	@ x, x
	ldr	r0, [r0, #0]	@ unaligned	@,* x
	ldr	r1, [r3, #4]	@ unaligned	@,
	bx	lr	@

This is way better of course, but only because this was compiled for ARMv7. In this case the compiler knows that the hardware can do unaligned word access. This isn't that obvious for foo(), but if we remove the get_unaligned() from bar as follows:

long long bar (long long *x) {return *x; }

then the resulting code is:

bar:

	ldmia	r0, {r0, r1}	@ x,,
	bx	lr	@

So this proves that the presumed aligned vs unaligned cases does have influence on the instructions the compiler may use and that the above unaligned code results are not just an accident.

Still... this isn't fully conclusive without at least looking at the resulting assembly fron a pre ARMv6 compilation. Let's see with an ARMv5 target:

foo:

	ldrb	r3, [r0, #0]	@ zero_extendqisi2	@ tmp139,* x
	ldrb	r1, [r0, #1]	@ zero_extendqisi2	@ tmp140,
	ldrb	r2, [r0, #2]	@ zero_extendqisi2	@ tmp143,
	ldrb	r0, [r0, #3]	@ zero_extendqisi2	@ tmp146,
	orr	r3, r3, r1, asl #8	@, tmp142, tmp139, tmp140,
	orr	r3, r3, r2, asl #16	@, tmp145, tmp142, tmp143,
	orr	r0, r3, r0, asl #24	@,, tmp145, tmp146,
	bx	lr	@

bar:

	stmfd	sp!, {r4, r5, r6, r7}	@,
	ldrb	r2, [r0, #0]	@ zero_extendqisi2	@ tmp139,* x
	ldrb	r7, [r0, #1]	@ zero_extendqisi2	@ tmp140,
	ldrb	r3, [r0, #4]	@ zero_extendqisi2	@ tmp149,
	ldrb	r6, [r0, #5]	@ zero_extendqisi2	@ tmp150,
	ldrb	r5, [r0, #2]	@ zero_extendqisi2	@ tmp143,
	ldrb	r4, [r0, #6]	@ zero_extendqisi2	@ tmp153,
	ldrb	r1, [r0, #7]	@ zero_extendqisi2	@ tmp156,
	ldrb	ip, [r0, #3]	@ zero_extendqisi2	@ tmp146,
	orr	r2, r2, r7, asl #8	@, tmp142, tmp139, tmp140,
	orr	r3, r3, r6, asl #8	@, tmp152, tmp149, tmp150,
	orr	r2, r2, r5, asl #16	@, tmp145, tmp142, tmp143,
	orr	r3, r3, r4, asl #16	@, tmp155, tmp152, tmp153,
	orr	r0, r2, ip, asl #24	@,, tmp145, tmp146,
	orr	r1, r3, r1, asl #24	@,, tmp155, tmp156,
	ldmfd	sp!, {r4, r5, r6, r7}
	bx	lr

Compared to the initial results, this is really nicely optimized and I couldn't do much better if I were to hand code it myself.

Signed-off-by Rob Herring <[email protected]>

Reviewed-by Nicolas Pitre <[email protected]>

---
 arch/arm/include/asm/Kbuild      |    1 +
 arch/arm/include/asm/unaligned.h |   19 -------------------
 2 files changed, 1 insertion(+), 19 deletions(-)
 delete mode 100644 arch/arm/include/asm/unaligned.h

arch/arm/include/asm/Kbuild +1 -0

index 6cc8a13..a86cabf 100644
@@ -33,3 +33,4 @@ generic-y += sockios.h
 generic-y += termbits.h
 generic-y += timex.h
 generic-y += types.h
+generic-y += unaligned.h

arch/arm/include/asm/unaligned.h deleted +0 -19

deleted file mode 100644
index 44593a8..0000000
@@ -1,19 +0,0 @@
-#ifndef _ASM_ARM_UNALIGNED_H
-#define _ASM_ARM_UNALIGNED_H
-
-#include <linux/unaligned/le_byteshift.h>
-#include <linux/unaligned/be_byteshift.h>
-#include <linux/unaligned/generic.h>
-
-/*
- * Select endianness
- */
-#ifndef __ARMEB__
-#define get_unaligned	__get_unaligned_le
-#define put_unaligned	__put_unaligned_le
-#else
-#define get_unaligned	__get_unaligned_be
-#define put_unaligned	__put_unaligned_be
-#endif
-
-#endif /* _ASM_ARM_UNALIGNED_H */

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

This patch has been merged into mainline as commit below.

d25c881 ARM: 7493/1: use generic unaligned.h

It introduces a regression for me. Check out the commit on mainline, build a v7 only kernel (imx5/6) with imx_v6_v7_defconfig, the kernel halts in decompressor. But v6/v7 kernel (imx3/5/6) works fine. The kernel built on the parent commit below works all fine.

ef1c209 ARM: 7492/1: add strstr declaration for decompressors

Shawn

Quoted 174 lines · Rob Herring

From: Rob Herring <[email protected]>

This moves ARM over to the asm-generic/unaligned.h header. This has the benefit of better code generated especially for ARMv7 on gcc 4.7+ compilers.

As Arnd Bergmann, points out: The asm-generic version uses the "struct" version for native-endian unaligned access and the "byteshift" version for the opposite endianess. The current ARM version however uses the "byteshift" implementation for both.

Thanks to Nicolas Pitre for the excellent analysis:

Test case:

int foo (int *x) { return get_unaligned(x); } long long bar (long long *x) { return get_unaligned(x); }

With the current ARM version:

foo:

	ldrb	r3, [r0, #2]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 2B], MEM[(const u8 *)x_1(D) + 2B]
	ldrb	r1, [r0, #1]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 1B], MEM[(const u8 *)x_1(D) + 1B]
	ldrb	r2, [r0, #0]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D)], MEM[(const u8 *)x_1(D)]
	mov	r3, r3, asl #16	@ tmp154, MEM[(const u8 *)x_1(D) + 2B],
	ldrb	r0, [r0, #3]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 3B], MEM[(const u8 *)x_1(D) + 3B]
	orr	r3, r3, r1, asl #8	@, tmp155, tmp154, MEM[(const u8 *)x_1(D) + 1B],
	orr	r3, r3, r2	@ tmp157, tmp155, MEM[(const u8 *)x_1(D)]
	orr	r0, r3, r0, asl #24	@,, tmp157, MEM[(const u8 *)x_1(D) + 3B],
	bx	lr	@

bar:

	stmfd	sp!, {r4, r5, r6, r7}	@,
	mov	r2, #0	@ tmp184,
	ldrb	r5, [r0, #6]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 6B], MEM[(const u8 *)x_1(D) + 6B]
	ldrb	r4, [r0, #5]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 5B], MEM[(const u8 *)x_1(D) + 5B]
	ldrb	ip, [r0, #2]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 2B], MEM[(const u8 *)x_1(D) + 2B]
	ldrb	r1, [r0, #4]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 4B], MEM[(const u8 *)x_1(D) + 4B]
	mov	r5, r5, asl #16	@ tmp175, MEM[(const u8 *)x_1(D) + 6B],
	ldrb	r7, [r0, #1]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 1B], MEM[(const u8 *)x_1(D) + 1B]
	orr	r5, r5, r4, asl #8	@, tmp176, tmp175, MEM[(const u8 *)x_1(D) + 5B],
	ldrb	r6, [r0, #7]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 7B], MEM[(const u8 *)x_1(D) + 7B]
	orr	r5, r5, r1	@ tmp178, tmp176, MEM[(const u8 *)x_1(D) + 4B]
	ldrb	r4, [r0, #0]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D)], MEM[(const u8 *)x_1(D)]
	mov	ip, ip, asl #16	@ tmp188, MEM[(const u8 *)x_1(D) + 2B],
	ldrb	r1, [r0, #3]	@ zero_extendqisi2	@ MEM[(const u8 *)x_1(D) + 3B], MEM[(const u8 *)x_1(D) + 3B]
	orr	ip, ip, r7, asl #8	@, tmp189, tmp188, MEM[(const u8 *)x_1(D) + 1B],
	orr	r3, r5, r6, asl #24	@,, tmp178, MEM[(const u8 *)x_1(D) + 7B],
	orr	ip, ip, r4	@ tmp191, tmp189, MEM[(const u8 *)x_1(D)]
	orr	ip, ip, r1, asl #24	@, tmp194, tmp191, MEM[(const u8 *)x_1(D) + 3B],
	mov	r1, r3	@,
	orr	r0, r2, ip	@ tmp171, tmp184, tmp194
	ldmfd	sp!, {r4, r5, r6, r7}
	bx	lr

In both cases the code is slightly suboptimal. One may wonder why wasting r2 with the constant 0 in the second case for example. And all the mov's could be folded in subsequent orr's, etc.

Now with the asm-generic version:

foo:

	ldr	r0, [r0, #0]	@ unaligned	@,* x
	bx	lr	@

bar:

	mov	r3, r0	@ x, x
	ldr	r0, [r0, #0]	@ unaligned	@,* x
	ldr	r1, [r3, #4]	@ unaligned	@,
	bx	lr	@

This is way better of course, but only because this was compiled for ARMv7. In this case the compiler knows that the hardware can do unaligned word access. This isn't that obvious for foo(), but if we remove the get_unaligned() from bar as follows:

long long bar (long long *x) {return *x; }

then the resulting code is:

bar:

	ldmia	r0, {r0, r1}	@ x,,
	bx	lr	@

So this proves that the presumed aligned vs unaligned cases does have influence on the instructions the compiler may use and that the above unaligned code results are not just an accident.

Still... this isn't fully conclusive without at least looking at the resulting assembly fron a pre ARMv6 compilation. Let's see with an ARMv5 target:

foo:

	ldrb	r3, [r0, #0]	@ zero_extendqisi2	@ tmp139,* x
	ldrb	r1, [r0, #1]	@ zero_extendqisi2	@ tmp140,
	ldrb	r2, [r0, #2]	@ zero_extendqisi2	@ tmp143,
	ldrb	r0, [r0, #3]	@ zero_extendqisi2	@ tmp146,
	orr	r3, r3, r1, asl #8	@, tmp142, tmp139, tmp140,
	orr	r3, r3, r2, asl #16	@, tmp145, tmp142, tmp143,
	orr	r0, r3, r0, asl #24	@,, tmp145, tmp146,
	bx	lr	@

bar:

	stmfd	sp!, {r4, r5, r6, r7}	@,
	ldrb	r2, [r0, #0]	@ zero_extendqisi2	@ tmp139,* x
	ldrb	r7, [r0, #1]	@ zero_extendqisi2	@ tmp140,
	ldrb	r3, [r0, #4]	@ zero_extendqisi2	@ tmp149,
	ldrb	r6, [r0, #5]	@ zero_extendqisi2	@ tmp150,
	ldrb	r5, [r0, #2]	@ zero_extendqisi2	@ tmp143,
	ldrb	r4, [r0, #6]	@ zero_extendqisi2	@ tmp153,
	ldrb	r1, [r0, #7]	@ zero_extendqisi2	@ tmp156,
	ldrb	ip, [r0, #3]	@ zero_extendqisi2	@ tmp146,
	orr	r2, r2, r7, asl #8	@, tmp142, tmp139, tmp140,
	orr	r3, r3, r6, asl #8	@, tmp152, tmp149, tmp150,
	orr	r2, r2, r5, asl #16	@, tmp145, tmp142, tmp143,
	orr	r3, r3, r4, asl #16	@, tmp155, tmp152, tmp153,
	orr	r0, r2, ip, asl #24	@,, tmp145, tmp146,
	orr	r1, r3, r1, asl #24	@,, tmp155, tmp156,
	ldmfd	sp!, {r4, r5, r6, r7}
	bx	lr

Compared to the initial results, this is really nicely optimized and I couldn't do much better if I were to hand code it myself.

Signed-off-by Rob Herring <[email protected]>

Reviewed-by Nicolas Pitre <[email protected]>

---
 arch/arm/include/asm/Kbuild      |    1 +
 arch/arm/include/asm/unaligned.h |   19 -------------------
 2 files changed, 1 insertion(+), 19 deletions(-)
 delete mode 100644 arch/arm/include/asm/unaligned.h

arch/arm/include/asm/Kbuild +1 -0

index 6cc8a13..a86cabf 100644
@@ -33,3 +33,4 @@ generic-y += sockios.h
 generic-y += termbits.h
 generic-y += timex.h
 generic-y += types.h
+generic-y += unaligned.h

arch/arm/include/asm/unaligned.h deleted +0 -19

deleted file mode 100644
index 44593a8..0000000
@@ -1,19 +0,0 @@
-#ifndef _ASM_ARM_UNALIGNED_H
-#define _ASM_ARM_UNALIGNED_H
-
-#include <linux/unaligned/le_byteshift.h>
-#include <linux/unaligned/be_byteshift.h>
-#include <linux/unaligned/generic.h>
-
-/*
- * Select endianness
- */
-#ifndef __ARMEB__
-#define get_unaligned	__get_unaligned_le
-#define put_unaligned	__put_unaligned_le
-#else
-#define get_unaligned	__get_unaligned_be
-#define put_unaligned	__put_unaligned_be
-#endif
-
-#endif /* _ASM_ARM_UNALIGNED_H */

Signature · 7 lines

1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 8 lines · Shawn Guo

This patch has been merged into mainline as commit below.

d25c881 ARM: 7493/1: use generic unaligned.h

It introduces a regression for me. Check out the commit on mainline, build a v7 only kernel (imx5/6) with imx_v6_v7_defconfig, the kernel halts in decompressor. But v6/v7 kernel (imx3/5/6) works fine. The kernel built on the parent commit below works all fine.

It actually fails in the decompressor or that's the last output you get?

I compared the decompressor disassembly of both cases and get the same number of ldrb/strb instructions, so I don't think it is directly related to alignment.

I tried the XY decompressor as that is one difference, but that works fine for me on highbank.

Does it work with an empty uncompress.h functions? That should be the only difference in our decompressor code.

Quoted 11 lines · Rob Herring

Quoted 8 lines · Shawn Guo

This patch has been merged into mainline as commit below.

d25c881 ARM: 7493/1: use generic unaligned.h

It introduces a regression for me. Check out the commit on mainline, build a v7 only kernel (imx5/6) with imx_v6_v7_defconfig, the kernel halts in decompressor. But v6/v7 kernel (imx3/5/6) works fine. The kernel built on the parent commit below works all fine.

It actually fails in the decompressor or that's the last output you get?

I think it fails in the decompressor, because what I see is

Uncompressing Linux...

not

Uncompressing Linux... done, booting the kernel.

Quoted 7 lines

I compared the decompressor disassembly of both cases and get the same number of ldrb/strb instructions, so I don't think it is directly related to alignment.

I tried the XY decompressor as that is one difference, but that works fine for me on highbank.

Quoted 2 lines

Does it work with an empty uncompress.h functions? That should be the only difference in our decompressor code.

No, empty putc() in uncompress.h does not help.

New finding is that it only fails with LZO decompressor while the other 3 Gzip, LZMA and XZ all work good. We happen to have LZO as the default one in imx_v6_v7_defconfig.

I must have had an old config with XZ. LZO fails because of this:

lib/decompress_unlzo.c: version = get_unaligned_be16(parse);
lib/decompress_unlzo.c: if (get_unaligned_be32(parse) & HEADER_HAS_FILTER)
lib/decompress_unlzo.c:         dst_len = get_unaligned_be32(in_buf);
lib/decompress_unlzo.c:         src_len = get_unaligned_be32(in_buf);

This was what I was afraid of. The decompressor runs with the sysctrl register A bit in whatever state the bootloader left it in. In the case of u-boot it is set, and the maintainers are pretty set on not allowing unaligned accesses if you've seen the recent discussion.

This should fix things.

Rob

8<---------------------------------------------------------------------

arch/arm/boot/compressed/head.S +0 -0

index bc67cbf..1f87d22 100644

Quoted 28 lines · Shawn Guo

Quoted 11 lines · Rob Herring

Quoted 8 lines · Shawn Guo

This patch has been merged into mainline as commit below.

d25c881 ARM: 7493/1: use generic unaligned.h

It introduces a regression for me. Check out the commit on mainline, build a v7 only kernel (imx5/6) with imx_v6_v7_defconfig, the kernel halts in decompressor. But v6/v7 kernel (imx3/5/6) works fine. The kernel built on the parent commit below works all fine.

It actually fails in the decompressor or that's the last output you get?

I think it fails in the decompressor, because what I see is

Uncompressing Linux...

not

Uncompressing Linux... done, booting the kernel.

Quoted 7 lines

I compared the decompressor disassembly of both cases and get the same number of ldrb/strb instructions, so I don't think it is directly related to alignment.

I tried the XY decompressor as that is one difference, but that works fine for me on highbank.

Quoted 8 lines

Quoted 2 lines

Does it work with an empty uncompress.h functions? That should be the only difference in our decompressor code.

No, empty putc() in uncompress.h does not help.

New finding is that it only fails with LZO decompressor while the other 3 Gzip, LZMA and XZ all work good. We happen to have LZO as the default one in imx_v6_v7_defconfig.

I must have had an old config with XZ. LZO fails because of this:

lib/decompress_unlzo.c: version = get_unaligned_be16(parse);
lib/decompress_unlzo.c: if (get_unaligned_be32(parse) & HEADER_HAS_FILTER)
lib/decompress_unlzo.c:         dst_len = get_unaligned_be32(in_buf);
lib/decompress_unlzo.c:         src_len = get_unaligned_be32(in_buf);

This was what I was afraid of. The decompressor runs with the sysctrl register A bit in whatever state the bootloader left it in. In the case of u-boot it is set, and the maintainers are pretty set on not allowing unaligned accesses if you've seen the recent discussion.

This should fix things.

Rob

8<---------------------------------------------------------------------

arch/arm/boot/compressed/head.S +0 -0

index bc67cbf..1f87d22 100644

Quoted 45 lines · Rob Herring

Quoted 28 lines · Shawn Guo

Quoted 11 lines · Rob Herring

Quoted 8 lines · Shawn Guo

This patch has been merged into mainline as commit below.

d25c881 ARM: 7493/1: use generic unaligned.h

It introduces a regression for me. Check out the commit on mainline, build a v7 only kernel (imx5/6) with imx_v6_v7_defconfig, the kernel halts in decompressor. But v6/v7 kernel (imx3/5/6) works fine. The kernel built on the parent commit below works all fine.

It actually fails in the decompressor or that's the last output you get?

I think it fails in the decompressor, because what I see is

Uncompressing Linux...

not

Uncompressing Linux... done, booting the kernel.

Quoted 7 lines

I compared the decompressor disassembly of both cases and get the same number of ldrb/strb instructions, so I don't think it is directly related to alignment.

I tried the XY decompressor as that is one difference, but that works fine for me on highbank.

Quoted 8 lines

Quoted 2 lines

Does it work with an empty uncompress.h functions? That should be the only difference in our decompressor code.

No, empty putc() in uncompress.h does not help.

New finding is that it only fails with LZO decompressor while the other 3 Gzip, LZMA and XZ all work good. We happen to have LZO as the default one in imx_v6_v7_defconfig.

I must have had an old config with XZ. LZO fails because of this:

lib/decompress_unlzo.c: version = get_unaligned_be16(parse);
lib/decompress_unlzo.c: if (get_unaligned_be32(parse) & HEADER_HAS_FILTER)
lib/decompress_unlzo.c:         dst_len = get_unaligned_be32(in_buf);
lib/decompress_unlzo.c:         src_len = get_unaligned_be32(in_buf);

Quoted 4 lines

This was what I was afraid of. The decompressor runs with the sysctrl register A bit in whatever state the bootloader left it in. In the case of u-boot it is set, and the maintainers are pretty set on not allowing unaligned accesses if you've seen the recent discussion.

This is not an u-Boot issue. The kernel code expects misaligned accesses to be handled by the hardware on ARMv6+ now. So it better enforce proper A bit state itself.

Quoted 6 lines

This should fix things.

Rob

8<---------------------------------------------------------------------

arch/arm/boot/compressed/head.S __armv7_mmu_cache_on: · line 660

Quoted 13 lines

arch/arm/boot/compressed/head.S +1 -0

index bc67cbf..1f87d22 100644
@@ -654,6 +654,7 @@ __armv7_mmu_cache_on:
 #endif
                mrc     p15, 0, r0, c1, c0, 0   @ read control reg
                bic     r0, r0, #1 << 28        @ clear SCTLR.TRE
+               bic     r0, r0, #1 << 1         @ clear SCTLR.A
                orr     r0, r0, #0x5000         @ I-cache enable, RR cache replacement
                orr     r0, r0, #0x003c         @ write buffer
 #ifdef CONFIG_MMU

That's OK for ARMv7, but in the ARMv6 case we use __armv4_mmu_cache_on. So this patch is incomplete.

Quoted 62 lines · Nicolas Pitre

Quoted 45 lines · Rob Herring

Quoted 28 lines · Shawn Guo

Quoted 11 lines · Rob Herring

Quoted 8 lines · Shawn Guo

This patch has been merged into mainline as commit below.

d25c881 ARM: 7493/1: use generic unaligned.h

It introduces a regression for me. Check out the commit on mainline, build a v7 only kernel (imx5/6) with imx_v6_v7_defconfig, the kernel halts in decompressor. But v6/v7 kernel (imx3/5/6) works fine. The kernel built on the parent commit below works all fine.

It actually fails in the decompressor or that's the last output you get?

I think it fails in the decompressor, because what I see is

Uncompressing Linux...

not

Uncompressing Linux... done, booting the kernel.

Quoted 7 lines

I compared the decompressor disassembly of both cases and get the same number of ldrb/strb instructions, so I don't think it is directly related to alignment.

I tried the XY decompressor as that is one difference, but that works fine for me on highbank.

Quoted 8 lines

Quoted 2 lines

Does it work with an empty uncompress.h functions? That should be the only difference in our decompressor code.

No, empty putc() in uncompress.h does not help.

New finding is that it only fails with LZO decompressor while the other 3 Gzip, LZMA and XZ all work good. We happen to have LZO as the default one in imx_v6_v7_defconfig.

I must have had an old config with XZ. LZO fails because of this:

lib/decompress_unlzo.c: version = get_unaligned_be16(parse);
lib/decompress_unlzo.c: if (get_unaligned_be32(parse) & HEADER_HAS_FILTER)
lib/decompress_unlzo.c:         dst_len = get_unaligned_be32(in_buf);
lib/decompress_unlzo.c:         src_len = get_unaligned_be32(in_buf);

Quoted 4 lines

This was what I was afraid of. The decompressor runs with the sysctrl register A bit in whatever state the bootloader left it in. In the case of u-boot it is set, and the maintainers are pretty set on not allowing unaligned accesses if you've seen the recent discussion.

This is not an u-Boot issue. The kernel code expects misaligned accesses to be handled by the hardware on ARMv6+ now. So it better enforce proper A bit state itself.

Quoted 6 lines

This should fix things.

Rob

8<---------------------------------------------------------------------

Quoted 16 lines

arch/arm/boot/compressed/head.S __armv7_mmu_cache_on: · line 660

Quoted 13 lines

arch/arm/boot/compressed/head.S +1 -0

index bc67cbf..1f87d22 100644
@@ -654,6 +654,7 @@ __armv7_mmu_cache_on:
 #endif
                mrc     p15, 0, r0, c1, c0, 0   @ read control reg
                bic     r0, r0, #1 << 28        @ clear SCTLR.TRE
+               bic     r0, r0, #1 << 1         @ clear SCTLR.A
                orr     r0, r0, #0x5000         @ I-cache enable, RR cache replacement
                orr     r0, r0, #0x003c         @ write buffer
 #ifdef CONFIG_MMU

That's OK for ARMv7, but in the ARMv6 case we use __armv4_mmu_cache_on. So this patch is incomplete.

It is still optional on v6 and the compiler will not emit unaligned accesses for v6 (which is why it worked when v6 platforms were enabled).

That does raise a bigger question. If we're doing combined v6 and v7 builds, do we want this to require unaligned accesses are enabled and if so, can we force that on in the compiler?

NAK for two reasons.

1. It hasn't been on the list (I can't find a match for "clear SCTLR.A" in my mailbox)

2. The behaviour of unaligned accesses vary depending on CPU. Some fix-up the access, others load the word and then rotate it. If we have decompressors which perform unaligned accesses, we need to fix this properly to avoid the CPU specific behaviour, rather than tweaking control bits to hide the problem.

The contents of this were already reviewed on this thread, so I sent this to the patch system and this was Russell's reply:

Quoted 10 lines

NAK for two reasons.

1. It hasn't been on the list (I can't find a match for "clear SCTLR.A" in my mailbox)

2. The behaviour of unaligned accesses vary depending on CPU. Some fix-up the access, others load the word and then rotate it. If we have decompressors which perform unaligned accesses, we need to fix this properly to avoid the CPU specific behaviour, rather than tweaking control bits to hide the problem.

I'm simply matching the behavior of the kernel itself. The A bit is cleared for v7 kernels and compilers only generate unaligned accesses for v7. Without this the initial state of the A bit is undefined as a bootloader could have cleared it already. We should document the required state or set it to what we want.

Quoted 2 lines · Rob Herring

The contents of this were already reviewed on this thread, so I sent this to the patch system and this was Russell's reply:

So that's why I couldn't find it - the mailing list thread has a different subject line to the patch. Don't do that. Given the amount of list traffic we have today, that's as good as not having been posted at all.

Quoted 11 lines

Quoted 10 lines

NAK for two reasons.

1. It hasn't been on the list (I can't find a match for "clear SCTLR.A" in my mailbox)

2. The behaviour of unaligned accesses vary depending on CPU. Some fix-up the access, others load the word and then rotate it. If we have decompressors which perform unaligned accesses, we need to fix this properly to avoid the CPU specific behaviour, rather than tweaking control bits to hide the problem.

Quoted 5 lines

I'm simply matching the behavior of the kernel itself. The A bit is cleared for v7 kernels and compilers only generate unaligned accesses for v7. Without this the initial state of the A bit is undefined as a bootloader could have cleared it already. We should document the required state or set it to what we want.

Irrespective of this, (2) still stands. Unaligned accesses in the decompressor without a fixup (which will be very hard to provide) will return different data depending on the CPU as I mention in point 2.

So, using unaligned accesses in the decompressor may not give expected results in all cases, so they're best avoided.

And if they're avoided, then we don't care about the setting of the A bit.

Quoted 25 lines · Russell King - ARM Linux

Quoted 2 lines · Rob Herring

The contents of this were already reviewed on this thread, so I sent this to the patch system and this was Russell's reply:

So that's why I couldn't find it - the mailing list thread has a different subject line to the patch. Don't do that. Given the amount of list traffic we have today, that's as good as not having been posted at all.

Quoted 11 lines

Quoted 10 lines

NAK for two reasons.

1. It hasn't been on the list (I can't find a match for "clear SCTLR.A" in my mailbox)

2. The behaviour of unaligned accesses vary depending on CPU. Some fix-up the access, others load the word and then rotate it. If we have decompressors which perform unaligned accesses, we need to fix this properly to avoid the CPU specific behaviour, rather than tweaking control bits to hide the problem.

Quoted 5 lines

I'm simply matching the behavior of the kernel itself. The A bit is cleared for v7 kernels and compilers only generate unaligned accesses for v7. Without this the initial state of the A bit is undefined as a bootloader could have cleared it already. We should document the required state or set it to what we want.

Quoted 4 lines

Irrespective of this, (2) still stands. Unaligned accesses in the decompressor without a fixup (which will be very hard to provide) will return different data depending on the CPU as I mention in point 2.

This only affects v7 cores. It should not vary for v7 cores as unaligned access is a required feature. So how is it going to vary on v7 CPUs? We've got bigger problems if there are v7 cores that don't handle unaligned accesses.

Quoted 31 lines · Rob Herring

Quoted 25 lines · Russell King - ARM Linux

Quoted 2 lines · Rob Herring

The contents of this were already reviewed on this thread, so I sent this to the patch system and this was Russell's reply:

So that's why I couldn't find it - the mailing list thread has a different subject line to the patch. Don't do that. Given the amount of list traffic we have today, that's as good as not having been posted at all.

Quoted 11 lines

Quoted 10 lines

NAK for two reasons.

1. It hasn't been on the list (I can't find a match for "clear SCTLR.A" in my mailbox)

2. The behaviour of unaligned accesses vary depending on CPU. Some fix-up the access, others load the word and then rotate it. If we have decompressors which perform unaligned accesses, we need to fix this properly to avoid the CPU specific behaviour, rather than tweaking control bits to hide the problem.

Quoted 5 lines

I'm simply matching the behavior of the kernel itself. The A bit is cleared for v7 kernels and compilers only generate unaligned accesses for v7. Without this the initial state of the A bit is undefined as a bootloader could have cleared it already. We should document the required state or set it to what we want.

Quoted 4 lines

Irrespective of this, (2) still stands. Unaligned accesses in the decompressor without a fixup (which will be very hard to provide) will return different data depending on the CPU as I mention in point 2.

Quoted 4 lines

This only affects v7 cores. It should not vary for v7 cores as unaligned access is a required feature. So how is it going to vary on v7 CPUs? We've got bigger problems if there are v7 cores that don't handle unaligned accesses.

Rob,

Your patch may only affect v7 cores, but you've raised the issue of the decompressor performing unaligned accesses in general. Shall I re-repeat my point over that or is the problem here going to finally sink in?

Quoted 37 lines · Russell King - ARM Linux

Quoted 31 lines · Rob Herring

Quoted 25 lines · Russell King - ARM Linux

Quoted 2 lines · Rob Herring

The contents of this were already reviewed on this thread, so I sent this to the patch system and this was Russell's reply:

So that's why I couldn't find it - the mailing list thread has a different subject line to the patch. Don't do that. Given the amount of list traffic we have today, that's as good as not having been posted at all.

Quoted 11 lines

Quoted 10 lines

NAK for two reasons.

1. It hasn't been on the list (I can't find a match for "clear SCTLR.A" in my mailbox)

2. The behaviour of unaligned accesses vary depending on CPU. Some fix-up the access, others load the word and then rotate it. If we have decompressors which perform unaligned accesses, we need to fix this properly to avoid the CPU specific behaviour, rather than tweaking control bits to hide the problem.

Quoted 5 lines

I'm simply matching the behavior of the kernel itself. The A bit is cleared for v7 kernels and compilers only generate unaligned accesses for v7. Without this the initial state of the A bit is undefined as a bootloader could have cleared it already. We should document the required state or set it to what we want.

Quoted 4 lines

Irrespective of this, (2) still stands. Unaligned accesses in the decompressor without a fixup (which will be very hard to provide) will return different data depending on the CPU as I mention in point 2.

Quoted 4 lines

This only affects v7 cores. It should not vary for v7 cores as unaligned access is a required feature. So how is it going to vary on v7 CPUs? We've got bigger problems if there are v7 cores that don't handle unaligned accesses.

Quoted 5 lines

Rob,

Your patch may only affect v7 cores, but you've raised the issue of the decompressor performing unaligned accesses in general. Shall I re-repeat my point over that or is the problem here going to finally sink in?

The decompressor is not performing direct unaligned accesses. It uses the get_unaligned() and put_unaligned() accessors. That means that we're in control of how this is happening.

So let's talk about the how. On pre ARMv7, those accesses are performed with a series of byte accesses. When compiling for ARMv7, gcc knows and that the hardware can do unaligned accesses, and it does optimize its output by using ldr/str instructions. But the A bit has to be cleared in that case, and only in that case. This is why the patch clears the A bit only for ARMv7.

So this patch is only setting up the hardware to match gcc's expectations when generating code from the use of get_unaligned() and put_unaligned() when optimizing for ARMv7.

As always, any code doing unaligned access and _not_ using those accessors is broken.

So is there a chance that this patch will be applied for 3.7?

Currently I can't boot anymore Armada XP or Armada 370, if kernel is compressed in LZO. It's annoying.

Russell, did Nicolas manage to convince you?

This only affects v7 cores. It should not vary for v7 cores as unaligned access is a required feature. So how is it going to vary on v7 CPUs? We've got bigger problems if there are v7 cores that don't handle unaligned accesses.

Quoted 4 lines · Rob Herring

This only affects v7 cores. It should not vary for v7 cores as unaligned access is a required feature. So how is it going to vary on v7 CPUs? We've got bigger problems if there are v7 cores that don't handle unaligned accesses.

Oh, and this gives me a third reason to NAK this patch. Why only ensure that the A bit is clear for v7 CPUs? Why not v6, v5, v4 too? Why does ARMv7 get this special treatment?

With recent compilers and move to generic unaligned.h in commit d25c881 (ARM: 7493/1: use generic unaligned.h), unaligned accesses will be used by the LZO decompressor on v7 cores. So we need to make sure unaligned accesses are allowed by clearing the SCTLR A bit.

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 5 lines · Rob Herring

With recent compilers and move to generic unaligned.h in commit d25c881 (ARM: 7493/1: use generic unaligned.h), unaligned accesses will be used by the LZO decompressor on v7 cores. So we need to make sure unaligned accesses are allowed by clearing the SCTLR A bit.

I just read this in http://gcc.gnu.org/gcc-4.7/changes.html:

On ARM, when compiling for ARMv6 (but not ARMv6-M), ARMv7-A, ARMv7-R, or ARMv7-M, the new option -munaligned-access is active by default, which for some source codes generates code that accesses memory on unaligned addresses. This will require the kernel of those systems to enable such accesses (controlled by CP15 register c1, refer to ARM documentation). Alternatively or for compatibility with kernels where unaligned accesses are not supported, all code has to be compiled with -mno-unaligned-access. Linux/ARM in official releases has automatically and unconditionally supported unaligned accesses as emitted by GCC due to this option being active, since Linux version 2.6.28.

My understanding is that gcc, using the same generic unaligned.h source code, will generate code for ARMv6 and ARMv7 that uses unaligned access, while for ARMv5 and older it won't. So it seems gcc requires Linux to clear SCTLR.A and set SCTLR.U for ARMv6+.

Or add -mno-unaligned-access.

Is my understanding correct?

Quoted 3 lines

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

not true according to the gcc changes page

Quoted 9 lines · Rob Herring

Quoted 5 lines

With recent compilers and move to generic unaligned.h in commit d25c881 (ARM: 7493/1: use generic unaligned.h), unaligned accesses will be used by the LZO decompressor on v7 cores. So we need to make sure unaligned accesses are allowed by clearing the SCTLR A bit.

I just read this in http://gcc.gnu.org/gcc-4.7/changes.html:

Quoted 9 lines

On ARM, when compiling for ARMv6 (but not ARMv6-M), ARMv7-A, ARMv7-R, or ARMv7-M, the new option -munaligned-access is active by default, which for some source codes generates code that accesses memory on unaligned addresses. This will require the kernel of those systems to enable such accesses (controlled by CP15 register c1, refer to ARM documentation). Alternatively or for compatibility with kernels where unaligned accesses are not supported, all code has to be compiled with -mno-unaligned-access. Linux/ARM in official releases has automatically and unconditionally supported unaligned accesses as emitted by GCC due to this option being active, since Linux version 2.6.28.

I don't think there is such a thing as ARMv6-M.

Quoted 6 lines · Johannes Stezenbach

Quoted 3 lines · Rob Herring

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 1 line

not true according to the gcc changes page

What are you going to believe: documentation or what the compiler emitted? At least for ubuntu/linaro 4.6.3 which has the unaligned access support backported and 4.7.2, unaligned accesses are emitted for v7 only. I guess default here means it is the default unless you change the default in your build of gcc.

Quoted 7 lines · Rob Herring

Quoted 6 lines · Johannes Stezenbach

Quoted 3 lines · Rob Herring

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 7 lines

Quoted 1 line

not true according to the gcc changes page

What are you going to believe: documentation or what the compiler emitted? At least for ubuntu/linaro 4.6.3 which has the unaligned access support backported and 4.7.2, unaligned accesses are emitted for v7 only. I guess default here means it is the default unless you change the default in your build of gcc.

Since ARMv6 can handle unaligned access in the same way as ARMv7 it seems a clear bug in gcc which might hopefully get fixed. Thus in this case I think it is reasonable to follow the gcc documentation, otherwise the code would break for ARMv6 when gcc gets fixed.

Quoted 16 lines · Johannes Stezenbach

Quoted 7 lines · Rob Herring

Quoted 6 lines · Johannes Stezenbach

Quoted 3 lines · Rob Herring

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 7 lines

Quoted 1 line

not true according to the gcc changes page

What are you going to believe: documentation or what the compiler emitted? At least for ubuntu/linaro 4.6.3 which has the unaligned access support backported and 4.7.2, unaligned accesses are emitted for v7 only. I guess default here means it is the default unless you change the default in your build of gcc.

Quoted 5 lines

Since ARMv6 can handle unaligned access in the same way as ARMv7 it seems a clear bug in gcc which might hopefully get fixed. Thus in this case I think it is reasonable to follow the gcc documentation, otherwise the code would break for ARMv6 when gcc gets fixed.

But the compiler can't assume the state of the U bit. I think it is still legal on v6 to not support unaligned accesses, but on v7 it is required. All the standard v6 ARM cores support it, but I'm not sure about custom cores or if there are SOCs with buses that don't support unaligned accesses properly.

Quoted 16 lines · Johannes Stezenbach

Quoted 7 lines · Rob Herring

Quoted 6 lines · Johannes Stezenbach

Quoted 3 lines · Rob Herring

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 7 lines

Quoted 1 line

not true according to the gcc changes page

What are you going to believe: documentation or what the compiler emitted? At least for ubuntu/linaro 4.6.3 which has the unaligned access support backported and 4.7.2, unaligned accesses are emitted for v7 only. I guess default here means it is the default unless you change the default in your build of gcc.

Quoted 5 lines

Since ARMv6 can handle unaligned access in the same way as ARMv7 it seems a clear bug in gcc which might hopefully get fixed. Thus in this case I think it is reasonable to follow the gcc documentation, otherwise the code would break for ARMv6 when gcc gets fixed.

But the compiler can't assume the state of the U bit. I think it is still legal on v6 to not support unaligned accesses, but on v7 it is required. All the standard v6 ARM cores support it, but I'm not sure about custom cores or if there are SOCs with buses that don't support unaligned accesses properly.

Quoted 23 lines · Rob Herring

Quoted 16 lines · Johannes Stezenbach

Quoted 7 lines · Rob Herring

Quoted 6 lines · Johannes Stezenbach

Quoted 3 lines · Rob Herring

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 7 lines

Quoted 1 line

not true according to the gcc changes page

What are you going to believe: documentation or what the compiler emitted? At least for ubuntu/linaro 4.6.3 which has the unaligned access support backported and 4.7.2, unaligned accesses are emitted for v7 only. I guess default here means it is the default unless you change the default in your build of gcc.

Quoted 5 lines

Since ARMv6 can handle unaligned access in the same way as ARMv7 it seems a clear bug in gcc which might hopefully get fixed. Thus in this case I think it is reasonable to follow the gcc documentation, otherwise the code would break for ARMv6 when gcc gets fixed.

Quoted 5 lines

But the compiler can't assume the state of the U bit. I think it is still legal on v6 to not support unaligned accesses, but on v7 it is required. All the standard v6 ARM cores support it, but I'm not sure about custom cores or if there are SOCs with buses that don't support unaligned accesses properly.

Well, I read the "...since Linux version 2.6.28" comment in the gcc changes page in the way that they assume the U-bit is set. (Although I'm not sure it really is???)

From the compiler's point of view we have always implemented the

Looking at it from another side, if using the hw unaligned access capability gives a performance benefit then it would be useful to support it even if gcc doesn't behave as documented. One could still use a special unaligned.h for ARMv6 to get the performance benefit. (If it doesn't give performance benfit, then why bother, let's just use -mno-unaligned-access for v7, too.)

In the ARMv6 ARM unaligned support and the U-bit is not optional, so you could use the same SoC bus argument for some hypothetical v7 SoCs.

Johannes

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 30 lines · Johannes Stezenbach

Quoted 28 lines · Rob Herring

Quoted 16 lines · Johannes Stezenbach

Quoted 7 lines · Rob Herring

Quoted 6 lines · Johannes Stezenbach

Quoted 3 lines · Rob Herring

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 7 lines

Quoted 1 line

not true according to the gcc changes page

What are you going to believe: documentation or what the compiler emitted? At least for ubuntu/linaro 4.6.3 which has the unaligned access support backported and 4.7.2, unaligned accesses are emitted for v7 only. I guess default here means it is the default unless you change the default in your build of gcc.

Quoted 5 lines

Since ARMv6 can handle unaligned access in the same way as ARMv7 it seems a clear bug in gcc which might hopefully get fixed. Thus in this case I think it is reasonable to follow the gcc documentation, otherwise the code would break for ARMv6 when gcc gets fixed.

But the compiler can't assume the state of the U bit. I think it is still legal on v6 to not support unaligned accesses, but on v7 it is required. All the standard v6 ARM cores support it, but I'm not sure about custom cores or if there are SOCs with buses that don't support unaligned accesses properly.

Quoted 3 lines

Well, I read the "...since Linux version 2.6.28" comment in the gcc changes page in the way that they assume the U-bit is set. (Although I'm not sure it really is???)

Actually, the kernel checks the arch version and the U bit on boot, and chooses the appropriate setting for the A bit depending on the result. (See arch/arm/mm/alignment.c:alignment_init().)

Currently, we depend on the CPU reset behaviour or firmware/ bootloader to set the U bit for v6, but the behaviour should be correct either way, though unaligned accesses will obviously perform (much) better with U=1.

For v7, we should definitely use -munaligned-access where available (unless it's the default?)

Quoted 35 lines · Dave Martin

Quoted 33 lines · Johannes Stezenbach

Quoted 23 lines · Rob Herring

Quoted 16 lines · Johannes Stezenbach

Quoted 7 lines · Rob Herring

Quoted 6 lines · Johannes Stezenbach

Quoted 3 lines · Rob Herring

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 7 lines

Quoted 1 line

not true according to the gcc changes page

What are you going to believe: documentation or what the compiler emitted? At least for ubuntu/linaro 4.6.3 which has the unaligned access support backported and 4.7.2, unaligned accesses are emitted for v7 only. I guess default here means it is the default unless you change the default in your build of gcc.

Quoted 5 lines

Since ARMv6 can handle unaligned access in the same way as ARMv7 it seems a clear bug in gcc which might hopefully get fixed. Thus in this case I think it is reasonable to follow the gcc documentation, otherwise the code would break for ARMv6 when gcc gets fixed.

Quoted 5 lines

But the compiler can't assume the state of the U bit. I think it is still legal on v6 to not support unaligned accesses, but on v7 it is required. All the standard v6 ARM cores support it, but I'm not sure about custom cores or if there are SOCs with buses that don't support unaligned accesses properly.

Well, I read the "...since Linux version 2.6.28" comment in the gcc changes page in the way that they assume the U-bit is set. (Although I'm not sure it really is???)

Quoted 3 lines

Actually, the kernel checks the arch version and the U bit on boot, and chooses the appropriate setting for the A bit depending on the result. (See arch/arm/mm/alignment.c:alignment_init().)

That is in the kernel itself, _after_ the decompressor has run. It is not relevant to any discussion about the decompressor.

Quoted 4 lines

Currently, we depend on the CPU reset behaviour or firmware/ bootloader to set the U bit for v6, but the behaviour should be correct either way, though unaligned accesses will obviously perform (much) better with U=1.

Will someone _PLEASE_ address my initial comments against this patch in light of the fact that it's now been proven _NOT_ to be just a V7 issue, rather than everyone seemingly buring their heads in the sand over this.

The fact is, unaligned accesses in the decompressor are *undefined* at present.

Quoted 2 lines

For v7, we should definitely use -munaligned-access where available (unless it's the default?)

No such option on my compiler - according to the manual I have, the only option there is starting -munaligned is on SPARC for -munaligned-doubles.

However, I believe GCC does believe that unaligned accesses are fine on V6 and above.

Quoted 6 lines · Russell King - ARM Linux

Quoted 2 lines · Dave Martin

For v7, we should definitely use -munaligned-access where available (unless it's the default?)

No such option on my compiler - according to the manual I have, the only option there is starting -munaligned is on SPARC for -munaligned-doubles.

OK, I guess that's something backported into the Linaro toolchain I'm currently using then. But it seems a good idea to use this if available, because it allows the compiler to generate better code in some situations, especially for packed struct access.

Quoted 2 lines

However, I believe GCC does believe that unaligned accesses are fine on V6 and above.

Possibly, but I've never seen it use them deliberately, prior to the -munaligned-access support.

Why not allow unaligned accesses in the decompressor, though, both for v6 and v7?

Quoted 2 lines · Dave Martin

Why not allow unaligned accesses in the decompressor, though, both for v6 and v7?

EXACTLY.

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Dave Martin

Why not allow unaligned accesses in the decompressor, though, both for v6 and v7?

EXACTLY.

I have no objections to that. In fact, I made a remark to this effect in my initial review of this patch. Whether or not gcc does take advantage of this hardware ability in the end is orthogonal.

From 160a5576b53264951ff8164775146b2d4feddecb Mon Sep 17 00:00:00 2001

Quoted 11 lines · Nicolas Pitre

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Dave Martin

Why not allow unaligned accesses in the decompressor, though, both for v6 and v7?

EXACTLY.

I have no objections to that. In fact, I made a remark to this effect in my initial review of this patch. Whether or not gcc does take advantage of this hardware ability in the end is orthogonal.

For the sake of argument, here's how it might look.

Currently, I make no attempt to restore the original state of the U bit. The A bit if forced later by the kernel during boot, after a short window during which we should only run low-level arch code and therefore where no unaligned accesses should happen.

Does anyone think these issues are likely to be important?

From 76c2b7685397f13aa53f426822128430fc24b8a0 Mon Sep 17 00:00:00 2001

Quoted 20 lines · Dave Martin

Quoted 11 lines · Nicolas Pitre

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Dave Martin

Why not allow unaligned accesses in the decompressor, though, both for v6 and v7?

EXACTLY.

I have no objections to that. In fact, I made a remark to this effect in my initial review of this patch. Whether or not gcc does take advantage of this hardware ability in the end is orthogonal.

For the sake of argument, here's how it might look.

Currently, I make no attempt to restore the original state of the U bit. The A bit if forced later by the kernel during boot, after a short window during which we should only run low-level arch code and therefore where no unaligned accesses should happen.

Quoted 2 lines

Does anyone think these issues are likely to be important?

And here is my updated version that does v6 somewhat differently:

8<------------------------------------------------------------------

Quoted 1 line

From 76c2b7685397f13aa53f426822128430fc24b8a0 Mon Sep 17 00:00:00 2001

From: Rob Herring <[email protected]>
Date: Mon, 5 Nov 2012 11:39:48 -0600
Subject: [PATCH v2] ARM: decompressor: clear SCTLR.A bit for v6 and v7 cores

With recent compilers and move to generic unaligned.h in commit d25c881 (ARM: 7493/1: use generic unaligned.h), unaligned accesses will be used by the LZO decompressor on v7 cores. So we need to make sure unaligned accesses are allowed by clearing the SCTLR A bit.

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. In case this changes and to align with the kernel behavior, we clear the A bit and set the U bit.

Signed-off-by Rob Herring <[email protected]>

Acked-by Nicolas Pitre <[email protected]>

Tested-by Shawn Guo <[email protected]>

---
 arch/arm/boot/compressed/head.S |    6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/compressed/head.S +6 -0

index bc67cbf..f14d7ec 100644
@@ -629,6 +629,11 @@ __armv4_mmu_cache_on:
 		mcr	p15, 0, r0, c7, c10, 4	@ drain write buffer
 		mcr	p15, 0, r0, c8, c7, 0	@ flush I,D TLBs
 		mrc	p15, 0, r0, c1, c0, 0	@ read control reg
+		mrc	p15, 0, r11, c0, c0	@ get processor ID
+		and	r11, r11, #0xf0000
+		tst	r11, #0x70000		@ ARMv6
+		orreq	r0, r0, #1 << 22	@ set SCTLR.U
+		biceq	r0, r0, #1 << 1		@ clear SCTLR.A
 		orr	r0, r0, #0x5000		@ I-cache enable, RR cache replacement
 		orr	r0, r0, #0x0030
 #ifdef CONFIG_CPU_ENDIAN_BE8

@@ -654,6 +659,7 @@ __armv7_mmu_cache_on:
 #endif
 		mrc	p15, 0, r0, c1, c0, 0	@ read control reg
 		bic	r0, r0, #1 << 28	@ clear SCTLR.TRE
+		bic	r0, r0, #1 << 1		@ clear SCTLR.A
 		orr	r0, r0, #0x5000		@ I-cache enable, RR cache replacement
 		orr	r0, r0, #0x003c		@ write buffer
 #ifdef CONFIG_MMU

Signature · 1 line

1.7.10.4

Quoted 11 lines · Nicolas Pitre

Quoted 5 lines · Russell King - ARM Linux

Quoted 2 lines · Dave Martin

Why not allow unaligned accesses in the decompressor, though, both for v6 and v7?

EXACTLY.

I have no objections to that. In fact, I made a remark to this effect in my initial review of this patch. Whether or not gcc does take advantage of this hardware ability in the end is orthogonal.

For the sake of argument, here's how it might look.

Currently, I make no attempt to restore the original state of the U bit. The A bit if forced later by the kernel during boot, after a short window during which we should only run low-level arch code and therefore where no unaligned accesses should happen.

Does anyone think these issues are likely to be important?

Cheers
---Dave

Quoted 1 line

From 160a5576b53264951ff8164775146b2d4feddecb Mon Sep 17 00:00:00 2001

From: Dave Martin <[email protected]>
Date: Mon, 5 Nov 2012 16:34:57 +0000
Subject: [PATCH] ARM: decompressor: Enable unaligned memory access for v6 and above

Modern GCC can generate code which makes use of the CPU's native unaligned memory access capabilities. This is useful for the C decompressor implementations used for unpacking compressed kernels.

This patch disables the alignment faults and enabled the v6 unaligned access on CPUs which support these features (i.e., v6 and later), allowing full unaligned access support for C code in the decompressor.

The decompressor C code must not be built to assume that unaligned access works if support for v5 or older platforms is included in the kernel.

Signed-off-by Dave Martin <[email protected]>

---
Note: I have only build-tested this so far.

 arch/arm/boot/compressed/head.S |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

arch/arm/boot/compressed/head.S +11 -1

index 90275f0..cfbb4c9 100644
@@ -652,6 +652,15 @@ __setup_mmu:	sub	r3, r4, #16384		@ Page directory size
 		mov	pc, lr
 ENDPROC(__setup_mmu)
 
+@ Enable unaligned access on v6, to allow better code generation
+@ for the decompressor C code:
+__armv6_mmu_cache_on:
+		mrc	p15, 0, r0, c1, c0, 0	@ read SCTLR
+		bic	r0, r0, #2		@ A (no unaligned access fault)
+		orr	r0, r0, #1 << 22	@ U (v6 unaligned access model)
+		mcr	p15, 0, r0, c1, c0, 0	@ write SCTLR
+		b	__armv4_mmu_cache_on
+
 __arm926ejs_mmu_cache_on:
 #ifdef CONFIG_CPU_DCACHE_WRITETHROUGH
 		mov	r0, #4			@ put dcache in WT mode

@@ -694,6 +703,7 @@ __armv7_mmu_cache_on:
 		bic	r0, r0, #1 << 28	@ clear SCTLR.TRE
 		orr	r0, r0, #0x5000		@ I-cache enable, RR cache replacement
 		orr	r0, r0, #0x003c		@ write buffer
+		bic	r0, r0, #2		@ A (no unaligned access fault)
 #ifdef CONFIG_MMU
 #ifdef CONFIG_CPU_ENDIAN_BE8
 		orr	r0, r0, #1 << 25	@ big-endian page tables

@@ -914,7 +924,7 @@ proc_types:
 
 		.word	0x0007b000		@ ARMv6
 		.word	0x000ff000
-		W(b)	__armv4_mmu_cache_on
+		W(b)	__armv6_mmu_cache_on
 		W(b)	__armv4_mmu_cache_off
 		W(b)	__armv6_mmu_cache_flush
 

Signature · 6 lines

1.7.4.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 31 lines · Dave Martin

Quoted 30 lines · Johannes Stezenbach

Quoted 28 lines · Rob Herring

Quoted 16 lines · Johannes Stezenbach

Quoted 7 lines · Rob Herring

Quoted 6 lines · Johannes Stezenbach

Quoted 3 lines · Rob Herring

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 7 lines

Quoted 1 line

not true according to the gcc changes page

What are you going to believe: documentation or what the compiler emitted? At least for ubuntu/linaro 4.6.3 which has the unaligned access support backported and 4.7.2, unaligned accesses are emitted for v7 only. I guess default here means it is the default unless you change the default in your build of gcc.

Quoted 5 lines

Since ARMv6 can handle unaligned access in the same way as ARMv7 it seems a clear bug in gcc which might hopefully get fixed. Thus in this case I think it is reasonable to follow the gcc documentation, otherwise the code would break for ARMv6 when gcc gets fixed.

But the compiler can't assume the state of the U bit. I think it is still legal on v6 to not support unaligned accesses, but on v7 it is required. All the standard v6 ARM cores support it, but I'm not sure about custom cores or if there are SOCs with buses that don't support unaligned accesses properly.

Quoted 7 lines

Quoted 3 lines

Well, I read the "...since Linux version 2.6.28" comment in the gcc changes page in the way that they assume the U-bit is set. (Although I'm not sure it really is???)

Actually, the kernel checks the arch version and the U bit on boot, and chooses the appropriate setting for the A bit depending on the result. (See arch/arm/mm/alignment.c:alignment_init().)

That is in the kernel itself, _after_ the decompressor has run. It is not relevant to any discussion about the decompressor.

Quoted 4 lines

Currently, we depend on the CPU reset behaviour or firmware/ bootloader to set the U bit for v6, but the behaviour should be correct either way, though unaligned accesses will obviously perform (much) better with U=1.

Will someone _PLEASE_ address my initial comments against this patch in light of the fact that it's now been proven _NOT_ to be just a V7 issue, rather than everyone seemingly buring their heads in the sand over this.

The fact is, unaligned accesses in the decompressor are *undefined* at present.

Quoted 2 lines

For v7, we should definitely use -munaligned-access where available (unless it's the default?)

No such option on my compiler - according to the manual I have, the only option there is starting -munaligned is on SPARC for -munaligned-doubles.

However, I believe GCC does believe that unaligned accesses are fine on V6 and above.

Quoted 48 lines · Russell King - ARM Linux

Quoted 31 lines · Dave Martin

Quoted 30 lines · Johannes Stezenbach

Quoted 28 lines · Rob Herring

Quoted 16 lines · Johannes Stezenbach

Quoted 7 lines · Rob Herring

Quoted 6 lines · Johannes Stezenbach

Quoted 3 lines · Rob Herring

While v6 can support unaligned accesses, it is optional and current compilers won't emit unaligned accesses. So we don't clear the A bit for v6.

Quoted 7 lines

Quoted 1 line

not true according to the gcc changes page

What are you going to believe: documentation or what the compiler emitted? At least for ubuntu/linaro 4.6.3 which has the unaligned access support backported and 4.7.2, unaligned accesses are emitted for v7 only. I guess default here means it is the default unless you change the default in your build of gcc.

Quoted 5 lines

Since ARMv6 can handle unaligned access in the same way as ARMv7 it seems a clear bug in gcc which might hopefully get fixed. Thus in this case I think it is reasonable to follow the gcc documentation, otherwise the code would break for ARMv6 when gcc gets fixed.

But the compiler can't assume the state of the U bit. I think it is still legal on v6 to not support unaligned accesses, but on v7 it is required. All the standard v6 ARM cores support it, but I'm not sure about custom cores or if there are SOCs with buses that don't support unaligned accesses properly.

Quoted 7 lines

Quoted 3 lines

Well, I read the "...since Linux version 2.6.28" comment in the gcc changes page in the way that they assume the U-bit is set. (Although I'm not sure it really is???)

Actually, the kernel checks the arch version and the U bit on boot, and chooses the appropriate setting for the A bit depending on the result. (See arch/arm/mm/alignment.c:alignment_init().)

That is in the kernel itself, _after_ the decompressor has run. It is not relevant to any discussion about the decompressor.

Quoted 4 lines

Currently, we depend on the CPU reset behaviour or firmware/ bootloader to set the U bit for v6, but the behaviour should be correct either way, though unaligned accesses will obviously perform (much) better with U=1.

Quoted 4 lines

Will someone _PLEASE_ address my initial comments against this patch in light of the fact that it's now been proven _NOT_ to be just a V7 issue, rather than everyone seemingly buring their heads in the sand over this.

I tried adding -munaligned-accesses on a v6 build and still get byte accesses rather than unaligned word accesses. So this does seem to be a v7 only issue based on what gcc will currently produce. Copying Michael Hope who can hopefully provide some insight on why v6 unaligned accesses are not enabled.

Quoted 6 lines · Rob Herring

I tried adding -munaligned-accesses on a v6 build and still get byte accesses rather than unaligned word accesses. So this does seem to be a v7 only issue based on what gcc will currently produce. Copying Michael Hope who can hopefully provide some insight on why v6 unaligned accesses are not enabled.

This looks like a bug. Unaligned access is enabled for armv6 but seems to only take effect for cores with Thumb-2. Here's a test case both with unaligned field access and unaligned block copy:

struct foo
{
char a;
int b;
struct

  {
    int x[3];

} c;
} __attribute__((packed));

int get_field(struct foo *p)
{
return p->b;
}

int copy_block(struct foo *p, struct foo *q)
{
p->c = q->c;
}

With -march=armv7-a you get the correct:

bar:

	ldr	r0, [r0, #1]	@ unaligned	@ 11	unaligned_loadsi/2	[length = 4]
	bx	lr	@ 21	*arm_return	[length = 12]

baz:

	str	r4, [sp, #-4]!	@ 25	*push_multi	[length = 4]
	mov	r2, r0	@ 2	*arm_movsi_vfp/1	[length = 4]
	ldr	r4, [r1, #5]!	@ unaligned	@ 9	unaligned_loadsi/2	[length = 4]
	ldr	ip, [r1, #4]	@ unaligned	@ 10	unaligned_loadsi/2	[length = 4]
	ldr	r1, [r1, #8]	@ unaligned	@ 11	unaligned_loadsi/2	[length = 4]
	str	r4, [r2, #5]	@ unaligned	@ 12	unaligned_storesi/2	[length = 4]
	str	ip, [r2, #9]	@ unaligned	@ 13	unaligned_storesi/2	[length = 4]
	str	r1, [r2, #13]	@ unaligned	@ 14	unaligned_storesi/2	[length = 4]
	ldmfd	sp!, {r4}
	bx	lr

With -march=armv6 you get a byte-by-byte field access and a correct unaligned block copy:

bar:

	ldrb	r1, [r0, #2]	@ zero_extendqisi2
	ldrb	r3, [r0, #1]	@ zero_extendqisi2
	ldrb	r2, [r0, #3]	@ zero_extendqisi2
	ldrb	r0, [r0, #4]	@ zero_extendqisi2
	orr	r3, r3, r1, asl #8
	orr	r3, r3, r2, asl #16
	orr	r0, r3, r0, asl #24
	bx	lr

baz:

	str	r4, [sp, #-4]!
	mov	r2, r0
	ldr	r4, [r1, #5]!	@ unaligned
	ldr	ip, [r1, #4]	@ unaligned
	ldr	r1, [r1, #8]	@ unaligned
	str	r4, [r2, #5]	@ unaligned
	str	ip, [r2, #9]	@ unaligned
	str	r1, [r2, #13]	@ unaligned
	ldmfd	sp!, {r4}
	bx	lr

readelf -A shows that the compiler planned to use unaligned access in both. My suspicion is that GCC is using the extv pattern to extract the field from memory, and that pattern is only enabled for Thumb-2 capable cores.

I've logged PR55218. We'll discuss it at our next meeting.

Replying to old thread, for full context see
http://thread.gmane.org/gmane.linux.ports.arm.kernel/180226/focus=197914

On Tue, Nov 06, 2012 at 11:02:27AM +1300, Michael Hope wrote:

From: Rob Herring <[email protected]>

Inspired by the AArgh64 claim that it should be separate from ARM and one reason was being able to use more asm-generic headers. Doing a diff of arch/arm/include/asm and include/asm-generic there are numerous asm headers which are functionally identical to their asm-generic counterparts and others which can use more of the generic versions.

V2 separates unaligned.h and termios.h changes to separate patches. Also, it adds a fix for build failures on non-gzip decompressor compile.

Rob

Rob Herring (5):
ARM: use generic version of identical asm headers
ARM: add strstr declaration for decompressors
ARM: use generic unaligned.h
ARM: use generic termios.h
ARM: convert asm/irqflags.h to use asm-generic/irqflags.h

From: Rob Herring <[email protected]>

Inspired by the AArgh64 claim that it should be separate from ARM and one reason was being able to use more asm-generic headers. Doing a diff of arch/arm/include/asm and include/asm-generic there are numerous asm headers which are functionally identical to their asm-generic counterparts and others which can use more of the generic versions.

V2 separates unaligned.h and termios.h changes to separate patches. Also, it adds a fix for build failures on non-gzip decompressor compile.