Problem with 64-bit registers on i.MX53

Problem with 64-bit registers on i.MX53

[Michael Olbrich]

Hi,

I have a problem that looks like that 64-bit registers (I think) are not
saved/restored correctly on a context switch. I've reduced it to the
following test case:

- Latest Linux mainline kernel (v3.6-8559-ge9eca4d)
  v3.5 is also affected
- imx_v6_v7_defconfig
- arch/arm/boot/dts/imx53-evk.dts

The following test program is compiled with "-mcpu=cortex-a8 -mfpu=neon
-O2".
------------------------>8--------------------------------
#include <inttypes.h>
#include <assert.h>

volatile int x = 2;
volatile int64_t y = 2;

int main() {
	volatile int a = 0;
	volatile int64_t b = 0;
	while (1) {
		a = (a + x) % (1 << 30);
		b = (b + y) % (1 << 30);
		assert(a == b);
	}
}
------------------------>8--------------------------------
The ".. (b + y) .." should result in "vadd.i64 d19, d18, d16" or
something like that.

If I start the program twice in parallel, then on aborts immediately with
the assert().
When looking at it with the debugger, it seems as if the calculation
"b = (b + y) % (1 << 30)" produces an incorrect value.

A single instance of the program runs longer, but hits the assert()
eventually. So running multiple processes that use 64-bit registers
triggers the problem really fast. This is why I think the problem has
something to do with context switches.
Not using 64-bit registers (-msoft-float) makes the problem go away.
So it's definitely related to 64-bit registers.

If I disable all MX31/MX35 boards (and therefore all ARMv6 support), then
it works as expected.

Any ideas what goes wrong here?

Regards,
Michael Olbrich

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Problem with 64-bit registers on i.MX53

[Russell King - ARM Linux]

On Mon, Oct 08, 2012 at 06:08:41PM +0200, Michael Olbrich wrote:
> 
> Hi,
> 
> I have a problem that looks like that 64-bit registers (I think) are not
> saved/restored correctly on a context switch. I've reduced it to the
> following test case:
> 
> - Latest Linux mainline kernel (v3.6-8559-ge9eca4d)
>   v3.5 is also affected
> - imx_v6_v7_defconfig
> - arch/arm/boot/dts/imx53-evk.dts
> 
> The following test program is compiled with "-mcpu=cortex-a8 -mfpu=neon
> -O2".
> ------------------------>8--------------------------------
> #include <inttypes.h>
> #include <assert.h>
> 
> volatile int x = 2;
> volatile int64_t y = 2;
> 
> int main() {
> 	volatile int a = 0;
> 	volatile int64_t b = 0;
> 	while (1) {
> 		a = (a + x) % (1 << 30);
> 		b = (b + y) % (1 << 30);
> 		assert(a == b);
> 	}
> }
> ------------------------>8--------------------------------
> The ".. (b + y) .." should result in "vadd.i64 d19, d18, d16" or
> something like that.

Hmm.

Can you send me the output of 'grep ^Features /proc/cpuinfo' please?

Thanks.

Problem with 64-bit registers on i.MX53

[Russell King - ARM Linux]

On Mon, Oct 08, 2012 at 06:01:24PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 08, 2012 at 06:08:41PM +0200, Michael Olbrich wrote:
> > 
> > Hi,
> > 
> > I have a problem that looks like that 64-bit registers (I think) are not
> > saved/restored correctly on a context switch. I've reduced it to the
> > following test case:
> > 
> > - Latest Linux mainline kernel (v3.6-8559-ge9eca4d)
> >   v3.5 is also affected
> > - imx_v6_v7_defconfig
> > - arch/arm/boot/dts/imx53-evk.dts
> > 
> > The following test program is compiled with "-mcpu=cortex-a8 -mfpu=neon
> > -O2".
> > ------------------------>8--------------------------------
> > #include <inttypes.h>
> > #include <assert.h>
> > 
> > volatile int x = 2;
> > volatile int64_t y = 2;
> > 
> > int main() {
> > 	volatile int a = 0;
> > 	volatile int64_t b = 0;
> > 	while (1) {
> > 		a = (a + x) % (1 << 30);
> > 		b = (b + y) % (1 << 30);
> > 		assert(a == b);
> > 	}
> > }
> > ------------------------>8--------------------------------
> > The ".. (b + y) .." should result in "vadd.i64 d19, d18, d16" or
> > something like that.
> 
> Hmm.
> 
> Can you send me the output of 'grep ^Features /proc/cpuinfo' please?

You may also like to try the patch below... it will probably fix your
problem.

