Messages in this thread | | | Date | Thu, 28 Jan 2010 14:27:54 -0600 | From | Jason Wessel <> | Subject | Re: [PATCH 1/4] x86,hw_breakpoint,kgdb: kgdb to usehw_breakpointAPI |
| |
Frederic Weisbecker wrote: > On Thu, Jan 28, 2010 at 11:44:52AM -0600, Jason Wessel wrote: > >> Frederic Weisbecker wrote: >> >>> Good simplification, but that doesn't appear related to kgdb, >>> this should be done in a separate patch, for the perf/core tree. >>> >>> >>> >> Specifically this is required so that kgdb can modify the state of dr7 >> by installing and removing breakpoints. Without this change, on return >> from the callback the dr7 was not correct. >> >> As far as I know, only kgdb was altering the dr registers during a call >> back.. >> > > > > Ok. Well not sure how/where it needs to modify dr7 directly. > >
This is not needed any more with the patch I already followed up with.
To provide an explanation though, dr7 got updated as a result of installing some breakpoints while in kgdb while in the call back. And that value got squashed by what ever was saved on the stack as a local in the perf breakpoint handler.
> > >> I admit I did not test running a kvm instance, so I don't know what kind >> of conflict there would be here. I went and further looked at the kvm >> code, and they call the function for the same reason kgdb does. They >> want the original system values back on resuming normal kernel >> execution. KVM can modify dr7 or other regs directly on entry for its >> guest execution. Kgdb does the same sort of thing so as to prevent the >> debugger from interrupting itself. >> > > > > You mean kgdb needs to disable dr7 while handling a breakpoint to > avoid recursion? In this case this is something already done > from the x86 breakpoint handler. > > >
True, that is done for the master core, but not the slave cores, which we stop dead in their tracks with a nmi, so they have not had dr7 zeroed out.
Obviously we restore it on resume.
> > >>> Would be nice to have bptype set to the generic flags >>> we have already in linux/hw_breakpoint.h: >>> >>> enum { >>> HW_BREAKPOINT_R = 1, >>> HW_BREAKPOINT_W = 2, >>> HW_BREAKPOINT_X = 4, >>> }; >>> >>> >>> >>> >> These numbers have to get translated somewhere from the GDB version >> which it handed off via the gdb serial protocol. They could be >> translated in the gdb stub, but for now they are in the arch specific >> stub. Or you can choose to use the same numbering scheme as gdb for the >> breakpoint types and the values could be used directly. >> > > > > Ah ok. Well, translating gdb <-> generic values will make you > move this code from x86 to core at least. > >
I'll move this to the gdbstub in the next merge window, because it looks like there might also be other archs that gain the arch/*/kernel/hw_breakpoint.c.
I can also move the logic for the install / remove to be owned by either hw_breakpoint.c or the debug core because that will become arch independent as well as more archs pickup the hw_breakpoint.c methodology.
Thanks, Jason.
| |