| Message ID | 20090727001317.GC13562@in.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Gibson |
| Headers | show |
On Mon, Jul 27, 2009 at 05:43:17AM +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] > +/* > + * 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_kernel, stepped = 1; > + > + is_kernel = (hbp_kernel_pos == HBP_NUM) ? 0 : 1; > + > + /* Disable breakpoints during exception handling */ > + set_dabr(0); > + > + cpu = get_cpu(); > + /* Determine whether kernel- or user-space address is the trigger */ > + bp = is_kernel ? > + per_cpu(this_hbp_kernel[0], cpu) : current->thread.hbp[0]; > + /* > + * 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; > + > + per_cpu(dabr_data, cpu) = is_kernel ? kdabr : current->thread.dabr; > + > + /* 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 (is_kernel && > + !((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); It bothers me that the trigger function is executed before the trapping instruction, but the SIGTRAP occurs afterwards. Since they're both responses to the trap, it seems logical to me that they should occur at the same time (from the trapping program's point of view, at least). > + /* > + * Return early without restoring DABR if the breakpoint is from > + * user-space which always operates in one-shot mode > + */ > + if (!is_kernel) { > + 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(); > + } > + 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(); > + > +out: > + /* > + * A put_cpu() call is required to complement the get_cpu() > + * call used initially > + */ > + put_cpu(); > + return ret; > +} > + > +/* > + * Handle debug exception notifications. > + */ > +int __kprobes hw_breakpoint_exceptions_notify( Um.. there seems to be a pretty glaring problem here, in that AFAICT in this revision of the series, this function is never invoked, and so your breakpoint handling code will never be executed. i.e. the changes to do_dabr to connect your code seem to be missing. > + 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 > @@ -755,6 +755,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) > +{ > +} This is never used, so does not belong in this patch. > 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 >
On Fri, Jul 31, 2009 at 04:16:46PM +1000, David Gibson wrote: > On Mon, Jul 27, 2009 at 05:43:17AM +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] > > +/* > > + * 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_kernel, stepped = 1; > > + > > + is_kernel = (hbp_kernel_pos == HBP_NUM) ? 0 : 1; > > + > > + /* Disable breakpoints during exception handling */ > > + set_dabr(0); > > + > > + cpu = get_cpu(); > > + /* Determine whether kernel- or user-space address is the trigger */ > > + bp = is_kernel ? > > + per_cpu(this_hbp_kernel[0], cpu) : current->thread.hbp[0]; > > + /* > > + * 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; > > + > > + per_cpu(dabr_data, cpu) = is_kernel ? kdabr : current->thread.dabr; > > + > > + /* 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 (is_kernel && > > + !((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); > > It bothers me that the trigger function is executed before the > trapping instruction, but the SIGTRAP occurs afterwards. Since > they're both responses to the trap, it seems logical to me that they > should occur at the same time (from the trapping program's point of > view, at least). > How about moving the triggered function to the single-step handler code for both kernel- and user-space? That would make it behave like a trigger-after-execute (and synchronised with the signal-delivery timing). > > + /* > > + * Return early without restoring DABR if the breakpoint is from > > + * user-space which always operates in one-shot mode > > + */ > > + if (!is_kernel) { > > + 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(); > > + } > > + 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(); > > + > > +out: > > + /* > > + * A put_cpu() call is required to complement the get_cpu() > > + * call used initially > > + */ > > + put_cpu(); > > + return ret; > > +} > > + > > +/* > > + * Handle debug exception notifications. > > + */ > > +int __kprobes hw_breakpoint_exceptions_notify( > > Um.. there seems to be a pretty glaring problem here, in that AFAICT > in this revision of the series, this function is never invoked, and so > your breakpoint handling code will never be executed. i.e. the > changes to do_dabr to connect your code seem to be missing. > I realised it only after you pointed out...some remnants from the previous version have caused it. While the patch should have treated only ptrace in a special manner (one-shot), it erroneously does it for all user-space. I will change it in the next version of the patchset. Thanks, K.Prasad
On Tue, Aug 04, 2009 at 02:29:38AM +0530, K.Prasad wrote: > On Fri, Jul 31, 2009 at 04:16:46PM +1000, David Gibson wrote: > > On Mon, Jul 27, 2009 at 05:43:17AM +0530, K.Prasad wrote: [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 (is_kernel && > > > + !((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); > > > > It bothers me that the trigger function is executed before the > > trapping instruction, but the SIGTRAP occurs afterwards. Since > > they're both responses to the trap, it seems logical to me that they > > should occur at the same time (from the trapping program's point of > > view, at least). > > > > How about moving the triggered function to the single-step handler code > for both kernel- and user-space? > > That would make it behave like a trigger-after-execute (and synchronised > with the signal-delivery timing). I think that would be an improvement, yes. I definitely think the SIGTRAP and the callback function should happen at the same time in all cases. Possibly even have the callback function issue the SIGTRAP rather than special casing that.
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 @@ -126,6 +126,7 @@ config PPC select HAVE_SYSCALL_WRAPPERS if PPC64 select GENERIC_ATOMIC64 if PPC32 select HAVE_PERF_COUNTERS + 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 @@ -35,7 +35,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 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(); +} + +/* + * 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) + return -EINVAL; + + 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_kernel, stepped = 1; + + is_kernel = (hbp_kernel_pos == HBP_NUM) ? 0 : 1; + + /* Disable breakpoints during exception handling */ + set_dabr(0); + + cpu = get_cpu(); + /* Determine whether kernel- or user-space address is the trigger */ + bp = is_kernel ? + per_cpu(this_hbp_kernel[0], cpu) : current->thread.hbp[0]; + /* + * 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; + + per_cpu(dabr_data, cpu) = is_kernel ? kdabr : current->thread.dabr; + + /* 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 (is_kernel && + !((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); + /* + * Return early without restoring DABR if the breakpoint is from + * user-space which always operates in one-shot mode + */ + if (!is_kernel) { + 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(); + } + 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(); + +out: + /* + * A put_cpu() call is required to complement the get_cpu() + * call used initially + */ + put_cpu(); + 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 @@ -755,6 +755,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) {
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(-)