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

[RFC 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 |   49 +++++
 arch/powerpc/include/asm/processor.h     |    1 
 arch/powerpc/include/asm/reg.h           |    4 

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,49 @@
+#ifndef	_PPC64_HW_BREAKPOINT_H
+#define	_PPC64_HW_BREAKPOINT_H
+
+#ifdef	__KERNEL__
+#define	__ARCH_HW_BREAKPOINT_H
+
+struct arch_hw_breakpoint {
+	char		*name; /* Contains name of the symbol to set bkpt */
+	unsigned long	address;
+	u8		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_RW
+
+#define HW_BREAKPOINT_ALIGN 0x7
+#define HW_BREAKPOINT_LEN INSTRUCTION_LEN
+
+extern struct hw_breakpoint *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 switch_to_thread_hw_breakpoint(struct task_struct *tsk);
+
+static inline void hw_breakpoint_disable(void)
+{
+	set_dabr(0);
+}
+
+#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,9 +186,11 @@
 #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)
+#define   DABR_DATA_RW		(3UL << 0)
 #define SPRN_DABR2	0x13D	/* e300 */
 #define SPRN_DABRX	0x3F7	/* Data Address Breakpoint Register Extension */
 #define   DABRX_USER	(1UL << 0)

[RFC 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 |  281 ++++++++++++++++++++++++++++++++++++
 3 files changed, 283 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,281 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * Copyright (C) 2009 IBM Corporation
+ */
+
+/*
+ * HW_breakpoint: a unified kernel/user-space hardware breakpoint facility,
+ * using the CPU's debug registers.
+ */
+
+#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;
+	bp = hbp_kernel[hbp_kernel_pos];
+	if (bp == NULL)
+		kdabr = 0;
+	else
+		kdabr = bp->info.address | bp->info.type | DABR_TRANSLATION;
+	set_dabr(kdabr);
+}
+
+/*
+ * 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);
+}
+
+/*
+ * Check for virtual address in user space.
+ */
+static int arch_check_va_in_userspace(unsigned long va)
+{
+	return (!(is_kernel_addr(va)));
+}
+
+/*
+ * Check for virtual address in kernel space.
+ */
+static int arch_check_va_in_kernelspace(unsigned long va)
+{
+	return is_kernel_addr(va);
+}
+
+/*
+ * Store a breakpoint's encoded address, length, and type.
+ */
+int arch_store_info(struct hw_breakpoint *bp)
+{
+	/*
+	 * 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 ret = -EINVAL;
+
+	if (!bp)
+		return ret;
+
+	switch (bp->info.type) {
+	case DABR_DATA_READ:
+		break;
+	case DABR_DATA_WRITE:
+		break;
+	case DABR_DATA_RW:
+		break;
+	default:
+		return ret;
+	}
+
+	if (bp->triggered)
+		ret = arch_store_info(bp);
+
+	/* Check for double word alignment - 8 bytes */
+	if (bp->info.address & HW_BREAKPOINT_ALIGN)
+		return -EINVAL;
+
+	/* Check that the virtual address is in the proper range */
+	if (tsk) {
+		if (!arch_check_va_in_userspace(bp->info.address))
+			return -EFAULT;
+	} else {
+		if (!arch_check_va_in_kernelspace(bp->info.address))
+			return -EFAULT;
+	}
+
+	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	| 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;
+	int cpu, stepped, is_kernel;
+
+	/* Disable breakpoints during exception handling */
+	set_dabr(0);
+
+	dar = regs->dar & (~HW_BREAKPOINT_ALIGN);
+	is_kernel = (dar >= TASK_SIZE) ? 1 : 0;
+
+	if (is_kernel)
+		bp = hbp_kernel[0];
+	else {
+		bp = current->thread.hbp[0];
+		/* Lazy debug register switching */
+		if (!bp)
+			return rc;
+		rc = NOTIFY_DONE;
+	}
+
+	(bp->triggered)(bp, regs);
+
+	cpu = get_cpu();
+	if (is_kernel)
+		per_cpu(dabr_data, cpu) = kdabr;
+	else
+		per_cpu(dabr_data, cpu) = current->thread.dabr;
+
+	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)
+		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;
+	put_cpu_no_resched();
+
+out:
+	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;
+}

[RFC 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             |   48 +++++
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,9 @@
 #include <asm/page.h>
 #include <asm/pgtable.h>
 #include <asm/system.h>
+#ifdef CONFIG_PPC64
+#include <asm/hw_breakpoint.h>
+#endif
 
 /*
  * does not yet catch signals sent when the child dies.
@@ -735,9 +738,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 +783,38 @@ int ptrace_set_debugreg(struct task_stru
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+#ifdef CONFIG_PPC64
+	bp = thread->hbp[0];
+	if ((data & ~HW_BREAKPOINT_ALIGN) == 0) {
+		if (bp) {
+			unregister_user_hw_breakpoint(task, bp);
+			kfree(bp);
+			thread->hbp[0] = NULL;
+		}
+		return 0;
+	}
+
+	if (bp) {
+		bp->info.type = data & DABR_DATA_RW;
+		task->thread.dabr = bp->info.address =
+						(data & ~HW_BREAKPOINT_ALIGN);
+		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 & DABR_DATA_RW;
+	bp->triggered = ptrace_triggered;
+	task->thread.dabr = bp->info.address = (data & ~HW_BREAKPOINT_ALIGN);
+
+	ret = register_user_hw_breakpoint(task, bp);
+	if (ret)
+		return ret;
+	set_tsk_thread_flag(task, TIF_DEBUG);
+#endif /* CONFIG_PPC64 */
+
 	/* Move contents to the DABR register */
 	task->thread.dabr = data;

[RFC Patch 4/6] Modify process handling code to handle 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.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/thread_info.h   |    2 
 arch/powerpc/kernel/process.c            |   18 +

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)))
+			switch_to_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)
@@ -605,6 +617,9 @@ int copy_thread(unsigned long clone_flag
 	struct pt_regs *childregs, *kregs;
 	extern void ret_from_fork(void);
 	unsigned long sp = (unsigned long)task_stack_page(p) + THREAD_SIZE;
+#ifdef CONFIG_PPC64
+	struct task_struct *tsk = current;
+#endif
 
 	CHECK_FULL_REGS(regs);
 	/* Copy registers */
@@ -672,6 +687,9 @@ int copy_thread(unsigned long clone_flag
 	 * function.
  	 */
 	kregs->nip = *((unsigned long *)ret_from_fork);
+
+	if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG)))
+		copy_thread_hw_breakpoint(tsk, p, clone_flags);
 #else
 	kregs->nip = (unsigned long)ret_from_fork;
 #endif

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

Index: linux-2.6-tip.hbkpt/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/powerpc/mm/fault.c	2009-05-14 00:17:24.000000000 +0530
+++ linux-2.6-tip.hbkpt/arch/powerpc/mm/fault.c	2009-05-14 00:58:05.000000000 +0530
@@ -137,6 +137,12 @@
 		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 @@
 	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;

[RFC 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   |    6 
 samples/hw_breakpoint/data_breakpoint.c  |    4 

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	2009-05-14 00:17:24.000000000 +0530
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/machine_kexec_64.c	2009-05-14 09:48:09.000000000 +0530
@@ -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,9 @@
 	put_cpu();
 
 	local_irq_disable();
+#ifdef CONFIG_PPC64
+	hw_breakpoint_disable();
+#endif
 }
 
 #else /* ! SMP */
@@ -233,6 +237,9 @@
 	if (ppc_md.kexec_cpu_down)
 		ppc_md.kexec_cpu_down(0, 0);
 	local_irq_disable();
+#ifdef CONFIG_PPC64
+	hw_breakpoint_disable();
+#endif
 }
 
 #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	2009-05-14 00:17:24.000000000 +0530
+++ linux-2.6-tip.hbkpt/samples/hw_breakpoint/data_breakpoint.c	2009-05-14 00:58:06.000000000 +0530
@@ -54,6 +54,10 @@
 	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 = DABR_DATA_WRITE;
+#endif /* CONFIG_PPC64 */
 
 	sample_hbp.triggered = (void *)sample_hbp_handler;
 

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

[Geert Uytterhoeven]

On Thu, 14 May 2009, K.Prasad wrote:
> 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   |    6 
>  samples/hw_breakpoint/data_breakpoint.c  |    4 
> 
> 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	2009-05-14 00:17:24.000000000 +0530
> +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/machine_kexec_64.c	2009-05-14 09:48:09.000000000 +0530
> @@ -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,9 @@
>  	put_cpu();
>  
>  	local_irq_disable();
> +#ifdef CONFIG_PPC64
   ^^^^^^^^^^^^^^^^^^^
> +	hw_breakpoint_disable();
> +#endif
   ^^^^^^

What about providing a dummy definition of hw_breakpoint_disable()
in <asm/hw_breakpoint.h> if !CONFIG_PPC64?

and if you make it safe to always include <asm/hw_breakpoint.h>, you can
get rid of the #ifdef in e.g.

--- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c
+++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c
@@ -37,6 +37,9 @@
 #include <asm/page.h>
 #include <asm/pgtable.h>
 #include <asm/system.h>
+#ifdef CONFIG_PPC64
+#include <asm/hw_breakpoint.h>
+#endif

With kind regards,

Geert Uytterhoeven
Software Architect
Techsoft Centre

Technology and Software Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

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

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

On Thu, 2009-05-14 at 15:59 +0200, Geert Uytterhoeven wrote:
> On Thu, 14 May 2009, K.Prasad wrote:
> > 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   |    6 
> >  samples/hw_breakpoint/data_breakpoint.c  |    4 
> > 
> > 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	2009-05-14 00:17:24.000000000 +0530
> > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/machine_kexec_64.c	2009-05-14 09:48:09.000000000 +0530
> > @@ -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,9 @@
> >  	put_cpu();
> >  
> >  	local_irq_disable();
> > +#ifdef CONFIG_PPC64
>    ^^^^^^^^^^^^^^^^^^^
> > +	hw_breakpoint_disable();
> > +#endif
>    ^^^^^^
> 
> What about providing a dummy definition of hw_breakpoint_disable()
> in <asm/hw_breakpoint.h> if !CONFIG_PPC64?

That would be good.

What would be better, is to notice that machine_kexec_64.c is only ever
built for 64-bit - hence the name. And so no ifdefs or anything else is
required.

cheers


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Geert Uytterhoeven]

