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

[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
@@ -137,6 +137,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 */
@@ -151,14 +157,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 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 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 |   59 +++++++++++++++++++++++++++++++
 arch/powerpc/include/asm/processor.h     |    1 
 arch/powerpc/include/asm/reg.h           |    3 +
 arch/powerpc/include/asm/thread_info.h   |    2 +
 4 files changed, 65 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,59 @@
+#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;
+	unsigned long	symbolsize;
+};
+
+#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)
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 | \

[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 |  298 ++++++++++++++++++++++++++++++++++++
 arch/powerpc/kernel/ptrace.c        |    4 
 4 files changed, 304 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,298 @@
+/*
+ * 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);
+}
+
+/*
+ * 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)
+{
+	/* Symbol names from user-space are rejected */
+	if (tsk) {
+		if (bp->info.name)
+			return -EINVAL;
+		else
+			return 0;
+	}
+	/*
+	 * 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)
+		if (kallsyms_lookup_size_offset(bp->info.address,
+						&(bp->info.symbolsize), NULL))
+			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;
+
+	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
+	per_cpu(dabr_data, cpu) = (hbp_kernel_pos == HBP_NUM) ?
+						current->thread.dabr : kdabr;
+
+	/* Verify if dar lies within the address range occupied by the symbol
+	 * being watched. Since we cannot get the symbol size for
+	 * user-space requests we skip this check in that case
+	 */
+	if ((hbp_kernel_pos == 0) &&
+	    !((bp->info.address <= dar) &&
+	     (dar <= (bp->info.address + bp->info.symbolsize))))
+		/*
+		 * 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'.
+		 */
+		goto out;
+
+	(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));
+
+out:
+	/* Enable pre-emption only if single-stepping is finished */
+	if (stepped) {
+		per_cpu(dabr_data, cpu) = 0;
+		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;
+}
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
@@ -735,6 +735,10 @@ 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)
+{
+}
+
 int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long 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 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/kernel/process.c |   15 +++++++++++++++
 arch/powerpc/kernel/smp.c     |    2 ++
 2 files changed, 17 insertions(+)

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 5/6] Modify Data storage exception code to recognise DABR match first

[David Gibson]

On Wed, Jun 10, 2009 at 02:38:24PM +0530, K.Prasad wrote:
> 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;
> +	}

Again, given the amount of work you're doing on all the breakpoint
paths, I really think you should be rewriting do_dabr(), not just
hooking in around 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 4/6] Modify process and processor handling code to recognise hardware debug registers

[David Gibson]

On Wed, Jun 10, 2009 at 02:38:18PM +0530, K.Prasad wrote:
> 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/kernel/process.c |   15 +++++++++++++++
>  arch/powerpc/kernel/smp.c     |    2 ++
>  2 files changed, 17 insertions(+)
> 
> 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

Won't this disable the check for breakpoints set by xmon - but I don't
see anything in this patch series to convert xmon to use the new
breakpoint interface instead.

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

[David Gibson]

On Wed, Jun 10, 2009 at 02:38:06PM +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]
> +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);
> +}
> +
> +/*
> + * 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)
> +{
> +	/* Symbol names from user-space are rejected */
> +	if (tsk) {
> +		if (bp->info.name)
> +			return -EINVAL;
> +		else
> +			return 0;
> +	}
> +	/*
> +	 * 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)
> +		if (kallsyms_lookup_size_offset(bp->info.address,
> +						&(bp->info.symbolsize), NULL))
> +			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);

Under what circumstances would triggered be NULL?  It's not clear to
me that you wouldn't still need arch_store_info() in this case.

> +
> +	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;
> +
> +	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
> +	per_cpu(dabr_data, cpu) = (hbp_kernel_pos == HBP_NUM) ?
> +						current->thread.dabr : kdabr;
> +
> +	/* Verify if dar lies within the address range occupied by the symbol
> +	 * being watched. Since we cannot get the symbol size for
> +	 * user-space requests we skip this check in that case
> +	 */
> +	if ((hbp_kernel_pos == 0) &&
> +	    !((bp->info.address <= dar) &&
> +	     (dar <= (bp->info.address + bp->info.symbolsize))))
> +		/*
> +		 * 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'.
> +		 */
> +		goto out;
> +
> +	(bp->triggered)(bp, regs);

So this confuses me.  You go to great efforts to step over the
instruction to generate a SIGTRAP after the instruction, for
consistency with x86.  But that SIGTRAP is *never* used, since the
only way to set userspace breakpoints is through ptrace at the moment.
At the same time, the triggered function is called here before the
instruction is executed, so not consistent with x86 anyway.

It just seems strange to me that sending a SIGTRAP is a special case
anyway.  Why can't sending a SIGTRAP be just a particular triggered
function.

> +	/*
> +	 * 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));
> +
> +out:
> +	/* Enable pre-emption only if single-stepping is finished */
> +	if (stepped) {
> +		per_cpu(dabr_data, cpu) = 0;
> +		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;
> +}
> 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
> @@ -735,6 +735,10 @@ 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)
> +{
> +}
> +
>  int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
>  			       unsigned long 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

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

[K.Prasad]

On Wed, Jun 17, 2009 at 02:14:20PM +1000, David Gibson wrote:
> On Wed, Jun 10, 2009 at 02:38:18PM +0530, K.Prasad wrote:
> > 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/kernel/process.c |   15 +++++++++++++++
> >  arch/powerpc/kernel/smp.c     |    2 ++
> >  2 files changed, 17 insertions(+)
> > 
> > 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
> 
> Won't this disable the check for breakpoints set by xmon - but I don't
> see anything in this patch series to convert xmon to use the new
> breakpoint interface instead.
> 

As noted by me here:
http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-May/071832.html the
Xmon integration is pending. When I tried to study and integrate Xmon, I
found that the HW Breakpoint triggering was broken as of 2.6.29 kernel
(tested on a Power5 box).

This would mean that if Xmon's hardware breakpoint infrastructure is
used in tandem with the given breakpoint interfaces, they would conflict
with each other resulting in difficult-to-predict behaviour (the last to
grab the register will use it).

I think that tidying up do_dabr() is best done along with Xmon
integration.

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

[K.Prasad]

On Wed, Jun 17, 2009 at 02:32:24PM +1000, David Gibson wrote:
> On Wed, Jun 10, 2009 at 02:38:06PM +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]
> > +int arch_store_info(struct hw_breakpoint *bp, struct task_struct *tsk)
> > +{
> > +	/* Symbol names from user-space are rejected */
> > +	if (tsk) {
> > +		if (bp->info.name)
> > +			return -EINVAL;
> > +		else
> > +			return 0;
> > +	}
> > +	/*
> > +	 * 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)
> > +		if (kallsyms_lookup_size_offset(bp->info.address,
> > +						&(bp->info.symbolsize), NULL))
> > +			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);
> 
> Under what circumstances would triggered be NULL?  It's not clear to
> me that you wouldn't still need arch_store_info() in this case.
> 

Simple, triggered would be NULL when a user fails to specify it!
This function would return EINVAL if that's the case.

> > +
> > +	is_kernel = is_kernel_addr(bp->info.address);
> > +	if ((tsk && is_kernel) || (!tsk && !is_kernel))
> > +		return -EINVAL;
> > +
> > +	return ret;
> > +}
> > +

<snipped>

> > +/*
> > + * 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;
> > +
> > +	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
> > +	per_cpu(dabr_data, cpu) = (hbp_kernel_pos == HBP_NUM) ?
> > +						current->thread.dabr : kdabr;
> > +
> > +	/* Verify if dar lies within the address range occupied by the symbol
> > +	 * being watched. Since we cannot get the symbol size for
> > +	 * user-space requests we skip this check in that case
> > +	 */
> > +	if ((hbp_kernel_pos == 0) &&
> > +	    !((bp->info.address <= dar) &&
> > +	     (dar <= (bp->info.address + bp->info.symbolsize))))
> > +		/*
> > +		 * 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'.
> > +		 */
> > +		goto out;
> > +
> > +	(bp->triggered)(bp, regs);
> 
> So this confuses me.  You go to great efforts to step over the
> instruction to generate a SIGTRAP after the instruction, for
> consistency with x86.  But that SIGTRAP is *never* used, since the
> only way to set userspace breakpoints is through ptrace at the moment.
> At the same time, the triggered function is called here before the
> instruction is executed, so not consistent with x86 anyway.
> 
> It just seems strange to me that sending a SIGTRAP is a special case
> anyway.  Why can't sending a SIGTRAP be just a particular triggered
> function.
>

The consistency in the interface across architectures that I referred to
in my previous mail was w.r.t. continuous triggering of breakpoints and
not to implement a trigger-before or trigger-after behaviour across
architectures. In fact, this behaviour differs even on the same
processor depending upon the breakpoint type (trigger-before for
instructions and trigger-after for data in x86), and introducing
uniformity might be a) at the cost of loss of some unique & innovative
uses for each of them b) may not be always possible e.g. trigger-after
to trigger-before.

Hope this resolves the confusion (that I might have inadvertently caused
in my previous mail)!

Thanks,
K.Prasad
 

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

[David Gibson]

On Thu, Jun 18, 2009 at 11:26:23PM +0530, K.Prasad wrote:
> On Wed, Jun 17, 2009 at 02:14:20PM +1000, David Gibson wrote:
> > On Wed, Jun 10, 2009 at 02:38:18PM +0530, K.Prasad wrote:
[snip]
> > > @@ -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
> > 
> > Won't this disable the check for breakpoints set by xmon - but I don't
> > see anything in this patch series to convert xmon to use the new
> > breakpoint interface instead.
> 
> As noted by me here:
> http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-May/071832.html the
> Xmon integration is pending. When I tried to study and integrate Xmon, I
> found that the HW Breakpoint triggering was broken as of 2.6.29 kernel
> (tested on a Power5 box).
> 
> This would mean that if Xmon's hardware breakpoint infrastructure is
> used in tandem with the given breakpoint interfaces, they would conflict
> with each other resulting in difficult-to-predict behaviour (the last to
> grab the register will use it).
> 
> I think that tidying up do_dabr() is best done along with Xmon
> integration.

Hmm, ok.

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

[David Gibson]

On Thu, Jun 18, 2009 at 11:50:45PM +0530, K.Prasad wrote:
> On Wed, Jun 17, 2009 at 02:32:24PM +1000, David Gibson wrote:
> > On Wed, Jun 10, 2009 at 02:38:06PM +0530, K.Prasad wrote:
[snip]
> > > +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);
> > 
> > Under what circumstances would triggered be NULL?  It's not clear to
> > me that you wouldn't still need arch_store_info() in this case.
> > 
> 
> Simple, triggered would be NULL when a user fails to specify it!
> This function would return EINVAL if that's the case.

Ah, right, yes.  This seems a really non obvious control flow for a
NULL triggered value to lead to EINVAL.  I'd prefer:

	if (!bp->triggered)
		return -EINVAL

	/* Then the kernel address test */

	return arch_store_info(bp, tsk);

[snip]
> > > +	/* Verify if dar lies within the address range occupied by the symbol
> > > +	 * being watched. Since we cannot get the symbol size for
> > > +	 * user-space requests we skip this check in that case
> > > +	 */
> > > +	if ((hbp_kernel_pos == 0) &&
> > > +	    !((bp->info.address <= dar) &&
> > > +	     (dar <= (bp->info.address + bp->info.symbolsize))))
> > > +		/*
> > > +		 * 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'.
> > > +		 */
> > > +		goto out;
> > > +
> > > +	(bp->triggered)(bp, regs);
> > 
> > So this confuses me.  You go to great efforts to step over the
> > instruction to generate a SIGTRAP after the instruction, for
> > consistency with x86.  But that SIGTRAP is *never* used, since the
> > only way to set userspace breakpoints is through ptrace at the moment.
> > At the same time, the triggered function is called here before the
> > instruction is executed, so not consistent with x86 anyway.
> > 
> > It just seems strange to me that sending a SIGTRAP is a special case
> > anyway.  Why can't sending a SIGTRAP be just a particular triggered
> > function.
> 
> The consistency in the interface across architectures that I referred to
> in my previous mail was w.r.t. continuous triggering of breakpoints and
> not to implement a trigger-before or trigger-after behaviour across
> architectures. In fact, this behaviour differs even on the same
> processor depending upon the breakpoint type (trigger-before for
> instructions and trigger-after for data in x86), and introducing
> uniformity might be a) at the cost of loss of some unique & innovative
> uses for each of them b) may not be always possible e.g. trigger-after
> to trigger-before.

Hrm.  Well (a) is why I was suggesting an option to allow trigger
before on powerpc.  Plus, I don't see why you think (a) is important
for the triggered function, but not for the timing of a SIGTRAP to
userspace.

As for (b), well there are already a bunch of things which can only be
done on certain archs/processors, so I don't see that's anything
really new.

How do you handle continuous breakpoints in the trigger-before case
(instruction breakpoints) on x86?  Do you need to step over an
instruction is the same manner as you do for powerpc?  In this case
where does x86 send the SIGTRAP?

-- 
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 19, 2009 at 03:04:09PM +1000, David Gibson wrote:
> On Thu, Jun 18, 2009 at 11:50:45PM +0530, K.Prasad wrote:
> > On Wed, Jun 17, 2009 at 02:32:24PM +1000, David Gibson wrote:
> > > On Wed, Jun 10, 2009 at 02:38:06PM +0530, K.Prasad wrote:
> [snip]

With apologies for the long delay here's my response. Some work that was
required to push the generic breakpoint patches into mainline (which is
now pushed to 2.6.32 merge window) and the preparation of a paper kept me
away from responding. Also to add that I will be on a week long vacation
starting next week followed by travel to Linux Symposium and I will not
be able to actively respond to mails till the week beginning 20th of
this month.

