powerpc: remove resume_execution() in kprobes

[Ananth N Mavinakayanahalli]

On Fri, May 28, 2010 at 12:05:56PM +1000, Paul Mackerras wrote:
> On Thu, May 27, 2010 at 07:42:03PM +0530, Ananth N Mavinakayanahalli wrote:
> 
> > While we are at it, can we also add nop to the list of emulated
> > instructions?
> 
> I have a patch in development that emulates most of the arithmetic,
> logical and shift/rotate instructions, including ori.

OK.

> While you're here (in a virtual sense at least :), could you explain
> what's going on with the emulate_step() call in resume_execution() in
> arch/powerpc/kernel/kprobes.c?  It looks like, having decided that
> emulate_step() can't handle the instruction, you single-step the
> instruction out of line and then call emulate_step again on the same
> instruction, in resume_execution().  Why on earth is it trying to
> emulate the instruction when it has already been executed at this
> point?  Is there any reason why we can't just remove the emulate_step
> call from resume_execution()?

You are right. We needed emulate_step() in resume_execution() before we
had the code to handle the emulation in kprobe_handler() at the time of
the breakpoint it. At the time we needed it mainly to ensure branch
targets are reflected correctly in regs->nip if the stepped instruction
was a branch.

However, we now don't get to post_kprobe_handler() at all if
emulate_step() returned 1 at the time of the breakpoint hit. It suffices
if we just fixup the nip here. Patch below. Tested for various
instructions that can and can't be emulated...

---
From: Ananth N Mavinakayanahalli <ananth at in.ibm.com>

emulate_step() in kprobe_handler() would've already determined if the
probed instruction can be emulated. We single-step in hardware only if
the instruction couldn't be emulated. resume_execution() therefore is
superfluous -- all we need is to fix up the instruction pointer after
single-stepping.

Thanks to Paul Mackerras for catching this.

Signed-off-by: Ananth N Mavinakayanahalli <ananth at in.ibm.com>
---
 arch/powerpc/kernel/kprobes.c |   14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

Index: linux-2.6.34/arch/powerpc/kernel/kprobes.c
===================================================================
--- linux-2.6.34.orig/arch/powerpc/kernel/kprobes.c
+++ linux-2.6.34/arch/powerpc/kernel/kprobes.c
@@ -375,17 +375,6 @@ static int __kprobes trampoline_probe_ha
  * single-stepped a copy of the instruction.  The address of this
  * copy is p->ainsn.insn.
  */
-static void __kprobes resume_execution(struct kprobe *p, struct pt_regs *regs)
-{
-	int ret;
-	unsigned int insn = *p->ainsn.insn;
-
-	regs->nip = (unsigned long)p->addr;
-	ret = emulate_step(regs, insn);
-	if (ret == 0)
-		regs->nip = (unsigned long)p->addr + 4;
-}
-
 static int __kprobes post_kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *cur = kprobe_running();
@@ -403,7 +392,8 @@ static int __kprobes post_kprobe_handler
 		cur->post_handler(cur, regs, 0);
 	}
 
-	resume_execution(cur, regs);
+	/* Adjust nip to after the single-stepped instruction */
+	regs->nip = (unsigned long)cur->addr + 4;
 	regs->msr |= kcb->kprobe_saved_msr;
 
 	/*Restore back the original saved kprobes variables and continue. */