powerpc: remove resume_execution() in kprobes

Message ID 20100528051919.GC25946@in.ibm.com
State Accepted, archived
Headers show

Commit Message

Ananth N Mavinakayanahalli May 28, 2010, 5:19 a.m.
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.


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

Thanks to Paul Mackerras for catching this.

Signed-off-by: Ananth N Mavinakayanahalli <ananth@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. */