Re: [PATCH] i386 Hardware watchpoints using kgdb-2-2.6.14.tar.gz

Dennis,

Overall looks all-right. This needs to be rebased to CVS patches.

Thanks.
-Amit

On Tuesday 31 Jan 2006 3:54 am, Dennis W. Tokarski wrote:
> Attached is my hardware break/watchpoint patch. Review
> and comments requested.
>
> This patch is meant to be applied to a kernel.org 2.6.14.3
> kernel which has first had the patches from kgdb-2-2.6.14.tar.gz,
> and then linux-2_6_14_3-dbgregs-smp.patch from my previous post.
>
> I've tried to use the debug register allocation interface in
> a way which keeps kgdb from conflicting with other in-kernel
> or user-space users of debug registers.
>
> Also, I've made an attempt to keep this SMP safe, but others
> should check me on this as I have no SMP machine to test
> this on.  My approach was to save dr6, dr7,
> and the type and address of any debug exception when the
> exception handler calls kgdb_disable_hw_debug() early on.
> That information is used on the way out in
> kgdb_arch_handle_exception(), which is also the only place
> dr6 is modified.  kgdb_correct_hw_break() is the only
> place where dr7 and the address registers are actually
> modified, and they are synchronized across cpus when that
> happens.  When gdb is clearing and resetting breakpoints,
> only the shadowed value of dr7 is actually changed.
>
> One thing I'm uncertain about is if this scheme
> holds up if more than one cpu happens to hit a hardware
> breakpoint nearly at the same time, regardless of
> whether they hit the same or different breakpoints.
>
>    --Dennis
>
> Dennis W. Tokarski wrote:
> > Amit Kale wrote:
> >> Thanks, Dennis. Let's stick to "debug register allocation" as
> >> suggested by you.
> >>
> >> kgdb_set_hw_break definition contains only one function parameter,
> >> which is address. Current i386.patch includes a kgdb_set_hw_break
> >> function which always sets a data watchpoints. That's the reason you
> >> couldn't get execution watchpoints as you have mentioned in an earlier
> >> email.
> >
> > Right, I discovered that.  It's called in response to a Z1 pacaket and
> > was intended, I think, to set a code breakpoint--at least it puts type=1
> > and length=1 into a breakinfo entry.  But then it copies those values
> > verbatim into dr7, which is wrong as the register fields are zero-based.
> > So you get a write-data watchpoint instead.
> >
> >> kgdb_ops->set_hw_breakpoint has three parameters, which is the correct
> >> way of doing it.
> >
> > Also right, which is why I chose to use it in the first place. Fewer
> > code changes that way.
> >
> > I expect to have a completed patch to post later today.
> >
> >   --Dennis
> >
> >> -Amit
> >>
> >> On Thursday 12 Jan 2006 8:52 am, Dennis W. Tokarski wrote:
> >>> Hey there Amit,
> >>>
> >>> Amit Kale wrote:
> >>>> On Saturday 07 Jan 2006 3:46 am, Dennis W. Tokarski wrote:
> >>>
> >>> [snip]
> >>>
> >>>>> 2.6.14.3 kernel that I'm using.  I did not include the second patch
> >>>>> which adds kwatch-points--it's a neat feature but not directly
> >>>>> related to kgdb.
> >>>>>
> >>>>> Do you want a copy of the resulting debug register allocation
> >>>>> patch?
> >>>>
> >>>> Not sure about this. When soon is kprobes patch expected to make it to
> >>>> the kernel? If not soon, there isn't any point in using that
> >>>> tmechanism as we can survive with some minimal code for that.
> >>>
> >>> Is that a change of position?  I incorporated the patch (which I should
> >>> probably have called a "debug register allocation" patch rather than
> >>> a "kprobe" patch) into my work mainly because of your earlier reply
> >>> to Tom's posting on 12/16/05:
> >>>
> >>> =============
> >>>
> >>> Amit Kale wrote:
> >>> > That's a good scheme. We definitely want to use it when enabling
> >>> > watchpoints in KGDB. Even if it gets accepted in the kernel in a
> >>> > different form or some other scheme gets accepted, we can quickly
> >>>
> >>> change
> >>>
> >>> > our code base accordingly.
> >>> >
> >>> > -Amit
> >>>
> >>> =============
> >>>
> >>> I would advocate keeping it.  Certainly something like it becomes
> >>> necessary
> >>> as soon as anything other than kgdb wants to use the hardware debug
> >>> registers in the kernel.  For anyone debugging a complex problem in
> >>> user space and the kernel concurently (and I've been there many times)
> >>> it's necessary as well to keep user space gdb and kgdb from
> >>> conflicting.
> >>>
> >>> The patch is quite minimal, being largely confined to its own header
> >>> file
> >>> and one arch-specific source file.  It touches the kernel elsewhere
> >>> only where it's unavoidable, and even then only a couple of lines here
> >>> and there.
> >>>
> >>> I don't see how it could be reduced without breaking something.
> >>>
> >>>
> >>>
> >>> [snip]
> >>>
> >>>>> Is one of those mechanisms deprecated? I chose for the moment to
> >>>>> use the call through kgdb_ops.
> >>>>
> >>>> kgdb_ops function pointers are deprecated. We should be using weak
> >>>> mechanisms only. set_breakpoint should also be changed this way.
> >>>>
> >>>> Milind, can you do the set_breakpoint change?
> >>>
> >>> OK, I'll do it that way.  I've captured the essentials of Milands
> >>> patch and will follow that example for the hw_break/watch stuff.
> >>>
> >>>>> OK, I lied, there is actually a third issue.  You can't, in
> >>>>> kernel/kgdb.c, just pass the ASCII type code from the Z packet
> >>>
> >>> [snip]
> >>>
> >>>> I would prefer changing the type of second parameter of
> >>>> set_hw_breakpoint
> >>>> to "const char bptype". gdb remote protocol defines the values
> >>>> bptype can
> >>>> take ('0-4'). Hence they are stable, besides they allow flexibility
> >>>> wrt.
> >>>> achitecture.
> >>>> -Amit
> >>>
> >>> Noted. I'll do that.
> >>>
> >>>   --Dennis
> >>>
> >>>>>  --Dennis
> >>>>>
> >>>>> Tom Rini wrote:
> >>>>>> On Fri, Dec 16, 2005 at 10:39:42AM +0530, Amit Kale wrote:
> >>>>>>> Dennis,
> >>>>>>>
> >>>>>>> That would be splendid. When a watchpoint is required, no other
> >>>>>>> feature
> >>>>>>> can do the job!
> >>>>>>
> >>>>>> Note that to do this correctly, you have to be aware of other
> >>>>>> potential
> >>>>>> users.  Prasanna Panchamukhi of the kprobes project forwarded me
> >>>>>> some code that would do this and I'll send that along to the list
> >>>>>> momentarily.
> >>>>>>
> >>>>>>> -Amit
> >>>>>>>
> >>>>>>> On Friday 16 Dec 2005 12:41 am, Dennis W. Tokarski wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> Amit Kale wrote:
> >>>>>>>>> On Thursday 15 Dec 2005 8:40 pm, Tom Rini wrote:
> >>>>>>>>>> On Fri, Dec 09, 2005 at 12:42:39PM +0530, Amit Kale wrote:
> >>>>>>>>>>> On Thursday 08 Dec 2005 11:13 pm, Dennis W. Tokarski wrote:
> >>>>>>>>
> >>>>>>>> [snip]
> >>>>>>>>
> >>>>>>>>>> I'm fairly certain we don't support watchpoints, and I don't
> >>>>>>>>>> _think_ we ever did on i386 (we may have on ia64 at some
> >>>>>>>>>> point).  If
> >>>>>>>>>> the packet isn't [zZ][01] we relpy back to GDB that it's
> >>>>>>>>>> unsupported.
> >>>>>>>>
> >>>>>>>> True. But the first problem was gdb wasn't sending out Z packets
> >>>>>>>> anyway. I have a temporary hack to fix that, but gkdb still
> >>>>>>>> doesn't set an execution break point with Z1. It seems instead to
> >>>>>>>> set a data
> >>>>>>>> access watchpoint.  As I mentioned before, I was able to manually
> >>>>>>>> send Z packets using the maintenance command and got the same
> >>>>>>>> result.
> >>>>>>>>
> >>>>>>>> So, kgdb isn't setting the watchpoint up even when it gets the
> >>>>>>>> right packets.  btw, I'm working exclusively on i386, so that's
> >>>>>>>> all I can speak to.
> >>>>>>>>
> >>>>>>>> Anyway, what I propose to do is look back at an earlier kgdb
> >>>>>>>> which did to all those things correctly--I've got a 2.4.18
> >>>>>>>> kernel here set up that way--and bring the code forward into
> >>>>>>>> the current kgdb for i386.  As part of that I'll handle the
> >>>>>>>> additional Z packets.
> >>>>>>>>
> >>>>>>>> And there's still the problem with gdb itself, and why they
> >>>>>>>> broke Z packets, but I'll take that up on the gdb list.
> >>>>>>>>
> >>>>>>>>> We didn't have support for official version of gdb watchpoints,
> >>>>>>>>> though one could send "y" packets using "maintenance packet"
> >>>>>>>>> commands. It worked. That was a couple of years ago, I believe. I
> >>>>>>>>> can't recall when we removed support for y packets. Present code
> >>>>>>>>> doesn't support it for sure.
> >>>>>>>>>
> >>>>>>>>> -Amit
> >>>>>>>>
> >>>>>>>> I was certainly working with the macros back in the 2.4.18 days,
> >>>>>>>> but this is the first time I've used kdgb in 2.6.  A lot has
> >>>>>>>> changed, not just with kgdb.
> >>>>>>>>
> >>>>>>>> --Dennis
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> -------------------------------------------------------
> >>>>>>>> This SF.net email is sponsored by: Splunk Inc. Do you grep
> >>>>>>>> through log
> >>>>>>>> files for problems?  Stop!  Download the new AJAX search engine
> >>>>>>>> that
> >>>>>>>> makes searching your log files as easy as surfing the  web.
> >>>>>>>> DOWNLOAD
> >>>>>>>> SPLUNK! http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> >>>>>>>> _______________________________________________
> >>>>>>>> Kgdb-bugreport mailing list
> >>>>>>>> Kgdb-bugreport@xxxxxxxxxxxxxxxxxxxxx
> >>>>>>>> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
> >>>>>>>
> >>>>>>> -------------------------------------------------------
> >>>>>>> This SF.net email is sponsored by: Splunk Inc. Do you grep
> >>>>>>> through log
> >>>>>>> files for problems?  Stop!  Download the new AJAX search engine
> >>>>>>> that makes searching your log files as easy as surfing the  web.
> >>>>>>> DOWNLOAD
> >>>>>>> SPLUNK! http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> >>>>>>> _______________________________________________
> >>>>>>> Kgdb-bugreport mailing list
> >>>>>>> Kgdb-bugreport@xxxxxxxxxxxxxxxxxxxxx
> >>>>>>> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
> >>>>>
> >>>>> -------------------------------------------------------
> >>>>> This SF.net email is sponsored by: Splunk Inc. Do you grep through
> >>>>> log files for problems?  Stop!  Download the new AJAX search engine
> >>>>> that makes searching your log files as easy as surfing the  web. 
> >>>>> DOWNLOAD SPLUNK!
> >>>>> http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> >>>>> _______________________________________________
> >>>>> Kgdb-bugreport mailing list
> >>>>> Kgdb-bugreport@xxxxxxxxxxxxxxxxxxxxx
> >>>>> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
> >>>
> >>> -------------------------------------------------------
> >>> This SF.net email is sponsored by: Splunk Inc. Do you grep through log
> >>> files for problems?  Stop!  Download the new AJAX search engine that
> >>> makes
> >>> searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
> >>> http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> >>> _______________________________________________
> >>> Kgdb-bugreport mailing list
> >>> Kgdb-bugreport@xxxxxxxxxxxxxxxxxxxxx
> >>> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
> >>
> >> !DSPAM:43c5f2da318441558920709!


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642