[PATCH] xen/hvc: Fix polling mode to work with kdb/kgdb

[PATCH] xen/hvc: Fix polling mode to work with kdb/kgdb

[Ben Guthro]

Fix the polling section of the hvc driver to use the global "last_hvc"
variable, rather than the ttys.

With this change debugging a xen dom0 kernel is possible via the
following kernel parameter:
kgdboc=hvc0

Signed-off-by: Ben Guthro <Benjamin.Guthro@citrix.com>


diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 2d691eb..3750e74 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -766,12 +766,10 @@ int hvc_poll_init(struct tty_driver *driver, int
line, char *options)

 static int hvc_poll_get_char(struct tty_driver *driver, int line)
 {
-       struct tty_struct *tty = driver->ttys[0];
-       struct hvc_struct *hp = tty->driver_data;
        int n;
        char ch;

-       n = hp->ops->get_chars(hp->vtermno, &ch, 1);
+       n = cons_ops[last_hvc]->get_chars(vtermnos[last_hvc], &ch, 1);

        if (n == 0)
                return NO_POLL_CHAR;
@@ -781,12 +779,10 @@ static int hvc_poll_get_char(struct tty_driver
*driver, int line)

 static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
 {
-       struct tty_struct *tty = driver->ttys[0];
-       struct hvc_struct *hp = tty->driver_data;
        int n;

        do {
-               n = hp->ops->put_chars(hp->vtermno, &ch, 1);
+               n = cons_ops[last_hvc]->put_chars(vtermnos[last_hvc], &ch, 1);
        } while (n <= 0);
 }
 #endif

Re: [Xen-devel] [PATCH] xen/hvc: Fix polling mode to work with kdb/kgdb

[Konrad Rzeszutek Wilk]

On Thu, Jun 07, 2012 at 09:30:06AM -0400, Ben Guthro wrote:
> Fix the polling section of the hvc driver to use the global "last_hvc"
> variable, rather than the ttys.

Could you just do:

       struct tty_struct *tty = driver->ttys[last_hvc];

as well? So how come the '0' one did not work? Is that b/c
of console=tty becoming '0' instead of hvc0? Is there a crash
involved with this? Or is that it just is listening on the
wrong console (and which one is that?)
> 
> With this change debugging a xen dom0 kernel is possible via the
> following kernel parameter:
> kgdboc=hvc0

Hm, if that is the problem then this should also be a problem on
IBM Power boxes I would think?

> 
> Signed-off-by: Ben Guthro <Benjamin.Guthro@citrix.com>
> 
> 
> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
> index 2d691eb..3750e74 100644
> --- a/drivers/tty/hvc/hvc_console.c
> +++ b/drivers/tty/hvc/hvc_console.c
> @@ -766,12 +766,10 @@ int hvc_poll_init(struct tty_driver *driver, int
> line, char *options)
> 
>  static int hvc_poll_get_char(struct tty_driver *driver, int line)
>  {
> -       struct tty_struct *tty = driver->ttys[0];
> -       struct hvc_struct *hp = tty->driver_data;
>         int n;
>         char ch;
> 
> -       n = hp->ops->get_chars(hp->vtermno, &ch, 1);
> +       n = cons_ops[last_hvc]->get_chars(vtermnos[last_hvc], &ch, 1);
> 
>         if (n == 0)
>                 return NO_POLL_CHAR;
> @@ -781,12 +779,10 @@ static int hvc_poll_get_char(struct tty_driver
> *driver, int line)
> 
>  static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
>  {
> -       struct tty_struct *tty = driver->ttys[0];
> -       struct hvc_struct *hp = tty->driver_data;
>         int n;
> 
>         do {
> -               n = hp->ops->put_chars(hp->vtermno, &ch, 1);
> +               n = cons_ops[last_hvc]->put_chars(vtermnos[last_hvc], &ch, 1);
>         } while (n <= 0);
>  }
>  #endif
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen/hvc: Fix polling mode to work with kdb/kgdb

[Ben Guthro]

On Thu, Jun 7, 2012 at 12:40 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Thu, Jun 07, 2012 at 09:30:06AM -0400, Ben Guthro wrote:
>> Fix the polling section of the hvc driver to use the global "last_hvc"
>> variable, rather than the ttys.
>
> Could you just do:
>
>       struct tty_struct *tty = driver->ttys[last_hvc];
>
> as well?

No. I tried this, and never got to the kdb prompt.
It seems that the problem is that you need to use the cons_ops variable

My efforts to fully understand the inner-workings of the console code
were thwarted by time. Its a twisty bunch of code.
If I used the cons_ops variable static to the module, it was OK.
If I used driver->ttys - {get,put}_chars() never got called.


> So how come the '0' one did not work? Is that b/c
> of console=tty becoming '0' instead of hvc0? Is there a crash
> involved with this? Or is that it just is listening on the
> wrong console (and which one is that?)

See above.

>>
>> With this change debugging a xen dom0 kernel is possible via the
>> following kernel parameter:
>> kgdboc=hvc0
>
> Hm, if that is the problem then this should also be a problem on
> IBM Power boxes I would think?

Not sure...but I think the original submitter of this code was

commit 762e77ae7dd055d0b77e0ad34d87db7416df109e
Author: Anton Blanchard <anton@samba.org>
Date:   Tue Jul 12 19:44:05 2011 +0000

    hvc_console: Add kdb support


Was that for IBM?

>
>>
>> Signed-off-by: Ben Guthro <Benjamin.Guthro@citrix.com>
>>
>>
>> diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
>> index 2d691eb..3750e74 100644
>> --- a/drivers/tty/hvc/hvc_console.c
>> +++ b/drivers/tty/hvc/hvc_console.c
>> @@ -766,12 +766,10 @@ int hvc_poll_init(struct tty_driver *driver, int
>> line, char *options)
>>
>>  static int hvc_poll_get_char(struct tty_driver *driver, int line)
>>  {
>> -       struct tty_struct *tty = driver->ttys[0];
>> -       struct hvc_struct *hp = tty->driver_data;
>>         int n;
>>         char ch;
>>
>> -       n = hp->ops->get_chars(hp->vtermno, &ch, 1);
>> +       n = cons_ops[last_hvc]->get_chars(vtermnos[last_hvc], &ch, 1);
>>
>>         if (n == 0)
>>                 return NO_POLL_CHAR;
>> @@ -781,12 +779,10 @@ static int hvc_poll_get_char(struct tty_driver
>> *driver, int line)
>>
>>  static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
>>  {
>> -       struct tty_struct *tty = driver->ttys[0];
>> -       struct hvc_struct *hp = tty->driver_data;
>>         int n;
>>
>>         do {
>> -               n = hp->ops->put_chars(hp->vtermno, &ch, 1);
>> +               n = cons_ops[last_hvc]->put_chars(vtermnos[last_hvc], &ch, 1);
>>         } while (n <= 0);
>>  }
>>  #endif
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen/hvc: Fix polling mode to work with kdb/kgdb

[Konrad Rzeszutek Wilk]

> > On Thu, Jun 07, 2012 at 09:30:06AM -0400, Ben Guthro wrote:
> >> Fix the polling section of the hvc driver to use the global "last_hvc"
> >> variable, rather than the ttys.
> >
> > Could you just do:
> >
> >       struct tty_struct *tty = driver->ttys[last_hvc];
> >
> > as well?
> 
> No. I tried this, and never got to the kdb prompt.
> It seems that the problem is that you need to use the cons_ops variable
> 
> My efforts to fully understand the inner-workings of the console code
> were thwarted by time. Its a twisty bunch of code.
> If I used the cons_ops variable static to the module, it was OK.
> If I used driver->ttys - {get,put}_chars() never got called.

Well, now that you guys are working for a big corporation
you can relax and afford to spend some time digging in the
inner-workings I think :-)
.. snip..
> > Hm, if that is the problem then this should also be a problem on
> > IBM Power boxes I would think?
> 
> Not sure...but I think the original submitter of this code was
> 
> commit 762e77ae7dd055d0b77e0ad34d87db7416df109e
> Author: Anton Blanchard <anton@samba.org>
> Date:   Tue Jul 12 19:44:05 2011 +0000
> 
>     hvc_console: Add kdb support
> 
> 
> Was that for IBM?

No. But the 'hvc' system is used on IBM Power machines as well.
Hence my thought that if it didn't work under Xen it probably
didn't work under IBM Power machines either.

Re: [Xen-devel] [PATCH] xen/hvc: Fix polling mode to work with kdb/kgdb

[Konrad Rzeszutek Wilk]

On Thu, Jun 07, 2012 at 02:03:48PM -0400, Ben Guthro wrote:
> On Thu, Jun 7, 2012 at 1:48 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> ...snip...
> 
> > Well, now that you guys are working for a big corporation
> > you can relax and afford to spend some time digging in the
> > inner-workings I think :-)
> 
> I'll pass along that suggestion.
> So far, that hasn't come to pass...
> 
> I understood it when I wrote this months ago. I really should have
> written this up then.
> I'll try to dig in a bit, and report back.

OK. Thanks!

Re: [Xen-devel] [PATCH] xen/hvc: Fix polling mode to work with kdb/kgdb

[Ben Guthro]

On Thu, Jun 7, 2012 at 1:48 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
...snip...

> Well, now that you guys are working for a big corporation
> you can relax and afford to spend some time digging in the
> inner-workings I think :-)

I'll pass along that suggestion.
So far, that hasn't come to pass...

I understood it when I wrote this months ago. I really should have
written this up then.
I'll try to dig in a bit, and report back.

Re: [Xen-devel] [PATCH] xen/hvc: Fix polling mode to work with kdb/kgdb

[Ben Guthro]

On Thu, Jun 7, 2012 at 2:01 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
<snip>
>> I'll try to dig in a bit, and report back.
>
> OK. Thanks!


OK, after throwing a bit of trace in, I remember now, the issue I was
working around.

The tty layer does not seem keep the console open.
It opens, and closes the tty based on whether there is data to print.

However, the CONFIG_CONSOLE_POLL code goes around all of this, and
uses the polling capabilities of the driver.
Most of the drivers that implement this mechanism are serial devices
where they are actually polling the UART.

When Alt+SysRq+g is pressed, the output is flushed, all references to
the hvc are released, and then hvc_close() is called.

However, since this code fully bypasses the tty layer, no reference is
grabbed - and so, when we come into hvc_poll_put_char, and the
necessary structures aren't available.

Below is an updated version of this patch that will use the
tty->driver_data, if available - but will fall back to using cons_ops.

This will guarantee that any existing users of hvc where the above
claim is not true will see no change in behavior.

However...The part of this that I don't really understand why we need
is in kernel/debug/debug_core.c
I don't like having to put the ifdef in there. I was hoping to be able
to remove it, with the upstreaming of the xen IPI code... but in
testing, it seems to still be necessary.



diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 2d691eb..7f965e1 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -767,11 +767,14 @@ int hvc_poll_init(struct tty_driver *driver, int
line, char *options)
 static int hvc_poll_get_char(struct tty_driver *driver, int line)
 {
        struct tty_struct *tty = driver->ttys[0];
-       struct hvc_struct *hp = tty->driver_data;
+       struct hvc_struct *hp = tty ? tty->driver_data : NULL;
+       struct hv_ops *ops = (hp && hp->ops) ? hp->ops : cons_ops[last_hvc];
+       uint32_t vtno = hp ? hp->vtermno : vtermnos[last_hvc];
+
        int n;
        char ch;

-       n = hp->ops->get_chars(hp->vtermno, &ch, 1);
+       n = ops->get_chars(vtno, &ch, 1);

        if (n == 0)
                return NO_POLL_CHAR;
@@ -782,11 +785,14 @@ static int hvc_poll_get_char(struct tty_driver
*driver, int line)
 static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
 {
        struct tty_struct *tty = driver->ttys[0];
-       struct hvc_struct *hp = tty->driver_data;
+       struct hvc_struct *hp = tty ? tty->driver_data : NULL;
+       struct hv_ops *ops = (hp && hp->ops) ? hp->ops : cons_ops[last_hvc];
+       uint32_t vtno = hp ? hp->vtermno : vtermnos[last_hvc];
+
        int n;

        do {
-               n = hp->ops->put_chars(hp->vtermno, &ch, 1);
+               n = ops->put_chars(vtno, &ch, 1);
        } while (n <= 0);
 }
 #endif
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 0557f24..853b1d7 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -579,12 +579,14 @@ return_normal:
                kgdb_roundup_cpus(flags);
 #endif

+#ifndef CONFIG_XEN
        /*
         * Wait for the other CPUs to be notified and be waiting for us:
         */
        while (kgdb_do_roundup && (atomic_read(&masters_in_kgdb) +
                                atomic_read(&slaves_in_kgdb)) != online_cpus)
                cpu_relax();
+#endif

        /*
         * At this point the primary processor is completely

Re: [Xen-devel] [PATCH] xen/hvc: Fix polling mode to work with kdb/kgdb

[Ben Guthro]

On Thu, Jun 7, 2012 at 4:07 PM, Ben Guthro <ben@guthro.net> wrote:

<snip>

> +#ifndef CONFIG_XEN
>        /*
>         * Wait for the other CPUs to be notified and be waiting for us:
>         */
>        while (kgdb_do_roundup && (atomic_read(&masters_in_kgdb) +
>                                atomic_read(&slaves_in_kgdb)) != online_cpus)
>                cpu_relax();
> +#endif
>
>        /*
>         * At this point the primary processor is completely


The cpu roundup seems to not be working as expected here.
While I get into the kgdb exception code on the master cpu - none of
the slave cpus ever get into the code that is counting them up.

Ignoring this CPU roundup is useful to get into the kdb code, but
probably not correct.

Does you have any thoughts as to why I might not be getting into this
code under Xen?