On Mon, Jun 15, 2009 at 04:40:45PM +1000, David Gibson wrote: > On Wed, Jun 10, 2009 at 12:13:49PM +0530, K.Prasad wrote: > > On Fri, Jun 05, 2009 at 03:11:58PM +1000, David Gibson wrote: > > > On Wed, Jun 03, 2009 at 10:05:11PM +0530, K.Prasad wrote: > > > > > > + else { > > > > + /* > > > > + * 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'. > > > > + */ > > > > + rc = NOTIFY_STOP; > > > > + goto out; > > > > + } > > > > + is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0; > > > > > > Ouch, explicitly special-casing ptrace_triggered is pretty nasty. > > > Since the bp_info is already arch specific, maybe it should include a > > > flag to indicate whether the breakpoint is one-shot or not. > > > > > > > The reason to check for ptrace_triggered is to contain the one-shot > > behaviour only to ptrace (thus retaining the semantics) and not to extend > > them to all user-space requests through > > register_user_hw_breakpoint(). > > Right, but couldn't you implement that withing ptrace_triggered > itself, without a special test here, by having it cancel the > breakpoint. >
A special check (either using the callback routine as above, or using a special flag) will be required in hw_breakpoint_handler() to enable early return (without single-stepping). I'm not sure if I got your suggestion right, and let me know if you think so. > > A one-shot behaviour for all user-space requests would create more work > > for the user-space programs (such as re-registration) and will leave open > > a small window of opportunity for debug register grabbing by kernel-space > > requests. > > > > So, in effect a request through register_user_hw_breakpoint() interface > > will behave as under: > > - Single-step over the causative instruction that triggered the > > breakpoint exception handler. > > - Deliver the SIGTRAP signal to user-space after executing the causative > > instruction. > > > > This behaviour is in consonance with that of kernel-space requests and > > those on x86 processors, and helps define a consistent behaviour across > > architectures for user-space. > > > > Let me know what you think on the same. > > I certainly see the value in consistent semantics across archs. > However, I can also see uses for the powerpc trap-before-execute > behaviour. That's why I'm suggesting it might be worth having an > arch-specific flag. > > [snip] So, you suggest that the 'one-shot' behaviour should be driven by user-request and not just confined to ptrace? (The default behaviour for all breakpoints-minus-ptrace will remain 'continuous' though). It can be implemented through an additional flag in 'struct arch_hw_breakpoint'. I can send a new version 7 of the patchset with this change (with the hope that the version 6 of the patchset looks fine in its present form!). Meanwhile, we'd like to know what uses you see in addition to the present one if the one-shot behaviour is made user-defined. Are those uses beyond what can be achieved through the present ptrace interface? > > > > +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); > > > > > > Uh.. I recall mentioning in my previous review that in order to match > > > previous behaviour we need to deliver the userspace signal *before* > > > stepping over the breakpointed instruction, rather than after (which > > > I guess is why breakpoints are one-shot in the old scheme). > > > > This code would implement the behaviour as stated in the comment for > > user-space requests above. > > And you're relying on the old trap-sending code in do_dabr for ptrace > requests? > Yes. > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ > _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson Thanks, K.Prasad _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev