[PATCH v2 3/4] powerpc/kprobes_on_ftrace: Skip livepatch_handler() for jprobes

[Naveen N. Rao]

On 2017/06/15 09:19PM, Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao at linux.vnet.ibm.com> writes:
> 
> > diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> > index fa0921410fa4..e6837e85ec28 100644
> > --- a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> > +++ b/arch/powerpc/kernel/trace/ftrace_64_mprofile.S
> > @@ -99,13 +99,37 @@ ftrace_call:
> >  	bl	ftrace_stub
> >  	nop
> >  
> > -	/* Load ctr with the possibly modified NIP */
> > -	ld	r3, _NIP(r1)
> > -	mtctr	r3
> > +	/* Load the possibly modified NIP */
> > +	ld	r15, _NIP(r1)
> > +
> >  #ifdef CONFIG_LIVEPATCH
> > -	cmpd	r14,r3		/* has NIP been altered? */
> > +	cmpd	r14, r15		/* has NIP been altered? */
> > +#endif
> > +
> > +#if defined(CONFIG_LIVEPATCH) && defined(CONFIG_KPROBES_ON_FTRACE)
> > +	beq	1f
> > +
> > +	/* Check if there is an active kprobe on us */
> > +	subi	r3, r14, 4
> > +	bl	is_current_kprobe_addr
> > +	nop
> > +
> > +	/*
> > +	 * If r3 == 1, then this is a kprobe/jprobe.
> > +	 * else, this is livepatched function.
> > +	 *
> > +	 * The subsequent conditional branch for livepatch_handler
> > +	 * will use the result of this compare. For kprobe/jprobe,
> > +	 * we just need to branch to the new NIP, so nothing special
> > +	 * is needed.
> > +	 */
> > +	cmpdi	r3, 1
> > +1:
> >  #endif
> 
> I added some more comments. Hopefully I got them right :D
> 
>  #if defined(CONFIG_LIVEPATCH) && defined(CONFIG_KPROBES_ON_FTRACE)
> +       /* NIP has not been altered, skip over further checks */
>         beq     1f
> 
>         /* Check if there is an active kprobe on us */
> @@ -118,10 +119,11 @@ ftrace_call:
>          * If r3 == 1, then this is a kprobe/jprobe.
>          * else, this is livepatched function.
>          *
> -        * The subsequent conditional branch for livepatch_handler
> -        * will use the result of this compare. For kprobe/jprobe,
> -        * we just need to branch to the new NIP, so nothing special
> -        * is needed.
> +        * The conditional branch for livepatch_handler below will use the
> +        * result of this comparison. For kprobe/jprobe, we just need to branch to
> +        * the new NIP, not call livepatch_handler. The branch below is bne, so we
> +        * want CR0[EQ] to be true if this is a kprobe/jprobe. Which means we want
> +        * CR0[EQ] = (r3 == 1).
>          */
>         cmpdi   r3, 1
>  1:
> @@ -147,7 +149,10 @@ ftrace_call:
>         addi r1, r1, SWITCH_FRAME_SIZE
> 
>  #ifdef CONFIG_LIVEPATCH
> -        /* Based on the cmpd above, if the NIP was altered handle livepatch */
> +        /*
> +        * Based on the cmpd or cmpdi above, if the NIP was altered and we're
> +        * not on a kprobe/jprobe, then handle livepatch.
> +        */
>         bne-    livepatch_handler
>  #endif

Oh yes, this makes the code logic a whole lot more clearer and explicit.  
Thanks for expanding on it!

- Naveen