On Fri, 15 May 2009, Michael Ellerman wrote:
> On Thu, 2009-05-14 at 15:59 +0200, Geert Uytterhoeven wrote:
> > On Thu, 14 May 2009, K.Prasad wrote:
> > > 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   |    6 
> > >  samples/hw_breakpoint/data_breakpoint.c  |    4 
> > > 
> > > 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	2009-05-14 00:17:24.000000000 +0530
> > > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/machine_kexec_64.c	2009-05-14 09:48:09.000000000 +0530
> > > @@ -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,9 @@
> > >  	put_cpu();
> > >  
> > >  	local_irq_disable();
> > > +#ifdef CONFIG_PPC64
> >    ^^^^^^^^^^^^^^^^^^^
> > > +	hw_breakpoint_disable();
> > > +#endif
> >    ^^^^^^
> > 
> > What about providing a dummy definition of hw_breakpoint_disable()
> > in <asm/hw_breakpoint.h> if !CONFIG_PPC64?
> 
> That would be good.
> 
> What would be better, is to notice that machine_kexec_64.c is only ever
> built for 64-bit - hence the name. And so no ifdefs or anything else is
> required.

Right ;-)

But that can't be said for other files, like e.g. arch/powerpc/kernel/ptrace.c.

With kind regards,

Geert Uytterhoeven
Software Architect
Techsoft Centre

Technology and Software Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

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

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 7522 bytes --]

On Thu, 2009-05-14 at 19:14 +0530, K.Prasad wrote:
> plain text document attachment (ppc64_arch_hwbkpt_implementation_02)
> Introduce PPC64 implementation for the generic hardware breakpoint interfaces
> defined in kernel/hw_breakpoint.c. Enable the HAVE_HW_BREAKPOINT flag and the
> Makefile.

Hi, some comments inline ...


> 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,281 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + * Copyright (C) 2009 IBM Corporation
> + */

Don't use (C), either use a proper ©, or just skip it. I don't know
why :)

> +/*
> + * HW_breakpoint: a unified kernel/user-space hardware breakpoint facility,
> + * using the CPU's debug registers.
> + */

This comment would normally go at the top of the file.

> +
> +#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);

How does this relate to the existing current_dabr per-cpu variable?

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

Should that be hbp_kernel_pos >= HBP_NUM, you're checking array bounds
right?

> +	bp = hbp_kernel[hbp_kernel_pos];
> +	if (bp == NULL)
> +		kdabr = 0;
> +	else
> +		kdabr = bp->info.address | bp->info.type | DABR_TRANSLATION;
> +	set_dabr(kdabr);
> +}
> +
> +/*
> + * Install the thread breakpoints in their debug registers.
> + */
> +void arch_install_thread_hw_breakpoint(struct task_struct *tsk)
> +{
> +	set_dabr(tsk->thread.dabr);

Can we avoid setting this value if it's not necessary? It might require
an hcall. See for example what we do in __switch_to().

> +/*
> + * Check for virtual address in user space.
> + */
> +static int arch_check_va_in_userspace(unsigned long va)
> +{
> +	return (!(is_kernel_addr(va)));
> +}
> +
> +/*
> + * Check for virtual address in kernel space.
> + */
> +static int arch_check_va_in_kernelspace(unsigned long va)
> +{
> +	return is_kernel_addr(va);
> +}

You don't need these two routines. Just use is_kernel_addr() directly,
that way people will know what the code is doing without having to
lookup these new functions.

> +/*
> + * Store a breakpoint's encoded address, length, and type.
> + */

This doesn't "store" in the sense I was thinking, it actually does a
lookup and returns info in the arg.

> +int arch_store_info(struct hw_breakpoint *bp)
> +{
> +	/*
> +	 * User-space requests will always have the address field populated
> +	 * For kernel-addresses, either the address or symbol name can be
> +	 * specified.
> +	 */

Do user-space requests never have the name populated? Otherwise aren't
you overwriting the supplied address with the one from kallsyms?

> +	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 ret = -EINVAL;
> +
> +	if (!bp)
> +		return ret;
> +
> +	switch (bp->info.type) {
> +	case DABR_DATA_READ:
> +		break;
> +	case DABR_DATA_WRITE:
> +		break;
> +	case DABR_DATA_RW:
> +		break;

You only need the final break here.

> +	default:
> +		return ret;
> +	}
> +
> +	if (bp->triggered)
> +		ret = arch_store_info(bp);

Shouldn't you check ret here, bp->info.address might be bogus.
> +
> +	/* Check for double word alignment - 8 bytes */
> +	if (bp->info.address & HW_BREAKPOINT_ALIGN)
> +		return -EINVAL;
> +
> +	/* Check that the virtual address is in the proper range */
> +	if (tsk) {
> +		if (!arch_check_va_in_userspace(bp->info.address))
> +			return -EFAULT;
> +	} else {
> +		if (!arch_check_va_in_kernelspace(bp->info.address))
> +			return -EFAULT;
> +	}

Which becomes:

is_kernel = is_kernel_addr(bp->info.address);
if (tsk && is_kernel || !tsk && !is_kernel)
	return -EFAULT;

> +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	| bp->info.type |
> +				DABR_TRANSLATION;

2nd place I've seen that pattern.

> +	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;
> +	int cpu, stepped, is_kernel;
> +
> +	/* Disable breakpoints during exception handling */
> +	set_dabr(0);
> +
> +	dar = regs->dar & (~HW_BREAKPOINT_ALIGN);
> +	is_kernel = (dar >= TASK_SIZE) ? 1 : 0;

is_kernel_addr() ?

> +	if (is_kernel)
> +		bp = hbp_kernel[0];
> +	else {
> +		bp = current->thread.hbp[0];
> +		/* Lazy debug register switching */
> +		if (!bp)
> +			return rc;

What if we keep hitting this case?

> +		rc = NOTIFY_DONE;
> +	}
> +
> +	(bp->triggered)(bp, regs);
> +
> +	cpu = get_cpu();
> +	if (is_kernel)
> +		per_cpu(dabr_data, cpu) = kdabr;
> +	else
> +		per_cpu(dabr_data, cpu) = current->thread.dabr;
> +
> +	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)
> +		put_cpu_no_resched();
> +	return rc;
> +}

Gotta run, laptop battery running out! :)

cheers


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC Patch 4/6] Modify process handling code to handle hardware debug registers

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2069 bytes --]

On Thu, 2009-05-14 at 19:15 +0530, K.Prasad wrote:
> plain text document attachment (ppc64_modify_process_handling_04)

> 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)))
> +			switch_to_thread_hw_breakpoint(new);
> +#else

To avoid all these ifdefs in the code we need something like this in a
header:

static inline int task_uses_debug_regs(struct task_struct *tsk)
{
#ifdef CONFIG_PPC64
	return test_tsk_thread_flag(tsk, TIF_DEBUG);
#else
	return 0;
#endif
}

> @@ -605,6 +617,9 @@ int copy_thread(unsigned long clone_flag
>  	struct pt_regs *childregs, *kregs;
>  	extern void ret_from_fork(void);
>  	unsigned long sp = (unsigned long)task_stack_page(p) + THREAD_SIZE;
> +#ifdef CONFIG_PPC64
> +	struct task_struct *tsk = current;
> +#endif
>  
>  	CHECK_FULL_REGS(regs);
>  	/* Copy registers */
> @@ -672,6 +687,9 @@ int copy_thread(unsigned long clone_flag
>  	 * function.
>   	 */
>  	kregs->nip = *((unsigned long *)ret_from_fork);
> +
> +	if (unlikely(test_tsk_thread_flag(tsk, TIF_DEBUG)))
> +		copy_thread_hw_breakpoint(tsk, p, clone_flags);

If you just use current here you don't need to define tsk above.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

On Thu, 2009-05-14 at 16:18 +0200, Geert Uytterhoeven wrote:
> On Fri, 15 May 2009, Michael Ellerman wrote:
> > On Thu, 2009-05-14 at 15:59 +0200, Geert Uytterhoeven wrote:
> > > On Thu, 14 May 2009, K.Prasad wrote:
> > > > 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   |    6 
> > > >  samples/hw_breakpoint/data_breakpoint.c  |    4 
> > > > 
> > > > 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	2009-05-14 00:17:24.000000000 +0530
> > > > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/machine_kexec_64.c	2009-05-14 09:48:09.000000000 +0530
> > > > @@ -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,9 @@
> > > >  	put_cpu();
> > > >  
> > > >  	local_irq_disable();
> > > > +#ifdef CONFIG_PPC64
> > >    ^^^^^^^^^^^^^^^^^^^
> > > > +	hw_breakpoint_disable();
> > > > +#endif
> > >    ^^^^^^
> > > 
> > > What about providing a dummy definition of hw_breakpoint_disable()
> > > in <asm/hw_breakpoint.h> if !CONFIG_PPC64?
> > 
> > That would be good.
> > 
> > What would be better, is to notice that machine_kexec_64.c is only ever
> > built for 64-bit - hence the name. And so no ifdefs or anything else is
> > required.
> 
> Right ;-)
> 
> But that can't be said for other files, like e.g. arch/powerpc/kernel/ptrace.c.

Yeah you're right, the rest of this series could use lots of #ifdefs in
headers love.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC Patch 6/6] Adapt kexec and samples code to recognise PPC64hardware breakpoint usage

[K.Prasad]

On Thu, May 14, 2009 at 03:59:45PM +0200, Geert Uytterhoeven wrote:
> On Thu, 14 May 2009, K.Prasad wrote:
> > 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   |    6 
> >  samples/hw_breakpoint/data_breakpoint.c  |    4 
> > 
> > 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	2009-05-14 00:17:24.000000000 +0530
> > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/machine_kexec_64.c	2009-05-14 09:48:09.000000000 +0530
> > @@ -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,9 @@
> >  	put_cpu();
> >  
> >  	local_irq_disable();
> > +#ifdef CONFIG_PPC64
>    ^^^^^^^^^^^^^^^^^^^
> > +	hw_breakpoint_disable();
> > +#endif
>    ^^^^^^
> 
> What about providing a dummy definition of hw_breakpoint_disable()
> in <asm/hw_breakpoint.h> if !CONFIG_PPC64?
>

Yes, that's a good idea. I will change it.

Thanks,
K.Prasad
 

Re: [RFC Patch 2/6] Introduce PPC64 specific Hardware Breakpointinterfaces

[K.Prasad]

On Fri, May 15, 2009 at 12:50:11AM +1000, Michael Ellerman wrote:
> On Thu, 2009-05-14 at 19:14 +0530, K.Prasad wrote:
> > plain text document attachment (ppc64_arch_hwbkpt_implementation_02)
> > Introduce PPC64 implementation for the generic hardware breakpoint interfaces
> > defined in kernel/hw_breakpoint.c. Enable the HAVE_HW_BREAKPOINT flag and the
> > Makefile.
> 
> Hi, some comments inline ...
> 
>

