[3/4] ppc32/kprobe: complete kprobe and migrate exception frame

>> You need to hook into "resume_kernel" instead.
>

I regenerate this with hooking into resume_kernel in below.

> Maybe I'm misunderstanding what you mean since as I recall you suggestion we
> should do this at the end of do_work.
> 
>> Also, we may want to simplify the whole thing, instead of checking user
>> vs. kernel first etc... we could instead have a single _TIF_WORK_MASK
>> which includes both the bits for user work and the new bit for kernel
>> work. With preempt, the kernel work bits would also include
>> _TIF_NEED_RESCHED.
>>
>> Then you have in the common exit path, a single test for that, with a
>> fast path that skips everything and just goes to "restore" for both
>> kernel and user.
>>
>> The only possible issue is the setting of dbcr0 for BookE and 44x and we
>> can keep that as a special case keyed of MSR_PR in the resume path under
>> ifdef BOOKE (we'll probably sanitize that later with some different
>> rework anyway). 
>>
>> So the exit path because something like:
>>
>> ret_from_except:
>> 	.. hard disable interrupts (unchanged) ...
>> 	read TIF flags
>> 	andi with _TIF_WORK_MASK
>> 		nothing set -> restore
>> 	check PR
>> 		set -> do_work_user
>> 		no set -> do_work_kernel (kprobes & preempt)
>> 		(both loop until relevant _TIF flags are all clear)
>> restore:
>> 	#ifdef BOOKE & 44x test PR & do dbcr0 stuff if needed
>> 	... nornal restore ...
> 
> Do you mean we should reorganize current ret_from_except for ppc32 as well?
> 

I assume it may not necessary to reorganize ret_from_except for *ppc32* .

>>>  do_user_signal:			/* r10 contains MSR_KERNEL here */
>>>  	ori	r10,r10,MSR_EE
>>>  	SYNC
>>> @@ -1202,6 +1204,30 @@ do_user_signal:			/* r10 contains MSR_KERNEL here */
>>>  	REST_NVGPRS(r1)
>>>  	b	recheck
>>>  
>>> +restore_kprobe:
>>> +	lwz	r3,GPR1(r1)
>>> +	subi    r3,r3,INT_FRAME_SIZE; /* Allocate a trampoline exception frame */
>>> +	mr	r4,r1
>>> +	bl	copy_exc_stack	/* Copy from the original to the trampoline */
>>> +
>>> +	/* Do real stw operation to complete stwu */
>>> +	mr	r4,r1
>>> +	addi	r4,r4,INT_FRAME_SIZE	/* Get kprobed entry */
>>> +	lwz	r5,GPR1(r1)		/* Backup r1 */
>>> +	stw	r4,GPR1(r1)		/* Now store that safely */
>> The above confuses me. Shouldn't you do instead something like
>>
>> 	lwz	r4,GPR1(r1)

Now GPR1(r1) is already pointed with new r1 in emulate_step().

>> 	subi	r3,r4,INT_FRAME_SIZE

Here we need this, 'mr r4,r1', since r1 holds current exception stack.

>> 	li	r5,INT_FRAME_SIZE
>> 	bl	memcpy

Then the current exception stack is migrated below the kprobed function stack.

stack flow:

--------------------------  -> old r1 when hit 'stwu r1, -AA(r1)' in our
        ^       ^           kprobed function entry.
        |       |
        |       |------------> AA allocated for the kprobed function
        |       |
        |       v
--------|-----------------  -> new r1, also GPR1(r1). It holds the kprobed
   ^    |                   function stack , -AA(r1).
   |    |
   |    |--------------------> INT_FRAME_SIZE for program exception
   |    |
   |    v
---|---------------------  -> r1 is updated to hold program exception stack.
   |
   |
   |------------------------> migrate the exception stack (r1) below the
   |                        kprobed after memcpy with INT_FRAME_SIZE.
   v
-------------------------  -> reroute this as r1 for program exception stack.

>>
> 
> Anyway I'll try this if you think memcpy is fine/safe in exception return codes.
> 
>> To start with, then you need to know the "old" r1 value which may or may
>> not be related to your current r1. The emulation code should stash it
> 
> If the old r1 is not related to our current r1, it shouldn't be possible to go
> restore_kprob since we set that new flag only for the current.
> 
> If I'm wrong please correct me :)

If you agree what I say above, and its also avoid any issue introduced with
orig_gpr3, please check the follow:
        rlwinm  r9,r1,0,0,(31-THREAD_SHIFT)
        lwz     r0,TI_PREEMPT(r9)
@@ -844,8 +875,6 @@ resume_kernel:
         */
        bl      trace_hardirqs_on
 #endif
-#else
-resume_kernel:
 #endif /* CONFIG_PREEMPT */

        /* interrupts are hard-disabled at this point */

Tiejun

On Tue, 2011-12-13 at 18:36 +0800, tiejun.chen wrote:
> >> You need to hook into "resume_kernel" instead.
> >
> 
> I regenerate this with hooking into resume_kernel in below.

 .../...

> I assume it may not necessary to reorganize ret_from_except for *ppc32* .

It might be cleaner but I can do that myself later.

> >>>  do_user_signal:			/* r10 contains MSR_KERNEL here */
> >>>  	ori	r10,r10,MSR_EE
> >>>  	SYNC
> >>> @@ -1202,6 +1204,30 @@ do_user_signal:			/* r10 contains MSR_KERNEL here */
> >>>  	REST_NVGPRS(r1)
> >>>  	b	recheck
> >>>  
> >>> +restore_kprobe:
> >>> +	lwz	r3,GPR1(r1)
> >>> +	subi    r3,r3,INT_FRAME_SIZE; /* Allocate a trampoline exception frame */
> >>> +	mr	r4,r1
> >>> +	bl	copy_exc_stack	/* Copy from the original to the trampoline */
> >>> +
> >>> +	/* Do real stw operation to complete stwu */
> >>> +	mr	r4,r1
> >>> +	addi	r4,r4,INT_FRAME_SIZE	/* Get kprobed entry */
> >>> +	lwz	r5,GPR1(r1)		/* Backup r1 */
> >>> +	stw	r4,GPR1(r1)		/* Now store that safely */
> >> The above confuses me. Shouldn't you do instead something like
> >>
> >> 	lwz	r4,GPR1(r1)
> 
> Now GPR1(r1) is already pointed with new r1 in emulate_step().

Right

> >> 	subi	r3,r4,INT_FRAME_SIZE
> 
> Here we need this, 'mr r4,r1', since r1 holds current exception stack.

Right.

> >> 	li	r5,INT_FRAME_SIZE
> >> 	bl	memcpy
> 
> Then the current exception stack is migrated below the kprobed function stack.
> 
> stack flow:
> 
> --------------------------  -> old r1 when hit 'stwu r1, -AA(r1)' in our
>         ^       ^           kprobed function entry.
>         |       |
>         |       |------------> AA allocated for the kprobed function
>         |       |
>         |       v
> --------|-----------------  -> new r1, also GPR1(r1). It holds the kprobed
>    ^    |                   function stack , -AA(r1).
>    |    |
>    |    |--------------------> INT_FRAME_SIZE for program exception
>    |    |
>    |    v
> ---|---------------------  -> r1 is updated to hold program exception stack.
>    |
>    |
>    |------------------------> migrate the exception stack (r1) below the
>    |                        kprobed after memcpy with INT_FRAME_SIZE.
>    v
> -------------------------  -> reroute this as r1 for program exception stack.

I see so you simply assume that the old r1 value is the current r1 +
INT_FRAME_SIZE, which is probably fair enough.

BTW. we should probably WARN_ON if emulate_step tries to set the new TIF
flag and sees it already set since that means we'll lose the previous
value.

> >>
> > 
> > Anyway I'll try this if you think memcpy is fine/safe in exception return codes.
> > 
> >> To start with, then you need to know the "old" r1 value which may or may
> >> not be related to your current r1. The emulation code should stash it
> > 
> > If the old r1 is not related to our current r1, it shouldn't be possible to go
> > restore_kprob since we set that new flag only for the current.
> > 
> > If I'm wrong please correct me :)
> 
> If you agree what I say above, and its also avoid any issue introduced with
> orig_gpr3, please check the follow:
> =========
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index 56212bc..277029d 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -813,9 +813,40 @@ restore_user:
> 
>  #ifdef CONFIG_PREEMPT
>         b       restore
> +#endif

The above means that if !PREEMPT, a userspace return -will- fo into
your new code, while with PREEMPT it won't. This is inconsistent. Now
we should never need that for userspace returns (and indeed you should
double check in emulate step that you are only applying this when
regs->msr & MSR_PR is 0). The above branch should basically become
unconditional.

> -/* N.B. the only way to get here is from the beq following ret_from_except. */
>  resume_kernel:
> +#ifdef CONFIG_KPROBES

Don't make this KPROBES specific. Anything using emulate_step (such as
xmon) might need that too.

> +       /* check current_thread_info, _TIF_EMULATE_STACK_STORE */
> +       rlwinm  r9,r1,0,0,(31-THREAD_SHIFT)
> +       lwz     r0,TI_FLAGS(r9)
> +       andis.  r0,r0,_TIF_EMULATE_STACK_STORE@h
> +       beq+    restore_kernel

So you are introducing a new symbol restore_kernel, you could just
branch to "restore". However, that would mean putting the preempt
case before the kprobe case. But don't we want to do that anyway ?

I don't like keeping that "offsetted" return stack accross a preempt.

> +       addi    r9,r1,INT_FRAME_SIZE    /* Get the kprobed function entry */
> +
> +       lwz     r3,GPR1(r1)
> +       subi    r3,r3,INT_FRAME_SIZE    /* dst: Allocate a trampoline exception
> frame */
> +       mr      r4,r1                   /* src:  current exception frame */
> +       li      r5,INT_FRAME_SIZE       /* size: INT_FRAME_SIZE */
> +       mr      r1,r3                   /* Reroute the trampoline frame to r1 */
> +       bl      memcpy                  /* Copy from the original to the
> trampoline */
> +
> +       /* Do real store operation to complete stwu */
> +       lwz     r5,GPR1(r1)
> +       stw     r9,0(r5)
>
> +       /* Clear _TIF_EMULATE_STACK_STORE flag */
> +       rlwinm  r9,r1,0,0,(31-THREAD_SHIFT)
> +       lwz     r0,TI_FLAGS(r9)
> +       rlwinm  r0,r0,0,_TIF_EMULATE_STACK_STORE
> +       stw     r0,TI_FLAGS(r9)

I think this needs to be an atomic operation, another CPU can be trying
to set _NEED_RESCHED at the same time.

> +restore_kernel:
> +#endif
> +
> +#ifdef CONFIG_PREEMPT
> +/* N.B. the only way to get here is from the beq following ret_from_except. */
>         /* check current_thread_info->preempt_count */
>         rlwinm  r9,r1,0,0,(31-THREAD_SHIFT)
>         lwz     r0,TI_PREEMPT(r9)
> @@ -844,8 +875,6 @@ resume_kernel:
>          */
>         bl      trace_hardirqs_on
>  #endif
> -#else
> -resume_kernel:
>  #endif /* CONFIG_PREEMPT */
> 
>         /* interrupts are hard-disabled at this point */
> 
> Tiejun

Cheers,
Ben.