Messages in this thread | | | Date | Thu, 28 Jan 2010 21:04:23 +0100 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 1/4] x86,hw_breakpoint,kgdb: kgdb to use hw_breakpointAPI |
| |
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.
> > hw_breakpoint_restore() is used by KVM only, for now. > > The cpu var cpu_debugreg[] contains values that > > are only saved when KVM switches to a guest, then > > this function is called when KVM switches back to the > > host. I bet this is not the function you need. > > In fact, I don't know what you intended to do there. > > > > > > I was looking to restore the proper contents of the debug registers when > resuming the general kernel execution. > > As far as I could tell it looked like the right function because > arch_install_breakpoint() uses the per_cpu vars. If there is a save > function that I need to call first that is a different issue. > > IE: > __get_cpu_var(cpu_debugreg[i]) = info->address; > > > 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.
> > 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.
> > Same here, see arch_build_bp_info(). > > Actually, arch_validate_hwbkpt_settings() would do all > > that for you. May require few changes though to adapt. > > > > Actually, I don't understand why you encumber with this > > breakinfo thing. Why not just keeping a per cpu array > > of perf events? You have everything you need inside: > > the generic breakpoint attributes in the attrs and > > the arch info in the hw_perf_event struct inside. > > > > I think the break info thing will go away via a refactor. For now I was > really looking to make it work. There was no way to tell at the time > what values were safe to use in attr struct provided by perf. I would > have further preferred to be able to use the simple -1 cpu in the bp > type and let perf do all the work, but there is no way to allocate a > perf hw wide break like this at the moment.
Ok.
> Realize that what is here is well tested, and aimed to first correct the > regression.
Sure.
> >> + } else if (recieved_hw_brk[raw_smp_processor_id()] == 1) { > >> + recieved_hw_brk[raw_smp_processor_id()] = 0; > >> + return NOTIFY_STOP; > >> + } else if (dr6_p && (*dr6_p & DR_TRAP_BITS) == 0) { > >> + return NOTIFY_DONE; > >> + } > >> > > > > > > > > So this is the debug handler, right? > > > > > > > > This ugly bit is all because the patch I had for returning something > from the event call back was tossed. > > Because perf does not honor the return code from a call back there is no > way to dismiss an event in the die notifier. The forces the debug > handler to do very nasty tricks so as not to handle the same event twice.
Right, we'll need to fix that later from perf, I think.
> >> > >> +static void kgdb_hw_bp(struct perf_event *bp, int nmi, > >> + struct perf_sample_data *data, > >> + struct pt_regs *regs) > >> +{ > >> + struct die_args args; > >> + int cpu = raw_smp_processor_id(); > >> + > >> + args.trapnr = 0; > >> + args.signr = 5; > >> + args.err = 0; > >> + args.regs = regs; > >> + args.str = "debug"; > >> + recieved_hw_brk[cpu] = 0; > >> + if (__kgdb_notify(&args, DIE_DEBUG) == NOTIFY_STOP) > >> + recieved_hw_brk[cpu] = 1; > >> + else > >> + recieved_hw_brk[cpu] = 0; > >> +} > >> > > > > > > > > And this looks like the perf event handler. > > > > I'm confused by the logic here. We have the x86 breakpoint > > handler which calls perf_bp_event which in turn will call > > the above. The above calls __kgdb_notify(), but it will > > also be called later as it is a debug notifier. > > > > > > > > Perf was eating my events if I had no call back. If you know a way > around this let me know.
The problem is not the perf callback. What I did not understand was the use of __kgdb_notify() from the callback, while it is still called after as a debug notifier.
> > And then you release these, ok. We should find a proper > > way for that later, but for now it should work. > > > > > > Previously, I was unable to convince anyone that the kernel debugger > needed to be able to do this. I had an API change to perf for it, but > it was dismissed along with the notify return value on the call back. > This is merely a work around to correct the regression.
We'll need to sanitize that after the regression fix.
| |