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@xxxxxxxx> > >> Cc: stable <stable@xxxxxxxxxxxxxxx> # 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 > >>