[PATCH 0/4] kgdb: Small usability improvements for x86

[PATCH 0/4] kgdb: Small usability improvements for x86

[Jan Kiszka]

[resent due to wrong list CC in first round]

This cleans up the "info register" result on x86 and adds gdb detach on
reboot/shutdown for this target arch. See patches for details.

CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: x86@kernel.org

Jan Kiszka (4):
  kgdb: x86: Return all segment registers also in 64-bit mode
  kgdb: Make gdbstub_exit a nop unless gdb is attached
  kgdb: Respect that flush op is optional
  kgdb: x86: Detach gdb if machine shuts down or reboots

 arch/x86/include/asm/kgdb.h |    6 +++++-
 arch/x86/kernel/kgdb.c      |    6 ++++--
 arch/x86/kernel/reboot.c    |    6 ++++++
 include/linux/kgdb.h        |    1 +
 kernel/debug/gdbstub.c      |    6 +++++-
 5 files changed, 21 insertions(+), 4 deletions(-)

-- 
1.7.3.4

[PATCH 1/4] kgdb: x86: Return all segment registers also in 64-bit mode

[Jan Kiszka]

Even if the content is always 0, gdb expects us to return also ds,
es, fs, and gs while in x86-64 mode. Do this to avoid ugly errors on
"info registers".

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/include/asm/kgdb.h |    6 +++++-
 arch/x86/kernel/kgdb.c      |    6 ++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kgdb.h b/arch/x86/include/asm/kgdb.h
index 77e95f5..e857f1a 100644
--- a/arch/x86/include/asm/kgdb.h
+++ b/arch/x86/include/asm/kgdb.h
@@ -64,9 +64,13 @@ enum regnames {
 	GDB_PS,			/* 17 */
 	GDB_CS,			/* 18 */
 	GDB_SS,			/* 19 */
+	GDB_DS,			/* 20 */
+	GDB_ES,			/* 21 */
+	GDB_FS,			/* 22 */
+	GDB_GS,			/* 23 */
 };
 #define GDB_ORIG_AX		57
-#define DBG_MAX_REG_NUM		20
+#define DBG_MAX_REG_NUM		24
 /* 17 64 bit regs and 3 32 bit regs */
 #define NUMREGBYTES		((17 * 8) + (3 * 4))
 #endif /* ! CONFIG_X86_32 */
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index faba577..fdc37b3 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -67,8 +67,6 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] =
 	{ "ss", 4, offsetof(struct pt_regs, ss) },
 	{ "ds", 4, offsetof(struct pt_regs, ds) },
 	{ "es", 4, offsetof(struct pt_regs, es) },
-	{ "fs", 4, -1 },
-	{ "gs", 4, -1 },
 #else
 	{ "ax", 8, offsetof(struct pt_regs, ax) },
 	{ "bx", 8, offsetof(struct pt_regs, bx) },
@@ -90,7 +88,11 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] =
 	{ "flags", 4, offsetof(struct pt_regs, flags) },
 	{ "cs", 4, offsetof(struct pt_regs, cs) },
 	{ "ss", 4, offsetof(struct pt_regs, ss) },
+	{ "ds", 4, -1 },
+	{ "es", 4, -1 },
 #endif
+	{ "fs", 4, -1 },
+	{ "gs", 4, -1 },
 };
 
 int dbg_set_reg(int regno, void *mem, struct pt_regs *regs)
-- 
1.7.3.4

Re: [PATCH 1/4] kgdb: x86: Return all segment registers also in 64-bit mode

[Jason Wessel]

On 03/16/2012 07:17 AM, Jan Kiszka wrote:
> Even if the content is always 0, gdb expects us to return also ds,
> es, fs, and gs while in x86-64 mode. Do this to avoid ugly errors on
> "info registers".
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/include/asm/kgdb.h |    6 +++++-
>  arch/x86/kernel/kgdb.c      |    6 ++++--
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/kgdb.h b/arch/x86/include/asm/kgdb.h
> index 77e95f5..e857f1a 100644
> --- a/arch/x86/include/asm/kgdb.h
> +++ b/arch/x86/include/asm/kgdb.h
> @@ -64,9 +64,13 @@ enum regnames {
>  	GDB_PS,			/* 17 */
>  	GDB_CS,			/* 18 */
>  	GDB_SS,			/* 19 */
> +	GDB_DS,			/* 20 */
> +	GDB_ES,			/* 21 */
> +	GDB_FS,			/* 22 */
> +	GDB_GS,			/* 23 */
>  };
>  #define GDB_ORIG_AX		57
> -#define DBG_MAX_REG_NUM		20
> +#define DBG_MAX_REG_NUM		24
>  /* 17 64 bit regs and 3 32 bit regs */
>  #define NUMREGBYTES		((17 * 8) + (3 * 4))

You bumped the register numbers correctly, but you also need to add the correct padding to the NUMREGBYTES or you might not get a large enough memory block for the register struct.

You added 2 32 bit regs, so it should change to:

/* 17 64 bit regs and 5 32 bit regs */
#define NUMREGBYTES        ((17 * 8) + (5 * 4))


The rest should be fine.   I can test this and add it to the merge queue with the NUMREGBYTES change, unless you disagree.

Cheers,
Jason.

Re: [PATCH 1/4] kgdb: x86: Return all segment registers also in 64-bit mode

[Jan Kiszka]

On 2012-03-16 16:57, Jason Wessel wrote:
> On 03/16/2012 07:17 AM, Jan Kiszka wrote:
>> Even if the content is always 0, gdb expects us to return also ds,
>> es, fs, and gs while in x86-64 mode. Do this to avoid ugly errors on
>> "info registers".
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/include/asm/kgdb.h |    6 +++++-
>>  arch/x86/kernel/kgdb.c      |    6 ++++--
>>  2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kgdb.h b/arch/x86/include/asm/kgdb.h
>> index 77e95f5..e857f1a 100644
>> --- a/arch/x86/include/asm/kgdb.h
>> +++ b/arch/x86/include/asm/kgdb.h
>> @@ -64,9 +64,13 @@ enum regnames {
>>  	GDB_PS,			/* 17 */
>>  	GDB_CS,			/* 18 */
>>  	GDB_SS,			/* 19 */
>> +	GDB_DS,			/* 20 */
>> +	GDB_ES,			/* 21 */
>> +	GDB_FS,			/* 22 */
>> +	GDB_GS,			/* 23 */
>>  };
>>  #define GDB_ORIG_AX		57
>> -#define DBG_MAX_REG_NUM		20
>> +#define DBG_MAX_REG_NUM		24
>>  /* 17 64 bit regs and 3 32 bit regs */
>>  #define NUMREGBYTES		((17 * 8) + (3 * 4))
> 
> You bumped the register numbers correctly, but you also need to add the correct padding to the NUMREGBYTES or you might not get a large enough memory block for the register struct.
> 
> You added 2 32 bit regs, so it should change to:
> 
> /* 17 64 bit regs and 5 32 bit regs */
> #define NUMREGBYTES        ((17 * 8) + (5 * 4))
> 

Oops. Most probably explains the random gs content I saw these days in a
64-bit debugging session.

> 
> The rest should be fine.   I can test this and add it to the merge queue with the NUMREGBYTES change, unless you disagree.

I don't disagree. :)

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/4] kgdb: x86: Return all segment registers also in 64-bit mode

[Jason Wessel]

On 03/19/2012 08:52 AM, Jan Kiszka wrote:
> On 2012-03-16 16:57, Jason Wessel wrote:
> The rest should be fine.   I can test this and add it to the merge queue with the NUMREGBYTES change, unless you disagree.
> I don't disagree. :)

Sounds fine.  I added this to the merge queue for the 3.4 kernel.

Cheers,
Jason.

[PATCH 2/4] kgdb: Make gdbstub_exit a nop unless gdb is attached

[Jan Kiszka]

This allows to call gdbstub_exit without worrying if
 - CONFIG_KGDB is enabled
 - if an kgdb I/O driver is loaded
 - if a gdb frontend is currently attached

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 include/linux/kgdb.h   |    1 +
 kernel/debug/gdbstub.c |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index fa39183..16410e2 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -306,6 +306,7 @@ extern atomic_t			kgdb_active;
 extern bool dbg_is_early;
 extern void __init dbg_late_init(void);
 #else /* ! CONFIG_KGDB */
+static inline void gdbstub_exit(int status) { }
 #define in_dbg_master() (0)
 #define dbg_late_init()
 #endif /* ! CONFIG_KGDB */
diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index c22d8c2..5d7ed0a 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -1111,6 +1111,9 @@ void gdbstub_exit(int status)
 	unsigned char checksum, ch, buffer[3];
 	int loop;
 
+	if (!dbg_io_ops || !kgdb_connected)
+		return;
+
 	buffer[0] = 'W';
 	buffer[1] = hex_asc_hi(status);
 	buffer[2] = hex_asc_lo(status);
-- 
1.7.3.4

Re: [PATCH 2/4] kgdb: Make gdbstub_exit a nop unless gdb is attached

[Jason Wessel]

On 03/16/2012 07:17 AM, Jan Kiszka wrote:
> This allows to call gdbstub_exit without worrying if
> - CONFIG_KGDB is enabled
> - if an kgdb I/O driver is loaded
> - if a gdb frontend is currently attached

If you took a look at the alternate patch I sent in the 4 of 4 response, I made a modification for kdb to continue to work properly, namely disconnect and do not emit characters if we are in kdb mode.

>
> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
> index c22d8c2..5d7ed0a 100644
> --- a/kernel/debug/gdbstub.c
> +++ b/kernel/debug/gdbstub.c
> @@ -1111,6 +1111,9 @@ void gdbstub_exit(int status)
> unsigned char checksum, ch, buffer[3];
> int loop;
>
> + if (!dbg_io_ops || !kgdb_connected)
> + return;
> +
> buffer[0] = 'W';
> buffer[1] = hex_asc_hi(status);
> buffer[2] = hex_asc_lo(status);

I patched against what you originally had there.

--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -1110,8 +1110,11 @@ void gdbstub_exit(int status)
 {
        unsigned char checksum, ch, buffer[3];
        int loop;
+       if (!kgdb_connected)
+               return;
+       kgdb_connected = 0;
 
-       if (!dbg_io_ops || !kgdb_connected)
+       if (!dbg_io_ops || dbg_kdb_mode)
                return;
 
        buffer[0] = 'W';

---

Depending on what we come to agreement on with patches 2-4 in your series I'll fold the changes into the reboot notifier.

It is possible to have both the reboot notifier and the low level modifications to the arch specific reboot hooks as a final catch, but I am interested to see what you think about just using the reboot notifier.

Cheers,
Jason.

[PATCH 3/4] kgdb: Respect that flush op is optional

[Jan Kiszka]

Not all kgdb I/O drivers implement a flush operation. Adjust
gdbstub_exit accordingly.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kernel/debug/gdbstub.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index 5d7ed0a..c174ea3 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -1132,5 +1132,6 @@ void gdbstub_exit(int status)
 	dbg_io_ops->write_char(hex_asc_lo(checksum));
 
 	/* make sure the output is flushed, lest the bootloader clobber it */
-	dbg_io_ops->flush();
+	if (dbg_io_ops->flush)
+		dbg_io_ops->flush();
 }
-- 
1.7.3.4

Re: [PATCH 3/4] kgdb: Respect that flush op is optional

[Jason Wessel]

On 03/16/2012 07:17 AM, Jan Kiszka wrote:
> Not all kgdb I/O drivers implement a flush operation. Adjust
> gdbstub_exit accordingly.

The flush is certainly optional.  The reason we never saw this bug is because it was only used by some code not in the mainline, where it is in fact patched.  The out of tree patch hooked into the reboot notifier.

It makes me wonder if using the reboot notifier is enough vs the call backs you placed in the critical power off points in your patch [4/4].   I'll respond separately to that thread because there are other folks in the CC line.

If we decide to roll with this series, I will definitely merge this patch, else I will remove the code since it is not used except for out of tree patches.

Thanks,
Jason.

Re: [PATCH 3/4] kgdb: Respect that flush op is optional

[Jan Kiszka]

On 2012-03-16 13:46, Jason Wessel wrote:
> On 03/16/2012 07:17 AM, Jan Kiszka wrote:
>> Not all kgdb I/O drivers implement a flush operation. Adjust
>> gdbstub_exit accordingly.
> 
> The flush is certainly optional.  The reason we never saw this bug is because it was only used by some code not in the mainline, where it is in fact patched.  The out of tree patch hooked into the reboot notifier.
> 
> It makes me wonder if using the reboot notifier is enough vs the call backs you placed in the critical power off points in your patch [4/4].   I'll respond separately to that thread because there are other folks in the CC line.

I looked at other (granted: not kgdb-) archs and found them hooking
their machine_xxx handlers. Also, do we trigger the notifier on all
reboot/shutdown types?

> 
> If we decide to roll with this series, I will definitely merge this patch, else I will remove the code since it is not used except for out of tree patches.

Even with a reboot notifier, you still need the logic of gdbstub_exit.
So I doubt it makes sense to be removed, rather start to be used - finally.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 3/4] kgdb: Respect that flush op is optional

[Jason Wessel]

On 03/16/2012 07:53 AM, Jan Kiszka wrote:
> On 2012-03-16 13:46, Jason Wessel wrote:
>> On 03/16/2012 07:17 AM, Jan Kiszka wrote:
>>> Not all kgdb I/O drivers implement a flush operation. Adjust
>>> gdbstub_exit accordingly.
>> The flush is certainly optional.  The reason we never saw this bug is because it was only used by some code not in the mainline, where it is in fact patched.  The out of tree patch hooked into the reboot notifier.
>>
>> It makes me wonder if using the reboot notifier is enough vs the call backs you placed in the critical power off points in your patch [4/4].   I'll respond separately to that thread because there are other folks in the CC line.
> I looked at other (granted: not kgdb-) archs and found them hooking
> their machine_xxx handlers. Also, do we trigger the notifier on all
> reboot/shutdown types?

>From what I can tell, all the safe shutdown types go through notifier, however if you do something like:

echo o > /proc/sysrq-trigger

This will avoid the reboot notifier hook.  It is a question of how aggressively you want to catch all these cases or patch the kernel such that these holes provide notifications.

>> If we decide to roll with this series, I will definitely merge this patch, else I will remove the code since it is not used except for out of tree patches.
> Even with a reboot notifier, you still need the logic of gdbstub_exit.
> So I doubt it makes sense to be removed, rather start to be used - finally.

You are correct about that, and it is actually used in the reboot notifier version.

Jason.

[PATCH 4/4] kgdb: x86: Detach gdb if machine shuts down or reboots

[Jan Kiszka]

Hook into machine restart/power-off/halt handlers and call gdbstub_exit
so that a attached gdb frontend is properly informed. If kgdb is
disabled or no frontend attached, gdbstub_exit will do nothing.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kernel/reboot.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index d840e69..926ac17 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -7,6 +7,7 @@
 #include <linux/sched.h>
 #include <linux/tboot.h>
 #include <linux/delay.h>
+#include <linux/kgdb.h>
 #include <acpi/reboot.h>
 #include <asm/io.h>
 #include <asm/apic.h>
@@ -683,6 +684,7 @@ void native_machine_shutdown(void)
 static void __machine_emergency_restart(int emergency)
 {
 	reboot_emergency = emergency;
+	gdbstub_exit(1);
 	machine_ops.emergency_restart();
 }
 
@@ -730,6 +732,7 @@ struct machine_ops machine_ops = {
 
 void machine_power_off(void)
 {
+	gdbstub_exit(0);
 	machine_ops.power_off();
 }
 
@@ -745,17 +748,20 @@ void machine_emergency_restart(void)
 
 void machine_restart(char *cmd)
 {
+	gdbstub_exit(0);
 	machine_ops.restart(cmd);
 }
 
 void machine_halt(void)
 {
+	gdbstub_exit(0);
 	machine_ops.halt();
 }
 
 #ifdef CONFIG_KEXEC
 void machine_crash_shutdown(struct pt_regs *regs)
 {
+	gdbstub_exit(1);
 	machine_ops.crash_shutdown(regs);
 }
 #endif
-- 
1.7.3.4

Re: [PATCH 4/4] kgdb: x86: Detach gdb if machine shuts down or reboots

[Jason Wessel]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On 03/16/2012 07:17 AM, Jan Kiszka wrote:
> Hook into machine restart/power-off/halt handlers and call gdbstub_exit
> so that a attached gdb frontend is properly informed. If kgdb is
> disabled or no frontend attached, gdbstub_exit will do nothing.
> 


We have long had an out of tree patch which hooked the reboot notifier, vs adding call backs to the actual reboot functions.  It seems at first glance that all of cases that Jan probably cares about are actually caught by the reboot notifier.

I attached the patch I am referencing, and perhaps Jan can try it out.   It applies on top of Jan's series at the moment.

A few more comments first.

>  static void __machine_emergency_restart(int emergency)
>  {
>  	reboot_emergency = emergency;
> +	gdbstub_exit(1);


If we do have to add callbacks at the arch level, my preference is to pass the stop code like you would have received in the reboot notifier.  Specifically something like:  gdbstub_exit(SYS_RESTART).

>  	machine_ops.emergency_restart();
>  }
>  
> @@ -730,6 +732,7 @@ struct machine_ops machine_ops = {
>  
>  void machine_power_off(void)
>  {
> +	gdbstub_exit(0);


Similarly gdbstub_exit(SYS_HALT) and so on.

Jan, what do you think about trying the reboot notifier version?

Thanks,
Jason.

[-- Attachment #2: kgdb-detach-on-reboot-notify.patch --]
[-- Type: text/x-diff, Size: 2320 bytes --]

From: Jason Wessel <jason.wessel@windriver.com>
Subject: [PATCH] kgdb,debug-core,gdbstub: Hook the reboot notifier for debugger detach

The gdbstub and kdb should get detached if the system is rebooting.
Calling gdbstub_exit() will set the proper debug core state and send a
message to any debugger that is connected to correctly detach.

An attached debugger will receive the exit code from
include/linux/reboot.h based on SYS_HALT, SYS_REBOOT, etc...

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 kernel/debug/debug_core.c |   17 +++++++++++++++++
 kernel/debug/gdbstub.c    |    5 ++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -41,6 +41,7 @@
 #include <linux/delay.h>
 #include <linux/sched.h>
 #include <linux/sysrq.h>
+#include <linux/reboot.h>
 #include <linux/init.h>
 #include <linux/kgdb.h>
 #include <linux/kdb.h>
@@ -784,6 +785,20 @@ void __init dbg_late_init(void)
 	kdb_init(KDB_INIT_FULL);
 }
 
+static int
+dbg_notify_reboot(struct notifier_block *this, unsigned long code, void *x)
+{
+	if (!dbg_kdb_mode)
+		gdbstub_exit(code);
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block dbg_reboot_notifier = {
+	.notifier_call		= dbg_notify_reboot,
+	.next			= NULL,
+	.priority		= INT_MAX,
+};
+
 static void kgdb_register_callbacks(void)
 {
 	if (!kgdb_io_module_registered) {
@@ -791,6 +806,7 @@ static void kgdb_register_callbacks(void
 		kgdb_arch_init();
 		if (!dbg_is_early)
 			kgdb_arch_late();
+		register_reboot_notifier(&dbg_reboot_notifier);
 		atomic_notifier_chain_register(&panic_notifier_list,
 					       &kgdb_panic_event_nb);
 #ifdef CONFIG_MAGIC_SYSRQ
@@ -812,6 +828,7 @@ static void kgdb_unregister_callbacks(vo
 	 */
 	if (kgdb_io_module_registered) {
 		kgdb_io_module_registered = 0;
+		unregister_reboot_notifier(&dbg_reboot_notifier);
 		atomic_notifier_chain_unregister(&panic_notifier_list,
 					       &kgdb_panic_event_nb);
 		kgdb_arch_exit();
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -1110,8 +1110,11 @@ void gdbstub_exit(int status)
 {
 	unsigned char checksum, ch, buffer[3];
 	int loop;
+	if (!kgdb_connected)
+		return;
+	kgdb_connected = 0;
 
-	if (!dbg_io_ops || !kgdb_connected)
+	if (!dbg_io_ops || dbg_kdb_mode)
 		return;
 
 	buffer[0] = 'W';

Re: [PATCH 4/4] kgdb: x86: Detach gdb if machine shuts down or reboots

[Jan Kiszka]

On 2012-03-16 16:36, Jason Wessel wrote:
> On 03/16/2012 07:17 AM, Jan Kiszka wrote:
>> Hook into machine restart/power-off/halt handlers and call gdbstub_exit
>> so that a attached gdb frontend is properly informed. If kgdb is
>> disabled or no frontend attached, gdbstub_exit will do nothing.
>>
> 
> 
> We have long had an out of tree patch which hooked the reboot notifier, vs adding call backs to the actual reboot functions.  It seems at first glance that all of cases that Jan probably cares about are actually caught by the reboot notifier.
> 
> I attached the patch I am referencing, and perhaps Jan can try it out.   It applies on top of Jan's series at the moment.
> 
> A few more comments first.
> 
>>  static void __machine_emergency_restart(int emergency)
>>  {
>>  	reboot_emergency = emergency;
>> +	gdbstub_exit(1);
> 
> 
> If we do have to add callbacks at the arch level, my preference is to pass the stop code like you would have received in the reboot notifier.  Specifically something like:  gdbstub_exit(SYS_RESTART).

Yes, that sounds reasonable.

> 
>>  	machine_ops.emergency_restart();
>>  }
>>  
>> @@ -730,6 +732,7 @@ struct machine_ops machine_ops = {
>>  
>>  void machine_power_off(void)
>>  {
>> +	gdbstub_exit(0);
> 
> 
> Similarly gdbstub_exit(SYS_HALT) and so on.
> 
> Jan, what do you think about trying the reboot notifier version?

An arch-independent hook has its advantages, no question. Maybe we
should just start this way and then evolved on top as needed.

I was wondering why those other archs do not use the notifier. Maybe one
motivation is to avoid that too much code is excluded from the debugger
by detaching too early. Could possibly be addressed by making
detach-on-reboot runtime configurable.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 4/4] kgdb: x86: Detach gdb if machine shuts down or reboots

[Jason Wessel]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

On 03/19/2012 08:49 AM, Jan Kiszka wrote:
> 
> An arch-independent hook has its advantages, no question. Maybe we
> should just start this way and then evolved on top as needed.
> 

Sounds good to me.  I added the reboot notifier patch into the merge queue for kgdb/kdb.

> I was wondering why those other archs do not use the notifier. Maybe one
> motivation is to avoid that too much code is excluded from the debugger
> by detaching too early. Could possibly be addressed by making
> detach-on-reboot runtime configurable.

There really isn't a whole lot of code between reboot hook invoked from kernel_restart_prepare(), and the place where the reset is invoked.  Specifically it looked like:

        usermodehelper_disable();
        device_shutdown();
        syscore_shutdown();
        machine_restart(cmd);

Attached is the patch you asked for the conditional behavior with respect to the reboot hook.  This also implements the ability to stop on a reboot that is not a panic().

Cheers,
Jason.

[-- Attachment #2: 0001-kgdb-debug_core-add-the-ability-to-control-the-reboo.patch --]
[-- Type: text/x-diff, Size: 3494 bytes --]

>From 7bd0d272730f276b996110d8777d8deacbcb3d15 Mon Sep 17 00:00:00 2001
From: Jason Wessel <jason.wessel@windriver.com>
Date: Mon, 19 Mar 2012 19:35:55 -0500
Subject: [PATCH] kgdb,debug_core: add the ability to control the reboot
 notifier

Sometimes it is desirable to stop the kernel debugger before allowing
a system to reboot either with kdb or kgdb.  This patch adds the
ability to turn the reboot notifier on and off or enter the debugger
and stop kernel execution before rebooting.

It is possible to change the setting after booting the kernel with the
following:

echo 1 > /sys/module/debug_core/parameters/kgdbreboot

It is also possible to change this setting using kdb / kgdb to
manipulate the variable directly.

Using KDB:
   mm kgdbreboot 1

Using gdb:
   set kgdbreboot=1

Reported-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 Documentation/DocBook/kgdb.tmpl |   17 +++++++++++++++++
 kernel/debug/debug_core.c       |   16 ++++++++++++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/Documentation/DocBook/kgdb.tmpl b/Documentation/DocBook/kgdb.tmpl
index d71b57f..4ee4ba3 100644
--- a/Documentation/DocBook/kgdb.tmpl
+++ b/Documentation/DocBook/kgdb.tmpl
@@ -362,6 +362,23 @@
    </para>
   </para>
   </sect1>
+   <sect1 id="kgdbreboot">
+   <title>Run time parameter: kgdbreboot</title>
+   <para> The kgdbreboot feature allows you to change how the debugger
+   deals with the reboot notification.  You have 3 choices for the
+   behavior.  The default behavior is always set to 0.</para>
+   <orderedlist>
+   <listitem><para>echo -1 > /sys/module/debug_core/parameters/kgdbreboot</para>
+   <para>Ignore the reboot notification entirely.</para>
+   </listitem>
+   <listitem><para>echo 0 > /sys/module/debug_core/parameters/kgdbreboot</para>
+   <para>Send the detach message to any attached debugger client.</para>
+   </listitem>
+   <listitem><para>echo 1 > /sys/module/debug_core/parameters/kgdbreboot</para>
+   <para>Enter the debugger on reboot notify.</para>
+   </listitem>
+   </orderedlist>
+  </sect1>
   </chapter>
   <chapter id="usingKDB">
   <title>Using kdb</title>
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 3c1ad4e..3f88a45 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -76,6 +76,8 @@ static int			exception_level;
 struct kgdb_io		*dbg_io_ops;
 static DEFINE_SPINLOCK(kgdb_registration_lock);
 
+/* Action for the reboot notifiter, a global allow kdb to change it */
+static int kgdbreboot;
 /* kgdb console driver is loaded */
 static int kgdb_con_registered;
 /* determine if kgdb console output should be used */
@@ -97,6 +99,7 @@ static int __init opt_kgdb_con(char *str)
 early_param("kgdbcon", opt_kgdb_con);
 
 module_param(kgdb_use_con, int, 0644);
+module_param(kgdbreboot, int, 0644);
 
 /*
  * Holds information about breakpoints in a kernel. These breakpoints are
@@ -788,8 +791,21 @@ void __init dbg_late_init(void)
 static int
 dbg_notify_reboot(struct notifier_block *this, unsigned long code, void *x)
 {
+	/*
+	 * Take the following action on reboot notify depending on value:
+	 *    1 == Enter debugger
+	 *    0 == [the default] detatch debug client
+	 *   -1 == Do nothing... and use this until the board resets
+	 */
+	switch (kgdbreboot) {
+	case 1:
+		kgdb_breakpoint();
+	case -1:
+		goto done;
+	}
 	if (!dbg_kdb_mode)
 		gdbstub_exit(code);
+done:
 	return NOTIFY_DONE;
 }
 
-- 
1.7.5.4

Re: [Kgdb-bugreport] [PATCH 4/4] kgdb: x86: Detach gdb if machine shuts down or reboots

[Sergei Shtylyov]

Hello.

On 20-03-2012 5:00, Jason Wessel wrote:

>> An arch-independent hook has its advantages, no question. Maybe we
>> should just start this way and then evolved on top as needed.

> Sounds good to me.  I added the reboot notifier patch into the merge queue for kgdb/kdb.

>> I was wondering why those other archs do not use the notifier. Maybe one
>> motivation is to avoid that too much code is excluded from the debugger
>> by detaching too early. Could possibly be addressed by making
>> detach-on-reboot runtime configurable.

> There really isn't a whole lot of code between reboot hook invoked from kernel_restart_prepare(),
> and the place where the reset is invoked.  Specifically it looked like:

>          usermodehelper_disable();
>          device_shutdown();
>          syscore_shutdown();
>          machine_restart(cmd);

> Attached is the patch you asked for the conditional behavior with respect to the reboot hook.

    You forgot to attach it?

> This also implements the ability to stop on a reboot that is not a panic().

WBR, Sergei

Re: [Kgdb-bugreport] [PATCH 4/4] kgdb: x86: Detach gdb if machine shuts down or reboots

[Jason Wessel]

On 03/20/2012 06:18 AM, Sergei Shtylyov wrote:
> Hello.
>
> On 20-03-2012 5:00, Jason Wessel wrote:
>
>>> An arch-independent hook has its advantages, no question. Maybe we
>>> should just start this way and then evolved on top as needed.
>> Sounds good to me.  I added the reboot notifier patch into the merge queue for kgdb/kdb.
>>> I was wondering why those other archs do not use the notifier. Maybe one
>>> motivation is to avoid that too much code is excluded from the debugger
>>> by detaching too early. Could possibly be addressed by making
>>> detach-on-reboot runtime configurable.
>> There really isn't a whole lot of code between reboot hook invoked from kernel_restart_prepare(),
>> and the place where the reset is invoked.  Specifically it looked like:
>>          usermodehelper_disable();
>>          device_shutdown();
>>          syscore_shutdown();
>>          machine_restart(cmd);
>> Attached is the patch you asked for the conditional behavior with respect to the reboot hook.
>     You forgot to attach it?

Source Forge [kgdb mailling list] is eating attachments.  I had definitely attached it.  See: 

https://lkml.org/lkml/2012/3/19/735

I also included this patch in the potential merge queue as patch 5 of 5, sent to the kgdb mailing list.

http://sourceforge.net/mailarchive/message.php?msg_id=29008292

Jason.


Jason.

[PATCH 0/4] kgdb: Small usability improvements for x86

[Jan Kiszka]

This cleans up the "info register" result on x86 and adds gdb detach on
reboot/shutdown for this target arch. See patches for details.

CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: x86@kernel.org

Jan Kiszka (4):
  kgdb: x86: Return all segment registers also in 64-bit mode
  kgdb: Make gdbstub_exit a nop unless gdb is attached
  kgdb: Respect that flush op is optional
  kgdb: x86: Detach gdb if machine shuts down or reboots

 arch/x86/include/asm/kgdb.h |    6 +++++-
 arch/x86/kernel/kgdb.c      |    6 ++++--
 arch/x86/kernel/reboot.c    |    6 ++++++
 include/linux/kgdb.h        |    1 +
 kernel/debug/gdbstub.c      |    6 +++++-
 5 files changed, 21 insertions(+), 4 deletions(-)

-- 
1.7.3.4

[PATCH 4/4] kgdb: x86: Detach gdb if machine shuts down or reboots

[Jan Kiszka]

Hook into machine restart/power-off/halt handlers and call gdbstub_exit
so that a attached gdb frontend is properly informed. If kgdb is
disabled or no frontend attached, gdbstub_exit will do nothing.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: x86@kernel.org
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kernel/reboot.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index d840e69..926ac17 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -7,6 +7,7 @@
 #include <linux/sched.h>
 #include <linux/tboot.h>
 #include <linux/delay.h>
+#include <linux/kgdb.h>
 #include <acpi/reboot.h>
 #include <asm/io.h>
 #include <asm/apic.h>
@@ -683,6 +684,7 @@ void native_machine_shutdown(void)
 static void __machine_emergency_restart(int emergency)
 {
 	reboot_emergency = emergency;
+	gdbstub_exit(1);
 	machine_ops.emergency_restart();
 }
 
@@ -730,6 +732,7 @@ struct machine_ops machine_ops = {
 
 void machine_power_off(void)
 {
+	gdbstub_exit(0);
 	machine_ops.power_off();
 }
 
@@ -745,17 +748,20 @@ void machine_emergency_restart(void)
 
 void machine_restart(char *cmd)
 {
+	gdbstub_exit(0);
 	machine_ops.restart(cmd);
 }
 
 void machine_halt(void)
 {
+	gdbstub_exit(0);
 	machine_ops.halt();
 }
 
 #ifdef CONFIG_KEXEC
 void machine_crash_shutdown(struct pt_regs *regs)
 {
+	gdbstub_exit(1);
 	machine_ops.crash_shutdown(regs);
 }
 #endif
-- 
1.7.3.4