kgdb cleanups

No real code changes, but cleanups all over the place. What about applying?

Ouch and arch-dependend code is moved to kernel/kgdb.c. I'll probably do x86-64 version so that is rather important.

Hi!

No real code changes, but cleanups all over the place. What about applying?

Ouch and arch-dependend code is moved to kernel/kgdb.c. I'll probably do x86-64 version so that is rather important.

Pavel

Quoted 9 lines · Pavel Machek

Hi!

No real code changes, but cleanups all over the place. What about applying?

Ouch and arch-dependend code is moved to kernel/kgdb.c. I'll probably do x86-64 version so that is rather important.

Pavel

A few comments:

I like the code seperation. Does it follow what Amit is doing? It would be nice if Amit's version and this one could come together around this.

I don't think we want to merge the eth and regular kgdb just yet. I would, however, like to keep eth completly out of the stub. Possibly a new module which just takes care of steering the I/O to the correct place.

Quoted 14 lines · George Anzinger

Quoted 9 lines · Pavel Machek

Hi!

No real code changes, but cleanups all over the place. What about applying?

Ouch and arch-dependend code is moved to kernel/kgdb.c. I'll probably do x86-64 version so that is rather important.

Pavel

A few comments:

Quoted 3 lines

I like the code seperation. Does it follow what Amit is doing? It would be nice if Amit's version and this one could come together around this.

I don't think we want to merge the eth and regular kgdb just yet. I would, however, like to keep eth completly out of the stub. Possibly a new module which just takes care of steering the I/O to the correct place.

I've sent Amit the start of an plug interface for abstracting the communication layer. Should be relatively painless and allow for starting sessions on the interface of your choice.

Quoted 24 lines · Matt Mackall

Quoted 20 lines · George Anzinger

Quoted 9 lines · Pavel Machek

Hi!

No real code changes, but cleanups all over the place. What about applying?

Ouch and arch-dependend code is moved to kernel/kgdb.c. I'll probably do x86-64 version so that is rather important.

Pavel

A few comments:

I like the code seperation. Does it follow what Amit is doing? It would be nice if Amit's version and this one could come together around this.

I don't think we want to merge the eth and regular kgdb just yet. I would, however, like to keep eth completly out of the stub. Possibly a new module which just takes care of steering the I/O to the correct place.

Quoted 4 lines

I've sent Amit the start of an plug interface for abstracting the communication layer. Should be relatively painless and allow for starting sessions on the interface of your choice.

May I see?

Quoted 20 lines

Quoted 20 lines

Quoted 18 lines

Quoted 9 lines

Hi!

No real code changes, but cleanups all over the place. What about applying?

Ouch and arch-dependend code is moved to kernel/kgdb.c. I'll probably do x86-64 version so that is rather important.

Pavel

A few comments:

I like the code seperation. Does it follow what Amit is doing? It would be nice if Amit's version and this one could come together around this.

I don't think we want to merge the eth and regular kgdb just yet. I would, however, like to keep eth completly out of the stub. Possibly a new module which just takes care of steering the I/O to the correct place.

Quoted 5 lines

Quoted 4 lines

I've sent Amit the start of an plug interface for abstracting the communication layer. Should be relatively painless and allow for starting sessions on the interface of your choice.

May I see?

Here's the interface plus the eth side of it:

Hi!

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

Quoted 10 lines

Quoted 9 lines

Hi!

No real code changes, but cleanups all over the place. What about applying?

Ouch and arch-dependend code is moved to kernel/kgdb.c. I'll probably do x86-64 version so that is rather important.

Pavel

Quoted 8 lines

A few comments:

I like the code seperation. Does it follow what Amit is doing? It would be nice if Amit's version and this one could come together around this.

I don't think we want to merge the eth and regular kgdb just yet. I would, however, like to keep eth completly out of the stub. Possibly a new module which just takes care of steering the I/O to the correct place.

I've sent Amit the start of an plug interface for abstracting the communication layer. Should be relatively painless and allow for starting sessions on the interface of your choice.

Quoted 3 lines

Quoted 1 line

May I see?

Here's the interface plus the eth side of it:

Does it work with those patches? (Amid's version does not seem to work over ethernet).

Quoted 25 lines · George Anzinger

Quoted 23 lines · Matt Mackall

Quoted 18 lines · George Anzinger

Quoted 9 lines · Pavel Machek

Hi!

No real code changes, but cleanups all over the place. What about applying?

Ouch and arch-dependend code is moved to kernel/kgdb.c. I'll probably do x86-64 version so that is rather important.

Pavel

A few comments:

I like the code seperation. Does it follow what Amit is doing? It would be nice if Amit's version and this one could come together around this.

I don't think we want to merge the eth and regular kgdb just yet. I would, however, like to keep eth completly out of the stub. Possibly a new module which just takes care of steering the I/O to the correct place.

Quoted 6 lines

Quoted 4 lines

I've sent Amit the start of an plug interface for abstracting the communication layer. Should be relatively painless and allow for starting sessions on the interface of your choice.

May I see?

Here's the interface plus the eth side of it:

 tiny-mpm/arch/i386/kernel/kgdb_stub.c |   59 +++++++++-------------------------
 tiny-mpm/drivers/net/kgdb_eth.c       |   26 +++++++++-----
 tiny-mpm/include/asm-i386/kgdb.h      |   14 +++++---
 3 files changed, 44 insertions(+), 55 deletions(-)

diff -puN include/asm-i386/kgdb.h~kgdb-plug include/asm-i386/kgdb.h

include/asm-i386/kgdb.h renamed from include/asm-i386/kgdb.h~kgdb-plug +7 -4

+	int (*getchar)(void);
+	void (*putchar)(int chr);
+	void (*flush)(void);
+	void (*trap)(int enable);
+};
+
+extern void kgdb_attach(struct kgdb_hook *hook);
 extern void kgdb_schedule_breakpoint(void);
 extern void kgdb_process_breakpoint(void);
 
-extern int kgdb_tty_hook(void);
-extern int kgdb_eth_hook(void);
-extern int kgdboe;
-
 /*
  * GDB debug stub (or any debug stub) can point the 'linux_debug_hook'
  * pointer to its routine and it will be entered as the first thing

diff -puN arch/i386/kernel/kgdb_stub.c~kgdb-plug arch/i386/kernel/kgdb_stub.c

arch/i386/kernel/kgdb_stub.c renamed from arch/i386/kernel/kgdb_stub.c~kgdb-plug +1 -11

@@ -130,12 +130,7 @@ typedef void (*Function) (void);	/* poin
 /* Thread reference */
 typedef unsigned char threadref[8];
 
-extern int tty_putDebugChar(int);     /* write a single character      */
-extern int tty_getDebugChar(void);    /* read and return a single char */
-extern void tty_flushDebugChar(void); /* flush pending characters      */
-extern int eth_putDebugChar(int);     /* write a single character      */
-extern int eth_getDebugChar(void);    /* read and return a single char */
-extern void eth_flushDebugChar(void); /* flush pending characters      */
+struct kgdb_hook *kh = 0;
 
 /************************************************************************/
 /* BUFMAX defines the maximum number of characters in inbound/outbound buffers*/

@@ -275,39 +270,25 @@ malloc(int size)
 	}
 }
 
-/*
- * I/O dispatch functions...
- * Based upon kgdboe, either call the ethernet
- * handler or the serial one..
- */
 void

arch/i386/kernel/kgdb_stub.c renamed from arch/i386/kernel/kgdb_stub.c~kgdb-plug +5 -14

 int
 getDebugChar(void)
 {
-	if (!kgdboe) {
-		return tty_getDebugChar();
-	} else {
-		return eth_getDebugChar();
-	}
+	if (kh)
+		return kh->getchar();
 }
 
 void
 flushDebugChar(void)
 {
-	if (!kgdboe) {
-		tty_flushDebugChar();
-	} else {
-		eth_flushDebugChar();
-	}
+	if (kh)
+		kh->flush();
 }
 
 /*

@@ -490,7 +471,7 @@ putpacket(char *buffer)
 
 	/*  $<packet info>#<checksum>. */
 
-	if (!kgdboe) {
+	if (kh && !kh->sendbuf) {
 		do {
 			if (remote_debug)
 				printk("T:%s\n", buffer);

@@ -516,10 +497,7 @@ putpacket(char *buffer)
 		 * We only transfer MAX_SEND_COUNT size bytes each time
 		 */
 
-#define MAX_SEND_COUNT 30
-
 		int send_count = 0, i = 0;
-		char send_buf[MAX_SEND_COUNT];
 
 		do {
 			if (remote_debug)

arch/i386/kernel/kgdb_stub.c renamed from arch/i386/kernel/kgdb_stub.c~kgdb-plug +6 -9

 					flushDebugChar();
 					send_count = 0;
 				} else {
-					send_buf[send_count] = ch;
+					kh->sendbuf[send_count] = ch;
 					checksum += ch;
 					count ++;
 					send_count++;
 				}
 			}
 			for(i = 0; i < send_count; i++)
-				putDebugChar(send_buf[i]);
+				putDebugChar(kh->sendbuf[i]);
 			putDebugChar('#');
 			putDebugChar(hexchars[checksum >> 4]);
 			putDebugChar(hexchars[checksum % 16]);

@@ -1272,12 +1250,9 @@ kgdb_handle_exception(int exceptionVecto
 		print_regs(&regs);
 		return (0);
 	}
-	/*
-	 * If we're using eth mode, set the 'mode' in the netdevice.
-	 */
 
-	if (kgdboe)
-		netpoll_set_trap(1);
+	if (kh)
+		kh->trap(1);
 
 	kgdb_local_irq_save(flags);
 

@@ -1727,8 +1702,8 @@ kgdb_handle_exception(int exceptionVecto
 				}
 			}
 
-			if (kgdboe)
-				netpoll_set_trap(0);
+			if(kh)
+				kh->trap(0);
 
 			correct_hw_break();
 			asm volatile ("movl %0, %%db6\n"::"r" (0));

diff -puN arch/i386/lib/kgdb_serial.c~kgdb-plug arch/i386/lib/kgdb_serial.c diff -puN drivers/net/kgdb_eth.c~kgdb-plug drivers/net/kgdb_eth.c

drivers/net/kgdb_eth.c renamed from drivers/net/kgdb_eth.c~kgdb-plug +17 -9

@@ -34,8 +34,6 @@ static int in_head, in_tail, out_count;
 static atomic_t in_count;
 int kgdboe = 0; /* Default to tty mode */
 
-extern void set_debug_traps(void);
-extern void breakpoint(void);
 static void rx_hook(struct netpoll *np, int port, char *msg, int len);
 
 static struct netpoll np = {

@@ -47,7 +45,7 @@ static struct netpoll np = {
 	.remote_mac = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
 };
 
-int eth_getDebugChar(void)
+static int getchar(void)
 {
 	int chr;
 

@@ -60,7 +58,7 @@ int eth_getDebugChar(void)
 	return chr;
 }
 
-void eth_flushDebugChar(void)
+static void flush(void)
 {
 	if(out_count && np.dev) {
 		netpoll_send_udp(&np, out_buf, out_count);

@@ -68,13 +66,24 @@ void eth_flushDebugChar(void)
 	}
 }
 
-void eth_putDebugChar(int chr)
+static void putchar(int chr)
 {
 	out_buf[out_count++] = chr;
 	if(out_count == OUT_BUF_SIZE)
 		eth_flushDebugChar();
 }
 
+#define MAX_SEND 30
+char sendbuf[MAX_SEND];
+static struct kgdb_hook kh = {
+	.sendbuf = sendbuf,
+	.maxsend = MAX_SEND
+	.getchar = getchar,
+	.putchar = putchar,
+	.flush = flush,
+	.trap = netpoll_set_trap
+};
+
 static void rx_hook(struct netpoll *np, int port, char *msg, int len)
 {
 	int i;

@@ -82,8 +91,10 @@ static void rx_hook(struct netpoll *np, 
 	np->remote_port = port;
 
 	/* Is this gdb trying to attach? */
-	if (!netpoll_trap() && len == 8 && !strncmp(msg, "$Hc-1#09", 8))
+	if (!netpoll_trap() && len == 8 && !strncmp(msg, "$Hc-1#09", 8)) {
+		kgdb_attach(kh);
 		kgdb_schedule_breakpoint();
+	}
 
 	for (i = 0; i < len; i++) {
 		if (msg[i] == 3)

@@ -117,12 +128,9 @@ static int init_kgdboe(void)
 	}
 #endif
 
-	set_debug_traps();
-
 	if(!np.remote_ip || netpoll_setup(&np))
 		return 1;
 
-	kgdboe = 1;
 	printk(KERN_INFO "kgdb: debugging over ethernet enabled\n");
 
 	return 0;

_

For the internal kgdb stuff I have created kdgb_local.h which I intended to be local to the workings of kgdb and not to contain anything a user would need.

include/asm-i386/kgdb.h extern void breakpoint(void); · line 24

Quoted 3 lines

include/asm-i386/kgdb.h renamed from include/asm-i386/kgdb.h~kgdb-plug +3 -0

+struct kgdb_hook {
+	char *sendbuf;
+	int maxsend;

I don't see the need of maxsend, or sendbuff, for that matter, as kgdb uses it now (for the eth code) it is redundant, in that the eth putchar also does the same thing as is being done in the kgdb_stub.c code. I think this should be removed from the stub and the limit in the ethcode relied upon.

Quoted 11 lines

void
putDebugChar(int c)
{

arch/i386/kernel/kgdb_stub.c renamed from arch/i386/kernel/kgdb_stub.c~kgdb-plug +2 -5

-	if (!kgdboe) {
-		tty_putDebugChar(c);
-	} else {
-		eth_putDebugChar(c);
-	}
+	if (kh)
+		kh->putchar(c);

}

I was thinking that this might read something like:

         if (xxx[kh].putchar(c))
                xxx[kh].putchar(c);

One might further want to do something like:

         if (!xxx[kh].putchar(c))
                kh = 0;

In otherwords, an array (xxx must, of course, be renamed) of stuct kgdb_hook (which name should also be changed to relate to I/O, kgdb_IO_hook, for example). Then reserve entry 0 for the rs232 I/O code.

An alternate possibility is an array of pointer to struct kgdb_hook which
allows one to define the struct contents as below and to build the array, all at compile/link time. A legal entry MUST define get and put, but why not define them all, using dummy functions for the ones that make no sense in a particular interface.

Quoted 3 lines

For the internal kgdb stuff I have created kdgb_local.h which I intended to be local to the workings of kgdb and not to contain anything a user would need.

Agreed, I just haven't touched it since you last mentioned it.

Hi!

Quoted 3 lines

Quoted 1 line

For the internal kgdb stuff I have created kdgb_local.h which I intended to be local to the workings of kgdb and not to contain anything a user would need.

Agreed, I just haven't touched it since you last mentioned it.

I believe we need better name than kgdb_local.h.... Hmm, but I'm not sure what the name should be.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

For the internal kgdb stuff I have created kdgb_local.h which I intended to be local to the workings of kgdb and not to contain anything a user would need.

Agreed, I just haven't touched it since you last mentioned it.

I believe we need better name than kgdb_local.h.... Hmm, but I'm not sure what the name should be.

Sure. How about kgdb_internal.h ??

Quoted 3 lines · George Anzinger

For the internal kgdb stuff I have created kdgb_local.h which I intended to be local to the workings of kgdb and not to contain anything a user would need.

Agreed, I just haven't touched it since you last mentioned it.

Quoted 9 lines

include/asm-i386/kgdb.h extern void breakpoint(void); · line 24

Quoted 3 lines

include/asm-i386/kgdb.h renamed from include/asm-i386/kgdb.h~kgdb-plug +3 -0

+struct kgdb_hook {
+	char *sendbuf;
+	int maxsend;

I don't see the need of maxsend, or sendbuff, for that matter, as kgdb uses it now (for the eth code) it is redundant, in that the eth putchar also does the same thing as is being done in the kgdb_stub.c code. I think this should be removed from the stub and the limit in the ethcode relied upon.

Fair enough.

Quoted 16 lines

Quoted 11 lines

void
putDebugChar(int c)
{

arch/i386/kernel/kgdb_stub.c renamed from arch/i386/kernel/kgdb_stub.c~kgdb-plug +2 -5

-	if (!kgdboe) {
-		tty_putDebugChar(c);
-	} else {
-		eth_putDebugChar(c);
-	}
+	if (kh)
+		kh->putchar(c);

}

I was thinking that this might read something like:

         if (xxx[kh].putchar(c))
                xxx[kh].putchar(c);

Quoted 7 lines

One might further want to do something like:

         if (!xxx[kh].putchar(c))
                kh = 0;

In otherwords, an array (xxx must, of course, be renamed) of stuct kgdb_hook (which name should also be changed to relate to I/O, kgdb_IO_hook, for example). Then reserve entry 0 for the rs232 I/O code.

Dunno about that. Probably should work more like the console code, whoever registers first wins. Early boot will probably be the exclusive province of serial for a while yet, but designing it in is probably short-sighted.

Quoted 5 lines

An alternate possibility is an array of pointer to struct kgdb_hook
which allows one to define the struct contents as below and to build the array, all at compile/link time. A legal entry MUST define get and put, but why not define them all, using dummy functions for the ones that make no sense in a particular interface.

Throwing all the stubs in a special section could work well too. Then we could add an avail() function so that early boot debugging could discover if each one was available. The serial code could use this to kickstart itself while the eth code could test a local initialized flag and say "not a chance". Which gives us all the architecture to throw in other trivial interfaces (parallel, bus-snoopers, etc.).

Regarding pluggable iterfaces -
The version I have lets a user to choose the interface by supplying appropriate command line. (e.g. kgdbwait kgdb8250=... or kgdbwait kgdbeth=...) It supports an arbitrary number of interfaces. The kgdb core itself is independent of an interface. All interfaces are defined by a structure described below. An interface registers itself with kgdb core by assigning this structure to pointer kgdb_serial.

struct kgdb_serial {
	int chunksize;

	int (*read_char)(void);
	void (*write_char)(int);
	void (*flush)(void);
	int (*hook)(void);
	void (*begin_session)(void);
	void (*end_session)(void);
};

Where chunksize is maximum chunksize an interface can handle.

read_char and write_char are derived from getDebugChar and putDebugChar
flush flushes written characters. Flush control is given to kgdb core so that it can ensure that #checksum doesn't split.

begin_session and end_session inform an interface about a gdb communication session. (Haven't decided about console packets to gdb yet)

hook is interface initialization. It can return errors. This allows kgdb core to probe the interface for availability at multiple points. Because of this, there can be multiple debugger entry points
1. At very begining of start_kernel -> Only an 8250 interface with early boot enabled can respond to hook call.
2. After smp initialization -> An 8250 interface without an early boot can respond to this.
3. An ethernet interface can itself call debugger_entry to enter debugger after it's brought up from userland.

Other interfaces can come up at (1) or (2)

Quoted 64 lines · Matt Mackall

Quoted 3 lines · George Anzinger

For the internal kgdb stuff I have created kdgb_local.h which I intended to be local to the workings of kgdb and not to contain anything a user would need.

Agreed, I just haven't touched it since you last mentioned it.

Quoted 9 lines

include/asm-i386/kgdb.h extern void breakpoint(void); · line 24

Quoted 3 lines

include/asm-i386/kgdb.h renamed from include/asm-i386/kgdb.h~kgdb-plug +3 -0

+struct kgdb_hook {
+	char *sendbuf;
+	int maxsend;

I don't see the need of maxsend, or sendbuff, for that matter, as kgdb uses it now (for the eth code) it is redundant, in that the eth putchar also does the same thing as is being done in the kgdb_stub.c code. I think this should be removed from the stub and the limit in the ethcode relied upon.

Fair enough.

Quoted 16 lines

Quoted 11 lines

void
putDebugChar(int c)
{

arch/i386/kernel/kgdb_stub.c renamed from arch/i386/kernel/kgdb_stub.c~kgdb-plug +2 -5

-	if (!kgdboe) {
-		tty_putDebugChar(c);
-	} else {
-		eth_putDebugChar(c);
-	}
+	if (kh)
+		kh->putchar(c);

}

I was thinking that this might read something like:

        if (xxx[kh].putchar(c))
               xxx[kh].putchar(c);

Quoted 7 lines

One might further want to do something like:

        if (!xxx[kh].putchar(c))
               kh = 0;

In otherwords, an array (xxx must, of course, be renamed) of stuct kgdb_hook (which name should also be changed to relate to I/O, kgdb_IO_hook, for example). Then reserve entry 0 for the rs232 I/O code.

Dunno about that. Probably should work more like the console code, whoever registers first wins. Early boot will probably be the exclusive province of serial for a while yet, but designing it in is probably short-sighted.

Quoted 5 lines

An alternate possibility is an array of pointer to struct kgdb_hook which allows one to define the struct contents as below and to build the array, all at compile/link time. A legal entry MUST define get and put, but why not define them all, using dummy functions for the ones that make no sense in a particular interface.

Throwing all the stubs in a special section could work well too. Then we could add an avail() function so that early boot debugging could discover if each one was available. The serial code could use this to kickstart itself while the eth code could test a local initialized flag and say "not a chance". Which gives us all the architecture to throw in other trivial interfaces (parallel, bus-snoopers, etc.).

Quoted 8 lines · Amit S. Kale

Regarding pluggable iterfaces -
The version I have lets a user to choose the interface by supplying appropriate command line. (e.g. kgdbwait kgdb8250=... or kgdbwait kgdbeth=...) It supports an arbitrary number of interfaces. The kgdb core itself is independent of an interface. All interfaces are defined by a structure described below. An interface registers itself with kgdb core by assigning this structure to pointer kgdb_serial.

Quoted 2 lines

struct kgdb_serial {
	int chunksize;

Do we really need this? The only place I saw it used it did not seem to matter where the split occured and there was now endchunck/beginchunck stuff. I would MUCH rather see the interface code take care of this with out mucking up the core code (as the eth code already does). Did I miss something here?

Quoted 9 lines

	int (*read_char)(void);
	void (*write_char)(int);
	void (*flush)(void);
	int (*hook)(void);
	void (*begin_session)(void);
	void (*end_session)(void);
};

Quoted 5 lines

Where chunksize is maximum chunksize an interface can handle.

read_char and write_char are derived from getDebugChar and putDebugChar flush flushes written characters. Flush control is given to kgdb core so that it can ensure that #checksum doesn't split.

Actually, I think it is needed so that gdb knows that the kgdb stub has exited. This could, of course, be done with out the flush, but then the write code would have to recognize an end of record (not hard with the given protocol). I don't think there is any requirement that a checksum not be split. My assumption here is that the logical record is reassembled on the gdb end without concern about how many physical records are involved. Is this not true?

Quoted 3 lines

begin_session and end_session inform an interface about a gdb communication session. (Haven't decided about console packets to gdb yet)

I assume you mean entry to the stub/ exit the stub as a "session". This eliminates the old hook, right?

Quoted 10 lines

hook is interface initialization. It can return errors. This allows kgdb core to probe the interface for availability at multiple points. Because of this, there can be multiple debugger entry points
1. At very begining of start_kernel -> Only an 8250 interface with early boot enabled can respond to hook call.
2. After smp initialization -> An 8250 interface without an early boot can respond to this.
3. An ethernet interface can itself call debugger_entry to enter debugger after it's brought up from userland.

Hm.. Eth is up way before user land, else we could not nfs mount root and all that that implies. I think eth should be "available" when it is initialized. I am not sure I see a reason to support a way to get to kgdb from user land. We have ^C and also the Sys Rq entry. That should be enough.

Also, as mention in a prior email, I don't think it is a good idea to switch interfaces once communication is started, unless commanded to do so, even if the default request is to use an inteface that just came up.

I would suggest the the readyness of an interface be something one can easily determine from gdb (assuming we have established a connection). I suggest putting this in the kgdb_info structure. Possibly a pointer to an array of ??? where each entry is one of the interfaces. Should have an "up" value here (filled in by the hook calls), as well as, say a pointer to more details on the interface. The details might be baud, irq, address, for serial, and the ip address, etc for eth.

-g

Quoted 63 lines

Other interfaces can come up at (1) or (2)

Quoted 60 lines · Matt Mackall

Quoted 3 lines · George Anzinger

For the internal kgdb stuff I have created kdgb_local.h which I intended to be local to the workings of kgdb and not to contain anything a user would need.

Agreed, I just haven't touched it since you last mentioned it.

Quoted 9 lines

include/asm-i386/kgdb.h extern void breakpoint(void); · line 24

Quoted 3 lines

include/asm-i386/kgdb.h renamed from include/asm-i386/kgdb.h~kgdb-plug +3 -0

+struct kgdb_hook {
+	char *sendbuf;
+	int maxsend;

I don't see the need of maxsend, or sendbuff, for that matter, as kgdb uses it now (for the eth code) it is redundant, in that the eth putchar also does the same thing as is being done in the kgdb_stub.c code. I think this should be removed from the stub and the limit in the ethcode relied upon.

Fair enough.

Quoted 16 lines

Quoted 11 lines

void
putDebugChar(int c)
{

arch/i386/kernel/kgdb_stub.c renamed from arch/i386/kernel/kgdb_stub.c~kgdb-plug +2 -5

-	if (!kgdboe) {
-		tty_putDebugChar(c);
-	} else {
-		eth_putDebugChar(c);
-	}
+	if (kh)
+		kh->putchar(c);

}

I was thinking that this might read something like:

        if (xxx[kh].putchar(c))
               xxx[kh].putchar(c);

Quoted 7 lines

One might further want to do something like:

        if (!xxx[kh].putchar(c))
               kh = 0;

In otherwords, an array (xxx must, of course, be renamed) of stuct kgdb_hook (which name should also be changed to relate to I/O, kgdb_IO_hook, for example). Then reserve entry 0 for the rs232 I/O code.

Dunno about that. Probably should work more like the console code, whoever registers first wins. Early boot will probably be the exclusive province of serial for a while yet, but designing it in is probably short-sighted.

Quoted 5 lines

An alternate possibility is an array of pointer to struct kgdb_hook which allows one to define the struct contents as below and to build the array, all at compile/link time. A legal entry MUST define get and put, but why not define them all, using dummy functions for the ones that make no sense in a particular interface.

Throwing all the stubs in a special section could work well too. Then we could add an avail() function so that early boot debugging could discover if each one was available. The serial code could use this to kickstart itself while the eth code could test a local initialized flag and say "not a chance". Which gives us all the architecture to throw in other trivial interfaces (parallel, bus-snoopers, etc.).

Quoted 10 lines · George Anzinger

Quoted 8 lines · Amit S. Kale

Regarding pluggable iterfaces -
The version I have lets a user to choose the interface by supplying appropriate command line. (e.g. kgdbwait kgdb8250=... or kgdbwait kgdbeth=...) It supports an arbitrary number of interfaces. The kgdb core itself is independent of an interface. All interfaces are defined by a structure described below. An interface registers itself with kgdb core by assigning this structure to pointer kgdb_serial.

Quoted 8 lines

Quoted 2 lines

struct kgdb_serial {
	int chunksize;

Do we really need this? The only place I saw it used it did not seem to matter where the split occured and there was now endchunck/beginchunck stuff. I would MUCH rather see the interface code take care of this with out mucking up the core code (as the eth code already does). Did I miss something here?

Having an interface recognize a kgdb core record isn't a good design. Having kgdb core record know interface limitations isn't good either.

If kgdb calls flush at end of a packet and an interface splits a packet whenever its length goes above its limit, that'll be the right way of doing it.

Quoted 15 lines

Quoted 9 lines

	int (*read_char)(void);
	void (*write_char)(int);
	void (*flush)(void);
	int (*hook)(void);
	void (*begin_session)(void);
	void (*end_session)(void);
};

Quoted 5 lines

Where chunksize is maximum chunksize an interface can handle.

read_char and write_char are derived from getDebugChar and putDebugChar flush flushes written characters. Flush control is given to kgdb core so that it can ensure that #checksum doesn't split.

Quoted 7 lines

Actually, I think it is needed so that gdb knows that the kgdb stub has exited. This could, of course, be done with out the flush, but then the write code would have to recognize an end of record (not hard with the given protocol). I don't think there is any requirement that a checksum not be split. My assumption here is that the logical record is reassembled on the gdb end without concern about how many physical records are involved. Is this not true?

I guess yes. Splitting of #checksum may not matter.

Quoted 3 lines

Quoted 2 lines

begin_session and end_session inform an interface about a gdb communication session. (Haven't decided about console packets to gdb yet)

Quoted 2 lines

I assume you mean entry to the stub/ exit the stub as a "session". This eliminates the old hook, right?

Yes. begin_session and end_session mark entry and exit points into handle_exception. They are required to mark ethernet interface in trap mode.

What's old hook?

Quoted 1 line · George Anzinger

For the internal kgdb stuff I have created kdgb_local.h which I intended to be local to the workings of kgdb and not to contain anything a user would need.

Agreed, I just haven't touched it since you last mentioned it.

Quoted 5 lines

include/asm-i386/kgdb.h extern void breakpoint(void); · line 24

Quoted 3 lines

include/asm-i386/kgdb.h renamed from include/asm-i386/kgdb.h~kgdb-plug +3 -0

+struct kgdb_hook {
+	char *sendbuf;
+	int maxsend;

I don't see the need of maxsend, or sendbuff, for that matter, as kgdb uses it now (for the eth code) it is redundant, in that the eth putchar also does the same thing as is being done in the kgdb_stub.c code. I think this should be removed from the stub and the limit in the ethcode relied upon.

Fair enough.

Quoted 13 lines

Quoted 11 lines

void
putDebugChar(int c)
{

arch/i386/kernel/kgdb_stub.c renamed from arch/i386/kernel/kgdb_stub.c~kgdb-plug +2 -5

-	if (!kgdboe) {
-		tty_putDebugChar(c);
-	} else {
-		eth_putDebugChar(c);
-	}
+	if (kh)
+		kh->putchar(c);

}

I was thinking that this might read something like:

Quoted 8 lines

        if (xxx[kh].putchar(c))
               xxx[kh].putchar(c);

One might further want to do something like:

        if (!xxx[kh].putchar(c))
               kh = 0;

In otherwords, an array (xxx must, of course, be renamed) of stuct kgdb_hook (which name should also be changed to relate to I/O, kgdb_IO_hook, for example). Then reserve entry 0 for the rs232 I/O code.

Dunno about that. Probably should work more like the console code, whoever registers first wins. Early boot will probably be the exclusive province of serial for a while yet, but designing it in is probably short-sighted.

Quoted 1 line

An alternate possibility is an array of pointer to struct kgdb_hook which allows one to define the struct contents as below and to build the array, all at compile/link time. A legal entry MUST define get and put, but why not define them all, using dummy functions for the ones that make no sense in a particular interface.

Throwing all the stubs in a special section could work well too. Then we could add an avail() function so that early boot debugging could discover if each one was available. The serial code could use this to kickstart itself while the eth code could test a local initialized flag and say "not a chance". Which gives us all the architecture to throw in other trivial interfaces (parallel, bus-snoopers, etc.).

Quoted 54 lines · Matt Mackall

Quoted 3 lines · George Anzinger

For the internal kgdb stuff I have created kdgb_local.h which I intended to be local to the workings of kgdb and not to contain anything a user would need.

Agreed, I just haven't touched it since you last mentioned it.

Quoted 9 lines

include/asm-i386/kgdb.h extern void breakpoint(void); · line 24

Quoted 3 lines

include/asm-i386/kgdb.h renamed from include/asm-i386/kgdb.h~kgdb-plug +3 -0

+struct kgdb_hook {
+	char *sendbuf;
+	int maxsend;

I don't see the need of maxsend, or sendbuff, for that matter, as kgdb uses it now (for the eth code) it is redundant, in that the eth putchar also does the same thing as is being done in the kgdb_stub.c code. I think this should be removed from the stub and the limit in the ethcode relied upon.

Fair enough.

Quoted 16 lines

Quoted 11 lines

void
putDebugChar(int c)
{

arch/i386/kernel/kgdb_stub.c renamed from arch/i386/kernel/kgdb_stub.c~kgdb-plug +2 -5

-	if (!kgdboe) {
-		tty_putDebugChar(c);
-	} else {
-		eth_putDebugChar(c);
-	}
+	if (kh)
+		kh->putchar(c);

}

I was thinking that this might read something like:

        if (xxx[kh].putchar(c))
               xxx[kh].putchar(c);

Quoted 7 lines

One might further want to do something like:

        if (!xxx[kh].putchar(c))
               kh = 0;

In otherwords, an array (xxx must, of course, be renamed) of stuct kgdb_hook (which name should also be changed to relate to I/O, kgdb_IO_hook, for example). Then reserve entry 0 for the rs232 I/O code.

Dunno about that. Probably should work more like the console code, whoever registers first wins. Early boot will probably be the exclusive province of serial for a while yet, but designing it in is probably short-sighted.

Quoted 5 lines

An alternate possibility is an array of pointer to struct kgdb_hook which allows one to define the struct contents as below and to build the array, all at compile/link time. A legal entry MUST define get and put, but why not define them all, using dummy functions for the ones that make no sense in a particular interface.

Quoted 6 lines

Throwing all the stubs in a special section could work well too. Then we could add an avail() function so that early boot debugging could discover if each one was available. The serial code could use this to kickstart itself while the eth code could test a local initialized flag and say "not a chance". Which gives us all the architecture to throw in other trivial interfaces (parallel, bus-snoopers, etc.).

I am thinking of something more like what was done with the x86 timer code. Each timer option sets up a structure with an array of pointers to each option. There it is done at compile time, and the runtime code tries each. There it is done in order, but here we want to do it a bit differently.

Maybe we could have an "available" flag or just assume that the address being !=0 for getdebugchar means it is "available". I think there should be a prefered intface set at config time. Possibly over ride this with the command line. Then have a back up order in case kgdb wants to communicate prior to the prefered one being available.

We would also have a rule that the command line over ride only works if communication has not yet been established. Here, we would also like control from gdb/kgdb so we could switch to a different interface, but under gdb control at this point. Either a maintaince command or setting the "channel" with a memory modify command. We would want this to take effect only after the current command is acknowledged.

Quoted 24 lines · George Anzinger

Quoted 7 lines

Quoted 6 lines

An alternate possibility is an array of pointer to struct kgdb_hook which allows one to define the struct contents as below and to build the array, all at compile/link time. A legal entry MUST define get and put, but why not define them all, using dummy functions for the ones that make no sense in a particular interface.

Quoted 6 lines

Throwing all the stubs in a special section could work well too. Then we could add an avail() function so that early boot debugging could discover if each one was available. The serial code could use this to kickstart itself while the eth code could test a local initialized flag and say "not a chance". Which gives us all the architecture to throw in other trivial interfaces (parallel, bus-snoopers, etc.).

I am thinking of something more like what was done with the x86 timer code.
Each timer option sets up a structure with an array of pointers to each
option. There it is done at compile time, and the runtime code tries each. There it is done in order, but here we want to do it a bit differently.

Maybe we could have an "available" flag or just assume that the address being !=0 for getdebugchar means it is "available". I think there should be a prefered intface set at config time. Possibly over ride this with the command line. Then have a back up order in case kgdb wants to communicate prior to the prefered one being available.

Quoted 6 lines

We would also have a rule that the command line over ride only works if communication has not yet been established. Here, we would also like control from gdb/kgdb so we could switch to a different interface, but under gdb control at this point. Either a maintaince command or setting the "channel" with a memory modify command. We would want this to take effect only after the current command is acknowledged.

I have something similar in my patches.
Each interface has kgdb_hook function, which returns failure if an interface isn't ready.

Quoted 7 lines

No real code changes, but cleanups all over the place. What about applying?

Ouch and arch-dependend code is moved to kernel/kgdb.c. I'll probably do x86-64 version so that is rather important.

Pavel

A few comments:

I like the code seperation. Does it follow what Amit is doing? It would be nice if Amit's version and this one could come together around

Quoted 1 line

this.