[PATCH 0/3] powerpc: several patches for icache flush

[PATCH 0/3] powerpc: several patches for icache flush

[Kevin Hao]

These patches passed the build test with the following configurations.
	ppc40x_defconfig
        ppc64e_defconfig
        ppc64_defconfig
	mpc85xx_defconfig
	mpc85xx_smp_defconfig
        corenet32_smp_defconfig
        corenet64_smp_defconfig
        ppc44x_defconfig
	mpc885_ads_defconfig
        pseries_defconfig

Boot test on a p5020ds board.

---
Kevin Hao (3):
  powerpc: move the testing of CPU_FTR_COHERENT_ICACHE into    
    __flush_icache_range
  powerpc: remove the symbol __flush_icache_range
  powerpc: check CPU_FTR_COHERENT_ICACHE in __flush_dcache_icache for   
     64bit kernel

 arch/powerpc/include/asm/cacheflush.h | 8 +-------
 arch/powerpc/kernel/misc_32.S         | 2 +-
 arch/powerpc/kernel/misc_64.S         | 9 +++++++--
 arch/powerpc/kernel/ppc_ksyms.c       | 1 -
 4 files changed, 9 insertions(+), 11 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] powerpc: move the testing of CPU_FTR_COHERENT_ICACHE into __flush_icache_range

[Kevin Hao]

In function flush_icache_range(), we use cpu_has_feature() to test
the feature bit of CPU_FTR_COHERENT_ICACHE. But this seems not optimal
for two reasons:
 a) For ppc32, the function __flush_icache_range() already do this
    check with the macro END_FTR_SECTION_IFSET.
 b) Compare with the cpu_has_feature(), the method of using macro
    END_FTR_SECTION_IFSET will not introduce any runtime overhead.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 arch/powerpc/include/asm/cacheflush.h | 3 +--
 arch/powerpc/kernel/misc_64.S         | 4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
index b843e35..60b620d 100644
--- a/arch/powerpc/include/asm/cacheflush.h
+++ b/arch/powerpc/include/asm/cacheflush.h
@@ -35,8 +35,7 @@ extern void __flush_disable_L1(void);
 extern void __flush_icache_range(unsigned long, unsigned long);
 static inline void flush_icache_range(unsigned long start, unsigned long stop)
 {
-	if (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE))
-		__flush_icache_range(start, stop);
+	__flush_icache_range(start, stop);
 }
 
 extern void flush_icache_user_range(struct vm_area_struct *vma,
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 6820e45..74d87f1 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -68,7 +68,9 @@ PPC64_CACHES:
  */
 
 _KPROBE(__flush_icache_range)
-
+BEGIN_FTR_SECTION
+	blr
+END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
 /*
  * Flush the data cache to memory 
  * 
-- 
1.8.3.1

Re: [PATCH 1/3] powerpc: move the testing of CPU_FTR_COHERENT_ICACHE into __flush_icache_range

[Benjamin Herrenschmidt]

On Tue, 2013-08-06 at 18:23 +0800, Kevin Hao wrote:
> In function flush_icache_range(), we use cpu_has_feature() to test
> the feature bit of CPU_FTR_COHERENT_ICACHE. But this seems not optimal
> for two reasons:
>  a) For ppc32, the function __flush_icache_range() already do this
>     check with the macro END_FTR_SECTION_IFSET.
>  b) Compare with the cpu_has_feature(), the method of using macro
>     END_FTR_SECTION_IFSET will not introduce any runtime overhead.

Nak.

It adds the overhead of calling into a function :-)

What about modifying cpu_has_feature to use jump labels ? It might solve
the problem of no runtime overhead ... however it might also be hard to
keep the ability to remove the whole statement at compile time if the
bit doesn't fit in the POSSIBLE mask... unless you find the right macro
magic.

In any case, I suspect the function call introduces more overhead than
the bit test + conditional branch which will generally predict very
well, so the patch as-is is probably a regression.

Did you measure ?

Ben.


> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
>  arch/powerpc/include/asm/cacheflush.h | 3 +--
>  arch/powerpc/kernel/misc_64.S         | 4 +++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
> index b843e35..60b620d 100644
> --- a/arch/powerpc/include/asm/cacheflush.h
> +++ b/arch/powerpc/include/asm/cacheflush.h
> @@ -35,8 +35,7 @@ extern void __flush_disable_L1(void);
>  extern void __flush_icache_range(unsigned long, unsigned long);
>  static inline void flush_icache_range(unsigned long start, unsigned long stop)
>  {
> -	if (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE))
> -		__flush_icache_range(start, stop);
> +	__flush_icache_range(start, stop);
>  }
>  
>  extern void flush_icache_user_range(struct vm_area_struct *vma,
> diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> index 6820e45..74d87f1 100644
> --- a/arch/powerpc/kernel/misc_64.S
> +++ b/arch/powerpc/kernel/misc_64.S
> @@ -68,7 +68,9 @@ PPC64_CACHES:
>   */
>  
>  _KPROBE(__flush_icache_range)
> -
> +BEGIN_FTR_SECTION
> +	blr
> +END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
>  /*
>   * Flush the data cache to memory 
>   * 

Re: [PATCH 1/3] powerpc: move the testing of CPU_FTR_COHERENT_ICACHE into __flush_icache_range

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 4327 bytes --]

On Tue, Aug 06, 2013 at 08:35:10PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2013-08-06 at 18:23 +0800, Kevin Hao wrote:
> > In function flush_icache_range(), we use cpu_has_feature() to test
> > the feature bit of CPU_FTR_COHERENT_ICACHE. But this seems not optimal
> > for two reasons:
> >  a) For ppc32, the function __flush_icache_range() already do this
> >     check with the macro END_FTR_SECTION_IFSET.
> >  b) Compare with the cpu_has_feature(), the method of using macro
> >     END_FTR_SECTION_IFSET will not introduce any runtime overhead.
> 
> Nak.
> 
> It adds the overhead of calling into a function :-)
> 
> What about modifying cpu_has_feature to use jump labels ? 

That's a great idea. I would like to gave it a try later.

>It might solve
> the problem of no runtime overhead ... however it might also be hard to
> keep the ability to remove the whole statement at compile time if the
> bit doesn't fit in the POSSIBLE mask... unless you find the right macro
> magic.
> 
> In any case, I suspect the function call introduces more overhead than
> the bit test + conditional branch which will generally predict very
> well, so the patch as-is is probably a regression.

I don't think so. For the 64bit CPU which has a non-coherent icache,
there is no any effect introduced by this patch since (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE))
always yield true at compile time. But for the 64bit CPU which does have
the coherent icache, the following is the asm code before applying this patch:

  c000000000023b4c:	e9 22 86 68 	ld      r9,-31128(r2)
  c000000000023b50:	e9 29 00 00 	ld      r9,0(r9)
  c000000000023b54:	e9 29 00 10 	ld      r9,16(r9)
  c000000000023b58:	79 2a 07 e1 	clrldi. r10,r9,63
  c000000000023b5c:	41 82 00 94 	beq     c000000000023bf0 <.handle_rt_signal64+0x670>
  ... 
  c000000000023bf0:	7f 23 cb 78 	mr      r3,r25
  c000000000023bf4:	7f 64 db 78 	mr      r4,r27
  c000000000023bf8:	48 7a 65 99 	bl      c0000000007ca190 <.__flush_icache_range>

After applying this patch, the following code is generated:
  c000000000023b48:	7f 23 cb 78 	mr      r3,r25
  c000000000023b4c:	7f 64 db 78 	mr      r4,r27
  c000000000023b50:	48 7a 65 81 	bl      c0000000007ca0d0 <.flush_icache_range>


The run path for these two cases are:

        before                              after
  ld r9,-31128(r2)                        mr r3,r25
  ld r9,0(r9)                             mr r4,r27
  ld r9,16(r9)                            bl flush_icache_range
  clrldi. r10,r9,63                       blr
  beq   xxxx

So I don't think this will introduce more overhead than before.
On the contrary, I believe it yield less overhead than before.
Correct me if I am wrong.

> 
> Did you measure ?

No. I don't have a access to 64bit CPU which has a coherent icache.
For a non-coherent icache 64bit CPU this patch will not cause any effect.

Thanks,
Kevin

> 
> Ben.
> 
> 
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> >  arch/powerpc/include/asm/cacheflush.h | 3 +--
> >  arch/powerpc/kernel/misc_64.S         | 4 +++-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
> > index b843e35..60b620d 100644
> > --- a/arch/powerpc/include/asm/cacheflush.h
> > +++ b/arch/powerpc/include/asm/cacheflush.h
> > @@ -35,8 +35,7 @@ extern void __flush_disable_L1(void);
> >  extern void __flush_icache_range(unsigned long, unsigned long);
> >  static inline void flush_icache_range(unsigned long start, unsigned long stop)
> >  {
> > -	if (!cpu_has_feature(CPU_FTR_COHERENT_ICACHE))
> > -		__flush_icache_range(start, stop);
> > +	__flush_icache_range(start, stop);
> >  }
> >  
> >  extern void flush_icache_user_range(struct vm_area_struct *vma,
> > diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> > index 6820e45..74d87f1 100644
> > --- a/arch/powerpc/kernel/misc_64.S
> > +++ b/arch/powerpc/kernel/misc_64.S
> > @@ -68,7 +68,9 @@ PPC64_CACHES:
> >   */
> >  
> >  _KPROBE(__flush_icache_range)
> > -
> > +BEGIN_FTR_SECTION
> > +	blr
> > +END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> >  /*
> >   * Flush the data cache to memory 
> >   * 
> 
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

[PATCH 2/3] powerpc: remove the symbol __flush_icache_range

[Kevin Hao]

And now the function flush_icache_range() is just a wrapper which
only invoke the function __flush_icache_range() directly. So we
don't have reason to keep it anymore.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 arch/powerpc/include/asm/cacheflush.h | 7 +------
 arch/powerpc/kernel/misc_32.S         | 2 +-
 arch/powerpc/kernel/misc_64.S         | 2 +-
 arch/powerpc/kernel/ppc_ksyms.c       | 1 -
 4 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
index 60b620d..5b93122 100644
--- a/arch/powerpc/include/asm/cacheflush.h
+++ b/arch/powerpc/include/asm/cacheflush.h
@@ -32,12 +32,7 @@ extern void flush_dcache_page(struct page *page);
 
 extern void __flush_disable_L1(void);
 
-extern void __flush_icache_range(unsigned long, unsigned long);
-static inline void flush_icache_range(unsigned long start, unsigned long stop)
-{
-	__flush_icache_range(start, stop);
-}
-
+extern void flush_icache_range(unsigned long, unsigned long);
 extern void flush_icache_user_range(struct vm_area_struct *vma,
 				    struct page *page, unsigned long addr,
 				    int len);
diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
index e469f30..3f70dbf 100644
--- a/arch/powerpc/kernel/misc_32.S
+++ b/arch/powerpc/kernel/misc_32.S
@@ -327,7 +327,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_UNIFIED_ID_CACHE)
  *
  * flush_icache_range(unsigned long start, unsigned long stop)
  */
-_KPROBE(__flush_icache_range)
+_KPROBE(flush_icache_range)
 BEGIN_FTR_SECTION
 	blr				/* for 601, do nothing */
 END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 74d87f1..a781566 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -67,7 +67,7 @@ PPC64_CACHES:
  *   flush all bytes from start through stop-1 inclusive
  */
 
-_KPROBE(__flush_icache_range)
+_KPROBE(flush_icache_range)
 BEGIN_FTR_SECTION
 	blr
 END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
diff --git a/arch/powerpc/kernel/ppc_ksyms.c b/arch/powerpc/kernel/ppc_ksyms.c
index c296665..380a6f9 100644
--- a/arch/powerpc/kernel/ppc_ksyms.c
+++ b/arch/powerpc/kernel/ppc_ksyms.c
@@ -111,7 +111,6 @@ EXPORT_SYMBOL(giveup_spe);
 #ifndef CONFIG_PPC64
 EXPORT_SYMBOL(flush_instruction_cache);
 #endif
-EXPORT_SYMBOL(__flush_icache_range);
 EXPORT_SYMBOL(flush_dcache_range);
 
 #ifdef CONFIG_SMP
-- 
1.8.3.1

[PATCH 3/3] powerpc: check CPU_FTR_COHERENT_ICACHE in __flush_dcache_icache for 64bit kernel

[Kevin Hao]

We don't need to flush the dcache and invalidate the icache on the
CPU which has CPU_FTR_COHERENT_ICACHE set.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 arch/powerpc/kernel/misc_64.S | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index a781566..32e78e2 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -207,6 +207,9 @@ _GLOBAL(flush_inval_dcache_range)
  *	void __flush_dcache_icache(void *page)
  */
 _GLOBAL(__flush_dcache_icache)
+BEGIN_FTR_SECTION
+	blr
+END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
 /*
  * Flush the data cache to memory 
  * 
-- 
1.8.3.1

Re: [PATCH 3/3] powerpc: check CPU_FTR_COHERENT_ICACHE in __flush_dcache_icache for 64bit kernel

[Benjamin Herrenschmidt]

On Tue, 2013-08-06 at 18:23 +0800, Kevin Hao wrote:
> We don't need to flush the dcache and invalidate the icache on the
> CPU which has CPU_FTR_COHERENT_ICACHE set.

Actually we probably need an isync...

Ben.

> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
>  arch/powerpc/kernel/misc_64.S | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> index a781566..32e78e2 100644
> --- a/arch/powerpc/kernel/misc_64.S
> +++ b/arch/powerpc/kernel/misc_64.S
> @@ -207,6 +207,9 @@ _GLOBAL(flush_inval_dcache_range)
>   *	void __flush_dcache_icache(void *page)
>   */
>  _GLOBAL(__flush_dcache_icache)
> +BEGIN_FTR_SECTION
> +	blr
> +END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
>  /*
>   * Flush the data cache to memory 
>   * 

Re: [PATCH 3/3] powerpc: check CPU_FTR_COHERENT_ICACHE in __flush_dcache_icache for 64bit kernel

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

On Tue, Aug 06, 2013 at 08:36:38PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2013-08-06 at 18:23 +0800, Kevin Hao wrote:
> > We don't need to flush the dcache and invalidate the icache on the
> > CPU which has CPU_FTR_COHERENT_ICACHE set.
> 
> Actually we probably need an isync...

Will add.

Thanks,
Kevin

> 
> Ben.
> 
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> >  arch/powerpc/kernel/misc_64.S | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> > index a781566..32e78e2 100644
> > --- a/arch/powerpc/kernel/misc_64.S
> > +++ b/arch/powerpc/kernel/misc_64.S
> > @@ -207,6 +207,9 @@ _GLOBAL(flush_inval_dcache_range)
> >   *	void __flush_dcache_icache(void *page)
> >   */
> >  _GLOBAL(__flush_dcache_icache)
> > +BEGIN_FTR_SECTION
> > +	blr
> > +END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> >  /*
> >   * Flush the data cache to memory 
> >   * 
> 
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

[PATCH v2 3/3] powerpc: check CPU_FTR_COHERENT_ICACHE in __flush_dcache_icache for 64bit kernel

[Kevin Hao]

We don't need to flush the dcache and invalidate the icache on the
CPU which has CPU_FTR_COHERENT_ICACHE set. Also add the missing
required isync in this case.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
v2: Add the isync.

 arch/powerpc/kernel/misc_64.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 971d7e7..992a78e 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -207,6 +207,10 @@ _GLOBAL(flush_inval_dcache_range)
  *	void __flush_dcache_icache(void *page)
  */
 _GLOBAL(__flush_dcache_icache)
+BEGIN_FTR_SECTION
+	isync
+	blr
+END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
 /*
  * Flush the data cache to memory 
  * 
-- 
1.8.3.1