On Thu, May 20, 2010 at 09:36:03AM +0530, K.Prasad wrote: > > Right. However, the thread is running the signal handler without the > > DABR being set, which is unfortunate. > > > > In order to keep the breakpoint active during signal handling, a > PowerPC specific signal handling code, say do_signal_pending() in > arch/powerpc/kernel/signal.c, must be tapped to check for/restore > the breakpoint for the process (if it exists).
What I would suggest is something slightly different: anything that causes the thread to change where it's executing -- signal delivery, modification of NIP with ptrace -- should cancel the single-step and reinstall the breakpoint in the DABR. In other words we just forget that we hit the breakpoint, and rely on hitting it again if we ever get back to that instruction. I think that is by far the most reliable approach. That means that the hw-breakpoint code needs to export a function called, say, thread_change_pc(), which is called whenever we're changing a thread's userspace PC (NIP) value. If the hw-breakpoint code has put that thread into single-step mode, we cancel the single-step and if the thread is current, set DABR. > I'm afraid if this is more complexity than we want to handle in the > first patchset. I agree that this will create a 'blind-spot' of code > which cannot not be monitored using breakpoints and may limit debugging > efforts (specially for memory corruption); however suspecting that signal > handlers (especially those that don't return to original instruction) > would be rare, I feel that this could be a 'feature' that can be brought > later-in. What do you think? I think the thread_change_pc() approach is quite a bit simpler, and I think it's better to get this right at the outset rather than have it cause bugs later on, when we've all forgotten all the curly details. :) > > Imagine this scenario: we get the DABR match, set MSR_SE and return to > > the task. In the meantime another higher-priority task has become > > runnable and our need_resched flag is set, so we call schedule() on > > the way back out to usermode. The other task runs and then blocks and > > our task gets scheduled again. As part of the context switch, > > arch_install_hw_breakpoint() will get called and will set DABR. So > > we'll return to usermode with both DABR and MSE_SE set. > > > > I didn't foresee such a possibility. I think this can be handled by > putting a check in arch_install_hw_breakpoint() as shown below: > > int arch_install_hw_breakpoint(struct perf_event *bp) > { > struct arch_hw_breakpoint *info = counter_arch_bp(bp); > struct perf_event **slot = &__get_cpu_var(bp_per_reg); > > *slot = bp; > if (!current->thread.last_hit_ubp) > set_dabr(info->address | info->type | DABR_TRANSLATION); > return 0; > } Yes, basically, but don't we need to handle per-cpu breakpoints as well? That is, we only want the extra check if this breakpoint is a per-task breakpoint. Or am I not seeing enough context here? Also, I have just posted extensions to emulate_single_step() to handle loads and stores. I think this should be enough to handle DABR interrupts that occur in kernel mode, so we should never need to actually single-step in that case -- if emulate_step fails we should just print a warning, uninstall the breakpoint, and continue. That way we don't have to worry about all the interrupts and other eventualities that could occur while single-stepping in the kernel. For DABR interrupts that occur in user mode, I think the approach of single-stepping together with calls to thread_change_pc() in the signal delivery and ptrace code should handle all the cases, at least for per-task breakpoints. Per-cpu breakpoints that get hit in user mode will be a bit trickier, but I assume we can restrict per-cpu breakpoints to kernel addresses for now. If you want help adding the thread_change_pc() calls to signal delivery and ptrace, let me know. Paul. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev