[PATCH] MIPS: bugfix of local_r4k_flush_icache_range - added L2 flush

[PATCH] MIPS: bugfix of local_r4k_flush_icache_range - added L2 flush

[Leonid Yegoshin]

This function is used to flush code used in NMI and EJTAG debug exceptions.
However, during that exceptions the Status.ERL bit is set, which means
that code runs as UNCACHABLE. So, flush code down to memory is needed.

Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
---
 arch/mips/mm/c-r4k.c |   10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
index 0dbb65a51ce5..9f0299bb9a2a 100644
--- a/arch/mips/mm/c-r4k.c
+++ b/arch/mips/mm/c-r4k.c
@@ -666,17 +666,9 @@ static inline void local_r4k_flush_icache_range(unsigned long start, unsigned lo
 			break;
 		}
 	}
-#ifdef CONFIG_EVA
-	/*
-	 * Due to all possible segment mappings, there might cache aliases
-	 * caused by the bootloader being in non-EVA mode, and the CPU switching
-	 * to EVA during early kernel init. It's best to flush the scache
-	 * to avoid having secondary cores fetching stale data and lead to
-	 * kernel crashes.
-	 */
+
 	bc_wback_inv(start, (end - start));
 	__sync();
-#endif
 }
 
 static inline void local_r4k_flush_icache_range_ipi(void *args)

Re: [PATCH] MIPS: bugfix of local_r4k_flush_icache_range - added L2 flush

[Ralf Baechle]

On Thu, May 28, 2015 at 01:37:24PM -0700, Leonid Yegoshin wrote:

> This function is used to flush code used in NMI and EJTAG debug exceptions.
> However, during that exceptions the Status.ERL bit is set, which means
> that code runs as UNCACHABLE. So, flush code down to memory is needed.
> 
> Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
> ---
>  arch/mips/mm/c-r4k.c |   10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
> index 0dbb65a51ce5..9f0299bb9a2a 100644
> --- a/arch/mips/mm/c-r4k.c
> +++ b/arch/mips/mm/c-r4k.c
> @@ -666,17 +666,9 @@ static inline void local_r4k_flush_icache_range(unsigned long start, unsigned lo
>  			break;
>  		}
>  	}
> -#ifdef CONFIG_EVA
> -	/*
> -	 * Due to all possible segment mappings, there might cache aliases
> -	 * caused by the bootloader being in non-EVA mode, and the CPU switching
> -	 * to EVA during early kernel init. It's best to flush the scache
> -	 * to avoid having secondary cores fetching stale data and lead to
> -	 * kernel crashes.
> -	 */
> +
>  	bc_wback_inv(start, (end - start));
>  	__sync();
> -#endif
>  }

I was wondering why there was a cache flush at all so I dove into git
history and found:

commit 4676f9359fa5190ee6f42bbf2c27d28beb14d26a
Author: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
Date:   Tue Jan 21 09:48:48 2014 +0000

    MIPS: mm: c-r4k: Flush scache to avoid cache aliases
    
    There is a chance for the secondary cache to have memory
    aliases. This can happen if the bootloader is in a non-EVA mode
    (or even in EVA mode but with different mapping from the kernel)
    and the kernel switching to EVA afterwards. It's best to flush
    the icache to avoid having the secondary CPUs fetching stale
    data from it.
    
    Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
    Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>

flush_icache_range() really only is meant to deal with I-cache coherency
issues as they appear during normal kernel operation, that is code is
modified and will be executed from RAM.  I doesn't know about aliases
and it's not meant to know.

As I understand you only need this on startup.  Making this function work
for your special use results in a performance penalty for every other user
of this function.

How about reverting this commit and calling __flush_cache_all() to
make sure your kernel code gets flushed out to the other end of the
universe - or memory, what ever comes first?

  Ralf

Re: [PATCH] MIPS: bugfix of local_r4k_flush_icache_range - added L2 flush

[Leonid Yegoshin]

On 06/10/2015 03:28 AM, Ralf Baechle wrote:
> On Thu, May 28, 2015 at 01:37:24PM -0700, Leonid Yegoshin wrote:
>
>> ...
> I was wondering why there was a cache flush at all so I dove into git
> history and found:
>
> commit 4676f9359fa5190ee6f42bbf2c27d28beb14d26a
> Author: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
> Date:   Tue Jan 21 09:48:48 2014 +0000
>
>      MIPS: mm: c-r4k: Flush scache to avoid cache aliases
>      
>      There is a chance for the secondary cache to have memory
>      aliases. This can happen if the bootloader is in a non-EVA mode
>      (or even in EVA mode but with different mapping from the kernel)
>      and the kernel switching to EVA afterwards. It's best to flush
>      the icache to avoid having the secondary CPUs fetching stale
>      data from it.
>      
>      Signed-off-by: Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>
>      Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>
> flush_icache_range() really only is meant to deal with I-cache coherency
> issues as they appear during normal kernel operation, that is code is
> modified and will be executed from RAM.  I doesn't know about aliases
> and it's not meant to know.

1) OK, here is an explanation of memory aliasing (don't mix with cache 
aliasing).

Memory blocks bigger than 512MB should be put somewhere above 512MB 
physical address due to IO hole but we still need some memory at 
physical address 0x0000000. Board designers often prefer using masking 
of some address bits to MIRROR 256-512MB into 0x00000000 from a primary 
memory space location (for Malta - physical address 0x80000000). It 
decreases latency by some bus clocks (which are not CPU clocks but slowly).

So, in Malta the word at physical address 0x00000000-256MB is the same 
as word at physical address 0x80000000-0x90000000 (other boards may have 
another location). But cache subsystem knows NOTHING about it and caches 
that memory twice. This is a memory aliasing and it may cause a lot of 
trouble if system uses both aliasing address ranges. But it is pretty 
often that system initially uses low block 0x00000000->512MB during core 
startup and later switches to primary address space. So, some full L1D 
and L2 cache flush is needed after cache is initialized. However, after 
creation of exception handler code it is still needed additionally a 
flush of that code too, in L1D/I and including L2 in EVA, to avoid 
memory aliasing problem.

2) This patch actually notices that EJTAG and NMI exception handlers 
work with Status.ERL bit and pull exception handler code from uncachable 
memory, so a flushing exception handlers down to memory (including L2) 
is needed for that because of different reason. Actually, it is a result 
of debugging NMI with U-Boot. It is just by chance that it uses the same 
code and solution as EVA flushing.

>
> As I understand you only need this on startup.  Making this function work
> for your special use results in a performance penalty for every other user
> of this function.

Function local_r4k_flush_icache_range() can be used only at startup, 
before SMP is inialized. Original code has comment above this function:

/* This function is used only for local CPU only while boot etc */

I don't know why it disappeared.

>
> How about reverting this commit and calling __flush_cache_all() to
> make sure your kernel code gets flushed out to the other end of the
> universe - or memory, what ever comes first?

That function may call SMP IPI in some vendor CPUs and is not suitable.

But I agree that name local_r4k_flush_icache_range() is a not good now 
and it may be changed to something more appropriate. I am just not good 
in name changing game.

- Leonid.

>
>    Ralf