Messages in this thread | | | Date | Fri, 24 Apr 2009 11:26:37 +0530 | From | "K.Prasad" <> | Subject | Re: [Patch 00/11] Hardware Breakpoint interfaces |
| |
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
| |