lkml.org 
[lkml]   [2004]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2005-03-22 14:00    [W:0.480 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site