Thanks for reviewing the code.
 
> > 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,281 @@
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> > + *
> > + * Copyright (C) 2009 IBM Corporation
> > + */
> 
> Don't use (C), either use a proper ©, or just skip it. I don't know
> why :)
> 

Ok.

> > +/*
> > + * HW_breakpoint: a unified kernel/user-space hardware breakpoint facility,
> > + * using the CPU's debug registers.
> > + */
> 
> This comment would normally go at the top of the file.
> 

Ok.

> > +
> > +#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);
> 
> How does this relate to the existing current_dabr per-cpu variable?
> 

The present infrastructure assumes that kernel-space (through xmon and
KGDB) and user-space requests happen at mutually exclusive times.

current_dabr in arch/powerpc/kernel/process.c stores the value of DABR
as requested by the current process and helps initiate a set_dabr() call
if the new incoming process's DABR value is different (when neither of
them use DABR, the values are 0, hence no set_dabr() call).

The per-cpu 'dabr_data' seen above is used to store the value of DABR
temporarily between a HW Breakpoint exception handler
hw_breakpoint_handler() (where the breakpoint exception might be
temporarily disabled if emulate_step() failed) and a single-step
exception handler single_step_dabr_instruction() in which the DABR value
is restored from per-cpu 'dabr_data'.

This is required because PowerPC triggers DABR match exception before
execution of the causative instruction (such as load/store) and if the
DABR value remains enabled, the system will enter into an infinite loop.

> > +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;
> 
> Should that be hbp_kernel_pos >= HBP_NUM, you're checking array bounds
> right?
> 

In short, no. If hbp_kernel_pos > HBP_NUM it is only indicative of erroneous
code and not a condition that the system could enter.

To explain 'hbp_kernel_pos' further, it is that variable which points to
the last numbered debug register occupied by the kernel (which is more
meaningful on a processor having more than one debug register). 

In the above code, in arch_update_kernel_hw_breakpoint()
"hbp_kernel_pos == HBP_NUM" condition would be satisfied when
invoked from load_debug_registers() at initialisation time (kernel
requests are serviced starting from highest numbered register).

Having said that, I find that I've missed invoking
load_debug_registers() [a part of kernel/hw_breakpoint.c] in
start_secondary(). Thanks for raising this question which helped me
identify the missing invocation, I will add the same.

