Re: [Patch 00/11] Hardware Breakpoint interfaces

K.Prasad
2009-04-24T05:57:17Z

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