[PATCH] powerpc: Fix __flush_icache_range on 44x

[PATCH] powerpc: Fix __flush_icache_range on 44x

[Josh Boyer]

The ptrace POKETEXT interface allows a process to modify the text pages of
a child process being ptraced, usually to insert breakpoints via trap
instructions.  The kernel eventually calls copy_to_user_page, which in turn
calls __flush_icache_range to invalidate the icache lines for the child
process.

However, this function does not work on 44x due to the icache being virtually
indexed.  This was noticed by a breakpoint being triggered after it had been
cleared by ltrace on a 440EPx board.  The convenient solution is to do a
flash invalidate of the icache in the __flush_icache_range function.

Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>

---

diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
index 15f28e0..c9805a4 100644
--- a/arch/powerpc/kernel/misc_32.S
+++ b/arch/powerpc/kernel/misc_32.S
@@ -346,6 +346,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
 2:	icbi	0,r6
 	addi	r6,r6,L1_CACHE_BYTES
 	bdnz	2b
+#ifdef CONFIG_44x
+	iccci	r0, r0
+#endif
 	sync				/* additional sync needed on g4 */
 	isync
 	blr

Re: [PATCH] powerpc: Fix __flush_icache_range on 44x

[Josh Boyer]

On Mon, Aug 17, 2009 at 09:41:36AM -0400, Josh Boyer wrote:
>The ptrace POKETEXT interface allows a process to modify the text pages of
>a child process being ptraced, usually to insert breakpoints via trap
>instructions.  The kernel eventually calls copy_to_user_page, which in turn
>calls __flush_icache_range to invalidate the icache lines for the child
>process.
>
>However, this function does not work on 44x due to the icache being virtually
>indexed.  This was noticed by a breakpoint being triggered after it had been
>cleared by ltrace on a 440EPx board.  The convenient solution is to do a
>flash invalidate of the icache in the __flush_icache_range function.
>
>Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
>
>---
>
>diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
>index 15f28e0..c9805a4 100644
>--- a/arch/powerpc/kernel/misc_32.S
>+++ b/arch/powerpc/kernel/misc_32.S
>@@ -346,6 +346,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> 2:	icbi	0,r6
> 	addi	r6,r6,L1_CACHE_BYTES
> 	bdnz	2b
>+#ifdef CONFIG_44x
>+	iccci	r0, r0
>+#endif

Olof pointed out that we could probably do the iccci before the icbi loop and
just skip that loop entirely on 44x.  This is most certainly valid, but at
this particular moment I don't have time to try and reproduce the issue with
an alternative fix and I wanted to get _something_ out there to fix the issue.  

I suck for that, I know.

josh

Re: [PATCH] powerpc: Fix __flush_icache_range on 44x

[Benjamin Herrenschmidt]

On Mon, 2009-08-17 at 12:07 -0400, Josh Boyer wrote:
> 
> Olof pointed out that we could probably do the iccci before the icbi loop and
> just skip that loop entirely on 44x.  This is most certainly valid, but at
> this particular moment I don't have time to try and reproduce the issue with
> an alternative fix and I wanted to get _something_ out there to fix the issue.  
> 
> I suck for that, I know.

Well, I can massage your patch if you want. The fact is, the icbi loop
and iccci are definitely redundant :-)

Cheers,
Ben.

Re: [PATCH] powerpc: Fix __flush_icache_range on 44x

[Josh Boyer]

On Tue, Aug 18, 2009 at 07:46:28AM +1000, Benjamin Herrenschmidt wrote:
>On Mon, 2009-08-17 at 12:07 -0400, Josh Boyer wrote:
>> 
>> Olof pointed out that we could probably do the iccci before the icbi loop and
>> just skip that loop entirely on 44x.  This is most certainly valid, but at
>> this particular moment I don't have time to try and reproduce the issue with
>> an alternative fix and I wanted to get _something_ out there to fix the issue.  
>> 
>> I suck for that, I know.
>
>Well, I can massage your patch if you want. The fact is, the icbi loop
>and iccci are definitely redundant :-)

You can if you'd like.  My biggest concern is getting time to recreate.  I
think I'll have time later in the week if you'd like to wait until then.
I simply didn't want to send out a patch that I wasn't sure fixed the issue.

josh

Re: [PATCH] powerpc: Fix __flush_icache_range on 44x

[Benjamin Herrenschmidt]

On Mon, 2009-08-17 at 20:16 -0400, Josh Boyer wrote:
> 
> You can if you'd like.  My biggest concern is getting time to
> recreate.  I
> think I'll have time later in the week if you'd like to wait until
> then.
> I simply didn't want to send out a patch that I wasn't sure fixed the
> issue.
> 
That's ok. It's a bug fix so it's less constrained by the upcoming merge
window and we can send it back to -stable later.

Cheers,
Ben.

Re: [PATCH] powerpc: Fix __flush_icache_range on 44x

[Benjamin Herrenschmidt]

On Mon, 2009-08-17 at 09:41 -0400, Josh Boyer wrote:
> The ptrace POKETEXT interface allows a process to modify the text pages of
> a child process being ptraced, usually to insert breakpoints via trap
> instructions.  The kernel eventually calls copy_to_user_page, which in turn
> calls __flush_icache_range to invalidate the icache lines for the child
> process.
> 
> However, this function does not work on 44x due to the icache being virtually
> indexed.  This was noticed by a breakpoint being triggered after it had been
> cleared by ltrace on a 440EPx board.  The convenient solution is to do a
> flash invalidate of the icache in the __flush_icache_range function.
> 
> Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>

I've put it in -test for now, but I'd like you to respin when you get a
chance with:

 - removing the icbi loop in the 440 case
 - adding a nice fat comment explaining why next to the iccci
instruction itself so we don't be tempted to remove it.

Cheers,
Ben.

> ---
> 
> diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
> index 15f28e0..c9805a4 100644
> --- a/arch/powerpc/kernel/misc_32.S
> +++ b/arch/powerpc/kernel/misc_32.S
> @@ -346,6 +346,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
>  2:	icbi	0,r6
>  	addi	r6,r6,L1_CACHE_BYTES
>  	bdnz	2b
> +#ifdef CONFIG_44x
> +	iccci	r0, r0
> +#endif
>  	sync				/* additional sync needed on g4 */
>  	isync
>  	blr