kgdb: Small usability improvements for x86

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 <[email protected]>

---
 arch/x86/include/asm/kgdb.h |    6 +++++-
 arch/x86/kernel/kgdb.c      |    6 ++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

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

index 77e95f5..e857f1a 100644
@@ -64,9 +64,13 @@ enum regnames {
 	GDB_PS,			/* 17 */
 	GDB_CS,			/* 18 */
 	GDB_SS,			/* 19 */

Quoted 18 lines · 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 <[email protected]>

---
 arch/x86/include/asm/kgdb.h |    6 +++++-
 arch/x86/kernel/kgdb.c      |    6 ++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

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

index 77e95f5..e857f1a 100644
@@ -64,9 +64,13 @@ enum regnames {
 	GDB_PS,			/* 17 */
 	GDB_CS,			/* 18 */
 	GDB_SS,			/* 19 */

arch/x86/include/asm/kgdb.h enum regnames { · line 75

Quoted 10 lines

arch/x86/include/asm/kgdb.h +5 -1

+	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.

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. :)

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

kernel/debug/gdbstub.c +0 -0

index c22d8c2..5d7ed0a 100644

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

Quoted 2 lines · Jan Kiszka

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.

Quoted 3 lines · Jason Wessel

Quoted 2 lines · Jan Kiszka

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

Quoted 3 lines

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:

Quoted 1 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.

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.

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.

Quoted 4 lines · 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.

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.

Review

Quoted 4 lines

static void __machine_emergency_restart(int emergency) {

(quoted diff) +1 -0

 	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).

Quoted 6 lines

(quoted diff) +0 -0

 	machine_ops.emergency_restart();
 }

@@ -730,6 +732,7 @@ struct machine_ops machine_ops = {
 

Review struct machine_ops machine_ops = { · line 735

Quoted 3 lines

(quoted diff) +1 -0

 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.

From 7bd0d272730f276b996110d8777d8deacbcb3d15 Mon Sep 17 00:00:00 2001

Quoted 2 lines

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.

Quoted 4 lines

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().

Hello.

Quoted 7 lines · Jason Wessel

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 7 lines

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?