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