> > +	bp = hbp_kernel[hbp_kernel_pos];
> > +	if (bp == NULL)
> > +		kdabr = 0;
> > +	else
> > +		kdabr = bp->info.address | bp->info.type | DABR_TRANSLATION;
> > +	set_dabr(kdabr);
> > +}
> > +
> > +/*
> > + * Install the thread breakpoints in their debug registers.
> > + */
> > +void arch_install_thread_hw_breakpoint(struct task_struct *tsk)
> > +{
> > +	set_dabr(tsk->thread.dabr);
> 
> Can we avoid setting this value if it's not necessary? It might require
> an hcall. See for example what we do in __switch_to().
> 

I see that you're referring to this code in __switch_to() :
        if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
                set_dabr(new->thread.dabr);

arch_install_thread_hw_breakpoint()<--switch_to_thread_hw_breakpoint()
<--__switch_to() implementation is also similar.

In __switch_to(),
                if (unlikely(test_tsk_thread_flag(new, TIF_DEBUG)))
                        switch_to_thread_hw_breakpoint(new);

happens only when TIF_DEBUG flag is set. This flag is cleared when the
process unregisters any breakpoints it had requested earlier. So, the
set_dabr() call is avoided for processes not using the debug register.

> > +/*
> > + * Check for virtual address in user space.
> > + */
> > +static int arch_check_va_in_userspace(unsigned long va)
> > +{
> > +	return (!(is_kernel_addr(va)));
> > +}
> > +
> > +/*
> > + * Check for virtual address in kernel space.
> > + */
> > +static int arch_check_va_in_kernelspace(unsigned long va)
> > +{
> > +	return is_kernel_addr(va);
> > +}
> 
> You don't need these two routines. Just use is_kernel_addr() directly,
> that way people will know what the code is doing without having to
> lookup these new functions.
> 

These functions are here due to 'legacy' reasons (from x86 code) , but I
see that it can be made a lot simpler by using is_kernel() for PPC64. I
will change the code on the lines of what you've suggested below.

But on processors supporting variable range of addresses, the above
functions may be required. Well, it can be added when required!

> > +/*
> > + * Store a breakpoint's encoded address, length, and type.
> > + */
> 
> This doesn't "store" in the sense I was thinking, it actually does a
> lookup and returns info in the arg.
> 
> > +int arch_store_info(struct hw_breakpoint *bp)
> > +{
> > +	/*
> > +	 * User-space requests will always have the address field populated
> > +	 * For kernel-addresses, either the address or symbol name can be
> > +	 * specified.
> > +	 */
> 
> Do user-space requests never have the name populated? Otherwise aren't
> you overwriting the supplied address with the one from kallsyms?
> 

I see that I should also check for an empty bp->info.name when "if (tsk)"
is TRUE. I will add the same.

> > +	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 ret = -EINVAL;
> > +
> > +	if (!bp)
> > +		return ret;
> > +
> > +	switch (bp->info.type) {
> > +	case DABR_DATA_READ:
> > +		break;
> > +	case DABR_DATA_WRITE:
> > +		break;
> > +	case DABR_DATA_RW:
> > +		break;
> 
> You only need the final break here.
> 

Yes, will change.

> > +	default:
> > +		return ret;
> > +	}
> > +
> > +	if (bp->triggered)
> > +		ret = arch_store_info(bp);
> 
> Shouldn't you check ret here, bp->info.address might be bogus.

Only when bp->info.name is set for user-space. The additional check I
described above should help.

> > +
> > +	/* Check for double word alignment - 8 bytes */
> > +	if (bp->info.address & HW_BREAKPOINT_ALIGN)
> > +		return -EINVAL;
> > +
> > +	/* Check that the virtual address is in the proper range */
> > +	if (tsk) {
> > +		if (!arch_check_va_in_userspace(bp->info.address))
> > +			return -EFAULT;
> > +	} else {
> > +		if (!arch_check_va_in_kernelspace(bp->info.address))
> > +			return -EFAULT;
> > +	}
> 
> Which becomes:
> 
> is_kernel = is_kernel_addr(bp->info.address);
> if (tsk && is_kernel || !tsk && !is_kernel)
> 	return -EFAULT;
> 

Ok. I will change them.

> > +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	| bp->info.type |
> > +				DABR_TRANSLATION;
> 
> 2nd place I've seen that pattern.
> 

I don't recognise what issue you're referring to. Can you elaborate?

> > +	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;
> > +	int cpu, stepped, is_kernel;
> > +
> > +	/* Disable breakpoints during exception handling */
> > +	set_dabr(0);
> > +
> > +	dar = regs->dar & (~HW_BREAKPOINT_ALIGN);
> > +	is_kernel = (dar >= TASK_SIZE) ? 1 : 0;
> 
> is_kernel_addr() ?
> 

Ok.

> > +	if (is_kernel)
> > +		bp = hbp_kernel[0];
> > +	else {
> > +		bp = current->thread.hbp[0];
> > +		/* Lazy debug register switching */
> > +		if (!bp)
> > +			return rc;
> 
> What if we keep hitting this case?
> 

In short - no, they will not recur because we are returning after a
set_dabr(0).

The reason a breakpoint exception can trigger because of lazy debug
register switching is because the DABR value is not cleared when the
incoming process does not have TIF_DEBUG set i.e. does not use
breakpoint register. If the outgoing process used DABR while the
incoming process did not, and happened to read/write on the same address
such an exception can arise. It is only then that the DABR value is
cleared.

> > +		rc = NOTIFY_DONE;
> > +	}
> > +
> > +	(bp->triggered)(bp, regs);
> > +
> > +	cpu = get_cpu();
> > +	if (is_kernel)
> > +		per_cpu(dabr_data, cpu) = kdabr;
> > +	else
> > +		per_cpu(dabr_data, cpu) = current->thread.dabr;
> > +
> > +	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)
> > +		put_cpu_no_resched();
> > +	return rc;
> > +}
> 
> Gotta run, laptop battery running out! :)
> 
> cheers
> 

Thanks again for your reviews. I will post another patchset with the
changes I've agreed above.

Thanks,
K.Prasad

Re: [RFC Patch 2/6] Introduce PPC64 specific Hardware Breakpointinterfaces

[Alan Stern]

On Fri, 15 May 2009, K.Prasad wrote:

> I see that you're referring to this code in __switch_to() :
>         if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
>                 set_dabr(new->thread.dabr);
> 
> arch_install_thread_hw_breakpoint()<--switch_to_thread_hw_breakpoint()
> <--__switch_to() implementation is also similar.
> 
> In __switch_to(),
>                 if (unlikely(test_tsk_thread_flag(new, TIF_DEBUG)))
>                         switch_to_thread_hw_breakpoint(new);
> 
> happens only when TIF_DEBUG flag is set. This flag is cleared when the
> process unregisters any breakpoints it had requested earlier. So, the
> set_dabr() call is avoided for processes not using the debug register.

In the x86 code, shouldn't arch_update_user_hw_breakpoint set or clear
TIF_DEBUG, depending on whether or not there are any user breakpoints
remaining?

> > > +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;
> > > +	int cpu, stepped, is_kernel;
> > > +
> > > +	/* Disable breakpoints during exception handling */
> > > +	set_dabr(0);
> > > +
> > > +	dar = regs->dar & (~HW_BREAKPOINT_ALIGN);
> > > +	is_kernel = (dar >= TASK_SIZE) ? 1 : 0;
> > 
> > is_kernel_addr() ?
> > 
> 
> Ok.

Shouldn't this test hbp_kernel_pos instead?

