Re: [Patch 00/11] Hardware Breakpoint interfaces

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

On Fri, Apr 17, 2009 at 10:37:13AM -0400, Alan Stern wrote:
> On Fri, 17 Apr 2009, K.Prasad wrote:
> 

Hi Alan,
	I have modified the patch by accepting all of your suggestions
excepting one - which I try to explain below.

I will send the patchset containing changes suggested by you and a few
others (have explained that in the changelog).

Kindly review the new code.


> > 
> > > > +	on_each_cpu(arch_update_kernel_hw_breakpoints, NULL, 0);
> > > > +	hbp_kernel_pos++;
> > > 
> > > Don't you want to increment hbp_kernel_pos _before_ calling
> > > on_each_cpu()?  Otherwise you're telling the other CPUs to install an
> > > empty breakpoint.
> > > 
> > 
> > Incrementing hbp_kernel_pos after arch_update_kernel_hw_breakpoints()
> > during unregister_kernel_ is of the following significance:
> > 
> > - The 'pos' numbered debug register is reset to 0 through the code 
> >   "set_debugreg(hbp_kernel[i]->info.address, i)" where 'i' ranges from
> >   hbp_kernel_pos to HB_NUM. This is necessary during unregister operation.
> 
> It _isn't_ necessary.  The contents of that debug register don't matter 
> at all if it isn't enabled.  (And there's nothing special about 0; if 
> the breakpoint were enabled then you would get a breakpoint interrupt 
> whenever something tried to access address 0.)
> 
> Besides, the "set_debugreg" line of code in
> arch_update_kernel_hw_breakpoints doesn't get executed when
> hbp_kernel[i] is NULL.  So this whole point is moot; the debug register
> wouldn't be affected anyway.
> 
> > - The following statements would reset the bits corresponding to
> >   unregistered breakpoint only then.
> > 	kdr7 &= ~kdr7_masks[hbp_kernel_pos];
> > 	dr7 &= ~kdr7_masks[hbp_kernel_pos];
> 
> But if hbp_kernel_pos is wrong (i.e., if it hasn't been incremented)
> then these statements will set kdr7 and dr7 incorrectly.
> 
> > - Helps keep the arch_update_kernel_hw_breakpoints() code generic enough
> >   to be invoked during register and unregister operations.
> 
> This is another moot point.  Your code is _wrong_ -- how generic it is
> doesn't matter.
> 
>

The arch_update_kernel_hw_breakpoints() was designed to work like this -
it updates all registers beginning 'hbp_kernel_pos' to (HB_NUM - 1) with
the values stored in hbp_kernel[] array.

When inserting a new breakpoint, hbp_kernel_pos is decremented *before*
invoking arch_update_kernel_hw_breakpoints() so that the new value is
also written onto the physical debug register.

On removal, 'hbp_kernel_pos' is incremented *after*
arch_update_kernel_hw_breakpoints() so that the physical debug registers
i.e. both DR7 and DR<pos> are updated with the changes post removal and
compaction. I'm ready to make changes but don't see where the code
actually goes wrong. Can you explain that?

Thanks,
K.Prasad