[PATCH][0/6] A different KGDB stub

[PATCH][0/6] A different KGDB stub

[Tom Rini]

Hi Andrew.  As a reply to this message, I'm going to send you patches to
replace George's KGDB with a version that is Amit Kale's work, with a
number of additional cleanups (that I'll put in his CVS ASAP).  There
are 6 different patches:
core.patch: All of the non-arch specific bits, that aren't drivers.
8250.patch: The i/o driver for KGDB, via a standard PC uart.
kgdboe.patch: The i/o driver for KGDB, via netpoll.
i386.patch: The i386-specific code, tested.
ppc32.patch: The ppc32-specific code, tested.
x86_64.patch: The x86_64-specific bits, untested.

With this, there's a few design questions I have.  First, as I've done
things right now, a breakpoint at the first line of C code is untested.
It should work, but I have a question of how we want to handle setting
up the pointer to our i/o functions (if we have a pointer).  There's a
couple of ways that this could be solved, with pros and cons.  For
example, if we want to allow for both serial and enet to be used in the
same kernel, we could default to setting this pointer to the 8250
version, and allow for kgdb_arch_init to override (as PPC sometimes
does).  This is what I've done for now, but I don't know if I like how
it looks or not. If we don't care about allowing for > 1 i/o driver, we
can simply drop kgdb_serial as a function pointer and just call
kgdb_getDebugChar/kgdb_putDebugChar/etc.  Or someone else can suggest an
better way.

Next, what features of George's version are a must-have?  And what that
we have now, can we drop?  For example, up until I started working on
kgdboe+netpoll, I found KGBB_CONSOLE quite handy.  Now, I'm very happy
with netconsole, so I don't have a strong attachment to KGDB_CONSOLE
anymore.  But it's not much code anyhow.  And of course, what could be
done better?

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [PATCH][0/6] A different KGDB stub

[Andrew Morton]

Tom Rini <trini@kernel.crashing.org> wrote:
>
> Hi Andrew.  As a reply to this message, I'm going to send you patches to
> replace George's KGDB with a version that is Amit Kale's work, with a
> number of additional cleanups (that I'll put in his CVS ASAP).  There
> are 6 different patches:
> core.patch: All of the non-arch specific bits, that aren't drivers.
> 8250.patch: The i/o driver for KGDB, via a standard PC uart.
> kgdboe.patch: The i/o driver for KGDB, via netpoll.
> i386.patch: The i386-specific code, tested.
> ppc32.patch: The ppc32-specific code, tested.
> x86_64.patch: The x86_64-specific bits, untested.

OK.

> With this, there's a few design questions I have.  First, as I've done
> things right now, a breakpoint at the first line of C code is untested.
> It should work, but I have a question of how we want to handle setting
> up the pointer to our i/o functions (if we have a pointer).  There's a
> couple of ways that this could be solved, with pros and cons.  For
> example, if we want to allow for both serial and enet to be used in the
> same kernel, we could default to setting this pointer to the 8250
> version, and allow for kgdb_arch_init to override (as PPC sometimes
> does).  This is what I've done for now, but I don't know if I like how
> it looks or not. If we don't care about allowing for > 1 i/o driver, we
> can simply drop kgdb_serial as a function pointer and just call
> kgdb_getDebugChar/kgdb_putDebugChar/etc.  Or someone else can suggest an
> better way.

I don't think runtime selection is very important, personally.  You tend to
get things set up with a serial cable or ethernet and just leave it that
way.  Given that you need to recompile the kernel anyway, I'd say that
Kconfig-time selection is acceptable.


> Next, what features of George's version are a must-have?  And what that
> we have now, can we drop?  For example, up until I started working on
> kgdboe+netpoll, I found KGBB_CONSOLE quite handy.  Now, I'm very happy
> with netconsole, so I don't have a strong attachment to KGDB_CONSOLE
> anymore.  But it's not much code anyhow.  And of course, what could be
> done better?

I use KGDB_CONSOLE occasionally, although that's only when I can't stomach
the thought of using minicom ;)

A few things which I believe have debatable value are:

CONFIG_KGDB_MORE
CONFIG_KGDB_OPTIONS
CONFIG_NO_KGDB_CPUS
CONFIG_KGDB_TS
CONFIG_STACK_OVERFLOW_TEST
CONFIG_KGDB_SYSRQ		(Just turn it on by default?)

I have never used (or, as far as I know, needed) any of the above.

Re: [PATCH][0/6] A different KGDB stub

[Tom Rini]

On Wed, Feb 11, 2004 at 04:27:56PM -0800, Andrew Morton wrote:

> Tom Rini <trini@kernel.crashing.org> wrote:
> >
> > Hi Andrew.  As a reply to this message, I'm going to send you patches to
> > replace George's KGDB with a version that is Amit Kale's work, with a
> > number of additional cleanups (that I'll put in his CVS ASAP).  There
> > are 6 different patches:
> > core.patch: All of the non-arch specific bits, that aren't drivers.
> > 8250.patch: The i/o driver for KGDB, via a standard PC uart.
> > kgdboe.patch: The i/o driver for KGDB, via netpoll.
> > i386.patch: The i386-specific code, tested.
> > ppc32.patch: The ppc32-specific code, tested.
> > x86_64.patch: The x86_64-specific bits, untested.
> 
> OK.
> 
> > With this, there's a few design questions I have.  First, as I've done
> > things right now, a breakpoint at the first line of C code is untested.
> > It should work, but I have a question of how we want to handle setting
> > up the pointer to our i/o functions (if we have a pointer).  There's a
> > couple of ways that this could be solved, with pros and cons.  For
> > example, if we want to allow for both serial and enet to be used in the
> > same kernel, we could default to setting this pointer to the 8250
> > version, and allow for kgdb_arch_init to override (as PPC sometimes
> > does).  This is what I've done for now, but I don't know if I like how
> > it looks or not. If we don't care about allowing for > 1 i/o driver, we
> > can simply drop kgdb_serial as a function pointer and just call
> > kgdb_getDebugChar/kgdb_putDebugChar/etc.  Or someone else can suggest an
> > better way.
> 
> I don't think runtime selection is very important, personally.  You tend to
> get things set up with a serial cable or ethernet and just leave it that
> way.  Given that you need to recompile the kernel anyway, I'd say that
> Kconfig-time selection is acceptable.

I'll start on that then.

> > Next, what features of George's version are a must-have?  And what that
> > we have now, can we drop?  For example, up until I started working on
> > kgdboe+netpoll, I found KGBB_CONSOLE quite handy.  Now, I'm very happy
> > with netconsole, so I don't have a strong attachment to KGDB_CONSOLE
> > anymore.  But it's not much code anyhow.  And of course, what could be
> > done better?
> 
> I use KGDB_CONSOLE occasionally, although that's only when I can't stomach
> the thought of using minicom ;)
> 
> A few things which I believe have debatable value are:
> 
> CONFIG_KGDB_MORE
> CONFIG_KGDB_OPTIONS
> CONFIG_NO_KGDB_CPUS
> CONFIG_KGDB_TS
> CONFIG_STACK_OVERFLOW_TEST
> CONFIG_KGDB_SYSRQ		(Just turn it on by default?)
> 
> I have never used (or, as far as I know, needed) any of the above.

I think CONFIG_KGDB_SYSRQ can die since with the 8250 and enet drivers
you can try and connect at any point, which will schedule a breakpoint
and you can get in like that.  As for NO_KGDB_CPUS, I'm not entirely
certain why this can't go away and there'd be an array of NR_CPUS in
size.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [PATCH][0/6] A different KGDB stub

[Chris Wright]

* Tom Rini (trini@kernel.crashing.org) wrote:
> On Wed, Feb 11, 2004 at 04:27:56PM -0800, Andrew Morton wrote:
> > CONFIG_KGDB_SYSRQ		(Just turn it on by default?)
> > 
> > I have never used (or, as far as I know, needed) any of the above.
> 
> I think CONFIG_KGDB_SYSRQ can die since with the 8250 and enet drivers
> you can try and connect at any point, which will schedule a breakpoint
> and you can get in like that.  As for NO_KGDB_CPUS, I'm not entirely
> certain why this can't go away and there'd be an array of NR_CPUS in
> size.

Using kgdboe I've had numerous times where it gets a bit wedged and
only sysrq-g could get the breakpoint.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH][0/6] A different KGDB stub

[Tom Rini]

On Fri, Feb 13, 2004 at 10:58:38AM -0800, Chris Wright wrote:

> * Tom Rini (trini@kernel.crashing.org) wrote:
> > On Wed, Feb 11, 2004 at 04:27:56PM -0800, Andrew Morton wrote:
> > > CONFIG_KGDB_SYSRQ		(Just turn it on by default?)
> > > 
> > > I have never used (or, as far as I know, needed) any of the above.
> > 
> > I think CONFIG_KGDB_SYSRQ can die since with the 8250 and enet drivers
> > you can try and connect at any point, which will schedule a breakpoint
> > and you can get in like that.  As for NO_KGDB_CPUS, I'm not entirely
> > certain why this can't go away and there'd be an array of NR_CPUS in
> > size.
> 
> Using kgdboe I've had numerous times where it gets a bit wedged and
> only sysrq-g could get the breakpoint.

Alright.   We can just always have it on SYSRQ+KGDB then.

-- 
Tom Rini
http://gate.crashing.org/~trini/

[PATCH][0/6] A different KGDB stub

[Tom Rini]

Hi Andrew.  The following is my next attempt at a different KGDB stub
for your tree (and then hopefully into kernel.org).  This is against
2.6.3-rc4 + bk-netdev-rc3 (from your tree), and nothing else.  This has
been tested on PPC32 (KGDB=y/n) and i386 (KGDB=y and allmodconfig) and
not at all on x86_64 (no time to build a toolchain yet).  Since the last
time, I've done the following:
- SysRq-G is always part of SYSRQ && KGDB.
- choice for I/O (8250 serial, kgdboe and PPC 'simple' serial, for now).
- On x86_64 / PPC32, we don't bother with CHK_DEBUGGER, but instead
  always use the arch provided debugger hooks.

Thre are 6 different patches:
core.patch: All of the non-arch specific bits, that aren't drivers.
8250.patch: The i/o driver for KGDB, via a standard PC uart.
kgdboe.patch: The i/o driver for KGDB, via netpoll.
i386.patch: The i386-specific code, tested.
ppc32.patch: The ppc32-specific code, tested.
x86_64.patch: The x86_64-specific bits, untested.

One last note.  For some reason, kgdboe isn't working for me right now.
But it doesn't look like it's something specific to the kgdboe driver
(Dropping in the -mm one and changing names to match still shows it, and
the serial driver is solid) so I'm not sure if it's anything other than
my bad luck today.  So if nothing else, if someone else could give this
a shot and let me know if it works for them...

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [PATCH][0/6] A different KGDB stub

[Andrew Morton]

Tom Rini <trini@kernel.crashing.org> wrote:
>
> The following is my next attempt at a different KGDB stub
> for your tree

Is this the patch which everyone agrees on?

Re: [PATCH][0/6] A different KGDB stub

[Pavel Machek]

Hi!

> > The following is my next attempt at a different KGDB stub
> > for your tree
> 
> Is this the patch which everyone agrees on?

It is based on Amit's version, so I think answer is "yes". I certainly
like this one.
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [PATCH][0/6] A different KGDB stub

[Amit S. Kale]

On Wednesday 18 Feb 2004 4:22 am, Pavel Machek wrote:
> Hi!
>
> > > The following is my next attempt at a different KGDB stub
> > > for your tree
> >
> > Is this the patch which everyone agrees on?
>
> It is based on Amit's version, so I think answer is "yes". I certainly
> like this one.

I don't agree. I did a few more cleanups after Andi expressed concerns over 
globals kgdb_memerr and debugger_memerr_expected.

I liked Pavel's approach. Let's first get a minimal kgdb stub into mainline 
kernel. Even this much is going to involve some effort. We can merge other 
features later.

Let's create a cvs tree at kgdb.sourceforge.net for kgdb components to be 
pushed int mainline kernel. This split is to keep current kgdb unaffected. 
People who are already using it won't be affected.

May I suggest we breakup this task into following tasklets. I have expanded 
item 1 because Pavel has something that's already close. The rest of the 
items can be discussed in detail later. These need not be done in this order 
except for first 2 whose sequence is fixed.

1. A minimal kgdb stub
  core.patch: 
    kgdbstub.c full.
    No changes to module.c 
    No changes for CONFIG_KGDB_THREAD
    No changes to calling convention of do_IRQ (Needs to be done)
    CONFIG_KGDB_CONSOLE removed
  i386.patch
    No changes for CONFIG_KGDB_THREAD
    No manipulation of kernel stack before entry into do_IRQ
    No non-source level CFI directives.
2. Minimal x86_64.patch 
3. Patch to sync ppc kgdb with arch independent stub
4. Patch to sync other architecture kgdbs with arch independent stub on help 
from maintainers of those architectures.
5. KGDB_CONSOLE patch
   This is a must for embedded boards that have only one serial port
6. gdb automatic module loading
7. CONFIG_KGDB_THREAD patch
   This may or may not be a separate config option. This patch will include 
x86_64 support required to enable threads.
8. i386 thread support
9. Ethernet interface based on netconsole
10. ... Any other features
-- 
Amit Kale
EmSysSoft (http://www.emsyssoft.com)
KGDB: Linux Kernel Source Level Debugger (http://kgdb.sourceforge.net)

Re: [PATCH][0/6] A different KGDB stub

[Andrew Morton]

"Amit S. Kale" <amitkale@emsyssoft.com> wrote:
>
> 9. Ethernet interface based on netconsole

This will be simplified when the netpoll infrastructure is merged up.
Hopefully that will happen very soon.

Re: [PATCH][0/6] A different KGDB stub

[Pavel Machek]

Hi!

> > > > The following is my next attempt at a different KGDB stub
> > > > for your tree
> > >
> > > Is this the patch which everyone agrees on?
> >
> > It is based on Amit's version, so I think answer is "yes". I certainly
> > like this one.
> 
> I don't agree. I did a few more cleanups after Andi expressed concerns over 
> globals kgdb_memerr and debugger_memerr_expected.
> 
> I liked Pavel's approach. Let's first get a minimal kgdb stub into mainline 
> kernel. Even this much is going to involve some effort. We can merge other 
> features later.
> 
> Let's create a cvs tree at kgdb.sourceforge.net for kgdb components to be 
> pushed int mainline kernel. This split is to keep current kgdb unaffected. 
> People who are already using it won't be affected.

I do not think we want separate CVS tree.

What about simply splitting core.patch into core-lite.patch and
core.patch, maybe do the same with i386 patch, and be done with that?
[We do not have enough people for a fork, I think].

Hopefully soon after that *-lite is merged, so it disappears, and
stuff is easy once again.
									Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [PATCH][0/6] A different KGDB stub

[Amit S. Kale]

On Wednesday 18 Feb 2004 6:26 pm, Pavel Machek wrote:
> Hi!
>
> > > > > The following is my next attempt at a different KGDB stub
> > > > > for your tree
> > > >
> > > > Is this the patch which everyone agrees on?
> > >
> > > It is based on Amit's version, so I think answer is "yes". I certainly
> > > like this one.
> >
> > I don't agree. I did a few more cleanups after Andi expressed concerns
> > over globals kgdb_memerr and debugger_memerr_expected.
> >
> > I liked Pavel's approach. Let's first get a minimal kgdb stub into
> > mainline kernel. Even this much is going to involve some effort. We can
> > merge other features later.
> >
> > Let's create a cvs tree at kgdb.sourceforge.net for kgdb components to be
> > pushed int mainline kernel. This split is to keep current kgdb
> > unaffected. People who are already using it won't be affected.
>
> I do not think we want separate CVS tree.
>
> What about simply splitting core.patch into core-lite.patch and
> core.patch, maybe do the same with i386 patch, and be done with that?
> [We do not have enough people for a fork, I think].

Agreed. Let's create core-lite.patch and i386-lite.patch
It makes it somewhat difficult to maintain them, but should be easier than 
maintaining a separate CVS tree.

>
> Hopefully soon after that *-lite is merged, so it disappears, and
> stuff is easy once again.

Yes.
-- 
Amit Kale
EmSysSoft (http://www.emsyssoft.com)
KGDB: Linux Kernel Source Level Debugger (http://kgdb.sourceforge.net)

Re: [PATCH][0/6] A different KGDB stub

[Tom Rini]

On Wed, Feb 18, 2004 at 10:26:29AM +0530, Amit S. Kale wrote:
> On Wednesday 18 Feb 2004 4:22 am, Pavel Machek wrote:
> > Hi!
> >
> > > > The following is my next attempt at a different KGDB stub
> > > > for your tree
> > >
> > > Is this the patch which everyone agrees on?
> >
> > It is based on Amit's version, so I think answer is "yes". I certainly
> > like this one.
> 
> I don't agree. I did a few more cleanups after Andi expressed concerns over 
> globals kgdb_memerr and debugger_memerr_expected.
> 
> I liked Pavel's approach. Let's first get a minimal kgdb stub into mainline 
> kernel. Even this much is going to involve some effort. We can merge other 
> features later.
> 
> Let's create a cvs tree at kgdb.sourceforge.net for kgdb components to be 
> pushed int mainline kernel. This split is to keep current kgdb unaffected. 
> People who are already using it won't be affected.
> 
> May I suggest we breakup this task into following tasklets. I have expanded 
> item 1 because Pavel has something that's already close. The rest of the 
> items can be discussed in detail later. These need not be done in this order 
> except for first 2 whose sequence is fixed.
> 
> 1. A minimal kgdb stub
>   core.patch: 
>     kgdbstub.c full.
>     No changes to module.c 
>     No changes for CONFIG_KGDB_THREAD
>     No changes to calling convention of do_IRQ (Needs to be done)
>     CONFIG_KGDB_CONSOLE removed
>   i386.patch
>     No changes for CONFIG_KGDB_THREAD
>     No manipulation of kernel stack before entry into do_IRQ
>     No non-source level CFI directives.

My question is now, is there anything in CVS other than patches?
There's still a whole host of cleanups that need to be done to your
version that I've got around here.

> 4. Patch to sync other architecture kgdbs with arch independent stub on help 
> from maintainers of those architectures.
> 5. KGDB_CONSOLE patch
>    This is a must for embedded boards that have only one serial port
> 6. gdb automatic module loading
> 7. CONFIG_KGDB_THREAD patch
>    This may or may not be a separate config option. This patch will include 
> x86_64 support required to enable threads.
> 8. i386 thread support
> 9. Ethernet interface based on netconsole
> 10. ... Any other features

Personally, I'd swap 6 and 9.

-- 
Tom Rini
http://gate.crashing.org/~trini/

Re: [PATCH][0/6] A different KGDB stub

[Pavel Machek]

Hi!

> > > > > > The following is my next attempt at a different KGDB stub
> > > > > > for your tree
> > > > >
> > > > > Is this the patch which everyone agrees on?
> > > >
> > > > It is based on Amit's version, so I think answer is "yes". I certainly
> > > > like this one.
> > >
> > > I don't agree. I did a few more cleanups after Andi expressed concerns
> > > over globals kgdb_memerr and debugger_memerr_expected.
> > >
> > > I liked Pavel's approach. Let's first get a minimal kgdb stub into
> > > mainline kernel. Even this much is going to involve some effort. We can
> > > merge other features later.
> > >
> > > Let's create a cvs tree at kgdb.sourceforge.net for kgdb components to be
> > > pushed int mainline kernel. This split is to keep current kgdb
> > > unaffected. People who are already using it won't be affected.
> >
> > I do not think we want separate CVS tree.
> >
> > What about simply splitting core.patch into core-lite.patch and
> > core.patch, maybe do the same with i386 patch, and be done with that?
> > [We do not have enough people for a fork, I think].
> 
> Agreed. Let's create core-lite.patch and i386-lite.patch
> It makes it somewhat difficult to maintain them, but should be easier than 
> maintaining a separate CVS tree.

There's tool called quilt that is pretty good at exactly this. (It is
small enough to do by hand, but ...)

								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [PATCH][0/6] A different KGDB stub

[Amit S. Kale]

On Wednesday 18 Feb 2004 11:51 pm, Tom Rini wrote:
> On Wed, Feb 18, 2004 at 10:26:29AM +0530, Amit S. Kale wrote:
> > On Wednesday 18 Feb 2004 4:22 am, Pavel Machek wrote:
> > > Hi!
> > >
> > > > > The following is my next attempt at a different KGDB stub
> > > > > for your tree
> > > >
> > > > Is this the patch which everyone agrees on?
> > >
> > > It is based on Amit's version, so I think answer is "yes". I certainly
> > > like this one.
> >
> > I don't agree. I did a few more cleanups after Andi expressed concerns
> > over globals kgdb_memerr and debugger_memerr_expected.
> >
> > I liked Pavel's approach. Let's first get a minimal kgdb stub into
> > mainline kernel. Even this much is going to involve some effort. We can
> > merge other features later.
> >
> > Let's create a cvs tree at kgdb.sourceforge.net for kgdb components to be
> > pushed int mainline kernel. This split is to keep current kgdb
> > unaffected. People who are already using it won't be affected.
> >
> > May I suggest we breakup this task into following tasklets. I have
> > expanded item 1 because Pavel has something that's already close. The
> > rest of the items can be discussed in detail later. These need not be
> > done in this order except for first 2 whose sequence is fixed.
> >
> > 1. A minimal kgdb stub
> >   core.patch:
> >     kgdbstub.c full.
> >     No changes to module.c
> >     No changes for CONFIG_KGDB_THREAD
> >     No changes to calling convention of do_IRQ (Needs to be done)
> >     CONFIG_KGDB_CONSOLE removed
> >   i386.patch
> >     No changes for CONFIG_KGDB_THREAD
> >     No manipulation of kernel stack before entry into do_IRQ
> >     No non-source level CFI directives.
>
> My question is now, is there anything in CVS other than patches?

No.
The CVS tree contains patches only.
What I have described above are features of two patches: core.patch and 
i386.patch.

> There's still a whole host of cleanups that need to be done to your
> version that I've got around here.

You are welcome to bring them into the cvs tree.
Thanks.
-Amit

>
> > 4. Patch to sync other architecture kgdbs with arch independent stub on
> > help from maintainers of those architectures.
> > 5. KGDB_CONSOLE patch
> >    This is a must for embedded boards that have only one serial port
> > 6. gdb automatic module loading
> > 7. CONFIG_KGDB_THREAD patch
> >    This may or may not be a separate config option. This patch will
> > include x86_64 support required to enable threads.
> > 8. i386 thread support
> > 9. Ethernet interface based on netconsole
> > 10. ... Any other features
>
> Personally, I'd swap 6 and 9.