On Mon, Jan 25, 2010 at 10:26:37PM -0600, Jason Wessel wrote:
> @@ -466,7 +466,7 @@ static int __kprobes hw_breakpoint_handler(struct
> die_args *args)
> {
> int i, cpu, rc = NOTIFY_STOP;
> struct perf_event *bp;
> - unsigned long dr7, dr6;
> + unsigned long dr6;
> unsigned long *dr6_p;
>
> /* The DR6 value is pointed by args->err */
> @@ -477,7 +477,6 @@ static int __kprobes hw_breakpoint_handler(struct
> die_args *args)
> if ((dr6 & DR_TRAP_BITS) == 0)
> return NOTIFY_DONE;
>
> - get_debugreg(dr7, 7);
> /* Disable breakpoints during exception handling */
> set_debugreg(0UL, 7);
> /*
> @@ -525,7 +524,7 @@ static int __kprobes hw_breakpoint_handler(struct
> die_args *args)
> if (dr6 & (~DR_TRAP_BITS))
> rc = NOTIFY_DONE;
>
> - set_debugreg(dr7, 7);
> + set_debugreg(__get_cpu_var(cpu_dr7), 7);
> put_cpu();
Good simplification, but that doesn't appear related to kgdb,
this should be done in a separate patch, for the perf/core tree.
> static void kgdb_correct_hw_break(void)
> {
> - unsigned long dr7;
> - int correctit = 0;
> - int breakbit;
> int breakno;
>
> - get_debugreg(dr7, 7);
> for (breakno = 0; breakno < 4; breakno++) {
> - breakbit = 2 << (breakno << 1);
> - if (!(dr7 & breakbit) && breakinfo[breakno].enabled) {
> - correctit = 1;
> - dr7 |= breakbit;
> - dr7 &= ~(0xf0000 << (breakno << 2));
> - dr7 |= ((breakinfo[breakno].len << 2) |
> - breakinfo[breakno].type) <<
> - ((breakno << 2) + 16);
> - set_debugreg(breakinfo[breakno].addr, breakno);
> -
> - } else {
> - if ((dr7 & breakbit) && !breakinfo[breakno].enabled) {
> - correctit = 1;
> - dr7 &= ~breakbit;
> - dr7 &= ~(0xf0000 << (breakno << 2));
> - }
> - }
> + struct perf_event *bp;
> + struct arch_hw_breakpoint *info;
> + int val;
> + int cpu = raw_smp_processor_id();
> + if (!breakinfo[breakno].enabled)
> + continue;
> + bp = *per_cpu_ptr(breakinfo[breakno].pev, cpu);
> + info = counter_arch_bp(bp);
> + if (bp->attr.disabled != 1)
> + continue;
> + bp->attr.bp_addr = breakinfo[breakno].addr;
> + bp->attr.bp_len = breakinfo[breakno].len;
> + bp->attr.bp_type = breakinfo[breakno].type;
> + info->address = breakinfo[breakno].addr;
> + info->len = breakinfo[breakno].len;
> + info->type = breakinfo[breakno].type;
> + val = arch_install_hw_breakpoint(bp);
> + if (!val)
> + bp->attr.disabled = 0;
> }
> - if (correctit)
> - set_debugreg(dr7, 7);
> + hw_breakpoint_restore();
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.
> @@ -278,27 +285,38 @@ kgdb_set_hw_break(unsigned long addr, int len, enum
> kgdb_bptype bptype)
>
> switch (bptype) {
> case BP_HARDWARE_BREAKPOINT:
> - type = 0;
> - len = 1;
> + len = 1;
> + breakinfo[i].type = X86_BREAKPOINT_EXECUTE;
> break;
> case BP_WRITE_WATCHPOINT:
> - type = 1;
> + breakinfo[i].type = X86_BREAKPOINT_WRITE;
> break;
> case BP_ACCESS_WATCHPOINT:
> - type = 3;
> + breakinfo[i].type = X86_BREAKPOINT_RW;
> break;
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,
};
We have functions in x86 to do the conversion to
x86 values in arch/x86/kernel/hw_breakpoint.c
Nothing urgent though, as this patch is a regression fix,
this can be done later.
> + switch (len) {
> + case 1:
> + breakinfo[i].len = X86_BREAKPOINT_LEN_1;
> + break;
> + case 2:
> + breakinfo[i].len = X86_BREAKPOINT_LEN_2;
> + break;
> + case 4:
> + breakinfo[i].len = X86_BREAKPOINT_LEN_4;
> + break;
> +#ifdef CONFIG_X86_64
> + case 8:
> + breakinfo[i].len = X86_BREAKPOINT_LEN_8;
> + break;
> +#endif
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.
Hence you would be able to use the x86 breakpoint API
we have already, arch_validate_hwbkpt_settings() does
everything for you. This is going to shrink your code
and then make it a stronger argument to pull request
as a not-that-one-liner regression fix late in the
process (which I must confess is my bad, firstly: I
did the regression and secondly: I should have
reviewed these fixes sooner).
> @@ -313,8 +331,21 @@ kgdb_set_hw_break(unsigned long addr, int len, enum
> kgdb_bptype bptype)
> */
> void kgdb_disable_hw_debug(struct pt_regs *regs)
> {
> + int i;
> + int cpu = raw_smp_processor_id();
> + struct perf_event *bp;
> +
> /* Disable hardware debugging while we are in kgdb: */
> set_debugreg(0UL, 7);
> + for (i = 0; i < 4; i++) {
> + if (!breakinfo[i].enabled)
See? Here you could use simply bp->attr.disabled instead
of playing with this breakinfo.
> @@ -378,7 +409,6 @@ int kgdb_arch_handle_exception(int e_vector, int signo,
> int err_code,
> struct pt_regs *linux_regs)
> {
> unsigned long addr;
> - unsigned long dr6;
> char *ptr;
> int newPC;
>
> @@ -448,10 +464,12 @@ single_step_cont(struct pt_regs *regs, struct die_args
> *args)
> }
>
> static int was_in_debug_nmi[NR_CPUS];
> +static int recieved_hw_brk[NR_CPUS];
>
> static int __kgdb_notify(struct die_args *args, unsigned long cmd)
> {
> struct pt_regs *regs = args->regs;
> + unsigned long *dr6_p;
>
> switch (cmd) {
> case DIE_NMI:
> @@ -485,16 +503,24 @@ static int __kgdb_notify(struct die_args *args,
> unsigned long cmd)
> break;
>
> case DIE_DEBUG:
> - if (atomic_read(&kgdb_cpu_doing_single_step) ==
> - raw_smp_processor_id()) {
> + dr6_p = (unsigned long *)ERR_PTR(args->err);
> + if (atomic_read(&kgdb_cpu_doing_single_step) != -1) {
> + if (dr6_p && (*dr6_p & DR_STEP) == 0)
> + return NOTIFY_DONE;
> if (user_mode(regs))
> return single_step_cont(regs, args);
> break;
> - } else if (test_thread_flag(TIF_SINGLESTEP))
> + } else if (test_thread_flag(TIF_SINGLESTEP)) {
> /* This means a user thread is single stepping
> * a system call which should be ignored
> */
> return NOTIFY_DONE;
> + } 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?
>
> +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.
> +
> /**
> * kgdb_arch_init - Perform any architecture specific initalization.
> *
> @@ -539,7 +584,43 @@ static struct notifier_block kgdb_notifier = {
> */
> int kgdb_arch_init(void)
> {
> - return register_die_notifier(&kgdb_notifier);
> + int i, cpu;
> + int ret;
> + struct perf_event_attr attr;
> + struct perf_event **pevent;
> +
> + ret = register_die_notifier(&kgdb_notifier);
> + if (ret != 0)
> + return ret;
> + /*
> + * Pre-allocate the hw breakpoint structions in the non-atomic
> + * portion of kgdb because this operation requires mutexs to
> + * complete.
> + */
> + attr.bp_addr = (unsigned long)kgdb_arch_init;
> + attr.type = PERF_TYPE_BREAKPOINT;
> + attr.bp_len = HW_BREAKPOINT_LEN_1;
> + attr.bp_type = HW_BREAKPOINT_X;
> + attr.disabled = 1;
> + for (i = 0; i < 4; i++) {
> + breakinfo[i].pev = register_wide_hw_breakpoint(&attr,
> + kgdb_hw_bp);
By calling this, you are reserving all the breakpoint slots.
> + if (IS_ERR(breakinfo[i].pev)) {
> + printk(KERN_ERR "kgdb: Could not allocate hw
> breakpoints\n");
> + breakinfo[i].pev = NULL;
> + kgdb_arch_exit();
> + return -1;
> + }
> + for_each_online_cpu(cpu) {
> + pevent = per_cpu_ptr(breakinfo[i].pev, cpu);
> + pevent[0]->hw.sample_period = 1;
> + if (pevent[0]->destroy != NULL) {
> + pevent[0]->destroy = NULL;
> + release_bp_slot(*pevent);
And then you release these, ok. We should find a proper
way for that later, but for now it should work.
------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
_______________________________________________
Kgdb-bugreport mailing list
Kgdb-bugreport@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
