this is the "kgdb light" tree that has been also posted at:
   http://lkml.org/lkml/2008/2/9/236
it is available at:
   git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git
See the shortlog below.
various iterations of this have also been included in x86.git for the

past 3 months.
This is a slimmed-down and cleaned up version of KGDB that i've created

out of the original patches that we submitted two weeks ago. I went over

the kgdb patches with Thomas and we cut out everything that we did not

like, and cleaned up the result.
KGDB is still just as functional as it was before (i tested it on 32-bit

and 64-bit x86) - and any desired extra capability or complexity should

be added as a delta improvement, not in this initial merge.
The difference between the original kgdb submission and this submission

is best visible in the diffstat:
 before:     41 files changed, 4007 insertions(+), 33 deletions(-)

 after:      22 files changed, 3448 insertions(+),  2 deletions(-)
what got removed:
 - removed _all_ critical path impact, even if KGDB is enabled and

   active. The only notifier list it is registered in is the die

   notifiers, but even there it has the minimum priority of -INT_MAX, to

   be called as the last one of the die notifiers. I removed the 'early

   trap hook', the trap handler tweaks, everything. KGDB's only impact

   now are the arch details it implements in arch/x86/kernel/kgdb.c,

   nothing else.
 - removed all the lowlevel serial drivers. KGDB should not be in the

   business of writing special-purpose Linux drivers. In fact i found a

   testsystem where the KGDB 8250 driver would not work - it's simply

   reimplementing the wheel that drivers/serial already implements, and

   poorly so. Any "early debugging" functionality should be done via

   extending the early-console concept, not via special-purpose KGDB

   drivers.
 - I have added a redesigned and cleaned up version of the "KGDB over

   polled consoles" app...
[ message continues ]

Hi Ingo.
A few comments based on the git tree pulled as of

~11:00.
We have following header files in the core part:
linux/kgdb.h

asm-generic/kgdb.h
I would expect linux/kgdb.h to contain all the common definitions

needed by an arch.

And asm-generic/kgdb.h to list everything that the arch needs to

provide to support kgdb. + anything that rely on the arch

definitions provided by the arch.
Yet linux/kgdb.h says:

+/*

+ * Functions each KGDB-supporting architecture must provide:

+ */
So I am obviously wrong here. What triggered the division

in the two header files?
Some minor specific comments:

+/**

+ *	kgdb_skipexception - Bail of of KGDB when we've been triggered.

+ *	@exception: Exception vector number

+ *	@regs: Current &struct pt_regs.

+ *

+ *	On some architectures we need to skip a breakpoint exception when

+ *	it occurs after a breakpoint has been removed.

+ */

+int kgdb_skipexception(int exception, struct pt_regs *regs);

+

+/**

+ *	kgdb_post_master_code - Save error vector/code numbers.

+ *	@regs: Original pt_regs.

+ *	@e_vector: Original error vector.

+ *	@err_code: Original error code.

+ *

+ *	This is needed on architectures which support SMP and KGDB.

+ *	This function is called after all the slave cpus have been put

+ *	to a know spin state and the master CPU has control over KGDB.

+ */

+extern void kgdb_post_master_code(struct pt_regs *regs, int e_vector,

+				  int err_code);

+
Please be consistent in use of extern.

Either drop it all over or use it all over.
+#ifndef _KGDB_H_

+#define _KGDB_H_

+

+#include <asm/atomic.h>

+

+#ifdef CONFIG_KGDB

+

+#include <linux/serial_8250.h>

+#include <linux/linkage.h>

+#include <linux/init.h>

+

+#include <asm/kgdb.h>
I do not really see the point of the CONFIG_KGDB ifdef.

And if we want it then why not above the inclusion

of atomic.h?
Looking a bit closer it looks like all content from asm-generic/kgdb.h

is included in linux/kgdb.h - correct?

I assume t...
[ message continues ]

Thanks Sam for all your comments - i've implemented all of your

suggestions (see below for the fine details and shortlog and patches,

etc.). I've backmerged all the fixlets to keep the splitup clean, merged

the tree to latest -git, and uploaded the resulting new tree to:
   git://git.kernel.org/pub/scm/linux/kernel/git/mingo/linux-2.6-kgdb.git
(and i have build, boot and functionality tested it as well.) Shortlog,

diffstat and patch included further below. Tip of the tree is commit

b4c015cde3cb272ab931f88666f787766402688e.
But before i go into the fine details of your observations, let me

please point out one fundamental thing that people might be asking: why

are we arch/x86 maintainers even bothering about all this?
After all the Linux kernel has already got _three_ separate kgdb remote

debugging stubs implemented in the current upstream tree:
  arch/mips/kernel/gdb-stub.c

  arch/frv/kernel/gdb-stub.c

  arch/mn10300/kernel/gdb-stub.c
one of them was merged upstream just 2 days ago...
So we could have done it the same way, by doing cp kernel/kgdb.c

arch/x86/kernel/gdb-stub.c and merging that. Nobody could have said a

_single_ word - we already have lowlevel UART code in early_printk.c

that we could have reused.
but we wanted to do it _right_ and not add an arch/x86/kernel/gdb-stub.c

special hack. We wanted to do it the same way we did it with genirq,

gtod, hrtimers, clockevents, dynticks and all the other facilities. We

could have easily implemented dynticks the same way ARM did, via arch

level hackery.
so lets please all keep that goal in mind. This is not about "will we

have KGDB support or not", this is about "WHERE will we have KGDB

support", and i strongly support the notion that it should be in the 
oh, i've already eliminated asm-generic/kgdb.h altogether - just forgot

to delete the old asm-generic/kgdb.h file. If you look at the code

nothing references that file anymore. I zapped it from the tree and 
nothing, asm-generic/kgdb....
[ message continues ]

Am I missing something? (I'm due, so it's not really a rhetorical

question :-) .) Wouldn't
unsigned int void u64_to_hex(u64 val, unsigned char *buf)

{

         int i;

         for (i=15; i>=0; i--) {

                  buf[i] = hexchars[ val & 0x0f ];

                  val >>= 4;

         }

         return 16;

}

...

         buf += u64_to_hex(cpu_to_be64(tmp_ll), buf);
be clearer both visually, and code-as-intent? (And equivalent helpers

for u32, u16 -- though they could all be rolled into one.)

--

[This still runs fine here, but sharp eyes are always welcome!]
Cleanup of the way kgdb

 - accesses unsafe memory via probe_kernel_*

 - converts to/from hex representation

 - passes errors due to such accesses around
At this chance I also fix kgdb_ebin2mem, which was broken /wrt escape

sequence handling.
Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---

 include/linux/kgdb.h |    4

 kernel/kgdb.c        |  349 +++++++++++++++------------------------------------

 2 files changed, 109 insertions(+), 244 deletions(-)
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h

index 7f4ee55..0359280 100644

--- a/include/linux/kgdb.h

+++ b/include/linux/kgdb.h

@@ -313,8 +313,8 @@ extern int kgdb_register_io_module(struct kgdb_io *local_kgdb_io_ops);

 extern void kgdb_unregister_io_module(struct kgdb_io *local_kgdb_io_ops);
 extern int kgdb_hex2long(char **ptr, long *long_val);

-extern char *kgdb_mem2hex(char *mem, char *buf, int count);

-extern char *kgdb_hex2mem(char *buf, char *mem, int count);

+extern int kgdb_mem2hex(char *mem, char *buf, int count);

+extern int kgdb_hex2mem(char *buf, char *mem, int count);
 extern int kgdb_isremovedbreak(unsigned long addr);
diff --git a/kernel/kgdb.c b/kernel/kgdb.c

index d55fdd1..ac9d196 100644

--- a/kernel/kgdb.c

+++ b/kernel/kgdb.c

@@ -145,38 +145,20 @@ static struct notifier_block kgdb_reboot_notifier = {

  * Finally, some KGDB code :-)

  */
-static char *kgdb_get_mem(char *addr, unsigned char *buf, int count)

+static int kgdb_get_mem(char *addr, unsigned char *buf, int count)

 {

-	while (count) {

-		if ((unsigned long)addr < TASK_SIZE)

-			return ERR_PTR(-EINVAL);

+	if ((unsigned long)addr < TASK_SIZE)

+		return -EFAULT;
-		if (probe_kernel_address(addr, *buf))

-			return ERR_PTR(-EINVAL);

-

-		buf++;

-		addr++;

-		count--;

-	}

-

-	return NULL;

+	return probe_kernel_read(buf, addr, count);

 }
-static char *kgdb_set_mem(char *addr, unsigned char *buf, int count)

+static int kgdb_set_m...
[ message continues ]

Ok, so this is a pretty function after all the cleanups, but I actually

don't think that "if ((unsigned long)addr < TASK_SIZE)" is really even

asked for.
Why not let kgdb look at user memory? I'd argue that in a lot of cases, it

might be quite nice to do, to see what user arguments in memory are etc

etc (think things like futexes, where user memory contents really do

matter).
So I'd suggest getting rid of the whole "kgdb_{get|set}_mem()" functions,

and just using "probe_kernel_{read|write}()" directly instead.
(Not that I necessarily love those names either, but whatever..)
The TASK_SIZE checks make more sense in kgdb_validate_break_address() and

friends, where it actually does make sense to check that it's really a

*kernel* address.
But even there, I'm not sure if the right check is to compare against

TASK_SIZE, since kernel and user memory addresses can in theory be

distinct (that's why we have "set_fs()" historically, and while it's no

longer true on x86 and hasn't been in a long time, the kernel conceptually

allows it - see my previous reply about that whole get_fs/set_fs thing in 
There's absolutely no reason to care about the alignment, since if you now

use "probe_kernel_read()", the sane thing to do is to just do
	err = probe_kernel_read(tmp, mem, count);

	if (!err) {

		while (count > 0) {

			buf = pack_hex_byte(buf, *tmp);

			tmp++;

			count--;

	}
and you're all done. No?
		Linus

--

ok, on a second thought: kgdb_{get|set}_mem() is _only_ used to validate

and set the software breakpoint (int3). And i think kgdb correctly

restricts that to kernel-space addresses only - you can typo an address

down into user-space and overwrite user-space memory and not know what

hit you ... [you can still explicitly touch user-space memory, but that

has to be done intentionally]
So to reduce the confusion i've removed these functions and open-coded

the probe_kernel_*() functions into kgdb_validate_break_address() and

kgdb_arch_set_breakpoint().
all other places already use probe_kernel_{read|write}. (Now, there are

a few stray TASK_SIZE checks still, i'll double check them and convert

them to access_ok() checks.)
btw., based on your previous comment about alignment, i found another

function that used weird alignment checks, kgdb_hex2mem():
	if (count == 2 && ((long)mem & 1) == 0)

		err = probe_kernel_write(mem, tmp_raw, 2);

	else if (count == 4 && ((long)mem & 3) == 0)

		err = probe_kernel_write(mem, tmp_raw, 4);

	else if (count == 8 && ((long)mem & 7) == 0)

		err = probe_kernel_write(mem, tmp_raw, 8);

	else

		err = probe_kernel_write(mem, tmp_raw, count);
	return err;

}
I just converted it to:
	return probe_kernel_write(mem, tmp_raw, count);
which looks _a lot_ cleaner.
	Ingo

--

all the TASK_SIZE checks relate to the soft breakpoint write accesses.
and access_ok() does not cut it: it's also a bit dangerous from debug

context: uses current->address_space, which is task dependent and can

accidentally allow an int3 write to userspace if executed in a kernel

thread that has lazy-inherited the TLB from a user task, etc., and it

also does not give enough protection on some other architectures.
is_kernel_text() is not good, because it does not cover modules.

is_module_address() is not good either, because it also covers module

data areas, and is a bit thick (hence crash-risky) as well. So there's

no existing facility to cover this.
so i'd say the safest would be to remove the TASK_SIZE check altogether.

If someone typoes a raw breakpoint - it is still enumerated by gdb and

can still be cleared. It's not like kgdb cannot be used to shoot in

one's own foot ...
	Ingo

--

Maybe, maybe not. I followed the comment in the original code, saying=20

that we need word-wise access for I/O memory poking. Can I assume across =
a all archs that __copy_to/from_user will not perform byte accesses if=20

count is 2, 4, or 8? I would be glad if we can kill other couple of line.=
Ingo, if you are close to an editor, please pick those up? Here are some =
offline things cooking on my side...
Jan

those architectures should extend mm/maccess.c accordingly. It's now the 
yeah, fixed these in my tree.
	Ingo

--

yes. We should allow kgdb to look at just about anything that can be

done safely - and we've got all the necessary protections against 
hm, is access_ok() safe on all architectures from irq context? That's 
yes, the full function now looks like this:
int kgdb_mem2hex(char *mem, char *buf, int count)

{

	char *tmp;

	int err;
	/*

	 * We use the upper half of buf as an intermediate buffer for the

	 * raw memory copy.  Hex conversion will work against this one.

	 */

	tmp = buf + count;
	err = probe_kernel_read(tmp, mem, count);

	if (!err) {

		while (count > 0) {

			buf = pack_hex_byte(buf, *tmp);

			tmp++;

			count--;

		}
		*buf = 0;

	}
	return err;

}
i'll test this a bit.
	Ingo
--

great! Applied. The kgdb memory accesses look _much_ cleaner now, and

it's a significant simplification as well:
   2 files changed, 109 insertions(+), 244 deletions(-)
the uaccess.h macros look a bit ugly now (it all got inherited from the

existing [and ancient] probe_kernel_address() macro) and because we now

touch it materially anyway, we might as well do it right?
	Ingo

--

Yes, will come, I just produced some ETOOMANYCHANGESATONCE issue while

improving this. The code goes down by more than 150 lines!
Jan

Thanks for responding. I'd mentioned it twice already and gotten no

response, so wasn't sure if my microphone was on :-).
If you're feeling energetic, the same sort of cleanup can be done against:
+#ifdef __BIG_ENDIAN

+               tmp_s |= hex(*buf++) << 12;

+               tmp_s |= hex(*buf++) << 8;

+               tmp_s |= hex(*buf++) << 4;

+               tmp_s |= hex(*buf++);

+#else

+               tmp_s |= hex(*buf++) << 4;

+               tmp_s |= hex(*buf++);

+               tmp_s |= hex(*buf++) << 12;

+               tmp_s |= hex(*buf++) << 8;

+#endif

+               if (probe_kernel_write(mem, tmp_s))

+                       return ERR_PTR(-EINVAL);

+

+               mem += 2;

+       } else if ((count == 4) && (((long)mem & 3) == 0)) {

+               u32 tmp_l = 0;

+

+#ifdef __BIG_ENDIAN

+               tmp_l |= hex(*buf++) << 28;

+               tmp_l |= hex(*buf++) << 24;

+               tmp_l |= hex(*buf++) << 20;

+               tmp_l |= hex(*buf++) << 16;

+               tmp_l |= hex(*buf++) << 12;

+               tmp_l |= hex(*buf++) << 8;

+               tmp_l |= hex(*buf++) << 4;

+               tmp_l |= hex(*buf++);

+#else

+               tmp_l |= hex(*buf++) << 4;

+               tmp_l |= hex(*buf++);

+               tmp_l |= hex(*buf++) << 12;

+               tmp_l |= hex(*buf++) << 8;

+               tmp_l |= hex(*buf++) << 20;

+               tmp_l |= hex(*buf++) << 16;

+               tmp_l |= hex(*buf++) << 28;

+               tmp_l |= hex(*buf++) << 24;

+#endif

+               if (probe_kernel_write(mem, tmp_l))

+                       return ERR_PTR(-EINVAL);

+               mem += 4;
As in, write a helper to parse a hex16, hex32, hex64, then do a

be[16|32|64]_to_cpu, and probe_kernel_write the result.
u64 hex_to_u64(unsigned char *buf)

{

        int i;

        u64 val = 0;
        for (i=0; i...
[ message continues ]

This is a leftover from the old jmp-on-fault logic that was missed by

this tree. I have a patch under test that kills this (and further 150
Is the patch below OK? I also added an "if KGB" to unbreak kgdb's

kconfig menu again and included two minor cleanups I posted yesterday.
Jan
PS: Ingo, kgdboc runtime re-configuration is broken as you dropped the

__init removal from uart_parse_options and uart_set_option (probably

due to the ifdef'ery). Better remove it unconditionally?
--------------
Refactor KGDB kbuild menu.
Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---

 arch/x86/Kconfig        |    4 +---

 drivers/serial/Makefile |    2 +-

 lib/Kconfig.kgdb        |   15 ++++++++++-----

 3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig

index 85bcc23..5e0fab5 100644

--- a/arch/x86/Kconfig

+++ b/arch/x86/Kconfig

@@ -22,6 +22,7 @@ config X86

 	select HAVE_IDE

 	select HAVE_OPROFILE

 	select HAVE_KPROBES

+	select HAVE_ARCH_KGDB
 config GENERIC_LOCKBREAK

 	def_bool n

@@ -144,9 +145,6 @@ config AUDIT_ARCH

 config ARCH_SUPPORTS_AOUT

 	def_bool y
-config HAVE_ARCH_KGDB

-	def_bool y

-

 # Use the generic interrupt handling code in kernel/irq/:

 config GENERIC_HARDIRQS

 	bool

diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile

index 1d8ee05..dfd8e72 100644

--- a/drivers/serial/Makefile

+++ b/drivers/serial/Makefile

@@ -66,5 +66,5 @@ obj-$(CONFIG_SERIAL_UARTLITE) += uartlite.o

 obj-$(CONFIG_SERIAL_NETX) += netx-serial.o

 obj-$(CONFIG_SERIAL_OF_PLATFORM) += of_serial.o

 obj-$(CONFIG_SERIAL_KS8695) += serial_ks8695.o

-obj-$(CONFIG_KGDBOC) += kgdboc.o

+obj-$(CONFIG_KGDB_IO_SERIAL_CONSOLE) += kgdboc.o

 obj-$(CONFIG_SERIAL_QE) += ucc_uart.o

diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb

index 00263c0..947c7f1 100644

--- a/lib/Kconfig.kgdb

+++ b/lib/Kconfig.kgdb

@@ -10,12 +10,16 @@ menuconfig KGDB

 	  at http://kgdb.sourceforge.net as well as in DocBook form

 	  in Documentation/DocBook/.  If ...
[ message continues ]

Looks OK - but I think Ingo already addressed this.
	Sam

--

i picked up everything from Jan that would apply - please yell if you

still see something not absolutely squeeky clean ;-)
btw., we should convert all those current:
 config ARCH_POPULATES_NODE_MAP

         def_bool y
 config AUDIT_ARCH

         bool

         default X86_64
 config ARCH_SUPPORTS_AOUT

         def_bool y
instances to select lines after config X86 / config X86_64.
KGDB just followed the current Kconfig style status quo.
	Ingo

--

Indeed. I just do not have time to do so. A day-time job that

is not about kernel-hacking, three kids and a wife etc. consumes

some decent amount of time and leaves only a few hours for linux.

And I am a strange person that actually needs to sleep several

hours each night (as my baby daughter permits me to).
I promote the "select HAVE_*" style on all new stuff and expect that

a herd of janitors one day pick it up and convert most of the old-style

stuff.

And if you look around then you will see that for this merge window

almost all new stuff used "select HAVE_" so we on the right track.
	Sam

--

yeah, it's not like i'm complaining - you are doing a terrific job with

kbuild and kconfig.
Perhaps instead of outright converting it (which needs thought and real

hard work), how about just running a quick script over all Kconfigs and

marking old-style entries as:
  # TODO: this stuff needs to be fixed, see Documentation/blah.txt
? Should be 15 minutes of awk and can be committed right after -rc1.

Annoying messages like that tend to get the attention of architecture

maintainers :-) The Kconfig rules are not always obvious so people need

help.
	Ingo

--

Should be merged meanwhile.
At this chance: Is it necessary that prompt-less configs like=20

HAVE_ARCH_KGDB break the menu indention? I had to work around this via=20

"if KGDB" (or even "depends on"). This is easy to miss IMHO.
Jan

From: Ingo Molnar <mingo@elte.hu>
I suspect something will however need to be done with watchdogs

and things of that nature which will get very confused if the

kernel sits in a breakpoint for a period of time whilst the user

looks at things from the kgdb prompt.
Just a heads up...

--