Message-ID: <1345696100.3338.21.camel@concordia>
Date:	Thu, 23 Aug 2012 14:28:20 +1000
From:	Michael Ellerman <michael@...erman.id.au>
To:	ananth@...ibm.com
Cc:	ppcdev <linuxppc-dev@...ts.ozlabs.org>,
	lkml <linux-kernel@...r.kernel.org>, benh@...nel.crashing.org,
	Paul Mackerras <paulus@...ba.org>,
	Anton Blanchard <anton@...ba.org>, peterz@...radead.org,
	oleg@...hat.com, Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH v4 2/2] powerpc: Uprobes port to powerpc

On Wed, 2012-08-22 at 13:57 +0530, Ananth N Mavinakayanahalli wrote:
> From: Ananth N Mavinakayanahalli <ananth@...ibm.com>
> 
> This is the port of uprobes to powerpc. Usage is similar to x86.

Hi Ananth,

Excuse my ignorance of uprobes, some comments inline ...


> [root@...x ~]# ./bin/perf probe -x /lib64/libc.so.6 malloc
> Added new event:
>   probe_libc:malloc    (on 0xb4860)
> 
> You can now use it in all perf tools, such as:
> 
> 	perf record -e probe_libc:malloc -aR sleep 1

Is there a test suite for any of this?


> Index: linux-tip-16aug/arch/powerpc/include/asm/uprobes.h
> ===================================================================
> --- /dev/null
> +++ linux-tip-16aug/arch/powerpc/include/asm/uprobes.h
> @@ -0,0 +1,58 @@
> +#ifndef _ASM_UPROBES_H
> +#define _ASM_UPROBES_H
> +/*
> + * User-space Probes (UProbes) for powerpc
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + * Copyright (C) IBM Corporation, 2007-2012

The lawyers say we shouldn't use (C).

Is it really copyright IBM 2007-2012? Or is that because you copied
another header?


> +typedef unsigned int uprobe_opcode_t;

I'd prefer u32.

It would be nice if someone could consolidate this with kprobe_opcode_t.


> +#define MAX_UINSN_BYTES			4
> +#define UPROBE_XOL_SLOT_BYTES		(MAX_UINSN_BYTES)
> +
> +#define UPROBE_SWBP_INSN		0x7fe00008

This is just "trap" ?

> +#define UPROBE_SWBP_INSN_SIZE		4 /* swbp insn size in bytes */
> +
> +#define IS_TW(instr)		(((instr) & 0xfc0007fe) == 0x7c000008)
> +#define IS_TD(instr)		(((instr) & 0xfc0007fe) == 0x7c000088)
> +#define IS_TDI(instr)		(((instr) & 0xfc000000) == 0x08000000)
> +#define IS_TWI(instr)		(((instr) & 0xfc000000) == 0x0c000000)
> +
> +#define is_trap(instr)	(IS_TW(instr) || IS_TD(instr) || \
> +			IS_TWI(instr) || IS_TDI(instr))

These seem to be duplicated in kprobes.h, can we consolidate them.

> +struct arch_uprobe {
> +	u8	insn[MAX_UINSN_BYTES];
> +};

Why not uprobe_opcode_t insn ?



> Index: linux-tip-16aug/arch/powerpc/kernel/signal.c
> ===================================================================
> --- linux-tip-16aug.orig/arch/powerpc/kernel/signal.c
> +++ linux-tip-16aug/arch/powerpc/kernel/signal.c
> @@ -11,6 +11,7 @@
>  
>  #include <linux/tracehook.h>
>  #include <linux/signal.h>
> +#include <linux/uprobes.h>
>  #include <linux/key.h>
>  #include <asm/hw_breakpoint.h>
>  #include <asm/uaccess.h>
> @@ -157,6 +158,11 @@ static int do_signal(struct pt_regs *reg
>  
>  void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
>  {
> +	if (thread_info_flags & _TIF_UPROBE) {
> +		clear_thread_flag(TIF_UPROBE);
> +		uprobe_notify_resume(regs);
> +	}

Presumably this ordering is crucial, ie. uprobes before signals.

>  	if (thread_info_flags & _TIF_SIGPENDING)
>  		do_signal(regs);
>  
> Index: linux-tip-16aug/arch/powerpc/kernel/uprobes.c
> ===================================================================
> --- /dev/null
> +++ linux-tip-16aug/arch/powerpc/kernel/uprobes.c
> @@ -0,0 +1,180 @@
> +/*
> + * User-space Probes (UProbes) for powerpc
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + * Copyright (C) IBM Corporation, 2007-2012
> + *
> + * Adapted from the x86 port by Ananth N Mavinakayanahalli <ananth@...ibm.com>
> + */
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/ptrace.h>
> +#include <linux/uprobes.h>
> +#include <linux/uaccess.h>
> +#include <linux/kdebug.h>
> +
> +#include <asm/sstep.h>
> +
> +#define UPROBE_TRAP_NR	UINT_MAX

In the comments below you talk about -1 a few times, but you actually
mean UINT_MAX.


> +/**
> + * arch_uprobe_analyze_insn

Analyze what about the instruction?

> + * @mm: the probed address space.
> + * @arch_uprobe: the probepoint information.
> + * @addr: vaddr to probe.
> + * Return 0 on success or a -ve number on error.
> + */
> +int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long addr)
> +{
> +	unsigned int insn;
> +
> +	if (addr & 0x03)
> +		return -EINVAL;
> +
> +	memcpy(&insn, auprobe->insn, MAX_UINSN_BYTES);

We shouldn't need to use memcpy, we know it's a u32.

> +	if (is_trap(insn))
> +		return -ENOTSUPP;

A comment saying why we can't handle this would be nice.

> +	return 0;
> +}


I am probably missing something, but why do we need to execute out of
line?

> +/*
> + * arch_uprobe_pre_xol - prepare to execute out of line.
> + * @auprobe: the probepoint information.
> + * @regs: reflects the saved user state of current task.
> + */
> +int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	struct arch_uprobe_task *autask = &current->utask->autask;
> +
> +	autask->saved_trap_nr = current->thread.trap_nr;
> +	current->thread.trap_nr = UPROBE_TRAP_NR;
> +	regs->nip = current->utask->xol_vaddr;
> +	return 0;
> +}
> +
> +/**
> + * uprobe_get_swbp_addr - compute address of swbp given post-swbp regs
> + * @regs: Reflects the saved state of the task after it has hit a breakpoint
> + * instruction.
> + * Return the address of the breakpoint instruction.
> + */
> +unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
> +{
> +	return instruction_pointer(regs);
> +}

This seems like it would be better in asm/uprobes.h as a static inline,
but that's not your fault.

> +/*
> + * If xol insn itself traps and generates a signal (SIGILL/SIGSEGV/etc),
> + * then detect the case where a singlestepped instruction jumps back to its
> + * own address. It is assumed that anything like do_page_fault/do_trap/etc
> + * sets thread.trap_nr != -1.
> + *
> + * arch_uprobe_pre_xol/arch_uprobe_post_xol save/restore thread.trap_nr,
> + * arch_uprobe_xol_was_trapped() simply checks that ->trap_nr is not equal to
> + * UPROBE_TRAP_NR == -1 set by arch_uprobe_pre_xol().
> + */
> +bool arch_uprobe_xol_was_trapped(struct task_struct *t)
> +{
> +	if (t->thread.trap_nr != UPROBE_TRAP_NR)
> +		return true;
> +
> +	return false;
> +}
> +
> +/*
> + * Called after single-stepping. To avoid the SMP problems that can
> + * occur when we temporarily put back the original opcode to
> + * single-step, we single-stepped a copy of the instruction.
> + *
> + * This function prepares to resume execution after the single-step.
> + */
> +int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask = current->utask;
> +
> +	WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
> +
> +	current->thread.trap_nr = utask->autask.saved_trap_nr;
> +
> +	/*
> +	 * On powerpc, except for loads and stores, most instructions
> +	 * including ones that alter code flow (branches, calls, returns)
> +	 * are emulated in the kernel. We get here only if the emulation
> +	 * support doesn't exist and have to fix-up the next instruction
> +	 * to be executed.
> +	 */
> +	regs->nip = utask->vaddr + MAX_UINSN_BYTES;
> +	return 0;
> +}
> +
> +/* callback routine for handling exceptions. */
> +int arch_uprobe_exception_notify(struct notifier_block *self, unsigned long val, void *data)
> +{
> +	struct die_args *args = data;
> +	struct pt_regs *regs = args->regs;
> +
> +	/* We are only interested in userspace traps */
> +	if (regs && !user_mode(regs))
> +		return NOTIFY_DONE;

Do we ever get here with a NULL regs?

> +	switch (val) {
> +	case DIE_BPT:
> +		if (uprobe_pre_sstep_notifier(regs))
> +			return NOTIFY_STOP;
> +		break;
> +	case DIE_SSTEP:
> +		if (uprobe_post_sstep_notifier(regs))
> +			return NOTIFY_STOP;
> +	default:
> +		break;
> +	}
> +	return NOTIFY_DONE;
> +}
> +
> +/*
> + * This function gets called when XOL instruction either gets trapped or
> + * the thread has a fatal signal, so reset the instruction pointer to its
> + * probed address.
> + */
> +void arch_uprobe_abort_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	struct uprobe_task *utask = current->utask;
> +
> +	current->thread.trap_nr = utask->autask.saved_trap_nr;
> +	instruction_pointer_set(regs, utask->vaddr);
> +}
> +
> +/*
> + * See if the instruction can be emulated.
> + * Returns true if instruction was emulated, false otherwise.
> + */
> +bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	int ret;
> +	unsigned int insn;
> +
> +	memcpy(&insn, auprobe->insn, MAX_UINSN_BYTES);

Why memcpy?

> +
> +	/*
> +	 * emulate_step() returns 1 if the insn was successfully emulated.
> +	 * For all other cases, we need to single-step in hardware.
> +	 */
> +	ret = emulate_step(regs, insn);
> +	if (ret > 0)
> +		return true;

This actually emulates the instruction, ie. the contents of regs are
changed based on the instruction.

That seems to differ vs x86, where arch_uprobe_skip_sstep() just checks
the instruction and returns true/false. Is that because on x86 they are
only returning true for nops? ie. there is no emulation to be done?

It's a little surprising that can_skip_sstep() actually emulates the
instruction, but again that's not your fault.

cheers

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists