KGDB: improve early init

PATCH In review Jan Kiszka 2 participants 5 messages Jan 29, 2008 – Jan 31, 2008

Subsystems (2)

x86 KGDB / KDB /debug_core

lore

Share & tools

lore

Jan Kiszka

author

2008-01-29 23:44 UTC #0 lore raw

PATCH 1/5 KGDB: improve early init

[Warning in advance]
This patch reintroduces the early KGDB_CONSOLE registration issue: If you switch this on, trigger the debugger before any tty-providing console was registered and you do not provide an explicit console= parameter, you end up with a panic due to lacking /dev/console. But this time I have a better fix (for register_console), will be posted separately later on.

<--snip-->

In case "kgdbwait" is passed as kernel parameter, KGDB tries to set up and connect to the front-end already during early_param evaluation. This fails on x86 as the exception stack is not yet initialized, effectively delaying kgdbwait until late-init.

Therefore, this patch hooks into the x86 trap initialization and re-triggers the KGDB setup, including a potential early rendezvous with gdb. As a precondition, KGDB's setup states are refactored once again to allow multiple invocations of kgdb_early_entry and correct tracking of pending kgdbwait requests.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/x86/kernel/traps_32.c |    4 +++
 arch/x86/kernel/traps_64.c |    4 +++
 include/linux/kgdb.h       |    7 ++++-
 kernel/kgdb.c              |   55 ++++++++++++++++++++++++---------------------
 4 files changed, 44 insertions(+), 26 deletions(-)

arch/x86/kernel/traps_32.c +4 -0

Index: b/arch/x86/kernel/traps_32.c
===================================================================
@@ -25,6 +25,7 @@
 #include <linux/utsname.h>
 #include <linux/kprobes.h>
 #include <linux/kexec.h>
+#include <linux/kgdb.h>
 #include <linux/unwind.h>
 #include <linux/uaccess.h>
 #include <linux/nmi.h>

@@ -1215,6 +1216,9 @@ void __init trap_init(void)
 	 */
 	cpu_init();
 
+	/* With the TSS set up, it's now save to arm early KGDB. */
+	kgdb_early_entry();
+
 	trap_init_hook();
 }

arch/x86/kernel/traps_64.c +4 -0

Index: b/arch/x86/kernel/traps_64.c
===================================================================
@@ -27,6 +27,7 @@
 #include <linux/nmi.h>
 #include <linux/kprobes.h>
 #include <linux/kexec.h>
+#include <linux/kgdb.h>
 #include <linux/unwind.h>
 #include <linux/uaccess.h>
 #include <linux/bug.h>

@@ -1162,6 +1163,9 @@ void __init trap_init(void)
 	 * Should be a barrier for any external CPU state.
 	 */
 	cpu_init();
+
+	/* With the TSS set up, it's now save to arm early KGDB. */
+	kgdb_early_entry();
 }

include/linux/kgdb.h +6 -1

Index: b/include/linux/kgdb.h
===================================================================
@@ -46,7 +46,8 @@ extern struct task_struct *kgdb_usethrea
 
 enum kgdb_initstate {
 	KGDB_UNINITIALIZED = 0,
-	KGDB_SEMI_INITIALIZED,
+	KGDB_ARCH_INITIALIZED,
+	KGDB_DELAYED_CONNECTION,
 	KGDB_FULLY_INITIALIZED
 };

@@ -286,11 +287,15 @@ int kgdb_handle_exception(int ex_vector,
 			  struct pt_regs *regs);
 int kgdb_nmihook(int cpu, void *regs);
 
+void __init kgdb_early_entry(void);
+
 extern int debugger_step;
 extern atomic_t debugger_active;
 
 #else /* !CONFIG_KGDB */
 static const atomic_t debugger_active = ATOMIC_INIT(0);
+
+static inline void kgdb_early_entry(void) { }
 #endif /* !CONFIG_KGDB */
 
 #endif /* _KGDB_H_ */

kernel/kgdb.c +30 ~~-25~~

121 lines not on this page · raw message

Jan Kiszka

author

2008-01-30 23:08 UTC #1 lore raw

[Here comes a rebased version against latest x86/mm]

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/x86/kernel/traps_32.c |    4 +++
 arch/x86/kernel/traps_64.c |    4 +++
 include/linux/kgdb.h       |    7 +++++-
 kernel/kgdb.c              |   52 +++++++++++++++++++++++++--------------------
 4 files changed, 44 insertions(+), 23 deletions(-)

arch/x86/kernel/traps_32.c +4 -0

Index: b/arch/x86/kernel/traps_32.c
===================================================================
@@ -25,6 +25,7 @@
 #include <linux/utsname.h>
 #include <linux/kprobes.h>
 #include <linux/kexec.h>
+#include <linux/kgdb.h>
 #include <linux/unwind.h>
 #include <linux/uaccess.h>
 #include <linux/nmi.h>

@@ -1215,6 +1216,9 @@ void __init trap_init(void)
 	 */
 	cpu_init();
 
+	/* With the TSS set up, it's now save to arm early KGDB. */
+	kgdb_early_entry();
+
 	trap_init_hook();
 }

arch/x86/kernel/traps_64.c +4 -0

Index: b/arch/x86/kernel/traps_64.c
===================================================================
@@ -27,6 +27,7 @@
 #include <linux/nmi.h>
 #include <linux/kprobes.h>
 #include <linux/kexec.h>
+#include <linux/kgdb.h>
 #include <linux/unwind.h>
 #include <linux/uaccess.h>
 #include <linux/bug.h>

@@ -1162,6 +1163,9 @@ void __init trap_init(void)
 	 * Should be a barrier for any external CPU state.
 	 */
 	cpu_init();
+
+	/* With the TSS set up, it's now save to arm early KGDB. */
+	kgdb_early_entry();
 }

include/linux/kgdb.h +6 -1

Index: b/include/linux/kgdb.h
===================================================================
@@ -43,7 +43,8 @@ extern struct task_struct	*kgdb_contthre
 
 enum kgdb_initstate {
 	KGDB_UNINITIALIZED = 0,
-	KGDB_SEMI_INITIALIZED,
+	KGDB_ARCH_INITIALIZED,
+	KGDB_DELAYED_CONNECTION,
 	KGDB_FULLY_INITIALIZED
 };

@@ -287,6 +288,8 @@ int kgdb_handle_exception(int ex_vector,
 			  struct pt_regs *regs);
 int kgdb_nmihook(int cpu, void *regs);
 
+void __init kgdb_early_entry(void);
+
 extern int		debugger_step;
 extern atomic_t		debugger_active;

@@ -296,6 +299,8 @@ extern atomic_t		debugger_active;
 
 #else /* !CONFIG_KGDB */
 static const atomic_t	debugger_active = ATOMIC_INIT(0);
+
+static inline void kgdb_early_entry(void) { }
 #endif /* !CONFIG_KGDB */
 
 #endif /* _KGDB_H_ */

kernel/kgdb.c +30 ~~-22~~

114 lines not on this page · raw message

George Anzinger

2008-01-31 14:30 UTC #2 lore raw

On 01/31/2008 01:36 AM, Jan Kiszka was caught saying:

Quoted 11 lines

Quoted 10 lines · Jan Kiszka
Quoted 9 lines · George Anzinger
On 01/30/2008 04:08 PM, Jan Kiszka was caught saying:
Quoted 7 lines
[Here comes a rebased version against latest x86/mm]
In case "kgdbwait" is passed as kernel parameter, KGDB tries to set up and connect to the front-end already during early_param evaluation. This
fails on x86 as the exception stack is not yet initialized, effectively delaying kgdbwait until late-init.

Quoted 8 lines

Quoted 8 lines
Quoted 3 lines
I wonder how much work it would take to just set up the exception stack and proceed. After all the kgbdwait is there to help debug very early kernel code...
In principle a valid question, but I'm not the one to answer it. I would not feel very well if I had to reorder this critical setup code. Look, we would have to move trap_init in start_kernel before parse_early_param, and that would affect _every_ arch...

I can not speak to other archs, but for x86 I called trap_init from the code that caught the kgdbwait. At that time (since I retired, I have not looked at the actual kernel code) it could be called again later by the kernel code. I.e. I did not try to reorder the kernel bring up code, but just added an additional call to trap_init and then only in the case of finding a kgdbwait.

As such, this would need to be arch specific...

Quoted 8 lines

Quoted 1 line
BTW, do you know if EXCEPTION_STACK_READY fails for other archs in parse_early_param as well? It should, because my under standing of trap_init is that it's the functions to arm things like... exception handlers? And that raises the question of the deeper purpose of this check (and the invocation of kgdb_early_init from the argument parsing function). Sigh, KGDB is still a quite improvable piece of code.

Likely. Once you get it in the main line kernel, one would hope that other arch code would be forth coming as many more "eyes" will be in play.

Quoted 4 lines

Jan
PS: Can we move this to some public list?

Sure, sorry I picked the wrong reply button, never intended it to be private.

Quoted 1 line

Signature · 1 line

George Anzinger   [email protected]

Jan Kiszka

author

2008-01-31 14:42 UTC #3 lore raw

Quoted 40 lines · George Anzinger

On 01/31/2008 01:36 AM, Jan Kiszka was caught saying:
Quoted 9 lines
Quoted 8 lines · Jan Kiszka
Quoted 7 lines · George Anzinger
On 01/30/2008 04:08 PM, Jan Kiszka was caught saying:
Quoted 6 lines
[Here comes a rebased version against latest x86/mm]
In case "kgdbwait" is passed as kernel parameter, KGDB tries to set up and connect to the front-end already during early_param evaluation. This
fails on x86 as the exception stack is not yet initialized,
effectively
Quoted 10 lines
Quoted 10 lines
Quoted 5 lines
Quoted 1 line
delaying kgdbwait until late-init.
I wonder how much work it would take to just set up the exception stack and proceed. After all the kgbdwait is there to help debug very early kernel code...
In principle a valid question, but I'm not the one to answer it. I would not feel very well if I had to reorder this critical setup code. Look, we would have to move trap_init in start_kernel before parse_early_param, and that would affect _every_ arch...
I can not speak to other archs, but for x86 I called trap_init from the code that caught the kgdbwait. At that time (since I retired, I have not looked at the actual kernel code) it could be called again later by the kernel code. I.e. I did not try to reorder the kernel bring up code, but just added an additional call to trap_init and then only in the case of finding a kgdbwait.
As such, this would need to be arch specific...
Quoted 8 lines
Quoted 1 line
BTW, do you know if EXCEPTION_STACK_READY fails for other archs in parse_early_param as well? It should, because my under standing of trap_init is that it's the functions to arm things like... exception handlers? And that raises the question of the deeper purpose of this check (and the invocation of kgdb_early_init from the argument parsing function). Sigh, KGDB is still a quite improvable piece of code.

Quoted 2 lines

Likely. Once you get it in the main line kernel, one would hope that other arch code would be forth coming as many more "eyes" will be in play.

Meanwhile I realized that there is early_trap_init - for x86-32 only! I assume now we are only lacking the same for x86-64 to get kgdb running there already during early_param-parsing.

Jan

Jan Kiszka

author

2008-01-31 23:06 UTC #4 lore raw

Quoted 8 lines · Jan Kiszka

Quoted 7 lines · George Anzinger
On 01/31/2008 01:36 AM, Jan Kiszka was caught saying:
Quoted 6 lines
BTW, do you know if EXCEPTION_STACK_READY fails for other archs in parse_early_param as well? It should, because my under standing of trap_init is that it's the functions to arm things like... exception handlers? And that raises the question of the deeper purpose of this check (and the invocation of kgdb_early_init from the argument parsing function). Sigh, KGDB is still a quite improvable piece of code.

Quoted 6 lines

Quoted 2 lines
Likely. Once you get it in the main line kernel, one would hope that other arch code would be forth coming as many more "eyes" will be in play.
Meanwhile I realized that there is early_trap_init - for x86-32 only! I assume now we are only lacking the same for x86-64 to get kgdb running there already during early_param-parsing.

Looks like that was the key. Thanks for pointing me at this, George. Here the updated patch:

------snip-------

This cleans up the early entry of kgdb. It introduces early_trap_init for x86-64, reloads the idt register also in the 32-bit variant, removes the now unneeded EXCEPTION_STACK_READY construction, and matures the init-state machine of kgdb.

Signed-off-by Jan Kiszka <[email protected]>

---
 arch/x86/kernel/setup_64.c |    4 +++
 arch/x86/kernel/traps_32.c |    3 +-
 arch/x86/kernel/traps_64.c |   10 ++++++++-
 include/asm-x86/kgdb.h     |    3 --
 include/linux/kgdb.h       |    7 +-----
 kernel/kgdb.c              |   47 +++++++++++++++++++--------------------------
 6 files changed, 37 insertions(+), 37 deletions(-)

arch/x86/kernel/traps_32.c +2 -1

Index: b/arch/x86/kernel/traps_32.c
===================================================================
@@ -1137,12 +1137,13 @@ asmlinkage void math_emulate(long arg)
 
 #endif /* CONFIG_MATH_EMULATION */
 
-/* Some traps need to be set early. */
+/* Set of traps needed for early debugging. */
 void __init early_trap_init(void)
 {
 	set_intr_gate(1, &debug);
 	set_system_intr_gate(3, &int3); /* int3 can be called from all */
 	set_intr_gate(14, &page_fault);
+	load_idt(&idt_descr);
 }
 
 void __init trap_init(void)

arch/x86/kernel/traps_64.c +9 -1

Index: b/arch/x86/kernel/traps_64.c
===================================================================
@@ -1129,6 +1129,15 @@ asmlinkage void math_state_restore(void)
 }
 EXPORT_SYMBOL_GPL(math_state_restore);
 
+/* Set of traps needed for early debugging. */
+void __init early_trap_init(void)
+{
+	set_intr_gate(1, &debug);
+	set_intr_gate(3, &int3);
+	set_intr_gate(14, &page_fault);
+	load_idt((const struct desc_ptr *)&idt_descr);
+}
+
 void __init trap_init(void)
 {
 	set_intr_gate(0,&divide_error);

@@ -1145,7 +1154,6 @@ void __init trap_init(void)
 	set_intr_gate(11,&segment_not_present);
 	set_intr_gate_ist(12,&stack_segment,STACKFAULT_STACK);
 	set_intr_gate(13,&general_protection);
-	set_intr_gate(14,&page_fault);
 	set_intr_gate(15,&spurious_interrupt_bug);
 	set_intr_gate(16,&coprocessor_error);
 	set_intr_gate(17,&alignment_check);

include/linux/kgdb.h +2 -5

Index: b/include/linux/kgdb.h
===================================================================
@@ -43,7 +43,8 @@ extern struct task_struct	*kgdb_contthre
 
 enum kgdb_initstate {
 	KGDB_UNINITIALIZED = 0,
-	KGDB_SEMI_INITIALIZED,
+	KGDB_ARCH_INITIALIZED,
+	KGDB_DELAYED_CONNECTION,
 	KGDB_FULLY_INITIALIZED
 };

@@ -290,10 +291,6 @@ int kgdb_nmihook(int cpu, void *regs);
 extern int		debugger_step;
 extern atomic_t		debugger_active;
 
-#ifndef EXCEPTION_STACK_READY
-# define EXCEPTION_STACK_READY()	1
-#endif
-
 #else /* !CONFIG_KGDB */
 static const atomic_t	debugger_active = ATOMIC_INIT(0);
 #endif /* !CONFIG_KGDB */

kernel/kgdb.c +20 ~~-27~~

96 lines not on this page · raw message

arch/x86/kernel/setup_64.c +4 -0

Index: b/arch/x86/kernel/setup_64.c
===================================================================
@@ -92,6 +92,8 @@ unsigned long saved_video_mode;
 
 int force_mwait __cpuinitdata;
 
+void early_trap_init(void);
+
 /*
  * Early DMI memory
  */

@@ -264,6 +266,8 @@ void __init setup_arch(char **cmdline_p)
 {
 	unsigned i;
 
+	early_trap_init();
+
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 
 	ROOT_DEV = old_decode_dev(boot_params.hdr.root_dev);

include/asm-x86/kgdb.h +0 -3

Index: b/include/asm-x86/kgdb.h
===================================================================
@@ -76,9 +76,6 @@ enum regnames { _AX,	/* 0 */
 
 #ifndef __ASSEMBLY__
 #define BREAKPOINT()		asm("   int $3");
-#ifndef CONFIG_X86_32
-#define EXCEPTION_STACK_READY() ((&__get_cpu_var(init_tss))[0].x86_tss.ist[0])
-#endif /* ! CONFIG_X86_32 */
 #define BREAK_INSTR_SIZE	1
 #define CACHE_FLUSH_IS_SAFE	1
 #endif				/* !__ASSEMBLY__ */