Messages in this thread Patch in this message | | | Date | Thu, 12 Feb 2004 08:50:55 -0700 | From | Tom Rini <> | Subject | Re: [PATCH][6/6] A different KGDB stub |
| |
On Thu, Feb 12, 2004 at 07:43:04AM +0100, Andi Kleen wrote:
> Tom Rini <trini@kernel.crashing.org> writes: > > Andrew, please don't add this broken version. > > > @@ -219,7 +219,7 @@ > > jnc sysret_signal > > sti > > pushq %rdi > > - call schedule > > + call user_schedule > > I really don't like this change. It is completely useless because you > can get the pt_regs as well from the stack. Please don't add it. > George's stub also didn't need it. > > > +#ifdef CONFIG_KGDB_THREAD > > +ENTRY(kern_schedule) > > Similar. No such crap please.
Amit? I seem to recall you and George (and Andi?) talking about how to get rid of these bits, did you ever write any of it up?
> > @@ -317,13 +318,26 @@ > > return; > > > > sum = read_pda(apic_timer_irqs); > > - if (last_irq_sums[cpu] == sum) { > > + if (atomic_read(&debugger_active)) { > > + > > + /* > > + * The machine is in debugger, hold this cpu if already > > + * not held. > > + */ > > + debugger_nmihook(cpu, regs); > > + alert_counter[cpu] = 0; > > This should be a notify_die.
Not being firmiliar with x86_64, do you mean: /* * The machine is in debugger, hold this cpu if already * not held. */ notify_die(cpu, regs); ? Or so?
> > + } else if (last_irq_sums[cpu] == sum) { > > + > > /* > > * Ayiee, looks like this CPU is stuck ... > > * wait a few IRQs (5 seconds) before doing the oops ... > > */ > > alert_counter[cpu]++; > > if (alert_counter[cpu] == 5*nmi_hz) { > > + > > + CHK_DEBUGGER(2,SIGSEGV,0,regs,) > > + > > if (notify_die(DIE_NMI, "nmi", regs, reason, 2, SIGINT) == NOTIFY_BAD) { > > That's complete crap. You have a debugger hook and you add your own > hook one line before it. Please fix that.
Yes, Amit's version does this on PPC32 as well, and I'd like to change it, but I've been cleaning up other things.
> > --- a/include/asm-x86_64/processor.h Wed Feb 11 15:42:06 2004 > > +++ b/include/asm-x86_64/processor.h Wed Feb 11 15:42:06 2004 > > @@ -252,6 +252,7 @@ > > unsigned long *io_bitmap_ptr; > > /* cached TLS descriptors. */ > > u64 tls_array[GDT_ENTRY_TLS_ENTRIES]; > > + void *debuggerinfo; > > }; > > This should not be needed
What would be used instead to pass around the pt_regs?
> > --- a/include/asm-x86_64/system.h Wed Feb 11 15:42:06 2004 > > +++ b/include/asm-x86_64/system.h Wed Feb 11 15:42:06 2004 > > @@ -19,6 +19,56 @@ > > #define __SAVE(reg,offset) "movq %%" #reg ",(14-" #offset ")*8(%%rsp)\n\t" > > #define __RESTORE(reg,offset) "movq (14-" #offset ")*8(%%rsp),%%" #reg "\n\t" > > > > +/* #ifdef CONFIG_KGDB */ > > + > > +/* full frame for the debug stub */ > > +/* Should be replaced with a dwarf2 cie/fde description, then gdb could > > + figure it out all by itself. */ > > +struct save_context_frame { > > That's completely broken too. We have a full CFI description in the kernel > now, so this isn't needed anymore.
Part of why I'm trying to get this into -mm is so that someone who has the hw and knowledge can try and merge some of the things that the other stubs got right into the stub that every arch can use.
-- Tom Rini http://gate.crashing.org/~trini/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |