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@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?

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