ARM: socfpga: put back v7_invalidate_l1 in socfpga_secondary_startup

From: Dinh Nguyen <[email protected]>

The commit "02b4e2756e01 ARM: v7 setup function should invalidate L1 cache" caused the SoCFPGA to not boot reliably. About 20% of the time or roughly (1 in 5), booting the platform would cause this kernel panic:

CPU1: thread -1, cpu 1, socket 0, mpidr 80000001

Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
Modules linked in:
CPU: 1 PID: 0 Comm: swapper/1 Not tainted
4.1.0-rc8-next-20150617-00002-gdd1f624 #1
Hardware name: Altera SOCFPGA
task: eecaeac0 ti: eecce000 task.ti: eecce000
PC is at vfp_notifier+0x58/0x12c
LR is at notifier_call_chain+0x44/0x84
pc : [<c000a6bc>]    lr : [<c003d134>]    psr: 80000193

sp : eeccff48 ip : c06563c8 fp : eeccffd4
r10: eecaef80 r9 : ef1f1300 r8 : 00000002
r7 : eecd0000 r6 : c0656bc0 r5 : 00000000 r4 : eecd0000 r3 : c000a664 r2 : eecd0000 r1 : 00000002 r0 : c06563c8
Flags: Nzcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 10c5387d Table: 0000404a DAC: 00000015
Process swapper/1 (pid: 0, stack limit = 0xeecce218)

Stack: (0xeeccff48 to 0xeecd0000)

ff40: c000a664 ffffffff 00000000 c003d134 eecd0018 eecaeac0
ff60: c06648e0 0b52d2f9 c048cfa8 c003d18c 00000000 f0002100 00000001 c003d1ac
ff80: 00000000 eecaeac0 c064f300 c001369c c064b304 c0013140 00000000 ef1ed328
ffa0: eeccffe8 c001e760 c0486ec4 2eba2000 c06957c0 c06524dc 00000015 c06957c0
ffc0: c048c778 c064b304 c06957c0 00000000 eeccffdc c0486ec4 eeccffe4 c0487138
ffe0: 00000001 c00544e8 c0009494 c0697bc0 00000000 000094ac 7ef5bffd 3f39b3f8 [<c000a6bc>] (vfp_notifier) from [<c003d134>] (notifier_call_chain+0x44/0x84) [<c003d134>] (notifier_call_chain) from [<c003d18c>] (__atomic_notifier_call_chain+0x18/0x20)
[<c003d18c>] (__atomic_notifier_call_chain) from [<c003d1ac>] (atomic_notifier_call_chain+0x18/0x20)
[<c003d1ac>] (atomic_notifier_call_chain) from [<c001369c>] (__switch_to+0x34/0x58)

Code: e3a03002 e5843208 e3a00000 e8bd8038 (eef85a10)
---[ end trace 9eaea9661b3b550a ]---
Kernel panic - not syncing: Attempted to kill the idle task!
SMP: failed to stop secondary CPUs
---[ end Kernel panic - not syncing: Attempted to kill the idle task!

So this patch puts back the call to v7_invalidate_l1 in the secondary_startup path, and the platform is now able to boot up reliably.

Signed-off-by Dinh Nguyen <[email protected]>

Quoted 46 lines · dinguyen at opensource.altera.com

From: Dinh Nguyen <[email protected]>

The commit "02b4e2756e01 ARM: v7 setup function should invalidate L1 cache" caused the SoCFPGA to not boot reliably. About 20% of the time or roughly (1 in 5), booting the platform would cause this kernel panic:

CPU1: thread -1, cpu 1, socket 0, mpidr 80000001

Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
Modules linked in:
CPU: 1 PID: 0 Comm: swapper/1 Not tainted
4.1.0-rc8-next-20150617-00002-gdd1f624 #1
Hardware name: Altera SOCFPGA
task: eecaeac0 ti: eecce000 task.ti: eecce000
PC is at vfp_notifier+0x58/0x12c
LR is at notifier_call_chain+0x44/0x84
pc : [<c000a6bc>]    lr : [<c003d134>]    psr: 80000193

sp : eeccff48 ip : c06563c8 fp : eeccffd4
r10: eecaef80 r9 : ef1f1300 r8 : 00000002
r7 : eecd0000 r6 : c0656bc0 r5 : 00000000 r4 : eecd0000 r3 : c000a664 r2 : eecd0000 r1 : 00000002 r0 : c06563c8
Flags: Nzcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel
Control: 10c5387d Table: 0000404a DAC: 00000015
Process swapper/1 (pid: 0, stack limit = 0xeecce218)

Stack: (0xeeccff48 to 0xeecd0000)

ff40: c000a664 ffffffff 00000000 c003d134 eecd0018 eecaeac0
ff60: c06648e0 0b52d2f9 c048cfa8 c003d18c 00000000 f0002100 00000001 c003d1ac
ff80: 00000000 eecaeac0 c064f300 c001369c c064b304 c0013140 00000000 ef1ed328
ffa0: eeccffe8 c001e760 c0486ec4 2eba2000 c06957c0 c06524dc 00000015 c06957c0
ffc0: c048c778 c064b304 c06957c0 00000000 eeccffdc c0486ec4 eeccffe4 c0487138
ffe0: 00000001 c00544e8 c0009494 c0697bc0 00000000 000094ac 7ef5bffd 3f39b3f8 [<c000a6bc>] (vfp_notifier) from [<c003d134>] (notifier_call_chain+0x44/0x84) [<c003d134>] (notifier_call_chain) from [<c003d18c>] (__atomic_notifier_call_chain+0x18/0x20)
[<c003d18c>] (__atomic_notifier_call_chain) from [<c003d1ac>] (atomic_notifier_call_chain+0x18/0x20)
[<c003d1ac>] (atomic_notifier_call_chain) from [<c001369c>] (__switch_to+0x34/0x58)

Code: e3a03002 e5843208 e3a00000 e8bd8038 (eef85a10)
---[ end trace 9eaea9661b3b550a ]---
Kernel panic - not syncing: Attempted to kill the idle task!
SMP: failed to stop secondary CPUs
---[ end Kernel panic - not syncing: Attempted to kill the idle task!

So this patch puts back the call to v7_invalidate_l1 in the secondary_startup path, and the platform is now able to boot up reliably.

Quoted 1 line

Signed-off-by Dinh Nguyen <[email protected]>

Can you print the value of CPACR in the oops and reproduce please? I suspect, somehow, the CPACR is not allowing CPU1 access to the VFP, but this is not controlled by data as such (it can only go wrong if the hotplug notifier call chain misses calling into the VFP code.)

The value of CPACR is 0x00F00000. So cp11 and cp10 are privileged and user mode access.

Quoted 2 lines · Dinh Nguyen

The value of CPACR is 0x00F00000. So cp11 and cp10 are privileged and user mode access.

Hmm.

I think what you've found is a(nother) latent bug in the CPU bring up code.

For SMP CPUs, the sequence we're following during early initialisation is:

1. Enable SMP coherency.
2. Invalidate the caches.

If the cache contains rubbish, enabling SMP coherency before invalidating the cache is plainly an absurd thing to do.

Can you try the patch below - not tested in any way, so you may need to tweak it, but it should allow us to prove that point.

arch/arm/mm/proc-v7.S +6 -1

index 0716bbe19872..db5137fc297d 100644
@@ -275,6 +275,10 @@ __v7_b15mp_setup:
 __v7_ca17mp_setup:
 	mov	r10, #0
 1:
+	adr	r12, __v7_setup_stack		@ the local stack
+	stmia	r12, {r0-r5, r7, r9-r11, lr}
+	bl      v7_invalidate_l1
+	ldmia	r12, {r0-r5, r7, r9-r11, lr}
 #ifdef CONFIG_SMP
 	ALT_SMP(mrc	p15, 0, r0, c1, c0, 1)
 	ALT_UP(mov	r0, #(1 << 6))		@ fake it for UP

@@ -283,7 +287,7 @@ __v7_ca17mp_setup:
 	orreq	r0, r0, r10			@ Enable CPU-specific SMP bits
 	mcreq	p15, 0, r0, c1, c0, 1
 #endif
-	b	__v7_setup
+	b	__v7_setup_cont
 
 /*
  * Errata:

@@ -417,6 +421,7 @@ __v7_setup:
 	bl      v7_invalidate_l1
 	ldmia	r12, {r0-r5, r7, r9, r11, lr}
 
+__v7_setup_cont:
 	and	r0, r9, #0xff000000		@ ARM?
 	teq	r0, #0x41000000
 	bne	__errata_finish

Quoted 2 lines · Dinh Nguyen

The value of CPACR is 0x00F00000. So cp11 and cp10 are privileged and user mode access.

Hmm.

I think what you've found is a(nother) latent bug in the CPU bring up code.

For SMP CPUs, the sequence we're following during early initialisation is:

1. Enable SMP coherency.
2. Invalidate the caches.

If the cache contains rubbish, enabling SMP coherency before invalidating the cache is plainly an absurd thing to do.

Can you try the patch below - not tested in any way, so you may need to tweak it, but it should allow us to prove that point.

arch/arm/mm/proc-v7.S +0 -0

index 0716bbe19872..db5137fc297d 100644

arch/arm/mm/proc-v7.S +3 -2

 #ifdef CONFIG_SMP
 	ALT_SMP(mrc	p15, 0, r0, c1, c0, 1)
 	ALT_UP(mov	r0, #(1 << 6))		@ fake it for UP

@@ -283,7 +287,7 @@ __v7_ca17mp_setup:
 	orreq	r0, r0, r10			@ Enable CPU-specific SMP bits
 	mcreq	p15, 0, r0, c1, c0, 1
 #endif
-	b	__v7_setup
+	b	__v7_setup_cont
 
 /*
  * Errata:

@@ -417,6 +421,7 @@ __v7_setup:
 	bl      v7_invalidate_l1
 	ldmia	r12, {r0-r5, r7, r9, r11, lr}
 
+__v7_setup_cont:
 	and	r0, r9, #0xff000000		@ ARM?
 	teq	r0, #0x41000000
 	bne	__errata_finish

@@ -480,7 +485,7 @@ ENDPROC(__v7_setup)
 
 	.align	2
 __v7_setup_stack:
-	.space	4 * 11				@ 11 registers
+	.space	4 * 12				@ 12 registers
 
 	__INITDATA
 

Dear Russell,

Quoted 25 lines · On Wed, 8 Jul 2015 22:07:34 +0100 Russell King - ARM Linux…

Quoted 2 lines · Dinh Nguyen

The value of CPACR is 0x00F00000. So cp11 and cp10 are privileged and user mode access.

Hmm.

I think what you've found is a(nother) latent bug in the CPU bring up code.

For SMP CPUs, the sequence we're following during early initialisation is:

1. Enable SMP coherency.
2. Invalidate the caches.

If the cache contains rubbish, enabling SMP coherency before invalidating the cache is plainly an absurd thing to do.

Can you try the patch below - not tested in any way, so you may need to tweak it, but it should allow us to prove that point.

arch/arm/mm/proc-v7.S +0 -0

index 0716bbe19872..db5137fc297d 100644

arch/arm/mm/proc-v7.S __v7_b15mp_setup: · line 281

Quoted 7 lines

arch/arm/mm/proc-v7.S +4 -0

@@ -275,6 +275,10 @@ __v7_b15mp_setup:
 __v7_ca17mp_setup:
 	mov	r10, #0
 1:
+	adr	r12, __v7_setup_stack		@ the local stack
+	stmia	r12, {r0-r5, r7, r9-r11, lr}
+	bl      v7_invalidate_l1
+	ldmia	r12, {r0-r5, r7, r9-r11, lr}

Some CPUs such as CA7 need enable SMP before any cache maintenance.

CA7 TRM says something about SMP bit:
"You must ensure this bit is set to 1 before the caches and MMU are enabled, or any cache and TLB maintenance operations are performed."

Also CA7 would invalidate L1 automatically once reset, can we remove the invalidate op in CA7 case?

Quoted 40 lines · Jisheng Zhang

Dear Russell,

Quoted 25 lines · On Wed, 8 Jul 2015 22:07:34 +0100 Russell King - ARM Linux…

Quoted 2 lines · Dinh Nguyen

The value of CPACR is 0x00F00000. So cp11 and cp10 are privileged and user mode access.

Hmm.

I think what you've found is a(nother) latent bug in the CPU bring up code.

For SMP CPUs, the sequence we're following during early initialisation is:

1. Enable SMP coherency.
2. Invalidate the caches.

If the cache contains rubbish, enabling SMP coherency before invalidating the cache is plainly an absurd thing to do.

Can you try the patch below - not tested in any way, so you may need to tweak it, but it should allow us to prove that point.

arch/arm/mm/proc-v7.S +0 -0

index 0716bbe19872..db5137fc297d 100644

arch/arm/mm/proc-v7.S __v7_b15mp_setup: · line 281

Quoted 7 lines

arch/arm/mm/proc-v7.S +4 -0

@@ -275,6 +275,10 @@ __v7_b15mp_setup:
 __v7_ca17mp_setup:
 	mov	r10, #0
 1:
+	adr	r12, __v7_setup_stack		@ the local stack
+	stmia	r12, {r0-r5, r7, r9-r11, lr}
+	bl      v7_invalidate_l1
+	ldmia	r12, {r0-r5, r7, r9-r11, lr}

Some CPUs such as CA7 need enable SMP before any cache maintenance.

Quoted 3 lines

CA7 TRM says something about SMP bit:
"You must ensure this bit is set to 1 before the caches and MMU are enabled, or any cache and TLB maintenance operations are performed."

Frankly, that's wrong for two reasons. Think about it for a moment...

If the cache contains crap - in other words, it contains random uninitialised data in the cache lines at random locations, some of which are marked valid and some of which are marked dirty - then enabling the SMP bit puts the caches into coherent mode, and they join the coherent cluster.

That means those cache lines containing crap become visible to other CPUs in the cluster, and can be migrated to other CPUs, and the crap data in them becomes visible to other CPUs. This leads to state corruption on other CPUs in the cluster.

Moreover, the cache invalidation of the local L1 cache is broadcast to other CPUs in the cluster, and _their_ caches are also invalidated, again, leading to state corruption on already running CPUs. We don't want the invalidation of the incoming CPU to be broadcast to the other CPUs.

This is all round a very bad thing.

Quoted 2 lines

Also CA7 would invalidate L1 automatically once reset, can we remove the invalidate op in CA7 case?

No, because we enter this path from multiple different situations, eg, after the decompressor has run, after the boot loader has run which may have enabled caches and not properly invalidated them prior to calling the kernel.

Dear Russell,

Quoted 69 lines · On Thu, 9 Jul 2015 08:57:17 +0100 Russell King - ARM Linux…

Quoted 40 lines · Jisheng Zhang

Dear Russell,

Quoted 25 lines · On Wed, 8 Jul 2015 22:07:34 +0100 Russell King - ARM Linux…

Quoted 2 lines · Dinh Nguyen

The value of CPACR is 0x00F00000. So cp11 and cp10 are privileged and user mode access.

Hmm.

I think what you've found is a(nother) latent bug in the CPU bring up code.

For SMP CPUs, the sequence we're following during early initialisation is:

1. Enable SMP coherency.
2. Invalidate the caches.

If the cache contains rubbish, enabling SMP coherency before invalidating the cache is plainly an absurd thing to do.

Can you try the patch below - not tested in any way, so you may need to tweak it, but it should allow us to prove that point.

arch/arm/mm/proc-v7.S +0 -0

index 0716bbe19872..db5137fc297d 100644

arch/arm/mm/proc-v7.S __v7_b15mp_setup: · line 281

Quoted 7 lines

arch/arm/mm/proc-v7.S +4 -0

@@ -275,6 +275,10 @@ __v7_b15mp_setup:
 __v7_ca17mp_setup:
 	mov	r10, #0
 1:
+	adr	r12, __v7_setup_stack		@ the local stack
+	stmia	r12, {r0-r5, r7, r9-r11, lr}
+	bl      v7_invalidate_l1
+	ldmia	r12, {r0-r5, r7, r9-r11, lr}

Some CPUs such as CA7 need enable SMP before any cache maintenance.

Quoted 3 lines

CA7 TRM says something about SMP bit:
"You must ensure this bit is set to 1 before the caches and MMU are enabled, or any cache and TLB maintenance operations are performed."

Frankly, that's wrong for two reasons. Think about it for a moment...

If the cache contains crap - in other words, it contains random uninitialised data in the cache lines at random locations, some of which are marked valid and some of which are marked dirty - then enabling the SMP bit puts the caches into coherent mode, and they join the coherent cluster.

That means those cache lines containing crap become visible to other CPUs in the cluster, and can be migrated to other CPUs, and the crap data in them becomes visible to other CPUs. This leads to state corruption on other CPUs in the cluster.

Moreover, the cache invalidation of the local L1 cache is broadcast to other CPUs in the cluster, and _their_ caches are also invalidated, again, leading to state corruption on already running CPUs. We don't want the invalidation of the incoming CPU to be broadcast to the other CPUs.

This is all round a very bad thing.

Quoted 2 lines

Also CA7 would invalidate L1 automatically once reset, can we remove the invalidate op in CA7 case?

Quoted 5 lines

No, because we enter this path from multiple different situations, eg, after the decompressor has run, after the boot loader has run which may have enabled caches and not properly invalidated them prior to calling the kernel.

Got it. Thanks very much for your detailed explanation!

Hi Russell,

Just wondering if you are still planning to send this patch and if you need me to do anything to help?

Quoted 4 lines · Dinh Nguyen

Hi Russell,

Just wondering if you are still planning to send this patch and if you need me to do anything to help?

Giving a tested-by tag would help. Thanks. :)

Quoted 2 lines · Dinh Nguyen

The value of CPACR is 0x00F00000. So cp11 and cp10 are privileged and user mode access.

Hmm.

I think what you've found is a(nother) latent bug in the CPU bring up code.

For SMP CPUs, the sequence we're following during early initialisation is:

1. Enable SMP coherency.
2. Invalidate the caches.

If the cache contains rubbish, enabling SMP coherency before invalidating the cache is plainly an absurd thing to do.

Can you try the patch below - not tested in any way, so you may need to tweak it, but it should allow us to prove that point.

arch/arm/mm/proc-v7.S +6 -1

index 0716bbe19872..db5137fc297d 100644
@@ -275,6 +275,10 @@ __v7_b15mp_setup:
 __v7_ca17mp_setup:
 	mov	r10, #0
 1:
+	adr	r12, __v7_setup_stack		@ the local stack
+	stmia	r12, {r0-r5, r7, r9-r11, lr}
+	bl      v7_invalidate_l1
+	ldmia	r12, {r0-r5, r7, r9-r11, lr}
 #ifdef CONFIG_SMP
 	ALT_SMP(mrc	p15, 0, r0, c1, c0, 1)
 	ALT_UP(mov	r0, #(1 << 6))		@ fake it for UP

@@ -283,7 +287,7 @@ __v7_ca17mp_setup:
 	orreq	r0, r0, r10			@ Enable CPU-specific SMP bits
 	mcreq	p15, 0, r0, c1, c0, 1
 #endif
-	b	__v7_setup
+	b	__v7_setup_cont
 
 /*
  * Errata:

@@ -417,6 +421,7 @@ __v7_setup:
 	bl      v7_invalidate_l1
 	ldmia	r12, {r0-r5, r7, r9, r11, lr}
 
+__v7_setup_cont:
 	and	r0, r9, #0xff000000		@ ARM?
 	teq	r0, #0x41000000
 	bne	__errata_finish

Quoted 59 lines · Russell King - ARM Linux

Quoted 2 lines · Dinh Nguyen

The value of CPACR is 0x00F00000. So cp11 and cp10 are privileged and user mode access.

Hmm.

I think what you've found is a(nother) latent bug in the CPU bring up code.

For SMP CPUs, the sequence we're following during early initialisation is:

1. Enable SMP coherency.
2. Invalidate the caches.

If the cache contains rubbish, enabling SMP coherency before invalidating the cache is plainly an absurd thing to do.

Can you try the patch below - not tested in any way, so you may need to tweak it, but it should allow us to prove that point.

arch/arm/mm/proc-v7.S +7 -2

index 0716bbe19872..db5137fc297d 100644
@@ -275,6 +275,10 @@ __v7_b15mp_setup:
 __v7_ca17mp_setup:
 	mov	r10, #0
 1:
+	adr	r12, __v7_setup_stack		@ the local stack
+	stmia	r12, {r0-r5, r7, r9-r11, lr}
+	bl      v7_invalidate_l1
+	ldmia	r12, {r0-r5, r7, r9-r11, lr}
 #ifdef CONFIG_SMP
 	ALT_SMP(mrc	p15, 0, r0, c1, c0, 1)
 	ALT_UP(mov	r0, #(1 << 6))		@ fake it for UP

@@ -283,7 +287,7 @@ __v7_ca17mp_setup:
 	orreq	r0, r0, r10			@ Enable CPU-specific SMP bits
 	mcreq	p15, 0, r0, c1, c0, 1
 #endif
-	b	__v7_setup
+	b	__v7_setup_cont
 
 /*
  * Errata:

@@ -417,6 +421,7 @@ __v7_setup:
 	bl      v7_invalidate_l1
 	ldmia	r12, {r0-r5, r7, r9, r11, lr}
 
+__v7_setup_cont:
 	and	r0, r9, #0xff000000		@ ARM?
 	teq	r0, #0x41000000
 	bne	__errata_finish

@@ -480,7 +485,7 @@ ENDPROC(__v7_setup)
 
 	.align	2
 __v7_setup_stack:
-	.space	4 * 11				@ 11 registers
+	.space	4 * 12				@ 12 registers
 

arch/arm/mm/proc-v7.S ENDPROC(__v7_setup) · line 491

Quoted 3 lines

arch/arm/mm/proc-v7.S +0 -0

 	__INITDATA
 

For this patch, please feel free to add:

Tested-by Dinh Nguyen <[email protected]>