Re: [RFC Patch 2/6] Introduce PPC64 specific Hardware Breakpointinterfaces

["K.Prasad" <prasad@linux.vnet.ibm.com>]

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