[Patch 1/6] Prepare the PowerPC platform for HW Breakpoint infrastructure

[Patch 1/6] Prepare the PowerPC platform for HW Breakpoint infrastructure

[K.Prasad]

Prepare the PowerPC code for HW Breakpoint infrastructure patches by including
relevant constant definitions and function declarations.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/hw_breakpoint.h |   58 +++++++++++++++++++++++++++++++
 arch/powerpc/include/asm/processor.h     |    1 
 arch/powerpc/include/asm/reg.h           |    3 +
 3 files changed, 62 insertions(+)

Index: linux-2.6-tip.hbkpt/arch/powerpc/include/asm/hw_breakpoint.h
===================================================================
--- /dev/null
+++ linux-2.6-tip.hbkpt/arch/powerpc/include/asm/hw_breakpoint.h
@@ -0,0 +1,58 @@
+#ifndef	_PPC64_HW_BREAKPOINT_H
+#define	_PPC64_HW_BREAKPOINT_H
+
+#ifdef	__KERNEL__
+#define	__ARCH_HW_BREAKPOINT_H
+#ifdef CONFIG_PPC64
+
+struct arch_hw_breakpoint {
+	char		*name; /* Contains name of the symbol to set bkpt */
+	unsigned long	address;
+	int		type;
+};
+
+#include <linux/kdebug.h>
+#include <asm/reg.h>
+#include <asm-generic/hw_breakpoint.h>
+
+#define HW_BREAKPOINT_READ DABR_DATA_READ
+#define HW_BREAKPOINT_WRITE DABR_DATA_WRITE
+#define HW_BREAKPOINT_RW (DABR_DATA_READ | DABR_DATA_WRITE)
+
+#define HW_BREAKPOINT_ALIGN 0x7
+#define HW_BREAKPOINT_LEN INSTRUCTION_LEN
+
+extern struct hw_breakpoint *hbp_kernel[HBP_NUM];
+DECLARE_PER_CPU(struct hw_breakpoint*, this_hbp_kernel[HBP_NUM]);
+extern unsigned int hbp_user_refcount[HBP_NUM];
+
+extern void arch_install_thread_hw_breakpoint(struct task_struct *tsk);
+extern void arch_uninstall_thread_hw_breakpoint(void);
+extern int arch_validate_hwbkpt_settings(struct hw_breakpoint *bp,
+						struct task_struct *tsk);
+extern void arch_update_user_hw_breakpoint(int pos, struct task_struct *tsk);
+extern void arch_flush_thread_hw_breakpoint(struct task_struct *tsk);
+extern void arch_update_kernel_hw_breakpoint(void *);
+extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
+				     unsigned long val, void *data);
+
+extern void flush_thread_hw_breakpoint(struct task_struct *tsk);
+extern int copy_thread_hw_breakpoint(struct task_struct *tsk,
+		struct task_struct *child, unsigned long clone_flags);
+extern void load_debug_registers(void );
+extern void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs);
+
+static inline void hw_breakpoint_disable(void)
+{
+	set_dabr(0);
+}
+
+#else
+static inline void hw_breakpoint_disable(void)
+{
+	/* Function is defined only on PPC64 for now */
+}
+#endif	/* CONFIG_PPC64 */
+#endif	/* __KERNEL__ */
+#endif	/* _PPC64_HW_BREAKPOINT_H */
+
Index: linux-2.6-tip.hbkpt/arch/powerpc/include/asm/processor.h
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/include/asm/processor.h
+++ linux-2.6-tip.hbkpt/arch/powerpc/include/asm/processor.h
@@ -177,6 +177,7 @@ struct thread_struct {
 #ifdef CONFIG_PPC64
 	unsigned long	start_tb;	/* Start purr when proc switched in */
 	unsigned long	accum_tb;	/* Total accumilated purr for process */
+	struct hw_breakpoint *hbp[HBP_NUM];
 #endif
 	unsigned long	dabr;		/* Data address breakpoint register */
 #ifdef CONFIG_ALTIVEC
Index: linux-2.6-tip.hbkpt/arch/powerpc/include/asm/reg.h
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/include/asm/reg.h
+++ linux-2.6-tip.hbkpt/arch/powerpc/include/asm/reg.h
@@ -26,6 +26,8 @@
 #include <asm/reg_8xx.h>
 #endif /* CONFIG_8xx */
 
+#define INSTRUCTION_LEN	4		/* Length of any instruction */
+
 #define MSR_SF_LG	63              /* Enable 64 bit mode */
 #define MSR_ISF_LG	61              /* Interrupt 64b mode valid on 630 */
 #define MSR_HV_LG 	60              /* Hypervisor state */
@@ -184,6 +186,7 @@
 #define   CTRL_TE	0x00c00000	/* thread enable */
 #define   CTRL_RUNLATCH	0x1
 #define SPRN_DABR	0x3F5	/* Data Address Breakpoint Register */
+#define   HBP_NUM	1	/* Number of physical HW breakpoint registers */
 #define   DABR_TRANSLATION	(1UL << 2)
 #define   DABR_DATA_WRITE	(1UL << 1)
 #define   DABR_DATA_READ	(1UL << 0)

[Patch 2/6] Introduce PPC64 specific Hardware Breakpoint interfaces

[K.Prasad]

Introduce PPC64 implementation for the generic hardware breakpoint interfaces
defined in kernel/hw_breakpoint.c. Enable the HAVE_HW_BREAKPOINT flag and the
Makefile.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/Kconfig                |    1 
 arch/powerpc/kernel/Makefile        |    2 
 arch/powerpc/kernel/hw_breakpoint.c |  290 ++++++++++++++++++++++++++++++++++++
 3 files changed, 292 insertions(+), 1 deletion(-)

Index: linux-2.6-tip.hbkpt/arch/powerpc/Kconfig
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/Kconfig
+++ linux-2.6-tip.hbkpt/arch/powerpc/Kconfig
@@ -125,6 +125,7 @@ config PPC
 	select USE_GENERIC_SMP_HELPERS if SMP
 	select HAVE_OPROFILE
 	select HAVE_SYSCALL_WRAPPERS if PPC64
+	select HAVE_HW_BREAKPOINT if PPC64
 
 config EARLY_PRINTK
 	bool
Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/Makefile
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/Makefile
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/Makefile
@@ -33,7 +33,7 @@ obj-$(CONFIG_PPC64)		+= setup_64.o sys_p
 				   signal_64.o ptrace32.o \
 				   paca.o cpu_setup_ppc970.o \
 				   cpu_setup_pa6t.o \
-				   firmware.o nvram_64.o
+				   firmware.o nvram_64.o hw_breakpoint.o
 obj64-$(CONFIG_RELOCATABLE)	+= reloc_64.o
 obj-$(CONFIG_PPC64)		+= vdso64/
 obj-$(CONFIG_ALTIVEC)		+= vecemu.o vector.o
Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/hw_breakpoint.c
===================================================================
--- /dev/null
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/hw_breakpoint.c
@@ -0,0 +1,290 @@
+/*
+ * HW_breakpoint: a unified kernel/user-space hardware breakpoint facility,
+ * using the CPU's debug registers.
+ */
+
+/*
+ * 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 © 2009 IBM Corporation
+ */
+
+#include <linux/notifier.h>
+#include <linux/kallsyms.h>
+#include <linux/kprobes.h>
+#include <linux/percpu.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/sched.h>
+#include <linux/init.h>
+#include <linux/smp.h>
+
+#include <asm/hw_breakpoint.h>
+#include <asm/processor.h>
+#include <asm/sstep.h>
+
+/* Store the kernel-space breakpoint address value */
+static unsigned long kdabr;
+
+/*
+ * Temporarily stores address for DABR before it is written by the
+ * single-step handler routine
+ */
+static DEFINE_PER_CPU(unsigned long, dabr_data);
+
+void arch_update_kernel_hw_breakpoint(void *unused)
+{
+	struct hw_breakpoint *bp;
+
+	/* Check if there is nothing to update */
+	if (hbp_kernel_pos == HBP_NUM)
+		return;
+
+	per_cpu(this_hbp_kernel[hbp_kernel_pos], get_cpu()) = bp =
+						hbp_kernel[hbp_kernel_pos];
+	if (bp == NULL)
+		kdabr = 0;
+	else
+		kdabr = (bp->info.address & ~HW_BREAKPOINT_ALIGN) |
+			bp->info.type | DABR_TRANSLATION;
+	set_dabr(kdabr);
+	put_cpu_no_resched();
+}
+
+/*
+ * Install the thread breakpoints in their debug registers.
+ */
+void arch_install_thread_hw_breakpoint(struct task_struct *tsk)
+{
+	set_dabr(tsk->thread.dabr);
+}
+
+/*
+ * Install the debug register values for just the kernel, no thread.
+ */
+void arch_uninstall_thread_hw_breakpoint()
+{
+	set_dabr(0);
+}
+
+/*
+ * Store a breakpoint's encoded address, length, and type.
+ */
+int arch_store_info(struct hw_breakpoint *bp, struct task_struct *tsk)
+{
+	/*
+	 * User-space requests will always have the address field populated
+	* Symbol names from user-space are rejected
+	*/
+	if (tsk && bp->info.name)
+		return -EINVAL;
+	/*
+	 * User-space requests will always have the address field populated
+	 * For kernel-addresses, either the address or symbol name can be
+	 * specified.
+	 */
+	if (bp->info.name)
+		bp->info.address = (unsigned long)
+					kallsyms_lookup_name(bp->info.name);
+	if (bp->info.address)
+		return 0;
+	return -EINVAL;
+}
+
+/*
+ * Validate the arch-specific HW Breakpoint register settings
+ */
+int arch_validate_hwbkpt_settings(struct hw_breakpoint *bp,
+						struct task_struct *tsk)
+{
+	int is_kernel, ret = -EINVAL;
+
+	if (!bp)
+		return ret;
+
+	switch (bp->info.type) {
+	case HW_BREAKPOINT_READ:
+	case HW_BREAKPOINT_WRITE:
+	case HW_BREAKPOINT_RW:
+		break;
+	default:
+		return ret;
+	}
+
+	if (bp->triggered)
+		ret = arch_store_info(bp, tsk);
+
+	is_kernel = is_kernel_addr(bp->info.address);
+	if ((tsk && is_kernel) || (!tsk && !is_kernel))
+		return -EINVAL;
+
+	return ret;
+}
+
+void arch_update_user_hw_breakpoint(int pos, struct task_struct *tsk)
+{
+	struct thread_struct *thread = &(tsk->thread);
+	struct hw_breakpoint *bp = thread->hbp[0];
+
+	if (bp)
+		thread->dabr = (bp->info.address & ~HW_BREAKPOINT_ALIGN) |
+				bp->info.type | DABR_TRANSLATION;
+	else
+		thread->dabr = 0;
+}
+
+void arch_flush_thread_hw_breakpoint(struct task_struct *tsk)
+{
+	struct thread_struct *thread = &(tsk->thread);
+
+	thread->dabr = 0;
+}
+
+/*
+ * Handle debug exception notifications.
+ */
+int __kprobes hw_breakpoint_handler(struct die_args *args)
+{
+	int rc = NOTIFY_STOP;
+	struct hw_breakpoint *bp;
+	struct pt_regs *regs = args->regs;
+	unsigned long dar = regs->dar;
+	int cpu, is_one_shot, stepped = 1;
+
+	/* Disable breakpoints during exception handling */
+	set_dabr(0);
+
+	cpu = get_cpu();
+	/* Determine whether kernel- or user-space address is the trigger */
+	bp = (hbp_kernel_pos == HBP_NUM) ? current->thread.hbp[0] :
+					per_cpu(this_hbp_kernel[0], cpu);
+	/*
+	 * bp can be NULL due to lazy debug register switching
+	 * or due to the delay between updates of hbp_kernel_pos
+	 * and this_hbp_kernel.
+	 */
+	if (!bp)
+		goto out;
+
+	if (dar == bp->info.address)
+		per_cpu(dabr_data, cpu) = (hbp_kernel_pos == HBP_NUM) ?
+						current->thread.dabr : kdabr;
+	else {
+		/*
+		 * This exception is triggered not because of a memory access on
+		 * the monitored variable but in the double-word address range
+		 * in which it is contained. We will consume this exception,
+		 * considering it as 'noise'.
+		 */
+		rc = NOTIFY_STOP;
+		goto out;
+	}
+	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
+
+	(bp->triggered)(bp, regs);
+	/*
+	 * Ptrace expects the HW Breakpoints to be one-shot. We will return
+	 * NOTIFY_DONE without restoring DABR with the breakpoint address. The
+	 * downstream code will generate SIGTRAP to the process
+	 */
+	if (is_one_shot) {
+		rc = NOTIFY_DONE;
+		goto out;
+	}
+
+	stepped = emulate_step(regs, regs->nip);
+	/*
+	 * Single-step the causative instruction manually if
+	 * emulate_step() could not execute it
+	 */
+	if (stepped == 0) {
+		regs->msr |= MSR_SE;
+		goto out;
+	}
+	set_dabr(per_cpu(dabr_data, cpu));
+	per_cpu(dabr_data, cpu) = 0;
+
+out:
+	/* Enable pre-emption only if single-stepping is finished */
+	if (stepped)
+		put_cpu_no_resched();
+	return rc;
+}
+
+/*
+ * Handle single-step exceptions following a DABR hit.
+ */
+int __kprobes single_step_dabr_instruction(struct die_args *args)
+{
+	struct pt_regs *regs = args->regs;
+	int cpu = get_cpu();
+	int ret = NOTIFY_DONE;
+	siginfo_t info;
+	unsigned long this_dabr_data = per_cpu(dabr_data, cpu);
+
+	/*
+	 * Check if we are single-stepping as a result of a
+	 * previous HW Breakpoint exception
+	 */
+	if (this_dabr_data == 0)
+		goto out;
+
+	regs->msr &= ~MSR_SE;
+	/* Deliver signal to user-space */
+	if (this_dabr_data < TASK_SIZE) {
+		info.si_signo = SIGTRAP;
+		info.si_errno = 0;
+		info.si_code = TRAP_HWBKPT;
+		info.si_addr = (void __user *)(per_cpu(dabr_data, cpu));
+		force_sig_info(SIGTRAP, &info, current);
+	}
+
+	set_dabr(this_dabr_data);
+	per_cpu(dabr_data, cpu) = 0;
+	ret = NOTIFY_STOP;
+	/*
+	 * If single-stepped after hw_breakpoint_handler(), pre-emption is
+	 * already disabled.
+	 */
+	put_cpu_no_resched();
+
+out:
+	/*
+	 * A put_cpu_no_resched() call is required to complement the get_cpu()
+	 * call used initially
+	 */
+	put_cpu_no_resched();
+	return ret;
+}
+
+/*
+ * Handle debug exception notifications.
+ */
+int __kprobes hw_breakpoint_exceptions_notify(
+		struct notifier_block *unused, unsigned long val, void *data)
+{
+	int ret = NOTIFY_DONE;
+
+	switch (val) {
+	case DIE_DABR_MATCH:
+		ret = hw_breakpoint_handler(data);
+		break;
+	case DIE_SSTEP:
+		ret = single_step_dabr_instruction(data);
+		break;
+	}
+
+	return ret;
+}

[Patch 3/6] Modify ptrace code to use Hardware Breakpoint interfaces

[K.Prasad]

Modify the ptrace code to use the hardware breakpoint interfaces for user-space.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c |   47 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
@@ -37,6 +37,7 @@
 #include <asm/page.h>
 #include <asm/pgtable.h>
 #include <asm/system.h>
+#include <asm/hw_breakpoint.h>
 
 /*
  * does not yet catch signals sent when the child dies.
@@ -735,9 +736,26 @@ void user_disable_single_step(struct tas
 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
 }
 
+void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
+{
+	/*
+	 * Unregister the breakpoint request here since ptrace has defined a
+	 * one-shot behaviour for breakpoint exceptions in PPC64.
+	 * The SIGTRAP signal is generated automatically for us in do_dabr().
+	 * We don't have to do anything here
+	 */
+	unregister_user_hw_breakpoint(current, bp);
+	kfree(bp);
+}
+
 int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
+#ifdef CONFIG_PPC64
+	struct thread_struct *thread = &(task->thread);
+	struct hw_breakpoint *bp;
+	int ret;
+#endif
 	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
 	 *  For embedded processors we support one DAC and no IAC's at the
 	 *  moment.
@@ -767,6 +785,35 @@ int ptrace_set_debugreg(struct task_stru
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+#ifdef CONFIG_PPC64
+	bp = thread->hbp[0];
+	if (data == 0) {
+		if (bp) {
+			unregister_user_hw_breakpoint(task, bp);
+			kfree(bp);
+		}
+		return 0;
+	}
+
+	if (bp) {
+		bp->info.type = data & HW_BREAKPOINT_RW;
+		task->thread.dabr = bp->info.address = data;
+		return modify_user_hw_breakpoint(task, bp);
+	}
+	bp = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL);
+	if (!bp)
+		return -ENOMEM;
+
+	/* Store the type of breakpoint */
+	bp->info.type = data & HW_BREAKPOINT_RW;
+	bp->triggered = ptrace_triggered;
+	task->thread.dabr = bp->info.address = data;
+
+	ret = register_user_hw_breakpoint(task, bp);
+	if (ret)
+		return ret;
+#endif /* CONFIG_PPC64 */
+
 	/* Move contents to the DABR register */
 	task->thread.dabr = data;
 

[Patch 4/6] Modify process and processor handling code to recognise hardware debug registers

[K.Prasad]

Modify process handling code to recognise hardware debug registers during copy
and flush operations. Introduce a new TIF_DEBUG task flag to indicate a
process's use of debug register. Load the debug register values into a
new CPU during initialisation.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/thread_info.h |    2 ++
 arch/powerpc/kernel/process.c          |   15 +++++++++++++++
 arch/powerpc/kernel/smp.c              |    2 ++
 3 files changed, 19 insertions(+)

Index: linux-2.6-tip.hbkpt/arch/powerpc/include/asm/thread_info.h
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/include/asm/thread_info.h
+++ linux-2.6-tip.hbkpt/arch/powerpc/include/asm/thread_info.h
@@ -114,6 +114,7 @@ static inline struct thread_info *curren
 #define TIF_FREEZE		14	/* Freezing for suspend */
 #define TIF_RUNLATCH		15	/* Is the runlatch enabled? */
 #define TIF_ABI_PENDING		16	/* 32/64 bit switch needed */
+#define TIF_DEBUG		17	/* uses debug registers */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -132,6 +133,7 @@ static inline struct thread_info *curren
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
 #define _TIF_RUNLATCH		(1<<TIF_RUNLATCH)
 #define _TIF_ABI_PENDING	(1<<TIF_ABI_PENDING)
+#define _TIF_DEBUG		(1<<TIF_DEBUG)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT|_TIF_SECCOMP)
 
 #define _TIF_USER_WORK_MASK	(_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/process.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/process.c
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/process.c
@@ -50,6 +50,7 @@
 #include <asm/syscalls.h>
 #ifdef CONFIG_PPC64
 #include <asm/firmware.h>
+#include <asm/hw_breakpoint.h>
 #endif
 #include <linux/kprobes.h>
 #include <linux/kdebug.h>
@@ -254,8 +255,10 @@ void do_dabr(struct pt_regs *regs, unsig
 			11, SIGSEGV) == NOTIFY_STOP)
 		return;
 
+#ifndef CONFIG_PPC64
 	if (debugger_dabr_match(regs))
 		return;
+#endif
 
 	/* Clear the DAC and struct entries.  One shot trigger */
 #if defined(CONFIG_BOOKE)
@@ -372,8 +375,13 @@ struct task_struct *__switch_to(struct t
 
 #endif /* CONFIG_SMP */
 
+#ifdef CONFIG_PPC64
+		if (unlikely(test_tsk_thread_flag(new, TIF_DEBUG)))
+			arch_install_thread_hw_breakpoint(new);
+#else
 	if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
 		set_dabr(new->thread.dabr);
+#endif /* CONFIG_PPC64 */
 
 #if defined(CONFIG_BOOKE)
 	/* If new thread DAC (HW breakpoint) is the same then leave it */
@@ -550,6 +558,10 @@ void show_regs(struct pt_regs * regs)
 void exit_thread(void)
 {
 	discard_lazy_cpu_state();
+#ifdef CONFIG_PPC64
+	if (unlikely(test_tsk_thread_flag(current, TIF_DEBUG)))
+		flush_thread_hw_breakpoint(current);
+#endif /* CONFIG_PPC64 */
 }
 
 void flush_thread(void)
@@ -672,6 +684,9 @@ int copy_thread(unsigned long clone_flag
 	 * function.
  	 */
 	kregs->nip = *((unsigned long *)ret_from_fork);
+
+	if (unlikely(test_tsk_thread_flag(current, TIF_DEBUG)))
+		copy_thread_hw_breakpoint(current, p, clone_flags);
 #else
 	kregs->nip = (unsigned long)ret_from_fork;
 #endif
Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/smp.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/smp.c
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/smp.c
@@ -48,6 +48,7 @@
 #include <asm/vdso_datapage.h>
 #ifdef CONFIG_PPC64
 #include <asm/paca.h>
+#include <asm/hw_breakpoint.h>
 #endif
 
 #ifdef DEBUG
@@ -536,6 +537,7 @@ int __devinit start_secondary(void *unus
 
 	local_irq_enable();
 
+	load_debug_registers();
 	cpu_idle();
 	return 0;
 }

[Patch 5/6] Modify Data storage exception code to recognise DABR match first

[K.Prasad]

Modify Data storage exception code to first lookout for a DABR match before
recognising a kprobe or xmon exception.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/mm/fault.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Index: linux-2.6-tip.hbkpt/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/mm/fault.c
+++ linux-2.6-tip.hbkpt/arch/powerpc/mm/fault.c
@@ -136,6 +136,12 @@ int __kprobes do_page_fault(struct pt_re
 		error_code &= 0x48200000;
 	else
 		is_write = error_code & DSISR_ISSTORE;
+
+	if (error_code & DSISR_DABRMATCH) {
+		/* DABR match */
+		do_dabr(regs, address, error_code);
+		return 0;
+	}
 #else
 	is_write = error_code & ESR_DST;
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
@@ -150,14 +156,6 @@ int __kprobes do_page_fault(struct pt_re
 	if (!user_mode(regs) && (address >= TASK_SIZE))
 		return SIGSEGV;
 
-#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
-  	if (error_code & DSISR_DABRMATCH) {
-		/* DABR match */
-		do_dabr(regs, address, error_code);
-		return 0;
-	}
-#endif /* !(CONFIG_4xx || CONFIG_BOOKE)*/
-
 	if (in_atomic() || mm == NULL) {
 		if (!user_mode(regs))
 			return SIGSEGV;

[Patch 6/6] Adapt kexec and samples code to recognise PPC64 hardware breakpoint usage

[K.Prasad]

Modify kexec code to disable DABR registers before a reboot. Adapt the samples
code to populate PPC64-arch specific fields.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/machine_kexec_64.c  |    3 +++
 samples/hw_breakpoint/data_breakpoint.c |    4 ++++
 2 files changed, 7 insertions(+)

Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/machine_kexec_64.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/machine_kexec_64.c
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/machine_kexec_64.c
@@ -24,6 +24,7 @@
 #include <asm/sections.h>	/* _end */
 #include <asm/prom.h>
 #include <asm/smp.h>
+#include <asm/hw_breakpoint.h>
 
 int default_machine_kexec_prepare(struct kimage *image)
 {
@@ -214,6 +215,7 @@ static void kexec_prepare_cpus(void)
 	put_cpu();
 
 	local_irq_disable();
+	hw_breakpoint_disable();
 }
 
 #else /* ! SMP */
@@ -233,6 +235,7 @@ static void kexec_prepare_cpus(void)
 	if (ppc_md.kexec_cpu_down)
 		ppc_md.kexec_cpu_down(0, 0);
 	local_irq_disable();
+	hw_breakpoint_disable();
 }
 
 #endif /* SMP */
Index: linux-2.6-tip.hbkpt/samples/hw_breakpoint/data_breakpoint.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/samples/hw_breakpoint/data_breakpoint.c
+++ linux-2.6-tip.hbkpt/samples/hw_breakpoint/data_breakpoint.c
@@ -54,6 +54,10 @@ static int __init hw_break_module_init(v
 	sample_hbp.info.type = HW_BREAKPOINT_WRITE;
 	sample_hbp.info.len = HW_BREAKPOINT_LEN_4;
 #endif /* CONFIG_X86 */
+#ifdef CONFIG_PPC64
+	sample_hbp.info.name = ksym_name;
+	sample_hbp.info.type = HW_BREAKPOINT_WRITE;
+#endif /* CONFIG_PPC64 */
 
 	sample_hbp.triggered = (void *)sample_hbp_handler;
 

Re: [Patch 2/6] Introduce PPC64 specific Hardware Breakpoint interfaces

[David Gibson]

On Wed, Jun 03, 2009 at 10:05:11PM +0530, K.Prasad wrote:
> Introduce PPC64 implementation for the generic hardware breakpoint interfaces
> defined in kernel/hw_breakpoint.c. Enable the HAVE_HW_BREAKPOINT flag and the
> Makefile.


[snip]
> +/*
> + * Install the debug register values for just the kernel, no thread.

This comment does seem to quite match the function below.

> + */
> +void arch_uninstall_thread_hw_breakpoint()
> +{
> +	set_dabr(0);
> +}
> +
> +/*
> + * Store a breakpoint's encoded address, length, and type.
> + */
> +int arch_store_info(struct hw_breakpoint *bp, struct task_struct *tsk)
> +{
> +	/*
> +	 * User-space requests will always have the address field populated
> +	* Symbol names from user-space are rejected
> +	*/
> +	if (tsk && bp->info.name)
> +		return -EINVAL;
> +	/*
> +	 * User-space requests will always have the address field populated
> +	 * For kernel-addresses, either the address or symbol name can be
> +	 * specified.
> +	 */
> +	if (bp->info.name)
> +		bp->info.address = (unsigned long)
> +					kallsyms_lookup_name(bp->info.name);

Archs don't have to implement this name lookup stuff, but it looks
like most of them would - so it looks like there ought to be a helper
function in generic code that will do the check / name lookup stuff.


> +	if (bp->info.address)
> +		return 0;

Hrm.. you realise there's no theoretical reason a userspace program
couldn't put a breakpoint at address 0...?

> +	return -EINVAL;
> +}
> +
> +/*
> + * Validate the arch-specific HW Breakpoint register settings
> + */
> +int arch_validate_hwbkpt_settings(struct hw_breakpoint *bp,
> +						struct task_struct *tsk)
> +{
> +	int is_kernel, ret = -EINVAL;
> +
> +	if (!bp)
> +		return ret;
> +
> +	switch (bp->info.type) {
> +	case HW_BREAKPOINT_READ:
> +	case HW_BREAKPOINT_WRITE:
> +	case HW_BREAKPOINT_RW:
> +		break;
> +	default:
> +		return ret;
> +	}
> +
> +	if (bp->triggered)
> +		ret = arch_store_info(bp, tsk);
> +
> +	is_kernel = is_kernel_addr(bp->info.address);
> +	if ((tsk && is_kernel) || (!tsk && !is_kernel))
> +		return -EINVAL;
> +
> +	return ret;
> +}
> +
> +void arch_update_user_hw_breakpoint(int pos, struct task_struct *tsk)
> +{
> +	struct thread_struct *thread = &(tsk->thread);
> +	struct hw_breakpoint *bp = thread->hbp[0];
> +
> +	if (bp)
> +		thread->dabr = (bp->info.address & ~HW_BREAKPOINT_ALIGN) |
> +				bp->info.type | DABR_TRANSLATION;
> +	else
> +		thread->dabr = 0;
> +}
> +
> +void arch_flush_thread_hw_breakpoint(struct task_struct *tsk)
> +{
> +	struct thread_struct *thread = &(tsk->thread);
> +
> +	thread->dabr = 0;
> +}
> +
> +/*
> + * Handle debug exception notifications.
> + */
> +int __kprobes hw_breakpoint_handler(struct die_args *args)
> +{
> +	int rc = NOTIFY_STOP;
> +	struct hw_breakpoint *bp;
> +	struct pt_regs *regs = args->regs;
> +	unsigned long dar = regs->dar;
> +	int cpu, is_one_shot, stepped = 1;
> +
> +	/* Disable breakpoints during exception handling */
> +	set_dabr(0);
> +
> +	cpu = get_cpu();
> +	/* Determine whether kernel- or user-space address is the trigger */
> +	bp = (hbp_kernel_pos == HBP_NUM) ? current->thread.hbp[0] :
> +					per_cpu(this_hbp_kernel[0], cpu);
> +	/*
> +	 * bp can be NULL due to lazy debug register switching
> +	 * or due to the delay between updates of hbp_kernel_pos
> +	 * and this_hbp_kernel.
> +	 */
> +	if (!bp)
> +		goto out;
> +
> +	if (dar == bp->info.address)
> +		per_cpu(dabr_data, cpu) = (hbp_kernel_pos == HBP_NUM) ?
> +						current->thread.dabr : kdabr;
> +	else {
> +		/*
> +		 * This exception is triggered not because of a memory access on
> +		 * the monitored variable but in the double-word address range
> +		 * in which it is contained. We will consume this exception,
> +		 * considering it as 'noise'.
> +		 */
> +		rc = NOTIFY_STOP;
> +		goto out;
> +	}
> +	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;

Ouch, explicitly special-casing ptrace_triggered is pretty nasty.
Since the bp_info is already arch specific, maybe it should include a
flag to indicate whether the breakpoint is one-shot or not.

> +	(bp->triggered)(bp, regs);
> +	/*
> +	 * Ptrace expects the HW Breakpoints to be one-shot. We will return
> +	 * NOTIFY_DONE without restoring DABR with the breakpoint address. The
> +	 * downstream code will generate SIGTRAP to the process
> +	 */
> +	if (is_one_shot) {
> +		rc = NOTIFY_DONE;
> +		goto out;

Don't you need to clear dabr_data?  Otherwise if we enter single step
for some other reason (e.g. gdb turns it on), won't we incorrectly hit
the code-path to step over a dabr breakpoint?

> +	}
> +
> +	stepped = emulate_step(regs, regs->nip);
> +	/*
> +	 * Single-step the causative instruction manually if
> +	 * emulate_step() could not execute it
> +	 */
> +	if (stepped == 0) {
> +		regs->msr |= MSR_SE;
> +		goto out;
> +	}
> +	set_dabr(per_cpu(dabr_data, cpu));
> +	per_cpu(dabr_data, cpu) = 0;
> +
> +out:
> +	/* Enable pre-emption only if single-stepping is finished */
> +	if (stepped)
> +		put_cpu_no_resched();
> +	return rc;
> +}
> +
> +/*
> + * Handle single-step exceptions following a DABR hit.
> + */
> +int __kprobes single_step_dabr_instruction(struct die_args *args)
> +{
> +	struct pt_regs *regs = args->regs;
> +	int cpu = get_cpu();
> +	int ret = NOTIFY_DONE;
> +	siginfo_t info;
> +	unsigned long this_dabr_data = per_cpu(dabr_data, cpu);
> +
> +	/*
> +	 * Check if we are single-stepping as a result of a
> +	 * previous HW Breakpoint exception
> +	 */
> +	if (this_dabr_data == 0)
> +		goto out;
> +
> +	regs->msr &= ~MSR_SE;
> +	/* Deliver signal to user-space */
> +	if (this_dabr_data < TASK_SIZE) {
> +		info.si_signo = SIGTRAP;
> +		info.si_errno = 0;
> +		info.si_code = TRAP_HWBKPT;
> +		info.si_addr = (void __user *)(per_cpu(dabr_data, cpu));
> +		force_sig_info(SIGTRAP, &info, current);

Uh.. I recall mentioning in my previous review that in order to match
previous behaviour we need to deliver the userspace signal *before*
stepping over the breakpointed instruction, rather than after (which
I guess is why breakpoints are one-shot in the old scheme).

> +	}
> +
> +	set_dabr(this_dabr_data);
> +	per_cpu(dabr_data, cpu) = 0;
> +	ret = NOTIFY_STOP;
> +	/*
> +	 * If single-stepped after hw_breakpoint_handler(), pre-emption is
> +	 * already disabled.
> +	 */
> +	put_cpu_no_resched();
> +
> +out:
> +	/*
> +	 * A put_cpu_no_resched() call is required to complement the get_cpu()
> +	 * call used initially
> +	 */
> +	put_cpu_no_resched();
> +	return ret;
> +}
> +
> +/*
> + * Handle debug exception notifications.
> + */
> +int __kprobes hw_breakpoint_exceptions_notify(
> +		struct notifier_block *unused, unsigned long val, void *data)
> +{
> +	int ret = NOTIFY_DONE;
> +
> +	switch (val) {
> +	case DIE_DABR_MATCH:
> +		ret = hw_breakpoint_handler(data);
> +		break;
> +	case DIE_SSTEP:
> +		ret = single_step_dabr_instruction(data);
> +		break;
> +	}
> +
> +	return ret;
> +}
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Patch 3/6] Modify ptrace code to use Hardware Breakpoint interfaces

[David Gibson]

On Wed, Jun 03, 2009 at 10:05:24PM +0530, K.Prasad wrote:
> Modify the ptrace code to use the hardware breakpoint interfaces for user-space.
> 
> Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/ptrace.c |   47 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c
> +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
> @@ -37,6 +37,7 @@
>  #include <asm/page.h>
>  #include <asm/pgtable.h>
>  #include <asm/system.h>
> +#include <asm/hw_breakpoint.h>
>  
>  /*
>   * does not yet catch signals sent when the child dies.
> @@ -735,9 +736,26 @@ void user_disable_single_step(struct tas
>  	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
>  }
>  
> +void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
> +{
> +	/*
> +	 * Unregister the breakpoint request here since ptrace has defined a
> +	 * one-shot behaviour for breakpoint exceptions in PPC64.
> +	 * The SIGTRAP signal is generated automatically for us in do_dabr().
> +	 * We don't have to do anything here
> +	 */
> +	unregister_user_hw_breakpoint(current, bp);
> +	kfree(bp);

Couldn't you also clear the saved dabr info here, to avoid having to
special case this in the actual breakpoint handler.

Also, I think you should be delivering the signal here - for gdb
compatibility I think we'll need to match the old behaviour which has
the TRAP delivered before executing the breakpointed instruction.

> +}
> +
>  int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
>  			       unsigned long data)
>  {
> +#ifdef CONFIG_PPC64
> +	struct thread_struct *thread = &(task->thread);
> +	struct hw_breakpoint *bp;
> +	int ret;
> +#endif
>  	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
>  	 *  For embedded processors we support one DAC and no IAC's at the
>  	 *  moment.
> @@ -767,6 +785,35 @@ int ptrace_set_debugreg(struct task_stru
>  	if (data && !(data & DABR_TRANSLATION))
>  		return -EIO;
>  
> +#ifdef CONFIG_PPC64
> +	bp = thread->hbp[0];
> +	if (data == 0) {
> +		if (bp) {
> +			unregister_user_hw_breakpoint(task, bp);
> +			kfree(bp);
> +		}
> +		return 0;
> +	}
> +
> +	if (bp) {
> +		bp->info.type = data & HW_BREAKPOINT_RW;
> +		task->thread.dabr = bp->info.address = data;
> +		return modify_user_hw_breakpoint(task, bp);
> +	}
> +	bp = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL);
> +	if (!bp)
> +		return -ENOMEM;
> +
> +	/* Store the type of breakpoint */
> +	bp->info.type = data & HW_BREAKPOINT_RW;
> +	bp->triggered = ptrace_triggered;
> +	task->thread.dabr = bp->info.address = data;
> +
> +	ret = register_user_hw_breakpoint(task, bp);
> +	if (ret)
> +		return ret;
> +#endif /* CONFIG_PPC64 */
> +
>  	/* Move contents to the DABR register */
>  	task->thread.dabr = data;
>  
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Patch 2/6] Introduce PPC64 specific Hardware Breakpoint interfaces

[K.Prasad]

On Fri, Jun 05, 2009 at 03:11:58PM +1000, David Gibson wrote:
> On Wed, Jun 03, 2009 at 10:05:11PM +0530, K.Prasad wrote:

Hi David,
	Sorry for the delay in response below. In the meanwhile, I
discovered an issue in detecting stray exceptions that affected
user-space handling of breakpoints. I've made some changes to correct
that behaviour which will be included in version VI of the patchset.

> > Introduce PPC64 implementation for the generic hardware breakpoint interfaces
> > defined in kernel/hw_breakpoint.c. Enable the HAVE_HW_BREAKPOINT flag and the
> > Makefile.
> 
> 
> [snip]
> > +/*
> > + * Install the debug register values for just the kernel, no thread.
> 
> This comment does seem to quite match the function below.
> 

Thanks for pointing out. Will change it to read thus:
/*
 * Clear the DABR which contains the thread-specific breakpoint address
 */

> > + */
> > +void arch_uninstall_thread_hw_breakpoint()
> > +{
> > +	set_dabr(0);
> > +}
> > +
> > +/*
> > + * Store a breakpoint's encoded address, length, and type.
> > + */
> > +int arch_store_info(struct hw_breakpoint *bp, struct task_struct *tsk)
> > +{
> > +	/*
> > +	 * User-space requests will always have the address field populated
> > +	* Symbol names from user-space are rejected
> > +	*/
> > +	if (tsk && bp->info.name)
> > +		return -EINVAL;
> > +	/*
> > +	 * User-space requests will always have the address field populated
> > +	 * For kernel-addresses, either the address or symbol name can be
> > +	 * specified.
> > +	 */
> > +	if (bp->info.name)
> > +		bp->info.address = (unsigned long)
> > +					kallsyms_lookup_name(bp->info.name);
> 
> Archs don't have to implement this name lookup stuff, but it looks
> like most of them would - so it looks like there ought to be a helper
> function in generic code that will do the check / name lookup stuff.
> 
>

It doesn't turn out to be very generic. The IO breakpoints in x86, the
address-range (only) breakpoints in S390 and perhaps 4xx powerpc
processors were what made me think that this should remain in
arch-specific code. In these cases, we might have to deal only with
breakpoint addresses and not names.

> > +	if (bp->info.address)
> > +		return 0;
> 
> Hrm.. you realise there's no theoretical reason a userspace program
> couldn't put a breakpoint at address 0...?
> 

I agree. I think there must be parts of code that works based on this
assumption. Will check and remove them.

> > +	return -EINVAL;
> > +}
> > +
> > +/*
> > + * Validate the arch-specific HW Breakpoint register settings
> > + */
> > +int arch_validate_hwbkpt_settings(struct hw_breakpoint *bp,
> > +						struct task_struct *tsk)
> > +{
> > +	int is_kernel, ret = -EINVAL;
> > +
> > +	if (!bp)
> > +		return ret;
> > +
> > +	switch (bp->info.type) {
> > +	case HW_BREAKPOINT_READ:
> > +	case HW_BREAKPOINT_WRITE:
> > +	case HW_BREAKPOINT_RW:
> > +		break;
> > +	default:
> > +		return ret;
> > +	}
> > +
> > +	if (bp->triggered)
> > +		ret = arch_store_info(bp, tsk);
> > +
> > +	is_kernel = is_kernel_addr(bp->info.address);
> > +	if ((tsk && is_kernel) || (!tsk && !is_kernel))
> > +		return -EINVAL;
> > +
> > +	return ret;
> > +}
> > +
> > +void arch_update_user_hw_breakpoint(int pos, struct task_struct *tsk)
> > +{
> > +	struct thread_struct *thread = &(tsk->thread);
> > +	struct hw_breakpoint *bp = thread->hbp[0];
> > +
> > +	if (bp)
> > +		thread->dabr = (bp->info.address & ~HW_BREAKPOINT_ALIGN) |
> > +				bp->info.type | DABR_TRANSLATION;
> > +	else
> > +		thread->dabr = 0;
> > +}
> > +
> > +void arch_flush_thread_hw_breakpoint(struct task_struct *tsk)
> > +{
> > +	struct thread_struct *thread = &(tsk->thread);
> > +
> > +	thread->dabr = 0;
> > +}
> > +
> > +/*
> > + * Handle debug exception notifications.
> > + */
> > +int __kprobes hw_breakpoint_handler(struct die_args *args)
> > +{
> > +	int rc = NOTIFY_STOP;
> > +	struct hw_breakpoint *bp;
> > +	struct pt_regs *regs = args->regs;
> > +	unsigned long dar = regs->dar;
> > +	int cpu, is_one_shot, stepped = 1;
> > +
> > +	/* Disable breakpoints during exception handling */
> > +	set_dabr(0);
> > +
> > +	cpu = get_cpu();
> > +	/* Determine whether kernel- or user-space address is the trigger */
> > +	bp = (hbp_kernel_pos == HBP_NUM) ? current->thread.hbp[0] :
> > +					per_cpu(this_hbp_kernel[0], cpu);
> > +	/*
> > +	 * bp can be NULL due to lazy debug register switching
> > +	 * or due to the delay between updates of hbp_kernel_pos
> > +	 * and this_hbp_kernel.
> > +	 */
> > +	if (!bp)
> > +		goto out;
> > +
> > +	if (dar == bp->info.address)
> > +		per_cpu(dabr_data, cpu) = (hbp_kernel_pos == HBP_NUM) ?
> > +						current->thread.dabr : kdabr;
> > +	else {
> > +		/*
> > +		 * This exception is triggered not because of a memory access on
> > +		 * the monitored variable but in the double-word address range
> > +		 * in which it is contained. We will consume this exception,
> > +		 * considering it as 'noise'.
> > +		 */
> > +		rc = NOTIFY_STOP;
> > +		goto out;
> > +	}
> > +	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
> 
> Ouch, explicitly special-casing ptrace_triggered is pretty nasty.
> Since the bp_info is already arch specific, maybe it should include a
> flag to indicate whether the breakpoint is one-shot or not.
> 

The reason to check for ptrace_triggered is to contain the one-shot
behaviour only to ptrace (thus retaining the semantics) and not to extend
them to all user-space requests through register_user_hw_breakpoint().

A one-shot behaviour for all user-space requests would create more work
for the user-space programs (such as re-registration) and will leave open
a small window of opportunity for debug register grabbing by kernel-space
requests.

So, in effect a request through register_user_hw_breakpoint() interface
will behave as under:
- Single-step over the causative instruction that triggered the
  breakpoint exception handler.
- Deliver the SIGTRAP signal to user-space after executing the causative
  instruction.

This behaviour is in consonance with that of kernel-space requests and
those on x86 processors, and helps define a consistent behaviour across
architectures for user-space.

Let me know what you think on the same.

> > +	(bp->triggered)(bp, regs);
> > +	/*
> > +	 * Ptrace expects the HW Breakpoints to be one-shot. We will return
> > +	 * NOTIFY_DONE without restoring DABR with the breakpoint address. The
> > +	 * downstream code will generate SIGTRAP to the process
> > +	 */
> > +	if (is_one_shot) {
> > +		rc = NOTIFY_DONE;
> > +		goto out;
> 
> Don't you need to clear dabr_data?  Otherwise if we enter single step
> for some other reason (e.g. gdb turns it on), won't we incorrectly hit
> the code-path to step over a dabr breakpoint?
> 

Yes, I missed it.

> > +	}
> > +
> > +	stepped = emulate_step(regs, regs->nip);
> > +	/*
> > +	 * Single-step the causative instruction manually if
> > +	 * emulate_step() could not execute it
> > +	 */
> > +	if (stepped == 0) {
> > +		regs->msr |= MSR_SE;
> > +		goto out;
> > +	}
> > +	set_dabr(per_cpu(dabr_data, cpu));
> > +	per_cpu(dabr_data, cpu) = 0;
> > +
> > +out:
> > +	/* Enable pre-emption only if single-stepping is finished */
> > +	if (stepped)
> > +		put_cpu_no_resched();
> > +	return rc;
> > +}
> > +
> > +/*
> > + * Handle single-step exceptions following a DABR hit.
> > + */
> > +int __kprobes single_step_dabr_instruction(struct die_args *args)
> > +{
> > +	struct pt_regs *regs = args->regs;
> > +	int cpu = get_cpu();
> > +	int ret = NOTIFY_DONE;
> > +	siginfo_t info;
> > +	unsigned long this_dabr_data = per_cpu(dabr_data, cpu);
> > +
> > +	/*
> > +	 * Check if we are single-stepping as a result of a
> > +	 * previous HW Breakpoint exception
> > +	 */
> > +	if (this_dabr_data == 0)
> > +		goto out;
> > +
> > +	regs->msr &= ~MSR_SE;
> > +	/* Deliver signal to user-space */
> > +	if (this_dabr_data < TASK_SIZE) {
> > +		info.si_signo = SIGTRAP;
> > +		info.si_errno = 0;
> > +		info.si_code = TRAP_HWBKPT;
> > +		info.si_addr = (void __user *)(per_cpu(dabr_data, cpu));
> > +		force_sig_info(SIGTRAP, &info, current);
> 
> Uh.. I recall mentioning in my previous review that in order to match
> previous behaviour we need to deliver the userspace signal *before*
> stepping over the breakpointed instruction, rather than after (which
> I guess is why breakpoints are one-shot in the old scheme).
> 

This code would implement the behaviour as stated in the comment for
user-space requests above.

> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@ozlabs.org
> > https://ozlabs.org/mailman/listinfo/linuxppc-dev
> > 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Thanks,
K.Prasad

Re: [Patch 3/6] Modify ptrace code to use Hardware Breakpoint interfaces

[K.Prasad]

On Fri, Jun 05, 2009 at 03:13:45PM +1000, David Gibson wrote:
> On Wed, Jun 03, 2009 at 10:05:24PM +0530, K.Prasad wrote:
> > Modify the ptrace code to use the hardware breakpoint interfaces for user-space.
> > 
> > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/ptrace.c |   47 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
> > ===================================================================
> > --- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c
> > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
> > @@ -37,6 +37,7 @@
> >  #include <asm/page.h>
> >  #include <asm/pgtable.h>
> >  #include <asm/system.h>
> > +#include <asm/hw_breakpoint.h>
> >  
> >  /*
> >   * does not yet catch signals sent when the child dies.
> > @@ -735,9 +736,26 @@ void user_disable_single_step(struct tas
> >  	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
> >  }
> >  
> > +void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
> > +{
> > +	/*
> > +	 * Unregister the breakpoint request here since ptrace has defined a
> > +	 * one-shot behaviour for breakpoint exceptions in PPC64.
> > +	 * The SIGTRAP signal is generated automatically for us in do_dabr().
> > +	 * We don't have to do anything here
> > +	 */
> > +	unregister_user_hw_breakpoint(current, bp);
> > +	kfree(bp);
> 
> Couldn't you also clear the saved dabr info here, to avoid having to
> special case this in the actual breakpoint handler.
> 

The saved dabr_data is created as a static variable and I didn't want to
modify its value across files.

> Also, I think you should be delivering the signal here - for gdb
> compatibility I think we'll need to match the old behaviour which has
> the TRAP delivered before executing the breakpointed instruction.
> 

We could do it either way. Return a NOTIFY_DONE from
hw_breakpoint_handler() and allow the do_dabr()
code to deliver the signal, or deliver a signal here and return a
NOTIFY_STOP in the exception handler. I chose the former as it doesn't
duplicate the code.

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Thanks,
K.Prasad

Re: [Patch 2/6] Introduce PPC64 specific Hardware Breakpoint interfaces

[David Gibson]

On Wed, Jun 10, 2009 at 12:13:49PM +0530, K.Prasad wrote:
> On Fri, Jun 05, 2009 at 03:11:58PM +1000, David Gibson wrote:
> > On Wed, Jun 03, 2009 at 10:05:11PM +0530, K.Prasad wrote:
> 
> Hi David,
> 	Sorry for the delay in response below. In the meanwhile, I
> discovered an issue in detecting stray exceptions that affected
> user-space handling of breakpoints. I've made some changes to correct
> that behaviour which will be included in version VI of the patchset.

[snip]
> > > +	/*
> > > +	 * User-space requests will always have the address field populated
> > > +	 * For kernel-addresses, either the address or symbol name can be
> > > +	 * specified.
> > > +	 */
> > > +	if (bp->info.name)
> > > +		bp->info.address = (unsigned long)
> > > +					kallsyms_lookup_name(bp->info.name);
> > 
> > Archs don't have to implement this name lookup stuff, but it looks
> > like most of them would - so it looks like there ought to be a helper
> > function in generic code that will do the check / name lookup stuff.
> 
> It doesn't turn out to be very generic. The IO breakpoints in x86, the
> address-range (only) breakpoints in S390 and perhaps 4xx powerpc
> processors were what made me think that this should remain in
> arch-specific code. In these cases, we might have to deal only with
> breakpoint addresses and not names.

Hrm, ok.  I was suggesting a helper function that those archs which
can use it call at need, not moving the address lookup to generic code
in the sense of being used everywhere.  Whatever, it's not that
important at this stage.

> > > +	if (bp->info.address)
> > > +		return 0;
> > 
> > Hrm.. you realise there's no theoretical reason a userspace program
> > couldn't put a breakpoint at address 0...?
> 
> I agree. I think there must be parts of code that works based on this
> assumption. Will check and remove them.

Ok, good.

[snip]
> > > +	else {
> > > +		/*
> > > +		 * This exception is triggered not because of a memory access on
> > > +		 * the monitored variable but in the double-word address range
> > > +		 * in which it is contained. We will consume this exception,
> > > +		 * considering it as 'noise'.
> > > +		 */
> > > +		rc = NOTIFY_STOP;
> > > +		goto out;
> > > +	}
> > > +	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
> > 
> > Ouch, explicitly special-casing ptrace_triggered is pretty nasty.
> > Since the bp_info is already arch specific, maybe it should include a
> > flag to indicate whether the breakpoint is one-shot or not.
> > 
> 
> The reason to check for ptrace_triggered is to contain the one-shot
> behaviour only to ptrace (thus retaining the semantics) and not to extend
> them to all user-space requests through
> register_user_hw_breakpoint().

Right, but couldn't you implement that withing ptrace_triggered
itself, without a special test here, by having it cancel the
breakpoint.

> A one-shot behaviour for all user-space requests would create more work
> for the user-space programs (such as re-registration) and will leave open
> a small window of opportunity for debug register grabbing by kernel-space
> requests.
> 
> So, in effect a request through register_user_hw_breakpoint() interface
> will behave as under:
> - Single-step over the causative instruction that triggered the
>   breakpoint exception handler.
> - Deliver the SIGTRAP signal to user-space after executing the causative
>   instruction.
> 
> This behaviour is in consonance with that of kernel-space requests and
> those on x86 processors, and helps define a consistent behaviour across
> architectures for user-space.
> 
> Let me know what you think on the same.

I certainly see the value in consistent semantics across archs.
However, I can also see uses for the powerpc trap-before-execute
behaviour.  That's why I'm suggesting it might be worth having an
arch-specific flag.

[snip]
> > > +int __kprobes single_step_dabr_instruction(struct die_args *args)
> > > +{
> > > +	struct pt_regs *regs = args->regs;
> > > +	int cpu = get_cpu();
> > > +	int ret = NOTIFY_DONE;
> > > +	siginfo_t info;
> > > +	unsigned long this_dabr_data = per_cpu(dabr_data, cpu);
> > > +
> > > +	/*
> > > +	 * Check if we are single-stepping as a result of a
> > > +	 * previous HW Breakpoint exception
> > > +	 */
> > > +	if (this_dabr_data == 0)
> > > +		goto out;
> > > +
> > > +	regs->msr &= ~MSR_SE;
> > > +	/* Deliver signal to user-space */
> > > +	if (this_dabr_data < TASK_SIZE) {
> > > +		info.si_signo = SIGTRAP;
> > > +		info.si_errno = 0;
> > > +		info.si_code = TRAP_HWBKPT;
> > > +		info.si_addr = (void __user *)(per_cpu(dabr_data, cpu));
> > > +		force_sig_info(SIGTRAP, &info, current);
> > 
> > Uh.. I recall mentioning in my previous review that in order to match
> > previous behaviour we need to deliver the userspace signal *before*
> > stepping over the breakpointed instruction, rather than after (which
> > I guess is why breakpoints are one-shot in the old scheme).
> 
> This code would implement the behaviour as stated in the comment for
> user-space requests above.

And you're relying on the old trap-sending code in do_dabr for ptrace
requests?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Patch 3/6] Modify ptrace code to use Hardware Breakpoint interfaces

[David Gibson]

On Wed, Jun 10, 2009 at 12:20:02PM +0530, K.Prasad wrote:
> On Fri, Jun 05, 2009 at 03:13:45PM +1000, David Gibson wrote:
> > On Wed, Jun 03, 2009 at 10:05:24PM +0530, K.Prasad wrote:
> > > Modify the ptrace code to use the hardware breakpoint interfaces for user-space.
> > > 
> > > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> > > ---
> > >  arch/powerpc/kernel/ptrace.c |   47 +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 47 insertions(+)
> > > 
> > > Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
> > > ===================================================================
> > > --- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c
> > > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
> > > @@ -37,6 +37,7 @@
> > >  #include <asm/page.h>
> > >  #include <asm/pgtable.h>
> > >  #include <asm/system.h>
> > > +#include <asm/hw_breakpoint.h>
> > >  
> > >  /*
> > >   * does not yet catch signals sent when the child dies.
> > > @@ -735,9 +736,26 @@ void user_disable_single_step(struct tas
> > >  	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
> > >  }
> > >  
> > > +void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
> > > +{
> > > +	/*
> > > +	 * Unregister the breakpoint request here since ptrace has defined a
> > > +	 * one-shot behaviour for breakpoint exceptions in PPC64.
> > > +	 * The SIGTRAP signal is generated automatically for us in do_dabr().
> > > +	 * We don't have to do anything here
> > > +	 */
> > > +	unregister_user_hw_breakpoint(current, bp);
> > > +	kfree(bp);
> > 
> > Couldn't you also clear the saved dabr info here, to avoid having to
> > special case this in the actual breakpoint handler.
> 
> The saved dabr_data is created as a static variable and I didn't want to
> modify its value across files.

Hrm.  I'm not sure which of these options is the uglier, to be honest.

> > Also, I think you should be delivering the signal here - for gdb
> > compatibility I think we'll need to match the old behaviour which has
> > the TRAP delivered before executing the breakpointed instruction.
> > 
> 
> We could do it either way. Return a NOTIFY_DONE from
> hw_breakpoint_handler() and allow the do_dabr()
> code to deliver the signal, or deliver a signal here and return a
> NOTIFY_STOP in the exception handler. I chose the former as it doesn't
> duplicate the code.

I see.  The thing is, since you're aiming to make *the* hardware
breakpoint interface here, I think you really should be rewriting
do_dabr entirely as part of this framework, not just adding your stuff
as one option in there alongside the old-style ways of doing it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Patch 2/6] Introduce PPC64 specific Hardware Breakpoint interfaces

[K.Prasad]

On Mon, Jun 15, 2009 at 04:40:45PM +1000, David Gibson wrote:
> On Wed, Jun 10, 2009 at 12:13:49PM +0530, K.Prasad wrote:
> > On Fri, Jun 05, 2009 at 03:11:58PM +1000, David Gibson wrote:
> > > On Wed, Jun 03, 2009 at 10:05:11PM +0530, K.Prasad wrote:
> > 
> > > > +	else {
> > > > +		/*
> > > > +		 * This exception is triggered not because of a memory access on
> > > > +		 * the monitored variable but in the double-word address range
> > > > +		 * in which it is contained. We will consume this exception,
> > > > +		 * considering it as 'noise'.
> > > > +		 */
> > > > +		rc = NOTIFY_STOP;
> > > > +		goto out;
> > > > +	}
> > > > +	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
> > > 
> > > Ouch, explicitly special-casing ptrace_triggered is pretty nasty.
> > > Since the bp_info is already arch specific, maybe it should include a
> > > flag to indicate whether the breakpoint is one-shot or not.
> > > 
> > 
> > The reason to check for ptrace_triggered is to contain the one-shot
> > behaviour only to ptrace (thus retaining the semantics) and not to extend
> > them to all user-space requests through
> > register_user_hw_breakpoint().
> 
> Right, but couldn't you implement that withing ptrace_triggered
> itself, without a special test here, by having it cancel the
> breakpoint.
> 

A special check (either using the callback routine as above, or using a
special flag) will be required in hw_breakpoint_handler() to enable
early return (without single-stepping). I'm not sure if I got your
suggestion right, and let me know if you think so.

> > A one-shot behaviour for all user-space requests would create more work
> > for the user-space programs (such as re-registration) and will leave open
> > a small window of opportunity for debug register grabbing by kernel-space
> > requests.
> > 
> > So, in effect a request through register_user_hw_breakpoint() interface
> > will behave as under:
> > - Single-step over the causative instruction that triggered the
> >   breakpoint exception handler.
> > - Deliver the SIGTRAP signal to user-space after executing the causative
> >   instruction.
> > 
> > This behaviour is in consonance with that of kernel-space requests and
> > those on x86 processors, and helps define a consistent behaviour across
> > architectures for user-space.
> > 
> > Let me know what you think on the same.
> 
> I certainly see the value in consistent semantics across archs.
> However, I can also see uses for the powerpc trap-before-execute
> behaviour.  That's why I'm suggesting it might be worth having an
> arch-specific flag.
> 
> [snip]

So, you suggest that the 'one-shot' behaviour should be driven by
user-request and not just confined to ptrace? (The default behaviour for
all breakpoints-minus-ptrace will remain 'continuous' though).

It can be implemented through an additional flag in 'struct
arch_hw_breakpoint'. I can send a new version 7 of the patchset with this
change (with the hope that the version 6 of the patchset looks fine in
its present form!). Meanwhile, we'd like to know what uses you see in
addition to the present one if the one-shot behaviour is made
user-defined. Are those uses beyond what can be achieved through the
present ptrace interface?

> > > > +int __kprobes single_step_dabr_instruction(struct die_args *args)
> > > > +{
> > > > +	struct pt_regs *regs = args->regs;
> > > > +	int cpu = get_cpu();
> > > > +	int ret = NOTIFY_DONE;
> > > > +	siginfo_t info;
> > > > +	unsigned long this_dabr_data = per_cpu(dabr_data, cpu);
> > > > +
> > > > +	/*
> > > > +	 * Check if we are single-stepping as a result of a
> > > > +	 * previous HW Breakpoint exception
> > > > +	 */
> > > > +	if (this_dabr_data == 0)
> > > > +		goto out;
> > > > +
> > > > +	regs->msr &= ~MSR_SE;
> > > > +	/* Deliver signal to user-space */
> > > > +	if (this_dabr_data < TASK_SIZE) {
> > > > +		info.si_signo = SIGTRAP;
> > > > +		info.si_errno = 0;
> > > > +		info.si_code = TRAP_HWBKPT;
> > > > +		info.si_addr = (void __user *)(per_cpu(dabr_data, cpu));
> > > > +		force_sig_info(SIGTRAP, &info, current);
> > > 
> > > Uh.. I recall mentioning in my previous review that in order to match
> > > previous behaviour we need to deliver the userspace signal *before*
> > > stepping over the breakpointed instruction, rather than after (which
> > > I guess is why breakpoints are one-shot in the old scheme).
> > 
> > This code would implement the behaviour as stated in the comment for
> > user-space requests above.
> 
> And you're relying on the old trap-sending code in do_dabr for ptrace
> requests?
> 

Yes.

> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

Thanks,
K.Prasad

Re: [Patch 2/6] Introduce PPC64 specific Hardware Breakpoint interfaces

[David Gibson]

On Mon, Jun 15, 2009 at 12:48:28PM +0530, K.Prasad wrote:
> On Mon, Jun 15, 2009 at 04:40:45PM +1000, David Gibson wrote:
> > On Wed, Jun 10, 2009 at 12:13:49PM +0530, K.Prasad wrote:
> > > On Fri, Jun 05, 2009 at 03:11:58PM +1000, David Gibson wrote:
> > > > On Wed, Jun 03, 2009 at 10:05:11PM +0530, K.Prasad wrote:
> > > 
> > > > > +	else {
> > > > > +		/*
> > > > > +		 * This exception is triggered not because of a memory access on
> > > > > +		 * the monitored variable but in the double-word address range
> > > > > +		 * in which it is contained. We will consume this exception,
> > > > > +		 * considering it as 'noise'.
> > > > > +		 */
> > > > > +		rc = NOTIFY_STOP;
> > > > > +		goto out;
> > > > > +	}
> > > > > +	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
> > > > 
> > > > Ouch, explicitly special-casing ptrace_triggered is pretty nasty.
> > > > Since the bp_info is already arch specific, maybe it should include a
> > > > flag to indicate whether the breakpoint is one-shot or not.
> > > > 
> > > 
> > > The reason to check for ptrace_triggered is to contain the one-shot
> > > behaviour only to ptrace (thus retaining the semantics) and not to extend
> > > them to all user-space requests through
> > > register_user_hw_breakpoint().
> > 
> > Right, but couldn't you implement that withing ptrace_triggered
> > itself, without a special test here, by having it cancel the
> > breakpoint.
> 
> A special check (either using the callback routine as above, or using a
> special flag) will be required in hw_breakpoint_handler() to enable
> early return (without single-stepping). I'm not sure if I got your
> suggestion right, and let me know if you think so.

Well.. you could also recheck after calling triggered whether the
breakpoint is still in existence.  So cancelling the breakpoint in
triggered would.

Or you could change the signature for the triggered function, so it
returns whether to leave the breakpoint active or not.  That would
have some advantages you could easily implement either one-shot,
continuous or N-shot, or keep firing until some specific event
behaviour from within the triggered function.


But all that's a bit moot, because of the fact that you try to make
the TRAP timing consistent (before or after execution of triggering
instruction) but you don't do so for a general triggered hook.

> > > A one-shot behaviour for all user-space requests would create more work
> > > for the user-space programs (such as re-registration) and will leave open
> > > a small window of opportunity for debug register grabbing by kernel-space
> > > requests.
> > > 
> > > So, in effect a request through register_user_hw_breakpoint() interface
> > > will behave as under:
> > > - Single-step over the causative instruction that triggered the
> > >   breakpoint exception handler.
> > > - Deliver the SIGTRAP signal to user-space after executing the causative
> > >   instruction.
> > > 
> > > This behaviour is in consonance with that of kernel-space requests and
> > > those on x86 processors, and helps define a consistent behaviour across
> > > architectures for user-space.
> > > 
> > > Let me know what you think on the same.
> > 
> > I certainly see the value in consistent semantics across archs.
> > However, I can also see uses for the powerpc trap-before-execute
> > behaviour.  That's why I'm suggesting it might be worth having an
> > arch-specific flag.
> > 
> > [snip]
> 
> So, you suggest that the 'one-shot' behaviour should be driven by
> user-request and not just confined to ptrace? (The default behaviour for
> all breakpoints-minus-ptrace will remain 'continuous' though).

Not one-shot behaviour as such, but whether the trigger fires before
or after execution of the triggering instruction.  The complication is
that combining fire-before semantics with continuous firing becomes
tricky.

> It can be implemented through an additional flag in 'struct
> arch_hw_breakpoint'. I can send a new version 7 of the patchset with this
> change (with the hope that the version 6 of the patchset looks fine in
> its present form!). Meanwhile, we'd like to know what uses you see in
> addition to the present one if the one-shot behaviour is made
> user-defined. Are those uses beyond what can be achieved through the
> present ptrace interface?
> 
> > > > > +int __kprobes single_step_dabr_instruction(struct die_args *args)
> > > > > +{
> > > > > +	struct pt_regs *regs = args->regs;
> > > > > +	int cpu = get_cpu();
> > > > > +	int ret = NOTIFY_DONE;
> > > > > +	siginfo_t info;
> > > > > +	unsigned long this_dabr_data = per_cpu(dabr_data, cpu);
> > > > > +
> > > > > +	/*
> > > > > +	 * Check if we are single-stepping as a result of a
> > > > > +	 * previous HW Breakpoint exception
> > > > > +	 */
> > > > > +	if (this_dabr_data == 0)
> > > > > +		goto out;
> > > > > +
> > > > > +	regs->msr &= ~MSR_SE;
> > > > > +	/* Deliver signal to user-space */
> > > > > +	if (this_dabr_data < TASK_SIZE) {
> > > > > +		info.si_signo = SIGTRAP;
> > > > > +		info.si_errno = 0;
> > > > > +		info.si_code = TRAP_HWBKPT;
> > > > > +		info.si_addr = (void __user *)(per_cpu(dabr_data, cpu));
> > > > > +		force_sig_info(SIGTRAP, &info, current);
> > > > 
> > > > Uh.. I recall mentioning in my previous review that in order to match
> > > > previous behaviour we need to deliver the userspace signal *before*
> > > > stepping over the breakpointed instruction, rather than after (which
> > > > I guess is why breakpoints are one-shot in the old scheme).
> > > 
> > > This code would implement the behaviour as stated in the comment for
> > > user-space requests above.
> > 
> > And you're relying on the old trap-sending code in do_dabr for ptrace
> > requests?
> 
> Yes.

Yeah, since you're taking over the whole breakpoint infrastructure, I
really think you should rewrite do_dabr completely as part of your code.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Patch 3/6] Modify ptrace code to use Hardware Breakpoint interfaces

[David Gibson]

On Mon, Jul 27, 2009 at 05:48:27AM +0530, K.Prasad wrote:
> Modify the ptrace code to use the hardware breakpoint interfaces for user-space.
> 
> Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/ptrace.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c
> +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
> @@ -37,6 +37,7 @@
>  #include <asm/page.h>
>  #include <asm/pgtable.h>
>  #include <asm/system.h>
> +#include <asm/hw_breakpoint.h>
>  
>  /*
>   * does not yet catch signals sent when the child dies.
> @@ -757,11 +758,24 @@ void user_disable_single_step(struct tas
>  
>  void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
>  {
> +	/*
> +	 * Unregister the breakpoint request here since ptrace has defined a
> +	 * one-shot behaviour for breakpoint exceptions in PPC64.
> +	 * The SIGTRAP signal is generated automatically for us in do_dabr().
> +	 * We don't have to do anything here
> +	 */
> +	unregister_user_hw_breakpoint(current, bp);
> +	kfree(bp);

This unregisters the breakpoint, but doesn't actually abort the
current breakpoint handling sequence it's invoked from.  So, if your
breakpoint handler was invoked at all, which as previously mentioned,
I don't think it is, wouldn't this result in *two* SIGTRAPs from a
ptrace breakpoint: one issued before the trapping instruction from
do_dabr() and another afterwards from your step-over code.

>  }
>  
>  int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
>  			       unsigned long data)
>  {
> +#ifdef CONFIG_PPC64
> +	struct thread_struct *thread = &(task->thread);
> +	struct hw_breakpoint *bp;
> +	int ret;
> +#endif
>  	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
>  	 *  For embedded processors we support one DAC and no IAC's at the
>  	 *  moment.
> @@ -791,6 +805,35 @@ int ptrace_set_debugreg(struct task_stru
>  	if (data && !(data & DABR_TRANSLATION))
>  		return -EIO;
>  
> +#ifdef CONFIG_PPC64
> +	bp = thread->hbp[0];
> +	if (data == 0) {
> +		if (bp) {
> +			unregister_user_hw_breakpoint(task, bp);
> +			kfree(bp);
> +		}
> +		return 0;
> +	}
> +
> +	if (bp) {
> +		bp->info.type = data & HW_BREAKPOINT_RW;
> +		task->thread.dabr = bp->info.address = data;
> +		return modify_user_hw_breakpoint(task, bp);
> +	}
> +	bp = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL);
> +	if (!bp)
> +		return -ENOMEM;
> +
> +	/* Store the type of breakpoint */
> +	bp->info.type = data & HW_BREAKPOINT_RW;
> +	bp->triggered = ptrace_triggered;
> +	task->thread.dabr = bp->info.address = data;
> +
> +	ret = register_user_hw_breakpoint(task, bp);
> +	if (ret)
> +		return ret;
> +#endif /* CONFIG_PPC64 */
> +
>  	/* Move contents to the DABR register */
>  	task->thread.dabr = data;
>  
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[Patch 3/6] Modify ptrace code to use Hardware Breakpoint interfaces

[K.Prasad]

Modify the ptrace code to use the hardware breakpoint interfaces for user-space.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
@@ -37,6 +37,7 @@
 #include <asm/page.h>
 #include <asm/pgtable.h>
 #include <asm/system.h>
+#include <asm/hw_breakpoint.h>
 
 /*
  * does not yet catch signals sent when the child dies.
@@ -757,11 +758,24 @@ void user_disable_single_step(struct tas
 
 void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
 {
+	/*
+	 * Unregister the breakpoint request here since ptrace has defined a
+	 * one-shot behaviour for breakpoint exceptions in PPC64.
+	 * The SIGTRAP signal is generated automatically for us in do_dabr().
+	 * We don't have to do anything here
+	 */
+	unregister_user_hw_breakpoint(current, bp);
+	kfree(bp);
 }
 
 int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
+#ifdef CONFIG_PPC64
+	struct thread_struct *thread = &(task->thread);
+	struct hw_breakpoint *bp;
+	int ret;
+#endif
 	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
 	 *  For embedded processors we support one DAC and no IAC's at the
 	 *  moment.
@@ -791,6 +805,35 @@ int ptrace_set_debugreg(struct task_stru
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+#ifdef CONFIG_PPC64
+	bp = thread->hbp[0];
+	if (data == 0) {
+		if (bp) {
+			unregister_user_hw_breakpoint(task, bp);
+			kfree(bp);
+		}
+		return 0;
+	}
+
+	if (bp) {
+		bp->info.type = data & HW_BREAKPOINT_RW;
+		task->thread.dabr = bp->info.address = data;
+		return modify_user_hw_breakpoint(task, bp);
+	}
+	bp = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL);
+	if (!bp)
+		return -ENOMEM;
+
+	/* Store the type of breakpoint */
+	bp->info.type = data & HW_BREAKPOINT_RW;
+	bp->triggered = ptrace_triggered;
+	task->thread.dabr = bp->info.address = data;
+
+	ret = register_user_hw_breakpoint(task, bp);
+	if (ret)
+		return ret;
+#endif /* CONFIG_PPC64 */
+
 	/* Move contents to the DABR register */
 	task->thread.dabr = data;
 

[Patch 3/6] Modify ptrace code to use Hardware Breakpoint interfaces

[K.Prasad]

Modify the ptrace code to use the hardware breakpoint interfaces for user-space.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
@@ -37,6 +37,7 @@
 #include <asm/page.h>
 #include <asm/pgtable.h>
 #include <asm/system.h>
+#include <asm/hw_breakpoint.h>
 
 /*
  * does not yet catch signals sent when the child dies.
@@ -737,11 +738,24 @@ void user_disable_single_step(struct tas
 
 void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
 {
+	/*
+	 * Unregister the breakpoint request here since ptrace has defined a
+	 * one-shot behaviour for breakpoint exceptions in PPC64.
+	 * The SIGTRAP signal is generated automatically for us in do_dabr().
+	 * We don't have to do anything here
+	 */
+	unregister_user_hw_breakpoint(current, bp);
+	kfree(bp);
 }
 
 int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
+#ifdef CONFIG_PPC64
+	struct thread_struct *thread = &(task->thread);
+	struct hw_breakpoint *bp;
+	int ret;
+#endif
 	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
 	 *  For embedded processors we support one DAC and no IAC's at the
 	 *  moment.
@@ -771,6 +785,35 @@ int ptrace_set_debugreg(struct task_stru
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+#ifdef CONFIG_PPC64
+	bp = thread->hbp[0];
+	if (data == 0) {
+		if (bp) {
+			unregister_user_hw_breakpoint(task, bp);
+			kfree(bp);
+		}
+		return 0;
+	}
+
+	if (bp) {
+		bp->info.type = data & HW_BREAKPOINT_RW;
+		task->thread.dabr = bp->info.address = data;
+		return modify_user_hw_breakpoint(task, bp);
+	}
+	bp = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL);
+	if (!bp)
+		return -ENOMEM;
+
+	/* Store the type of breakpoint */
+	bp->info.type = data & HW_BREAKPOINT_RW;
+	bp->triggered = ptrace_triggered;
+	task->thread.dabr = bp->info.address = data;
+
+	ret = register_user_hw_breakpoint(task, bp);
+	if (ret)
+		return ret;
+#endif /* CONFIG_PPC64 */
+
 	/* Move contents to the DABR register */
 	task->thread.dabr = data;
 

[Patch 3/6] Modify ptrace code to use Hardware Breakpoint interfaces

[K.Prasad]

Modify the ptrace code to use the hardware breakpoint interfaces for user-space.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c |   44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
@@ -37,6 +37,7 @@
 #include <asm/page.h>
 #include <asm/pgtable.h>
 #include <asm/system.h>
+#include <asm/hw_breakpoint.h>
 
 /*
  * does not yet catch signals sent when the child dies.
@@ -735,9 +736,22 @@ void user_disable_single_step(struct tas
 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
 }
 
+static void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs)
+{
+	/*
+	 * The SIGTRAP signal is generated automatically for us in do_dabr().
+	 * We don't have to do anything here
+	 */
+}
+
 int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
+#ifdef CONFIG_PPC64
+	struct thread_struct *thread = &(task->thread);
+	struct hw_breakpoint *bp;
+	int ret;
+#endif
 	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
 	 *  For embedded processors we support one DAC and no IAC's at the
 	 *  moment.
@@ -767,6 +781,36 @@ int ptrace_set_debugreg(struct task_stru
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+#ifdef CONFIG_PPC64
+	bp = thread->hbp[0];
+	if (data == 0) {
+		if (bp) {
+			unregister_user_hw_breakpoint(task, bp);
+			kfree(bp);
+			thread->hbp[0] = NULL;
+		}
+		return 0;
+	}
+
+	if (bp) {
+		bp->info.type = data & HW_BREAKPOINT_RW;
+		task->thread.dabr = bp->info.address = data;
+		return modify_user_hw_breakpoint(task, bp);
+	}
+	bp = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL);
+	if (!bp)
+		return -ENOMEM;
+
+	/* Store the type of breakpoint */
+	bp->info.type = data & HW_BREAKPOINT_RW;
+	bp->triggered = ptrace_triggered;
+	task->thread.dabr = bp->info.address = data;
+
+	ret = register_user_hw_breakpoint(task, bp);
+	if (ret)
+		return ret;
+#endif /* CONFIG_PPC64 */
+
 	/* Move contents to the DABR register */
 	task->thread.dabr = data;