ARM: v7 setup function should invalidate L1 cache
diff mbox

Message ID 2080993.BUNZpO9uLL@wuerfel
State New
Headers show

Commit Message

Arnd Bergmann May 19, 2015, 9:55 p.m. UTC
On Tuesday 19 May 2015 23:44:58 Heiko Stuebner wrote:
> 
> Michael Niewoehner tested this on a rk3188 (Cortex-A9) and wrote in [0]
> > Tested-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
> > 
> > Tested on Radxa Rock Pro with RK3188.
> > The kernel panics on reboot I had before and also a kernel BUG when running
> > "memtester 1900M" went away and the rock seems to run stable now.
> 

We should probably create a separate fix for that and add it to the stable
kernels then. I would suggest something like the untested patch below,
which takes advantage of the fact that we already have separate assignments
for the start_secondary function pointer for A9 and A17.

	Arnd

Comments

Russell King - ARM Linux May 19, 2015, 10:07 p.m. UTC | #1
On Tue, May 19, 2015 at 11:55:12PM +0200, Arnd Bergmann wrote:
> On Tuesday 19 May 2015 23:44:58 Heiko Stuebner wrote:
> > 
> > Michael Niewoehner tested this on a rk3188 (Cortex-A9) and wrote in [0]
> > > Tested-by: Michael Niewoehner <mniewoeh@stud.hs-offenburg.de>
> > > 
> > > Tested on Radxa Rock Pro with RK3188.
> > > The kernel panics on reboot I had before and also a kernel BUG when running
> > > "memtester 1900M" went away and the rock seems to run stable now.
> > 
> 
> We should probably create a separate fix for that and add it to the stable
> kernels then. I would suggest something like the untested patch below,
> which takes advantage of the fact that we already have separate assignments
> for the start_secondary function pointer for A9 and A17.

Your patch is also continuing the pattern with this code...

> diff --git a/arch/arm/mach-rockchip/headsmp.S b/arch/arm/mach-rockchip/headsmp.S
> index 46c22dedf632..ae0077e8fe98 100644
> --- a/arch/arm/mach-rockchip/headsmp.S
> +++ b/arch/arm/mach-rockchip/headsmp.S
> @@ -16,9 +16,6 @@
>  #include <linux/init.h>
>  
>  ENTRY(rockchip_secondary_startup)
> -	mrc	p15, 0, r0, c0, c0, 0	@ read main ID register
> -	ldr	r1, =0x00000c09		@ Cortex-A9 primary part number
> -	teq	r0, r1
>  	beq	v7_invalidate_l1
>  	b	secondary_startup

If you look carefully at this, you'll notice that it's utter crap.
(Sorry, but it is.)  It has two problems:

1. It'll never match a Cortex-A9 CPU.  Cortex-A9 has a MIDR value of
   0x412fc09a, not 0x00000c09.  The bit position of the part number
   field isn't even right.

2. If it does match, then we branch to "v7_invalidate_l1" without setting
   the link register: we'll never return back here (we'll return to whatever
   random value the link register contains) and so we'll never make it to
   secondary_startup.  *Thankfully*, because of (1), this branch will
   never be taken - this is it's saving grace.

Your patch introduces a /third/ form of crapiness:

3. If the PSR happens to have Z=1, the "beq" instruction will be taken,
   thereby crashing the system because of (2).

The /simplest/ change which would fix this problem is to just change
proc-v7.S.  The remainder is effectively a cleanup removing redundant
code.
Arnd Bergmann May 19, 2015, 10:18 p.m. UTC | #2
On Tuesday 19 May 2015 23:07:21 Russell King - ARM Linux wrote:
> If you look carefully at this, you'll notice that it's utter crap.
> (Sorry, but it is.)  It has two problems:
> 
> 1. It'll never match a Cortex-A9 CPU.  Cortex-A9 has a MIDR value of
>    0x412fc09a, not 0x00000c09.  The bit position of the part number
>    field isn't even right.
>
> 2. If it does match, then we branch to "v7_invalidate_l1" without setting
>    the link register: we'll never return back here (we'll return to whatever
>    random value the link register contains) and so we'll never make it to
>    secondary_startup.  *Thankfully*, because of (1), this branch will
>    never be taken - this is it's saving grace.

Yes, I've understood both before.
 
> Your patch introduces a /third/ form of crapiness:
> 
> 3. If the PSR happens to have Z=1, the "beq" instruction will be taken,
>    thereby crashing the system because of (2).

Right, this was the result of sloppiness on my side when fat-fingering
a patch for illustration.
 
> The /simplest/ change which would fix this problem is to just change
> proc-v7.S.  The remainder is effectively a cleanup removing redundant
> code.

Fair enough. I wasn't sure if we're confident enough about that
change already to put it into stable backports. If the risk is low
enough, that's fine.

	Arnd
Russell King - ARM Linux May 19, 2015, 10:32 p.m. UTC | #3
On Wed, May 20, 2015 at 12:18:43AM +0200, Arnd Bergmann wrote:
> On Tuesday 19 May 2015 23:07:21 Russell King - ARM Linux wrote:
> > The /simplest/ change which would fix this problem is to just change
> > proc-v7.S.  The remainder is effectively a cleanup removing redundant
> > code.
> 
> Fair enough. I wasn't sure if we're confident enough about that
> change already to put it into stable backports. If the risk is low
> enough, that's fine.

When the kernel is entered from the decompressor, the last steps just
before calling the kernel is to clean and invalidate the caches, then
turn them off, and then jump to the kernel.

So, there _should_ be no dirty cache lines which we rely upon at this
point - if there was dirty data in the cache, then it hasn't been flushed
to memory, and we're potentially going to crash because some part of
the decompressed kernel image hasn't been flushed out to memory.

So I'll put a confidence value of 99.999% on this.

The case which might bite people is if they bypass the decompressor.
Even then, leaving bits of the kernel image in the caches is really
dodgy.

Patch
diff mbox

diff --git a/arch/arm/mach-rockchip/headsmp.S b/arch/arm/mach-rockchip/headsmp.S
index 46c22dedf632..ae0077e8fe98 100644
--- a/arch/arm/mach-rockchip/headsmp.S
+++ b/arch/arm/mach-rockchip/headsmp.S
@@ -16,9 +16,6 @@ 
 #include <linux/init.h>
 
 ENTRY(rockchip_secondary_startup)
-	mrc	p15, 0, r0, c0, c0, 0	@ read main ID register
-	ldr	r1, =0x00000c09		@ Cortex-A9 primary part number
-	teq	r0, r1
 	beq	v7_invalidate_l1
 	b	secondary_startup
 ENDPROC(rockchip_secondary_startup)
diff --git a/arch/arm/mach-rockchip/platsmp.c b/arch/arm/mach-rockchip/platsmp.c
index 5b4ca3c3c879..5f46724cca2f 100644
--- a/arch/arm/mach-rockchip/platsmp.c
+++ b/arch/arm/mach-rockchip/platsmp.c
@@ -149,8 +149,7 @@  static int __cpuinit rockchip_boot_secondary(unsigned int cpu,
 		 * sram_base_addr + 8: start address for pc
 		 * */
 		udelay(10);
-		writel(virt_to_phys(rockchip_secondary_startup),
-			sram_base_addr + 8);
+		writel(virt_to_phys(secondary_startup), sram_base_addr + 8);
 		writel(0xDEADBEAF, sram_base_addr + 4);
 		dsb_sev();
 	}