But kindly drop in your comments as I would like to have the PPC64
patches ready well before the next merge window opens!

> 
> Ah, right, yes.  This seems a really non obvious control flow for a
> NULL triggered value to lead to EINVAL.  I'd prefer:
> 
> 	if (!bp->triggered)
> 		return -EINVAL
> 
> 	/* Then the kernel address test */
> 
> 	return arch_store_info(bp, tsk);
> 

Ok. It is a minor change and I will do it.

> [snip]
> > > > +	/* Verify if dar lies within the address range occupied by the symbol
> > > > +	 * being watched. Since we cannot get the symbol size for
> > > > +	 * user-space requests we skip this check in that case
> > > > +	 */
> > > > +	if ((hbp_kernel_pos == 0) &&
> > > > +	    !((bp->info.address <= dar) &&
> > > > +	     (dar <= (bp->info.address + bp->info.symbolsize))))
> > > > +		/*
> > > > +		 * 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'.
> > > > +		 */
> > > > +		goto out;
> > > > +
> > > > +	(bp->triggered)(bp, regs);
> > > 
> > > So this confuses me.  You go to great efforts to step over the
> > > instruction to generate a SIGTRAP after the instruction, for
> > > consistency with x86.  But that SIGTRAP is *never* used, since the
> > > only way to set userspace breakpoints is through ptrace at the moment.
> > > At the same time, the triggered function is called here before the
> > > instruction is executed, so not consistent with x86 anyway.

If the SIGTRAP generation code is in ptrace_triggered() then it would
benefit only ptrace (although it is the lone user of HW-breakpoints at
the moment). Given that SIGTRAP is the only means for the user-space to
detect a breakpoint 'hit' on one of its monitored addresses, we will
generate it for all cases by keeping the code in hw_breakpoint.c (and
not in ptrace_triggered()).

However I believe that your concern here is about the timing of the
SIGTRAP signal generation and I agree with it...please find further
comments about it below.

> > > 
> > > It just seems strange to me that sending a SIGTRAP is a special case
> > > anyway.  Why can't sending a SIGTRAP be just a particular triggered
> > > function.
> > 
> > The consistency in the interface across architectures that I referred to
> > in my previous mail was w.r.t. continuous triggering of breakpoints and
> > not to implement a trigger-before or trigger-after behaviour across
> > architectures. In fact, this behaviour differs even on the same
> > processor depending upon the breakpoint type (trigger-before for
> > instructions and trigger-after for data in x86), and introducing
> > uniformity might be a) at the cost of loss of some unique & innovative
> > uses for each of them b) may not be always possible e.g. trigger-after
> > to trigger-before.
> 
> Hrm.  Well (a) is why I was suggesting an option to allow trigger
> before on powerpc.  Plus, I don't see why you think (a) is important
> for the triggered function, but not for the timing of a SIGTRAP to
> userspace.
> 

True that the timing of signal generation is inconsistent for user-space
when requested through ptrace vs requested through the interface. The
requirement for having a continuous breakpoint (as opposed to the
one-shot behaviour of ptrace) doesn't allow the signal to be delivered
in the time-window between the exception and the instruction execution.

I'm contemplating a few ways to work-arounds to ensure consistent signal
delivery timing. One such work-around would be like this:

hbp_handler(1)-->SIGTRAP-->signal_handler-->hbp_handler(2)-->
disable_hbp+enable_ss-->single_step_handler(3)-->enable_hbp+disable_ss

This method would take three exceptions for every breakpoint-hit (as
enumerated above) and will be expensive. Any alternative suggestions
with lower run-time overhead would greatly benefit the patch.

> As for (b), well there are already a bunch of things which can only be
> done on certain archs/processors, so I don't see that's anything
> really new.

Yes, the timing of SIGTRAP generation will remain dependant on the host
processor but the user should be allowed to receive breakpoints
continuously irrespective of the architecture. A one-shot behaviour only
on PPC64 (and only in user-space) doesn't seem nice.

> 
> How do you handle continuous breakpoints in the trigger-before case
> (instruction breakpoints) on x86?  Do you need to step over an
> instruction is the same manner as you do for powerpc?  In this case
> where does x86 send the SIGTRAP?
> 

Instructions breakpoints are trigger-before in x86 but the hardware
eases the task by providing RF flag (in EFLAGS register) that would
temporarily disable the instruction breakpoint while the causative
instruction is executed.

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

[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
@@ -137,6 +137,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 */
@@ -151,14 +157,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;