[PATCH 1/3] MIPS: c-r4k: instruction_hazard should immediately follow cache op

[PATCH 1/3] MIPS: c-r4k: instruction_hazard should immediately follow cache op

[Matt Redfearn]

During ftrace initialisation, placeholder instructions in the prologue
of every kernel function not marked "notrace" are replaced with nops.
After the instructions are written (to the dcache), flush_icache_range()
is used to ensure that the icache will be updated with these replaced
instructions. Currently there is an instruction_hazard guard at the end
of __r4k_flush_icache_range, since a hazard can be created if the CPU
has already begun fetching the instructions that have have been
replaced. The placement, however, ignores the calls to preempt_enable(),
both in __r4k_flush_icache_range and r4k_on_each_cpu. When
CONFIG_PREEMPT is enabled, these expand out to at least calls to
preempt_count_sub(). The lack of an instruction hazard between icache
invalidate and the execution of preempt_count_sub, in rare
circumstances, was observed to cause weird crashes on Ci40, where the
CPU would end up taking a kernel unaligned access exception from the
middle of do_ade(), which it somehow reached from preempt_count_sub
without executing the start of do_ade.

Since the instruction hazard exists immediately after the dcache is
written back and icache invalidated, place the instruction_hazard()
within __local_r4k_flush_icache_range. The one at the end of
__r4k_flush_icache_range is too late, since all of the functions in the
call path of preempt_enable have already been executed, so remove it.

This fixes the crashes during ftrace initialisation on Ci40.

Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Cc: stable <stable@vger.kernel.org> # v4.9+

---

 arch/mips/mm/c-r4k.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
index 6f534b209971..ce7a54223504 100644
--- a/arch/mips/mm/c-r4k.c
+++ b/arch/mips/mm/c-r4k.c
@@ -760,6 +760,8 @@ static inline void __local_r4k_flush_icache_range(unsigned long start,
 			break;
 		}
 	}
+	/* Hazard to force new i-fetch */
+	instruction_hazard();
 }
 
 static inline void local_r4k_flush_icache_range(unsigned long start,
@@ -817,7 +819,6 @@ static void __r4k_flush_icache_range(unsigned long start, unsigned long end,
 	}
 	r4k_on_each_cpu(args.type, local_r4k_flush_icache_range_ipi, &args);
 	preempt_enable();
-	instruction_hazard();
 }
 
 static void r4k_flush_icache_range(unsigned long start, unsigned long end)
-- 
2.7.4

[PATCH 2/3] MIPS: Add barrier between dcache & icache flushes

[Matt Redfearn]

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.

Similarly, the MIPS CM version 2 and above serialises D->I hit-based
cache operations to the same address, but older CMs and systems without
a MIPS CM do not and require the same barrier to ensure ordering.

To ensure these conditions, always enforce a barrier between D and I
cache operations.

Suggested-by: Leonid Yegoshin <Leonid.Yegoshin@mips.com>
Suggested-by: Paul Burton <paul.burton@mips.com>
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Cc: James Hogan <james.hogan@mips.com>
Cc: stable <stable@vger.kernel.org> # v4.9+
---

 arch/mips/mm/c-r4k.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
index ce7a54223504..b7186d47184b 100644
--- a/arch/mips/mm/c-r4k.c
+++ b/arch/mips/mm/c-r4k.c
@@ -741,6 +741,9 @@ static inline void __local_r4k_flush_icache_range(unsigned long start,
 			else
 				blast_dcache_range(start, end);
 		}
+
+		/* Ensure dcache operation has completed */
+		mb();
 	}
 
 	if (type == R4K_INDEX ||
-- 
2.7.4

Re: [PATCH 2/3] MIPS: Add barrier between dcache & icache flushes

[James Hogan]

On Thu, Dec 21, 2017 at 11:16:03AM +0000, Matt Redfearn 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.
> 
> Similarly, the MIPS CM version 2 and above serialises D->I hit-based
> cache operations to the same address, but older CMs and systems without
> a MIPS CM do not and require the same barrier to ensure ordering.
> 
> To ensure these conditions, always enforce a barrier between D and I
> cache operations.
> 
> Suggested-by: Leonid Yegoshin <Leonid.Yegoshin@mips.com>
> Suggested-by: Paul Burton <paul.burton@mips.com>
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> Cc: James Hogan <james.hogan@mips.com>
> Cc: stable <stable@vger.kernel.org> # v4.9+

Looks reasonable to me,

Reviewed-by: James Hogan <jhogan@kernel.org>

Cheers
James

> ---
> 
>  arch/mips/mm/c-r4k.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
> index ce7a54223504..b7186d47184b 100644
> --- a/arch/mips/mm/c-r4k.c
> +++ b/arch/mips/mm/c-r4k.c
> @@ -741,6 +741,9 @@ static inline void __local_r4k_flush_icache_range(unsigned long start,
>  			else
>  				blast_dcache_range(start, end);
>  		}
> +
> +		/* Ensure dcache operation has completed */
> +		mb();
>  	}
>  
>  	if (type == R4K_INDEX ||
> -- 
> 2.7.4
> 

[PATCH 3/3] MIPS: Add barrier between icache flush and execution hazard barrier

[Matt Redfearn]

Hit-based icache operations may complete before the CM completes
intervention with the local L1. Thus code which invalidates the icache
and then attempts to execute those addresses must include a barrier to
prevent the scenario which:

  - icache instruction completes
  - icache fetch occurs
  - core executes icache data
  - CM completes icache invalidate

If the above were allowed to happen then the core would execute stale
instructions from the icache.

A barrier is required to prevent the core i-fetching before the icache
operation has completed. This goes together with the instruction_hazard
to ensure that the pipeline is stalled until the icache operation is
completed and the core will fetch the new instructions.

Suggested-by: Leonid Yegoshin <Leonid.Yegoshin@mips.com>
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
Cc: Paul Burton <paul.burton@mips.com>
Cc: James Hogan <james.hogan@mips.com>
Cc: stable <stable@vger.kernel.org> # v4.9+
---

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

diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
index b7186d47184b..844685e51109 100644
--- a/arch/mips/mm/c-r4k.c
+++ b/arch/mips/mm/c-r4k.c
@@ -763,6 +763,8 @@ static inline void __local_r4k_flush_icache_range(unsigned long start,
 			break;
 		}
 	}
+	/* Ensure icache operation has completed */
+	mb();
 	/* Hazard to force new i-fetch */
 	instruction_hazard();
 }
-- 
2.7.4

Re: [PATCH 1/3] MIPS: c-r4k: instruction_hazard should immediately follow cache op

[James Hogan]

On Thu, Dec 21, 2017 at 11:16:02AM +0000, Matt Redfearn wrote:
> During ftrace initialisation, placeholder instructions in the prologue
> of every kernel function not marked "notrace" are replaced with nops.
> After the instructions are written (to the dcache), flush_icache_range()
> is used to ensure that the icache will be updated with these replaced
> instructions. Currently there is an instruction_hazard guard at the end
> of __r4k_flush_icache_range, since a hazard can be created if the CPU
> has already begun fetching the instructions that have have been
> replaced. The placement, however, ignores the calls to preempt_enable(),
> both in __r4k_flush_icache_range and r4k_on_each_cpu. When
> CONFIG_PREEMPT is enabled, these expand out to at least calls to
> preempt_count_sub(). The lack of an instruction hazard between icache
> invalidate and the execution of preempt_count_sub, in rare
> circumstances, was observed to cause weird crashes on Ci40, where the
> CPU would end up taking a kernel unaligned access exception from the
> middle of do_ade(), which it somehow reached from preempt_count_sub
> without executing the start of do_ade.
> 
> Since the instruction hazard exists immediately after the dcache is
> written back and icache invalidated, place the instruction_hazard()
> within __local_r4k_flush_icache_range. The one at the end of
> __r4k_flush_icache_range is too late, since all of the functions in the
> call path of preempt_enable have already been executed, so remove it.
> 
> This fixes the crashes during ftrace initialisation on Ci40.
> 
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> Cc: stable <stable@vger.kernel.org> # v4.9+
> 
> ---
> 
>  arch/mips/mm/c-r4k.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
> index 6f534b209971..ce7a54223504 100644
> --- a/arch/mips/mm/c-r4k.c
> +++ b/arch/mips/mm/c-r4k.c
> @@ -760,6 +760,8 @@ static inline void __local_r4k_flush_icache_range(unsigned long start,
>  			break;
>  		}
>  	}
> +	/* Hazard to force new i-fetch */
> +	instruction_hazard();

By the sounds of it that is a hardware bug, that it didn't try and
execute either the old instruction or the new instruction. Maybe an
expanded comment would be worthwhile here. If it wasn't for that issue
it would I suppose be safe for it to be directly before the
preempt_enable() in __r4k_flush_icache_range().

Cheers
James

>  }
>  
>  static inline void local_r4k_flush_icache_range(unsigned long start,
> @@ -817,7 +819,6 @@ static void __r4k_flush_icache_range(unsigned long start, unsigned long end,
>  	}
>  	r4k_on_each_cpu(args.type, local_r4k_flush_icache_range_ipi, &args);
>  	preempt_enable();
> -	instruction_hazard();
>  }
>  
>  static void r4k_flush_icache_range(unsigned long start, unsigned long end)
> -- 
> 2.7.4
> 

Re: [PATCH 1/3] MIPS: c-r4k: instruction_hazard should immediately follow cache op

[Matt Redfearn]

Hi James,

On 21/12/17 15:14, James Hogan wrote:
> On Thu, Dec 21, 2017 at 11:16:02AM +0000, Matt Redfearn wrote:
>> During ftrace initialisation, placeholder instructions in the prologue
>> of every kernel function not marked "notrace" are replaced with nops.
>> After the instructions are written (to the dcache), flush_icache_range()
>> is used to ensure that the icache will be updated with these replaced
>> instructions. Currently there is an instruction_hazard guard at the end
>> of __r4k_flush_icache_range, since a hazard can be created if the CPU
>> has already begun fetching the instructions that have have been
>> replaced. The placement, however, ignores the calls to preempt_enable(),
>> both in __r4k_flush_icache_range and r4k_on_each_cpu. When
>> CONFIG_PREEMPT is enabled, these expand out to at least calls to
>> preempt_count_sub(). The lack of an instruction hazard between icache
>> invalidate and the execution of preempt_count_sub, in rare
>> circumstances, was observed to cause weird crashes on Ci40, where the
>> CPU would end up taking a kernel unaligned access exception from the
>> middle of do_ade(), which it somehow reached from preempt_count_sub
>> without executing the start of do_ade.
>>
>> Since the instruction hazard exists immediately after the dcache is
>> written back and icache invalidated, place the instruction_hazard()
>> within __local_r4k_flush_icache_range. The one at the end of
>> __r4k_flush_icache_range is too late, since all of the functions in the
>> call path of preempt_enable have already been executed, so remove it.
>>
>> This fixes the crashes during ftrace initialisation on Ci40.
>>
>> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
>> Cc: stable <stable@vger.kernel.org> # v4.9+
>>
>> ---
>>
>>   arch/mips/mm/c-r4k.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
>> index 6f534b209971..ce7a54223504 100644
>> --- a/arch/mips/mm/c-r4k.c
>> +++ b/arch/mips/mm/c-r4k.c
>> @@ -760,6 +760,8 @@ static inline void __local_r4k_flush_icache_range(unsigned long start,
>>   			break;
>>   		}
>>   	}
>> +	/* Hazard to force new i-fetch */
>> +	instruction_hazard();
> 
> By the sounds of it that is a hardware bug, that it didn't try and
> execute either the old instruction or the new instruction.

Yeah, possibly.

  Maybe an
> expanded comment would be worthwhile here. If it wasn't for that issue
> it would I suppose be safe for it to be directly before the
> preempt_enable() in __r4k_flush_icache_range().

No - there's another preempt_enable() in r4k_on_each_cpu (noted in the 
commit message) so by the time the local CPU gets to the 
preempt_enable() in __r4k_flush_icache_range, it has potentially already 
executed the preempt_enable path and died. That's why I put it here.

Thanks,
Matt

> 
> Cheers
> James
> 
>>   }
>>   
>>   static inline void local_r4k_flush_icache_range(unsigned long start,
>> @@ -817,7 +819,6 @@ static void __r4k_flush_icache_range(unsigned long start, unsigned long end,
>>   	}
>>   	r4k_on_each_cpu(args.type, local_r4k_flush_icache_range_ipi, &args);
>>   	preempt_enable();
>> -	instruction_hazard();
>>   }
>>   
>>   static void r4k_flush_icache_range(unsigned long start, unsigned long end)
>> -- 
>> 2.7.4
>>

Re: [PATCH 1/3] MIPS: c-r4k: instruction_hazard should immediately follow cache op

[James Hogan]

On Thu, Dec 21, 2017 at 03:19:35PM +0000, Matt Redfearn wrote:
> Hi James,
> 
> On 21/12/17 15:14, James Hogan wrote:
> > On Thu, Dec 21, 2017 at 11:16:02AM +0000, Matt Redfearn wrote:
> >> During ftrace initialisation, placeholder instructions in the prologue
> >> of every kernel function not marked "notrace" are replaced with nops.
> >> After the instructions are written (to the dcache), flush_icache_range()
> >> is used to ensure that the icache will be updated with these replaced
> >> instructions. Currently there is an instruction_hazard guard at the end
> >> of __r4k_flush_icache_range, since a hazard can be created if the CPU
> >> has already begun fetching the instructions that have have been
> >> replaced. The placement, however, ignores the calls to preempt_enable(),
> >> both in __r4k_flush_icache_range and r4k_on_each_cpu. When
> >> CONFIG_PREEMPT is enabled, these expand out to at least calls to
> >> preempt_count_sub(). The lack of an instruction hazard between icache
> >> invalidate and the execution of preempt_count_sub, in rare
> >> circumstances, was observed to cause weird crashes on Ci40, where the
> >> CPU would end up taking a kernel unaligned access exception from the
> >> middle of do_ade(), which it somehow reached from preempt_count_sub
> >> without executing the start of do_ade.
> >>
> >> Since the instruction hazard exists immediately after the dcache is
> >> written back and icache invalidated, place the instruction_hazard()
> >> within __local_r4k_flush_icache_range. The one at the end of
> >> __r4k_flush_icache_range is too late, since all of the functions in the
> >> call path of preempt_enable have already been executed, so remove it.
> >>
> >> This fixes the crashes during ftrace initialisation on Ci40.
> >>
> >> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> >> Cc: stable <stable@vger.kernel.org> # v4.9+
> >>
> >> ---
> >>
> >>   arch/mips/mm/c-r4k.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
> >> index 6f534b209971..ce7a54223504 100644
> >> --- a/arch/mips/mm/c-r4k.c
> >> +++ b/arch/mips/mm/c-r4k.c
> >> @@ -760,6 +760,8 @@ static inline void __local_r4k_flush_icache_range(unsigned long start,
> >>   			break;
> >>   		}
> >>   	}
> >> +	/* Hazard to force new i-fetch */
> >> +	instruction_hazard();
> > 
> > By the sounds of it that is a hardware bug, that it didn't try and
> > execute either the old instruction or the new instruction.
> 
> Yeah, possibly.
> 
>   Maybe an
> > expanded comment would be worthwhile here. If it wasn't for that issue
> > it would I suppose be safe for it to be directly before the
> > preempt_enable() in __r4k_flush_icache_range().
> 
> No - there's another preempt_enable() in r4k_on_each_cpu (noted in the 
> commit message) so by the time the local CPU gets to the 
> preempt_enable() in __r4k_flush_icache_range, it has potentially already 
> executed the preempt_enable path and died. That's why I put it here.

Right, but it wouldn't matter since it would still execute valid code?

Cheers
James

> 
> Thanks,
> Matt
> 
> > 
> > Cheers
> > James
> > 
> >>   }
> >>   
> >>   static inline void local_r4k_flush_icache_range(unsigned long start,
> >> @@ -817,7 +819,6 @@ static void __r4k_flush_icache_range(unsigned long start, unsigned long end,
> >>   	}
> >>   	r4k_on_each_cpu(args.type, local_r4k_flush_icache_range_ipi, &args);
> >>   	preempt_enable();
> >> -	instruction_hazard();
> >>   }
> >>   
> >>   static void r4k_flush_icache_range(unsigned long start, unsigned long end)
> >> -- 
> >> 2.7.4
> >>

Re: [PATCH 1/3] MIPS: c-r4k: instruction_hazard should immediately follow cache op

[Matt Redfearn]

Hi James,

On 21/12/17 15:30, James Hogan wrote:
> On Thu, Dec 21, 2017 at 03:19:35PM +0000, Matt Redfearn wrote:
>> Hi James,
>>
>> On 21/12/17 15:14, James Hogan wrote:
>>> On Thu, Dec 21, 2017 at 11:16:02AM +0000, Matt Redfearn wrote:
>>>> During ftrace initialisation, placeholder instructions in the prologue
>>>> of every kernel function not marked "notrace" are replaced with nops.
>>>> After the instructions are written (to the dcache), flush_icache_range()
>>>> is used to ensure that the icache will be updated with these replaced
>>>> instructions. Currently there is an instruction_hazard guard at the end
>>>> of __r4k_flush_icache_range, since a hazard can be created if the CPU
>>>> has already begun fetching the instructions that have have been
>>>> replaced. The placement, however, ignores the calls to preempt_enable(),
>>>> both in __r4k_flush_icache_range and r4k_on_each_cpu. When
>>>> CONFIG_PREEMPT is enabled, these expand out to at least calls to
>>>> preempt_count_sub(). The lack of an instruction hazard between icache
>>>> invalidate and the execution of preempt_count_sub, in rare
>>>> circumstances, was observed to cause weird crashes on Ci40, where the
>>>> CPU would end up taking a kernel unaligned access exception from the
>>>> middle of do_ade(), which it somehow reached from preempt_count_sub
>>>> without executing the start of do_ade.
>>>>
>>>> Since the instruction hazard exists immediately after the dcache is
>>>> written back and icache invalidated, place the instruction_hazard()
>>>> within __local_r4k_flush_icache_range. The one at the end of
>>>> __r4k_flush_icache_range is too late, since all of the functions in the
>>>> call path of preempt_enable have already been executed, so remove it.
>>>>
>>>> This fixes the crashes during ftrace initialisation on Ci40.
>>>>
>>>> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
>>>> Cc: stable <stable@vger.kernel.org> # v4.9+
>>>>
>>>> ---
>>>>
>>>>    arch/mips/mm/c-r4k.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
>>>> index 6f534b209971..ce7a54223504 100644
>>>> --- a/arch/mips/mm/c-r4k.c
>>>> +++ b/arch/mips/mm/c-r4k.c
>>>> @@ -760,6 +760,8 @@ static inline void __local_r4k_flush_icache_range(unsigned long start,
>>>>    			break;
>>>>    		}
>>>>    	}
>>>> +	/* Hazard to force new i-fetch */
>>>> +	instruction_hazard();
>>>
>>> By the sounds of it that is a hardware bug, that it didn't try and
>>> execute either the old instruction or the new instruction.
>>
>> Yeah, possibly.
>>
>>    Maybe an
>>> expanded comment would be worthwhile here. If it wasn't for that issue
>>> it would I suppose be safe for it to be directly before the
>>> preempt_enable() in __r4k_flush_icache_range().
>>
>> No - there's another preempt_enable() in r4k_on_each_cpu (noted in the
>> commit message) so by the time the local CPU gets to the
>> preempt_enable() in __r4k_flush_icache_range, it has potentially already
>> executed the preempt_enable path and died. That's why I put it here.
> 
> Right, but it wouldn't matter since it would still execute valid code?

You'd like to think so, but the Ci40 didn't - that's what led to the 
series :-)

Thanks,
Matt


> 
> Cheers
> James
> 
>>
>> Thanks,
>> Matt
>>
>>>
>>> Cheers
>>> James
>>>
>>>>    }
>>>>    
>>>>    static inline void local_r4k_flush_icache_range(unsigned long start,
>>>> @@ -817,7 +819,6 @@ static void __r4k_flush_icache_range(unsigned long start, unsigned long end,
>>>>    	}
>>>>    	r4k_on_each_cpu(args.type, local_r4k_flush_icache_range_ipi, &args);
>>>>    	preempt_enable();
>>>> -	instruction_hazard();
>>>>    }
>>>>    
>>>>    static void r4k_flush_icache_range(unsigned long start, unsigned long end)
>>>> -- 
>>>> 2.7.4
>>>>