> > > +	if (is_kernel)
> > > +		bp = hbp_kernel[0];
> > > +	else {
> > > +		bp = current->thread.hbp[0];
> > > +		/* Lazy debug register switching */
> > > +		if (!bp)
> > > +			return rc;

Shouldn't this test be moved outside the "if" statement, as in the x86 
code?

Alan Stern

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

[Alan Stern]

On Thu, 14 May 2009, K.Prasad wrote:

> Index: linux-2.6-tip.hbkpt/samples/hw_breakpoint/data_breakpoint.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/samples/hw_breakpoint/data_breakpoint.c	2009-05-14 00:17:24.000000000 +0530
> +++ linux-2.6-tip.hbkpt/samples/hw_breakpoint/data_breakpoint.c	2009-05-14 00:58:06.000000000 +0530
> @@ -54,6 +54,10 @@
>  	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 = DABR_DATA_WRITE;

This should be HW_BREAKPOINT_WRITE, not DABR_DATA_WRITE.

Alan Stern

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

[Benjamin Herrenschmidt]

On Thu, 2009-05-14 at 19:13 +0530, K.Prasad wrote:
> plain text document attachment (ppc64_prepare_code_01)
> Prepare the PowerPC code for HW Breakpoint infrastructure patches by including
> relevant constant definitions and function declarations.

Hi !

Some comments below...

> +#define   HBP_NUM	1	/* Number of physical HW breakpoint registers */

Can you use a more verbose constant ? reg.h is included everywhere so
the risk of collision is high.

>  #define   DABR_TRANSLATION	(1UL << 2)
>  #define   DABR_DATA_WRITE	(1UL << 1)
>  #define   DABR_DATA_READ	(1UL << 0)
> +#define   DABR_DATA_RW		(3UL << 0)

Do you really need that ? It's just DABR_DATA_WRITE | DABR_DATA_READ :-)

Cheers,
Ben.

Re: [RFC Patch 2/6] Introduce PPC64 specific Hardware Breakpointinterfaces

[K.Prasad]

On Thu, May 14, 2009 at 04:20:04PM -0400, Alan Stern wrote:
> On Fri, 15 May 2009, K.Prasad wrote:
> 
> > I see that you're referring to this code in __switch_to() :
> >         if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
> >                 set_dabr(new->thread.dabr);
> > 
> > arch_install_thread_hw_breakpoint()<--switch_to_thread_hw_breakpoint()
> > <--__switch_to() implementation is also similar.
> > 
> > In __switch_to(),
> >                 if (unlikely(test_tsk_thread_flag(new, TIF_DEBUG)))
> >                         switch_to_thread_hw_breakpoint(new);
> > 
> > happens only when TIF_DEBUG flag is set. This flag is cleared when the
> > process unregisters any breakpoints it had requested earlier. So, the
> > set_dabr() call is avoided for processes not using the debug register.
> 
> In the x86 code, shouldn't arch_update_user_hw_breakpoint set or clear
> TIF_DEBUG, depending on whether or not there are any user breakpoints
> remaining?
>

Yes. There's a bigger issue in setting TIF_DEBUG flag through ptrace
code. It should instead be done in register_user_hw_breakpoint() and
removed through unregister_user_hw_breakpoint() when the last breakpoint
request is being unregistered.

The unregister_user_hw_breakpoint() code should be like this:

void unregister_user_hw_breakpoint(struct task_struct *tsk,
						struct hw_breakpoint *bp)
{
	struct thread_struct *thread = &(tsk->thread);
	int i, pos = -1, clear_tsk_debug_counter = 0;

	spin_lock_bh(&hw_breakpoint_lock);
	for (i = 0; i < hbp_kernel_pos; i++) {
		if (thread->hbp[i])
			clear_tsk_debug_counter++;
		if (bp == thread->hbp[i]) {
			clear_tsk_debug_counter--;
			pos = i;
		}
	}
	if (pos >= 0)
		__unregister_user_hw_breakpoint(pos, tsk);

	if (!clear_tsk_debug_counter)
		clear_tsk_thread_flag(tsk, TIF_DEBUG);

	spin_unlock_bh(&hw_breakpoint_lock);
}

It needs modification in the generic HW Breakpoint code. I'm planning to
submit this as a patch over the earlier patchset (after it is pulled
into -tip tree).
 
> > > > +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;
> > > > +	int cpu, stepped, is_kernel;
> > > > +
> > > > +	/* Disable breakpoints during exception handling */
> > > > +	set_dabr(0);
> > > > +
> > > > +	dar = regs->dar & (~HW_BREAKPOINT_ALIGN);
> > > > +	is_kernel = (dar >= TASK_SIZE) ? 1 : 0;
> > > 
> > > is_kernel_addr() ?
> > > 
> > 
> > Ok.
> 
> Shouldn't this test hbp_kernel_pos instead?
> 

Testing hbp_kernel_pos should be sufficient for PPC64 with just one
breakpoint register. However the above code is more extensible to other
PowerPC implementations which have more than one breakpoint register.

> > > > +	if (is_kernel)
> > > > +		bp = hbp_kernel[0];
> > > > +	else {
> > > > +		bp = current->thread.hbp[0];
> > > > +		/* Lazy debug register switching */
> > > > +		if (!bp)
> > > > +			return rc;
> 
> Shouldn't this test be moved outside the "if" statement, as in the x86 
> code?
> 

Yes, I will do it. Another error here is the return code when exception
is raised from user-space address due to lazy debug register switching.
The return code should be NOTIFY_STOP (and not NOTIFY_DONE) since the
exception is a stray one and we don't want it to be propogated as a
signal to user-space. This change is required in both x86 and PPC64. I
will submit the x86 change as a separate patch.

> Alan Stern
> 

Thanks,
K.Prasad

Re: [RFC Patch 6/6] Adapt kexec and samples code to recognise PPC64hardware breakpoint usage

[K.Prasad]

On Thu, May 14, 2009 at 04:21:48PM -0400, Alan Stern wrote:
> On Thu, 14 May 2009, K.Prasad wrote:
> 
> > Index: linux-2.6-tip.hbkpt/samples/hw_breakpoint/data_breakpoint.c
> > ===================================================================
> > --- linux-2.6-tip.hbkpt.orig/samples/hw_breakpoint/data_breakpoint.c	2009-05-14 00:17:24.000000000 +0530
> > +++ linux-2.6-tip.hbkpt/samples/hw_breakpoint/data_breakpoint.c	2009-05-14 00:58:06.000000000 +0530
> > @@ -54,6 +54,10 @@
> >  	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 = DABR_DATA_WRITE;
> 
> This should be HW_BREAKPOINT_WRITE, not DABR_DATA_WRITE.
> 
> Alan Stern
>

Done. Thanks.

-- K.Prasad
 

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

[K.Prasad]

On Mon, May 18, 2009 at 01:35:32PM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2009-05-14 at 19:13 +0530, K.Prasad wrote:
> > plain text document attachment (ppc64_prepare_code_01)
> > Prepare the PowerPC code for HW Breakpoint infrastructure patches by including
> > relevant constant definitions and function declarations.
> 
> Hi !
> 
> Some comments below...
> 
> > +#define   HBP_NUM	1	/* Number of physical HW breakpoint registers */
> 
> Can you use a more verbose constant ? reg.h is included everywhere so
> the risk of collision is high.
>

This constant is used by the generic HW Breakpoint code in
kernel/hw_breakpoint.c too and renaming it here will require changes
there too, while I don't see any existing name-space clashes.

Instead the definition can be moved into arch/powerpc/include/asm/hw_breakpoint.h
and in arch/power/include/asm/processor.h. What do you think?
 
> >  #define   DABR_TRANSLATION	(1UL << 2)
> >  #define   DABR_DATA_WRITE	(1UL << 1)
> >  #define   DABR_DATA_READ	(1UL << 0)
> > +#define   DABR_DATA_RW		(3UL << 0)
> 
> Do you really need that ? It's just DABR_DATA_WRITE | DABR_DATA_READ :-)
> 

Yes, it looks silly. I will remove it. The HW_BREAKPOINT_RW defined in
arch/powerpc/include/asm/hw_breakpoint.h will instead be defined as:

#define HW_BREAKPOINT_RW (DABR_DATA_READ | DABR_DATA_WRITE)


> Cheers,
> Ben.
>

Thanks,
K.Prasad
 

Re: [RFC Patch 2/6] Introduce PPC64 specific Hardware Breakpointinterfaces

[Alan Stern]

On Mon, 18 May 2009, K.Prasad wrote:

> > > > > +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;
> > > > > +	int cpu, stepped, is_kernel;
> > > > > +
> > > > > +	/* Disable breakpoints during exception handling */
> > > > > +	set_dabr(0);
> > > > > +
> > > > > +	dar = regs->dar & (~HW_BREAKPOINT_ALIGN);
> > > > > +	is_kernel = (dar >= TASK_SIZE) ? 1 : 0;
> > > > 
> > > > is_kernel_addr() ?
> > > > 
> > > 
> > > Ok.
> > 
> > Shouldn't this test hbp_kernel_pos instead?
> > 
> 
> Testing hbp_kernel_pos should be sufficient for PPC64 with just one
> breakpoint register. However the above code is more extensible to other
> PowerPC implementations which have more than one breakpoint register.

Then maybe you don't want to test this at all.  Just compare the dar 
value with each of the breakpoint addresses.  That's more like what the 
x86 code does.

Alan Stern

Re: [RFC Patch 2/6] Introduce PPC64 specific Hardware Breakpointinterfaces

[K.Prasad]

On Mon, May 18, 2009 at 12:30:41PM -0400, Alan Stern wrote:
> On Mon, 18 May 2009, K.Prasad wrote:
> 
> > > > > > +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;
> > > > > > +	int cpu, stepped, is_kernel;
> > > > > > +
> > > > > > +	/* Disable breakpoints during exception handling */
> > > > > > +	set_dabr(0);
> > > > > > +
> > > > > > +	dar = regs->dar & (~HW_BREAKPOINT_ALIGN);
> > > > > > +	is_kernel = (dar >= TASK_SIZE) ? 1 : 0;
> > > > > 
> > > > > is_kernel_addr() ?
> > > > > 
> > > > 
> > > > Ok.
> > > 
> > > Shouldn't this test hbp_kernel_pos instead?
> > > 
> > 
> > Testing hbp_kernel_pos should be sufficient for PPC64 with just one
> > breakpoint register. However the above code is more extensible to other
> > PowerPC implementations which have more than one breakpoint register.
> 
> Then maybe you don't want to test this at all.  Just compare the dar 
> value with each of the breakpoint addresses.  That's more like what the 
> x86 code does.
> 
> Alan Stern
>

Comparing the DAR register value with each breakpoint address is
required to determine if the exception is the cause of a breakpoint hit
and I've added the code to hw_breakpoint_handler(). With this check in
place, I find that using hbp_kernel_pos to determine kernel/user space
origin is much easier (as you suggested) and the code is modified
accordingly.

Please find the changes in the new patchset being sent.

Thanks,
K.Prasad