Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1339784 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes Paul Burton <paul.burton@imgtec.com> - 2016-02-22 19:20 +0100
    Re: [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes Joshua Kinard <kumba@gentoo.org> - 2016-02-23 00:50 +0100
      Re: [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes Paul Burton <paul.burton@imgtec.com> - 2016-03-01 03:30 +0100
    Re: [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes Florian Fainelli <f.fainelli@gmail.com> - 2016-02-23 01:10 +0100
      Re: [PATCH 1/2] MIPS: Add barriers between dcache & icache flushes Paul Burton <paul.burton@imgtec.com> - 2016-03-01 03:30 +0100

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

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

[toc] | [next] | [standalone]

#1340088

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) {
> 

[toc] | [prev] | [next] | [standalone]

#1346301

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

[toc] | [prev] | [next] | [standalone]

#1340094

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

[toc] | [prev] | [next] | [standalone]

#1346302

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web