[PATCH 1/2] MIPS: Add barriers between dcache & icache flushes

[PATCH 1/2] MIPS: Add barriers between dcache & icache flushes

[Paul Burton]

Index-based cache operations may be arbitrarily reordered by out of
order CPUs. Thus code which writes back the dcache & then invalidates
the icache using indexed cache ops must include a barrier between
operating on the 2 caches in order to prevent the scenario in which:

  - icache invalidation occurs.

  - icache fetch occurs, due to speculation.

  - dcache writeback occurs.

If the above were allowed to happen then the icache would contain stale
data. Forcing the dcache writeback to complete before the icache
invalidation avoids this.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: James Hogan <james.hogan@imgtec.com>
---

 arch/mips/mm/c-r4k.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
index caac3d7..a49010c 100644
--- a/arch/mips/mm/c-r4k.c
+++ b/arch/mips/mm/c-r4k.c
@@ -449,6 +449,7 @@ static inline void local_r4k___flush_cache_all(void * args)
 
 	default:
 		r4k_blast_dcache();
+		mb(); /* cache instructions may be reordered */
 		r4k_blast_icache();
 		break;
 	}
@@ -493,8 +494,10 @@ static inline void local_r4k_flush_cache_range(void * args)
 		return;
 
 	r4k_blast_dcache();
-	if (exec)
+	if (exec) {
+		mb(); /* cache instructions may be reordered */
 		r4k_blast_icache();
+	}
 }
 
 static void r4k_flush_cache_range(struct vm_area_struct *vma,
@@ -599,8 +602,13 @@ static inline void local_r4k_flush_cache_page(void *args)
 	if (cpu_has_dc_aliases || (exec && !cpu_has_ic_fills_f_dc)) {
 		vaddr ? r4k_blast_dcache_page(addr) :
 			r4k_blast_dcache_user_page(addr);
-		if (exec && !cpu_icache_snoops_remote_store)
+		if (exec)
+			mb(); /* cache instructions may be reordered */
+
+		if (exec && !cpu_icache_snoops_remote_store) {
 			r4k_blast_scache_page(addr);
+			mb(); /* cache instructions may be reordered */
+		}
 	}
 	if (exec) {
 		if (vaddr && cpu_has_vtag_icache && mm == current->active_mm) {
@@ -660,6 +668,7 @@ static inline void local_r4k_flush_icache_range(unsigned long start, unsigned lo
 			R4600_HIT_CACHEOP_WAR_IMPL;
 			protected_blast_dcache_range(start, end);
 		}
+		mb(); /* cache instructions may be reordered */
 	}
 
 	if (end - start > icache_size)
@@ -798,6 +807,8 @@ static void local_r4k_flush_cache_sigtramp(void * arg)
 		protected_writeback_dcache_line(addr & ~(dc_lsize - 1));
 	if (!cpu_icache_snoops_remote_store && scache_size)
 		protected_writeback_scache_line(addr & ~(sc_lsize - 1));
+	if ((dc_lsize || scache_size) && ic_lsize)
+		mb(); /* cache instructions may be reordered */
 	if (ic_lsize)
 		protected_flush_icache_line(addr & ~(ic_lsize - 1));
 	if (MIPS4K_ICACHE_REFILL_WAR) {
-- 
2.7.1

[PATCH 2/2] MIPS: Flush highmem pages from dcache in __flush_icache_page

[Paul Burton]

When a page is to be mapped executable for userspace, we can presume
that the icache doesn't contain anything valid for its address range but
we cannot be sure that its content has been written back from the dcache
to L2 or memory further out. If the icache fills from those memories,
ie. does not fill from the dcache, then we need to ensure that content
has been flushed from the dcache. This was being done for lowmem pages
but not for highmem pages. Fix this by mapping the page & flushing its
content from the dcache in __flush_icache_page, before the page is
provided to userland.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>

---

 arch/mips/mm/cache.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
index 3f159ca..734cb2f 100644
--- a/arch/mips/mm/cache.c
+++ b/arch/mips/mm/cache.c
@@ -16,6 +16,7 @@
 #include <linux/mm.h>
 
 #include <asm/cacheflush.h>
+#include <asm/highmem.h>
 #include <asm/processor.h>
 #include <asm/cpu.h>
 #include <asm/cpu-features.h>
@@ -124,10 +125,14 @@ void __flush_icache_page(struct vm_area_struct *vma, struct page *page)
 	unsigned long addr;
 
 	if (PageHighMem(page))
-		return;
+		addr = (unsigned long)kmap_atomic(page);
+	else
+		addr = (unsigned long)page_address(page);
 
-	addr = (unsigned long) page_address(page);
 	flush_data_cache_page(addr);
+
+	if (PageHighMem(page))
+		__kunmap_atomic((void *)addr);
 }
 EXPORT_SYMBOL_GPL(__flush_icache_page);
 
-- 
2.7.1

Re: [PATCH 2/2] MIPS: Flush highmem pages from dcache in __flush_icache_page

[Lars Persson]

On 02/22/2016 07:09 PM, Paul Burton wrote:
> When a page is to be mapped executable for userspace, we can presume
> that the icache doesn't contain anything valid for its address range but
> we cannot be sure that its content has been written back from the dcache
> to L2 or memory further out. If the icache fills from those memories,
> ie. does not fill from the dcache, then we need to ensure that content
> has been flushed from the dcache. This was being done for lowmem pages
> but not for highmem pages. Fix this by mapping the page & flushing its
> content from the dcache in __flush_icache_page, before the page is
> provided to userland.
>

Reviewed-by: Lars Persson <larper@axis.com>

> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>
> ---
>
>   arch/mips/mm/cache.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
> index 3f159ca..734cb2f 100644
> --- a/arch/mips/mm/cache.c
> +++ b/arch/mips/mm/cache.c
> @@ -16,6 +16,7 @@
>   #include <linux/mm.h>
>
>   #include <asm/cacheflush.h>
> +#include <asm/highmem.h>
>   #include <asm/processor.h>
>   #include <asm/cpu.h>
>   #include <asm/cpu-features.h>
> @@ -124,10 +125,14 @@ void __flush_icache_page(struct vm_area_struct *vma, struct page *page)
>   	unsigned long addr;
>
>   	if (PageHighMem(page))
> -		return;
> +		addr = (unsigned long)kmap_atomic(page);
> +	else
> +		addr = (unsigned long)page_address(page);
>
> -	addr = (unsigned long) page_address(page);
>   	flush_data_cache_page(addr);
> +
> +	if (PageHighMem(page))
> +		__kunmap_atomic((void *)addr);
>   }
>   EXPORT_SYMBOL_GPL(__flush_icache_page);
>
>

Re: [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes

[Joshua Kinard]

On 02/22/2016 13:09, Paul Burton wrote:
> Index-based cache operations may be arbitrarily reordered by out of
> order CPUs. Thus code which writes back the dcache & then invalidates
> the icache using indexed cache ops must include a barrier between
> operating on the 2 caches in order to prevent the scenario in which:
> 
>   - icache invalidation occurs.
> 
>   - icache fetch occurs, due to speculation.
> 
>   - dcache writeback occurs.
> 
> If the above were allowed to happen then the icache would contain stale
> data. Forcing the dcache writeback to complete before the icache
> invalidation avoids this.

Is there a particular symptom one should look for to check for this issue
occurring?  I haven't seen any odd effects on my SGI systems that appear to
relate to this.  I believe the R1x000 family resolves all hazards in hardware,
so maybe this issue doesn't affect that CPU family?

If not, let me know what to look or test for so I can check the patch out on my
systems.

Thanks!

--J


> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: James Hogan <james.hogan@imgtec.com>
> ---
> 
>  arch/mips/mm/c-r4k.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
> index caac3d7..a49010c 100644
> --- a/arch/mips/mm/c-r4k.c
> +++ b/arch/mips/mm/c-r4k.c
> @@ -449,6 +449,7 @@ static inline void local_r4k___flush_cache_all(void * args)
>  
>  	default:
>  		r4k_blast_dcache();
> +		mb(); /* cache instructions may be reordered */
>  		r4k_blast_icache();
>  		break;
>  	}
> @@ -493,8 +494,10 @@ static inline void local_r4k_flush_cache_range(void * args)
>  		return;
>  
>  	r4k_blast_dcache();
> -	if (exec)
> +	if (exec) {
> +		mb(); /* cache instructions may be reordered */
>  		r4k_blast_icache();
> +	}
>  }
>  
>  static void r4k_flush_cache_range(struct vm_area_struct *vma,
> @@ -599,8 +602,13 @@ static inline void local_r4k_flush_cache_page(void *args)
>  	if (cpu_has_dc_aliases || (exec && !cpu_has_ic_fills_f_dc)) {
>  		vaddr ? r4k_blast_dcache_page(addr) :
>  			r4k_blast_dcache_user_page(addr);
> -		if (exec && !cpu_icache_snoops_remote_store)
> +		if (exec)
> +			mb(); /* cache instructions may be reordered */
> +
> +		if (exec && !cpu_icache_snoops_remote_store) {
>  			r4k_blast_scache_page(addr);
> +			mb(); /* cache instructions may be reordered */
> +		}
>  	}
>  	if (exec) {
>  		if (vaddr && cpu_has_vtag_icache && mm == current->active_mm) {
> @@ -660,6 +668,7 @@ static inline void local_r4k_flush_icache_range(unsigned long start, unsigned lo
>  			R4600_HIT_CACHEOP_WAR_IMPL;
>  			protected_blast_dcache_range(start, end);
>  		}
> +		mb(); /* cache instructions may be reordered */
>  	}
>  
>  	if (end - start > icache_size)
> @@ -798,6 +807,8 @@ static void local_r4k_flush_cache_sigtramp(void * arg)
>  		protected_writeback_dcache_line(addr & ~(dc_lsize - 1));
>  	if (!cpu_icache_snoops_remote_store && scache_size)
>  		protected_writeback_scache_line(addr & ~(sc_lsize - 1));
> +	if ((dc_lsize || scache_size) && ic_lsize)
> +		mb(); /* cache instructions may be reordered */
>  	if (ic_lsize)
>  		protected_flush_icache_line(addr & ~(ic_lsize - 1));
>  	if (MIPS4K_ICACHE_REFILL_WAR) {
> 

Re: [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes

[Paul Burton]

On Mon, Feb 22, 2016 at 06:39:30PM -0500, Joshua Kinard wrote:
> On 02/22/2016 13:09, Paul Burton wrote:
> > Index-based cache operations may be arbitrarily reordered by out of
> > order CPUs. Thus code which writes back the dcache & then invalidates
> > the icache using indexed cache ops must include a barrier between
> > operating on the 2 caches in order to prevent the scenario in which:
> > 
> >   - icache invalidation occurs.
> > 
> >   - icache fetch occurs, due to speculation.
> > 
> >   - dcache writeback occurs.
> > 
> > If the above were allowed to happen then the icache would contain stale
> > data. Forcing the dcache writeback to complete before the icache
> > invalidation avoids this.
> 
> Is there a particular symptom one should look for to check for this issue
> occurring?  I haven't seen any odd effects on my SGI systems that appear to
> relate to this.  I believe the R1x000 family resolves all hazards in hardware,
> so maybe this issue doesn't affect that CPU family?
> 
> If not, let me know what to look or test for so I can check the patch out on my
> systems.
> 
> Thanks!
> 
> --J

Hi Joshua,

It depends upon the implementation of the CPU, but the arch spec (MIPS64
BIS, MD00087, revision 6.02) does say:

> When implementing multiple level of caches and where the hardware maintains
> the smaller cache as a proper subset of a larger cache (every address which is
> resident in the smaller cache is also resident in the larger cache; also known
> as the inclusion property). It is recommended that the CACHE instructions
> which operate on the larger, outer-level cache; must first operate on the
> smaller, inner-level cache. For example, a Hit_Writeback _Invalidate operation
> targeting the Secondary cache, must first operate on the primary data
> cache first. If the CACHE instruction implementation does not follow
> this policy then any software which flushes the caches must mimic this
> behavior. That is, the software sequences must first operate on the
> inner cache then operate on the outer cache. The software must place a
> SYNC instruction after the CACHE instruction whenever there are
> possible writebacks from the inner cache to ensure that the writeback
> data is resident in the outer cache before operating on the outer
> cache. If neither the CACHE instruction implementation nor the
> software cache flush sequence follow this policy, then the inclusion
> property of the caches can be broken, which might be a condition that
> the cache management hardware cannot properly deal with.
>
> When implementing multiple level of caches without the inclusion
> property, the use of a SYNC instruction after the CACHE instruction is
> still needed whenever writeback data has to be resident in the next
> level of memory hierarchy.

If data is to transfer from dcache -> L2 -> icache then it has to be
written back to the L2 which would hit that situation of the data
needing "to be resident in the next level of memory hierarchy" after the
dcache. That is guaranteed by the sync instruction:

> The CACHE instruction and the memory transactions which are sourced by
> the CACHE instruction, such as cache refill or cache writeback, obey
> the ordering and completion rules of the SYNC instruction.

This is more something newer cores that reorder more agressively would
be expected to hit, to the best of my knowledge.

Thanks,
    Paul

Re: [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes

[Florian Fainelli]

On 22/02/16 10:09, Paul Burton wrote:
> Index-based cache operations may be arbitrarily reordered by out of
> order CPUs. Thus code which writes back the dcache & then invalidates
> the icache using indexed cache ops must include a barrier between
> operating on the 2 caches in order to prevent the scenario in which:
> 
>   - icache invalidation occurs.
> 
>   - icache fetch occurs, due to speculation.
> 
>   - dcache writeback occurs.
> 
> If the above were allowed to happen then the icache would contain stale
> data. Forcing the dcache writeback to complete before the icache
> invalidation avoids this.

Is that also true for CPUs with have cpu_has_ic_fills_dc?

> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: James Hogan <james.hogan@imgtec.com>
> ---
> 
>  arch/mips/mm/c-r4k.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
> index caac3d7..a49010c 100644
> --- a/arch/mips/mm/c-r4k.c
> +++ b/arch/mips/mm/c-r4k.c
> @@ -449,6 +449,7 @@ static inline void local_r4k___flush_cache_all(void * args)
>  
>  	default:
>  		r4k_blast_dcache();
> +		mb(); /* cache instructions may be reordered */
>  		r4k_blast_icache();
>  		break;
>  	}
> @@ -493,8 +494,10 @@ static inline void local_r4k_flush_cache_range(void * args)
>  		return;
>  
>  	r4k_blast_dcache();
> -	if (exec)
> +	if (exec) {
> +		mb(); /* cache instructions may be reordered */
>  		r4k_blast_icache();
> +	}
>  }
>  
>  static void r4k_flush_cache_range(struct vm_area_struct *vma,
> @@ -599,8 +602,13 @@ static inline void local_r4k_flush_cache_page(void *args)
>  	if (cpu_has_dc_aliases || (exec && !cpu_has_ic_fills_f_dc)) {
>  		vaddr ? r4k_blast_dcache_page(addr) :
>  			r4k_blast_dcache_user_page(addr);
> -		if (exec && !cpu_icache_snoops_remote_store)
> +		if (exec)
> +			mb(); /* cache instructions may be reordered */
> +
> +		if (exec && !cpu_icache_snoops_remote_store) {
>  			r4k_blast_scache_page(addr);
> +			mb(); /* cache instructions may be reordered */
> +		}
>  	}
>  	if (exec) {
>  		if (vaddr && cpu_has_vtag_icache && mm == current->active_mm) {
> @@ -660,6 +668,7 @@ static inline void local_r4k_flush_icache_range(unsigned long start, unsigned lo
>  			R4600_HIT_CACHEOP_WAR_IMPL;
>  			protected_blast_dcache_range(start, end);
>  		}
> +		mb(); /* cache instructions may be reordered */
>  	}
>  
>  	if (end - start > icache_size)
> @@ -798,6 +807,8 @@ static void local_r4k_flush_cache_sigtramp(void * arg)
>  		protected_writeback_dcache_line(addr & ~(dc_lsize - 1));
>  	if (!cpu_icache_snoops_remote_store && scache_size)
>  		protected_writeback_scache_line(addr & ~(sc_lsize - 1));
> +	if ((dc_lsize || scache_size) && ic_lsize)
> +		mb(); /* cache instructions may be reordered */
>  	if (ic_lsize)
>  		protected_flush_icache_line(addr & ~(ic_lsize - 1));
>  	if (MIPS4K_ICACHE_REFILL_WAR) {
> 


-- 
Florian

Re: [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes

[Paul Burton]

On Mon, Feb 22, 2016 at 04:02:09PM -0800, Florian Fainelli wrote:
> On 22/02/16 10:09, Paul Burton wrote:
> > Index-based cache operations may be arbitrarily reordered by out of
> > order CPUs. Thus code which writes back the dcache & then invalidates
> > the icache using indexed cache ops must include a barrier between
> > operating on the 2 caches in order to prevent the scenario in which:
> > 
> >   - icache invalidation occurs.
> > 
> >   - icache fetch occurs, due to speculation.
> > 
> >   - dcache writeback occurs.
> > 
> > If the above were allowed to happen then the icache would contain stale
> > data. Forcing the dcache writeback to complete before the icache
> > invalidation avoids this.
> 
> Is that also true for CPUs with have cpu_has_ic_fills_dc?

Hi Florian,

Good question. I imagine not, but probably need to think some more & ask
some questions.

Thanks,
    Paul

Re: [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes

[Sven Eckelmann]

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

On Monday, 22 February 2016 19:09:44 CET Paul Burton wrote:
> Index-based cache operations may be arbitrarily reordered by out of
> order CPUs. Thus code which writes back the dcache & then invalidates
> the icache using indexed cache ops must include a barrier between
> operating on the 2 caches in order to prevent the scenario in which:
> 
>   - icache invalidation occurs.
> 
>   - icache fetch occurs, due to speculation.
> 
>   - dcache writeback occurs.
> 
> If the above were allowed to happen then the icache would contain stale
> data. Forcing the dcache writeback to complete before the icache
> invalidation avoids this.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: James Hogan <james.hogan@imgtec.com>
> ---

What happened to this patch? Because it seems like it is required for some 
74kc devices to get them booting (instead of being stuck in an endless
tlbmiss_handler_setup_pgd loop):

* https://github.com/freifunk-gluon/gluon/issues/2784
* https://github.com/openwrt/openwrt/commit/ea6fb9c16dfb9763ea681803db65644b68bae873
* https://github.com/freifunk-gluon/gluon/pull/2810

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]