diff --git a/arch/arm/include/asm/vfpmacros.h b/arch/arm/include/asm/vfpmacros.h
index a7aadbd..6a6f1e4 100644
--- a/arch/arm/include/asm/vfpmacros.h
+++ b/arch/arm/include/asm/vfpmacros.h
@@ -28,7 +28,7 @@
 	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
 	ldr	\tmp, [\tmp, #0]
 	tst	\tmp, #HWCAP_VFPv3D16
-	ldceq	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
+	ldceql	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
 	addne	\base, \base, #32*4		    @ step over unused register space
 #else
 	VFPFMRX	\tmp, MVFR0			    @ Media and VFP Feature Register 0
@@ -52,7 +52,7 @@
 	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
 	ldr	\tmp, [\tmp, #0]
 	tst	\tmp, #HWCAP_VFPv3D16
-	stceq	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
+	stceql	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
 	addne	\base, \base, #32*4		    @ step over unused register space
 #else
 	VFPFMRX	\tmp, MVFR0			    @ Media and VFP Feature Register 0

Problem with 64-bit registers on i.MX53

[Dave Martin]

On Mon, Oct 08, 2012 at 06:10:11PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 08, 2012 at 06:01:24PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Oct 08, 2012 at 06:08:41PM +0200, Michael Olbrich wrote:
> > > 
> > > Hi,
> > > 
> > > I have a problem that looks like that 64-bit registers (I think) are not
> > > saved/restored correctly on a context switch. I've reduced it to the
> > > following test case:
> > > 
> > > - Latest Linux mainline kernel (v3.6-8559-ge9eca4d)
> > >   v3.5 is also affected
> > > - imx_v6_v7_defconfig
> > > - arch/arm/boot/dts/imx53-evk.dts
> > > 
> > > The following test program is compiled with "-mcpu=cortex-a8 -mfpu=neon
> > > -O2".
> > > ------------------------>8--------------------------------
> > > #include <inttypes.h>
> > > #include <assert.h>
> > > 
> > > volatile int x = 2;
> > > volatile int64_t y = 2;
> > > 
> > > int main() {
> > > 	volatile int a = 0;
> > > 	volatile int64_t b = 0;
> > > 	while (1) {
> > > 		a = (a + x) % (1 << 30);
> > > 		b = (b + y) % (1 << 30);
> > > 		assert(a == b);
> > > 	}
> > > }
> > > ------------------------>8--------------------------------
> > > The ".. (b + y) .." should result in "vadd.i64 d19, d18, d16" or
> > > something like that.

Just for my curiosity, can you let me know what compiler version you're
using and the disassembly?  I'm actually a little surprised to see
NEON code being generated here, though the patch below fixes what
definitely looks like a context switch bug for combined v6+v7 kernels...

Cheers
---Dave

> > 
> > Hmm.
> > 
> > Can you send me the output of 'grep ^Features /proc/cpuinfo' please?
> 
> You may also like to try the patch below... it will probably fix your
> problem.
> 
> diff --git a/arch/arm/include/asm/vfpmacros.h b/arch/arm/include/asm/vfpmacros.h
> index a7aadbd..6a6f1e4 100644
> --- a/arch/arm/include/asm/vfpmacros.h
> +++ b/arch/arm/include/asm/vfpmacros.h
> @@ -28,7 +28,7 @@
>  	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
>  	ldr	\tmp, [\tmp, #0]
>  	tst	\tmp, #HWCAP_VFPv3D16
> -	ldceq	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
> +	ldceql	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
>  	addne	\base, \base, #32*4		    @ step over unused register space
>  #else
>  	VFPFMRX	\tmp, MVFR0			    @ Media and VFP Feature Register 0
> @@ -52,7 +52,7 @@
>  	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
>  	ldr	\tmp, [\tmp, #0]
>  	tst	\tmp, #HWCAP_VFPv3D16
> -	stceq	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
> +	stceql	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
>  	addne	\base, \base, #32*4		    @ step over unused register space
>  #else
>  	VFPFMRX	\tmp, MVFR0			    @ Media and VFP Feature Register 0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Problem with 64-bit registers on i.MX53

[Russell King - ARM Linux]

On Mon, Oct 08, 2012 at 06:50:52PM +0100, Dave Martin wrote:
> Just for my curiosity, can you let me know what compiler version you're
> using and the disassembly?  I'm actually a little surprised to see
> NEON code being generated here, though the patch below fixes what
> definitely looks like a context switch bug for combined v6+v7 kernels...

Well, one such compiler is gcc 4.6.3 in Ubuntu Precise LTS.

Problem with 64-bit registers on i.MX53

[Dave Martin]

On Mon, Oct 08, 2012 at 07:03:37PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 08, 2012 at 06:50:52PM +0100, Dave Martin wrote:
> > Just for my curiosity, can you let me know what compiler version you're
> > using and the disassembly?  I'm actually a little surprised to see
> > NEON code being generated here, though the patch below fixes what
> > definitely looks like a context switch bug for combined v6+v7 kernels...
> 
> Well, one such compiler is gcc 4.6.3 in Ubuntu Precise LTS.

Hmmm, I really need to upgrade...

Cheers
---Dave

Problem with 64-bit registers on i.MX53

[Michael Olbrich]

On Mon, Oct 08, 2012 at 06:10:11PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 08, 2012 at 06:01:24PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Oct 08, 2012 at 06:08:41PM +0200, Michael Olbrich wrote:
> > > I have a problem that looks like that 64-bit registers (I think) are not
> > > saved/restored correctly on a context switch. I've reduced it to the
> > > following test case:
> > > 
> > > - Latest Linux mainline kernel (v3.6-8559-ge9eca4d)
> > >   v3.5 is also affected
> > > - imx_v6_v7_defconfig
> > > - arch/arm/boot/dts/imx53-evk.dts
> > > 
> > > The following test program is compiled with "-mcpu=cortex-a8 -mfpu=neon
> > > -O2".
> > > ------------------------>8--------------------------------
> > > #include <inttypes.h>
> > > #include <assert.h>
> > > 
> > > volatile int x = 2;
> > > volatile int64_t y = 2;
> > > 
> > > int main() {
> > > 	volatile int a = 0;
> > > 	volatile int64_t b = 0;
> > > 	while (1) {
> > > 		a = (a + x) % (1 << 30);
> > > 		b = (b + y) % (1 << 30);
> > > 		assert(a == b);
> > > 	}
> > > }
> > > ------------------------>8--------------------------------
> > > The ".. (b + y) .." should result in "vadd.i64 d19, d18, d16" or
> > > something like that.
> > 
> > Hmm.
> > 
> > Can you send me the output of 'grep ^Features /proc/cpuinfo' please?

Features        : swp half thumb fastmult vfp edsp neon vfpv3 tls 

> You may also like to try the patch below... it will probably fix your
> problem.

This does indeed fix my problem. Is this a real fix or just a test to
narrow down the issue? I don't really understand what it does.
If it is a real fix,

Tested-By: Michael Olbrich <m.olbrich@pengutronix.de>

Regards,
Michael

> diff --git a/arch/arm/include/asm/vfpmacros.h b/arch/arm/include/asm/vfpmacros.h
> index a7aadbd..6a6f1e4 100644
> --- a/arch/arm/include/asm/vfpmacros.h
> +++ b/arch/arm/include/asm/vfpmacros.h
> @@ -28,7 +28,7 @@
>  	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
>  	ldr	\tmp, [\tmp, #0]
>  	tst	\tmp, #HWCAP_VFPv3D16
> -	ldceq	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
> +	ldceql	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
>  	addne	\base, \base, #32*4		    @ step over unused register space
>  #else
>  	VFPFMRX	\tmp, MVFR0			    @ Media and VFP Feature Register 0
> @@ -52,7 +52,7 @@
>  	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
>  	ldr	\tmp, [\tmp, #0]
>  	tst	\tmp, #HWCAP_VFPv3D16
> -	stceq	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
> +	stceql	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
>  	addne	\base, \base, #32*4		    @ step over unused register space
>  #else
>  	VFPFMRX	\tmp, MVFR0			    @ Media and VFP Feature Register 0

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Problem with 64-bit registers on i.MX53

[Michael Olbrich]

On Mon, Oct 08, 2012 at 06:50:52PM +0100, Dave Martin wrote:
> On Mon, Oct 08, 2012 at 06:10:11PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Oct 08, 2012 at 06:01:24PM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Oct 08, 2012 at 06:08:41PM +0200, Michael Olbrich wrote:
> > > > I have a problem that looks like that 64-bit registers (I think) are not
> > > > saved/restored correctly on a context switch. I've reduced it to the
> > > > following test case:
> > > > 
> > > > - Latest Linux mainline kernel (v3.6-8559-ge9eca4d)
> > > >   v3.5 is also affected
> > > > - imx_v6_v7_defconfig
> > > > - arch/arm/boot/dts/imx53-evk.dts
> > > > 
> > > > The following test program is compiled with "-mcpu=cortex-a8 -mfpu=neon
> > > > -O2".
> > > > ------------------------>8--------------------------------
> > > > #include <inttypes.h>
> > > > #include <assert.h>
> > > > 
> > > > volatile int x = 2;
> > > > volatile int64_t y = 2;
> > > > 
> > > > int main() {
> > > > 	volatile int a = 0;
> > > > 	volatile int64_t b = 0;
> > > > 	while (1) {
> > > > 		a = (a + x) % (1 << 30);
> > > > 		b = (b + y) % (1 << 30);
> > > > 		assert(a == b);
> > > > 	}
> > > > }
> > > > ------------------------>8--------------------------------
> > > > The ".. (b + y) .." should result in "vadd.i64 d19, d18, d16" or
> > > > something like that.
> 
> Just for my curiosity, can you let me know what compiler version you're
> using and the disassembly?  I'm actually a little surprised to see
> NEON code being generated here,

Im using oselas.toolchain which includes a gcc-linaro-4.6-2011.11. It
generates quite a bit of NEON code actually. I originally tracked down the
issue to a commit in libxcb: "xcb_in: Use 64-bit sequence numbers
internally everywhere.". The compiler generated NEON code to calculate
sequence numbers...

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Problem with 64-bit registers on i.MX53

[Uwe Kleine-König]

Hello,

On Mon, Oct 08, 2012 at 06:10:11PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 08, 2012 at 06:01:24PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Oct 08, 2012 at 06:08:41PM +0200, Michael Olbrich wrote:
> You may also like to try the patch below... it will probably fix your
> problem.
> 
> diff --git a/arch/arm/include/asm/vfpmacros.h b/arch/arm/include/asm/vfpmacros.h
> index a7aadbd..6a6f1e4 100644
> --- a/arch/arm/include/asm/vfpmacros.h
> +++ b/arch/arm/include/asm/vfpmacros.h
> @@ -28,7 +28,7 @@
>  	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
>  	ldr	\tmp, [\tmp, #0]
>  	tst	\tmp, #HWCAP_VFPv3D16
> -	ldceq	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
> +	ldceql	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
>  	addne	\base, \base, #32*4		    @ step over unused register space
>  #else
>  	VFPFMRX	\tmp, MVFR0			    @ Media and VFP Feature Register 0
> @@ -52,7 +52,7 @@
>  	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
>  	ldr	\tmp, [\tmp, #0]
>  	tst	\tmp, #HWCAP_VFPv3D16
> -	stceq	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
> +	stceql	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
According to the ARMARM for v7-A and v7-R (ARM DDI 0406B errata 2010 Q2)
the syntax is "STC{L}<c> ...", with a note "The pre-UAL syntax STC<c>L
is equivalent to STCL<c>.". Maybe the UAL-syntax should better be used?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Problem with 64-bit registers on i.MX53

[Dave Martin]

On Tue, Oct 09, 2012 at 11:02:37AM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Mon, Oct 08, 2012 at 06:10:11PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Oct 08, 2012 at 06:01:24PM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Oct 08, 2012 at 06:08:41PM +0200, Michael Olbrich wrote:
> > You may also like to try the patch below... it will probably fix your
> > problem.
> > 
> > diff --git a/arch/arm/include/asm/vfpmacros.h b/arch/arm/include/asm/vfpmacros.h
> > index a7aadbd..6a6f1e4 100644
> > --- a/arch/arm/include/asm/vfpmacros.h
> > +++ b/arch/arm/include/asm/vfpmacros.h
> > @@ -28,7 +28,7 @@
> >  	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
> >  	ldr	\tmp, [\tmp, #0]
> >  	tst	\tmp, #HWCAP_VFPv3D16
> > -	ldceq	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
> > +	ldceql	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
> >  	addne	\base, \base, #32*4		    @ step over unused register space
> >  #else
> >  	VFPFMRX	\tmp, MVFR0			    @ Media and VFP Feature Register 0
> > @@ -52,7 +52,7 @@
> >  	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
> >  	ldr	\tmp, [\tmp, #0]
> >  	tst	\tmp, #HWCAP_VFPv3D16
> > -	stceq	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
> > +	stceql	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
> According to the ARMARM for v7-A and v7-R (ARM DDI 0406B errata 2010 Q2)
> the syntax is "STC{L}<c> ...", with a note "The pre-UAL syntax STC<c>L
> is equivalent to STCL<c>.". Maybe the UAL-syntax should better be used?

The older stc<c>l type of syntax is used all over the place.  Code which
might need to be built by tools which pre-date unified syntax needs to
use the old syntax, so it is in common usage in the kernel in general.

This code presumably only gets built by new-enough tools for the unified
syntax to be usable, but support for the old syntax isn't going to
disappear from the tools any time soon, AFAIK.


The bug here was that the presence or absence of the "L" suffix is used
to encode bit 4 of the starting d-register number for these instructions.
The comment says d16-d31, but the instructions as written are actually
saving and restoring d0-d15...which is not so helpful since we already
handled those registers in the neighbouring code.

We could avoid this kind of bug by writing those VFP instructions
using the unified syntax native mnemonics (vstmia, vldmia -- since d16-
d31 never existed while the old fldmiad/fstmiad mnemonics were in use,
and the assembler doesn't accept them), but it is tricky to change the
assembler's notion of target CPU and FPU on-the-fly inside a header or
macro without messing things up.

Cheers
---Dave



> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel