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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev