Re: Re: [PATCH 1 of 3] xen: Automatically find serial port on PCI/PCIe and AMT devices

Re: Re: [PATCH 1 of 3] xen: Automatically find serial port on PCI/PCIe and AMT devices

[Jan Beulich]

[-- Attachment #1.1: Type: text/plain, Size: 930 bytes --]

>>> Konrad Rzeszutek Wilk  07/07/11 6:36 PM >>>
>On Thu, Jul 07, 2011 at 04:56:23PM +0100, Jan Beulich wrote:
>> >>> On 07.07.11 at 15:59, Konrad Rzeszutek Wilk  wrote:
>> > +              bar0 = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);
>> 
>> Why would a serial port only be allowed to be on the port specified
>> with BAR0? E.g. if you have a serial card with multiple ports, multiple
>> BARs could be candidates.
>> 
>> Also, why would the first one found be it?
>
>Because that is usually COM1. You can still do com1=115200,8n1,magic
>com2=115200,8n1,0xd900,0 to have both of them available.

But the handling here is being called for com1= and com2=
equally. For something not just for somebody's special purposes,
the index from the calling function should minimally be passed
(and the BAR to look at adjusted accordingly).

Also I wonder why the token is "magic" rather than, say, "pci".

Jan


[-- Attachment #1.2: HTML --]
[-- Type: text/html, Size: 1350 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Re: [PATCH 1 of 3] xen: Automatically find serial port on PCI/PCIe and AMT devices

[Konrad Rzeszutek Wilk]

On Fri, Jul 08, 2011 at 08:04:40AM +0100, Jan Beulich wrote:
> >>> Konrad Rzeszutek Wilk  07/07/11 6:36 PM >>>
> >On Thu, Jul 07, 2011 at 04:56:23PM +0100, Jan Beulich wrote:
> >> >>> On 07.07.11 at 15:59, Konrad Rzeszutek Wilk  wrote:
> >> > +              bar0 = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);
> >> 
> >> Why would a serial port only be allowed to be on the port specified
> >> with BAR0? E.g. if you have a serial card with multiple ports, multiple
> >> BARs could be candidates.
> >> 
> >> Also, why would the first one found be it?
> >
> >Because that is usually COM1. You can still do com1=115200,8n1,magic
> >com2=115200,8n1,0xd900,0 to have both of them available.
> 
> But the handling here is being called for com1= and com2=
> equally. For something not just for somebody's special purposes,
> the index from the calling function should minimally be passed
> (and the BAR to look at adjusted accordingly).

<nods> Will prep a patch.
> 
> Also I wonder why the token is "magic" rather than, say, "pci".

No idea, but 'pci' sounds much better. Will prep up a patch for that.
> 
> Jan
> 

[PATCH 0 of 3] Patches for PCI serial cards (v1).

[Konrad Rzeszutek Wilk]

Hey,

I've been carrying these patches for some time and with Andrew's work on the KEXEC
it seems like a perfect time to provide them (as they save the BDF of the PCI
serial card and could be extended to provide that)

The first one is actually from XCP product and I think I got the right author
name for it - if not please correct me.

The other three patches are rebased patches that I've received from VirtualComputer
(www.virtualcomputer.com). They seem to find lot of issues on laptops/desktops and have been
using PCI express serial cards, MiniPCI-e serial cards, etc to get the serial output
to troubleshoot. And as such found some issues after a S3 resume which these patches fix.

Please take a look.

[PATCH 1 of 3] xen: Automatically find serial port on PCI/PCIe and AMT devices

[Konrad Rzeszutek Wilk]

# HG changeset patch
# User James Mckenzie <jamesmck@bob.uk.xensource.com>
# Date 1310047149 14400
# Node ID 7cdc5770d13bbd7fc2b958ba7d74787ff4e20eef
# Parent  2f63562df1c4230492a81793dce3672f93c93d9a
xen: Automatically find serial port on PCI/PCIe and AMT devices.

Instead of having to manually look the right I/O port on the PCI
devices or AMT devices, lets probe the card and find that
automatically.

This means that you don't have to have this:
 com1=115200,8n1,0xd800,0

But instead can have
 com1=115200,8n1,magic

Or if you have AMT:
 com1=19200,8n1,amt

Signed-off-by: James Mckenzie <jamesmck@bob.uk.xensource.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Tom Goetz <tom.goetz@virtualcomputer.com>

diff -r 2f63562df1c4 -r 7cdc5770d13b xen/drivers/char/ns16550.c
--- a/xen/drivers/char/ns16550.c	Mon Jun 27 17:37:12 2011 +0100
+++ b/xen/drivers/char/ns16550.c	Thu Jul 07 09:59:09 2011 -0400
@@ -17,6 +17,8 @@
 #include <xen/timer.h>
 #include <xen/serial.h>
 #include <xen/iocap.h>
+#include <xen/pci.h>
+#include <xen/pci_regs.h>
 #include <asm/io.h>
 
 /*
@@ -433,6 +435,64 @@ static int __init check_existence(struct
     return (status == 0x90);
 }
 
+static int
+magic_uart_config (struct ns16550 *uart,int skip_amt)
+{
+  uint16_t class;
+  uint32_t bar0, len;
+  int b, d, f;
+
+/*Skanky hack - start at bus 1 to avoid AMT, a plug in card cannot be on bus 1 */
+
+  if (skip_amt) b=1;
+	else b=0;
+
+  for (; b < 0x100; ++b)
+    {
+    for (d = 0; d < 0x20; ++d)
+        {
+          for (f = 0; f < 0x8; ++f)
+            {
+
+              class = pci_conf_read16 (b, d, f, PCI_CLASS_DEVICE);
+              if (class != 0x700)
+                continue;;
+
+              bar0 = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);
+
+              /* Not IO */
+              if (!(bar0 & 1))
+                continue;
+
+              pci_conf_write32 (b, d, f, PCI_BASE_ADDRESS_0, 0xffffffff);
+              len = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);
+              pci_conf_write32 (b, d, f, PCI_BASE_ADDRESS_0, bar0);
+
+              /* Not 8 bytes */
+              if ((len & 0xffff) != 0xfff9)
+                continue;
+
+              uart->io_base = bar0 & 0xfffe;
+              uart->irq = 0;
+
+              return 0;
+
+            }
+
+        }
+    }
+
+  if (!skip_amt)
+	return -1;
+
+  uart->io_base = 0x3f8;
+  uart->irq = 0;
+  uart->clock_hz  = UART_CLOCK_HZ;
+
+  return 0;
+}
+
+
 #define PARSE_ERR(_f, _a...)                 \
     do {                                     \
         printk( "ERROR: " _f "\n" , ## _a ); \
@@ -481,7 +541,18 @@ static void __init ns16550_parse_port_co
     if ( *conf == ',' )
     {
         conf++;
-        uart->io_base = simple_strtoul(conf, &conf, 0);
+        
+        if ( strncmp(conf,"magic",5) == 0 ) {
+	    if (magic_uart_config(uart,1)) 
+		return;
+	    conf+=5;
+        } else if ( strncmp(conf,"amt",3) == 0 ) {
+	    if (magic_uart_config(uart,0)) 
+		return;
+	    conf+=3;
+        } else {
+            uart->io_base = simple_strtoul(conf, &conf, 0);
+        }
 
         if ( *conf == ',' )
         {

Re: [PATCH 1 of 3] xen: Automatically find serial port on PCI/PCIe and AMT devices

[Jan Beulich]

>>> On 07.07.11 at 15:59, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> # HG changeset patch
> # User James Mckenzie <jamesmck@bob.uk.xensource.com>
> # Date 1310047149 14400
> # Node ID 7cdc5770d13bbd7fc2b958ba7d74787ff4e20eef
> # Parent  2f63562df1c4230492a81793dce3672f93c93d9a
> xen: Automatically find serial port on PCI/PCIe and AMT devices.
> 
> Instead of having to manually look the right I/O port on the PCI
> devices or AMT devices, lets probe the card and find that
> automatically.
> 
> This means that you don't have to have this:
>  com1=115200,8n1,0xd800,0
> 
> But instead can have
>  com1=115200,8n1,magic
> 
> Or if you have AMT:
>  com1=19200,8n1,amt
> 
> Signed-off-by: James Mckenzie <jamesmck@bob.uk.xensource.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Tom Goetz <tom.goetz@virtualcomputer.com>
> 
> diff -r 2f63562df1c4 -r 7cdc5770d13b xen/drivers/char/ns16550.c
> --- a/xen/drivers/char/ns16550.c	Mon Jun 27 17:37:12 2011 +0100
> +++ b/xen/drivers/char/ns16550.c	Thu Jul 07 09:59:09 2011 -0400
> @@ -17,6 +17,8 @@
>  #include <xen/timer.h>
>  #include <xen/serial.h>
>  #include <xen/iocap.h>
> +#include <xen/pci.h>
> +#include <xen/pci_regs.h>
>  #include <asm/io.h>
>  
>  /*
> @@ -433,6 +435,64 @@ static int __init check_existence(struct
>      return (status == 0x90);
>  }
>  
> +static int
> +magic_uart_config (struct ns16550 *uart,int skip_amt)
> +{
> +  uint16_t class;
> +  uint32_t bar0, len;
> +  int b, d, f;
> +
> +/*Skanky hack - start at bus 1 to avoid AMT, a plug in card cannot be on bus 
> 1 */
> +
> +  if (skip_amt) b=1;
> +	else b=0;
> +
> +  for (; b < 0x100; ++b)
> +    {
> +    for (d = 0; d < 0x20; ++d)
> +        {
> +          for (f = 0; f < 0x8; ++f)
> +            {
> +
> +              class = pci_conf_read16 (b, d, f, PCI_CLASS_DEVICE);
> +              if (class != 0x700)
> +                continue;;
> +
> +              bar0 = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);

Why would a serial port only be allowed to be on the port specified
with BAR0? E.g. if you have a serial card with multiple ports, multiple
BARs could be candidates.

Also, why would the first one found be it?

Jan

> +
> +              /* Not IO */
> +              if (!(bar0 & 1))
> +                continue;
> +
> +              pci_conf_write32 (b, d, f, PCI_BASE_ADDRESS_0, 0xffffffff);
> +              len = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);
> +              pci_conf_write32 (b, d, f, PCI_BASE_ADDRESS_0, bar0);
> +
> +              /* Not 8 bytes */
> +              if ((len & 0xffff) != 0xfff9)
> +                continue;
> +
> +              uart->io_base = bar0 & 0xfffe;
> +              uart->irq = 0;

Hmm, I found that without use of an interrupt the output isn't very
reliable. But yes, getting the proper number could be pretty difficult,
especially this early.

Jan

> +
> +              return 0;
> +
> +            }
> +
> +        }
> +    }
> +
> +  if (!skip_amt)
> +	return -1;
> +
> +  uart->io_base = 0x3f8;
> +  uart->irq = 0;
> +  uart->clock_hz  = UART_CLOCK_HZ;
> +
> +  return 0;
> +}
> +
> +
>  #define PARSE_ERR(_f, _a...)                 \
>      do {                                     \
>          printk( "ERROR: " _f "\n" , ## _a ); \
> @@ -481,7 +541,18 @@ static void __init ns16550_parse_port_co
>      if ( *conf == ',' )
>      {
>          conf++;
> -        uart->io_base = simple_strtoul(conf, &conf, 0);
> +        
> +        if ( strncmp(conf,"magic",5) == 0 ) {
> +	    if (magic_uart_config(uart,1)) 
> +		return;
> +	    conf+=5;
> +        } else if ( strncmp(conf,"amt",3) == 0 ) {
> +	    if (magic_uart_config(uart,0)) 
> +		return;
> +	    conf+=3;
> +        } else {
> +            uart->io_base = simple_strtoul(conf, &conf, 0);
> +        }
>  
>          if ( *conf == ',' )
>          {

Re: Re: [PATCH 1 of 3] xen: Automatically find serial port on PCI/PCIe and AMT devices

[Konrad Rzeszutek Wilk]

On Thu, Jul 07, 2011 at 04:56:23PM +0100, Jan Beulich wrote:
> >>> On 07.07.11 at 15:59, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > # HG changeset patch
> > # User James Mckenzie <jamesmck@bob.uk.xensource.com>
> > # Date 1310047149 14400
> > # Node ID 7cdc5770d13bbd7fc2b958ba7d74787ff4e20eef
> > # Parent  2f63562df1c4230492a81793dce3672f93c93d9a
> > xen: Automatically find serial port on PCI/PCIe and AMT devices.
> > 
> > Instead of having to manually look the right I/O port on the PCI
> > devices or AMT devices, lets probe the card and find that
> > automatically.
> > 
> > This means that you don't have to have this:
> >  com1=115200,8n1,0xd800,0
> > 
> > But instead can have
> >  com1=115200,8n1,magic
> > 
> > Or if you have AMT:
> >  com1=19200,8n1,amt
> > 
> > Signed-off-by: James Mckenzie <jamesmck@bob.uk.xensource.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Signed-off-by: Tom Goetz <tom.goetz@virtualcomputer.com>
> > 
> > diff -r 2f63562df1c4 -r 7cdc5770d13b xen/drivers/char/ns16550.c
> > --- a/xen/drivers/char/ns16550.c	Mon Jun 27 17:37:12 2011 +0100
> > +++ b/xen/drivers/char/ns16550.c	Thu Jul 07 09:59:09 2011 -0400
> > @@ -17,6 +17,8 @@
> >  #include <xen/timer.h>
> >  #include <xen/serial.h>
> >  #include <xen/iocap.h>
> > +#include <xen/pci.h>
> > +#include <xen/pci_regs.h>
> >  #include <asm/io.h>
> >  
> >  /*
> > @@ -433,6 +435,64 @@ static int __init check_existence(struct
> >      return (status == 0x90);
> >  }
> >  
> > +static int
> > +magic_uart_config (struct ns16550 *uart,int skip_amt)
> > +{
> > +  uint16_t class;
> > +  uint32_t bar0, len;
> > +  int b, d, f;
> > +
> > +/*Skanky hack - start at bus 1 to avoid AMT, a plug in card cannot be on bus 
> > 1 */
> > +
> > +  if (skip_amt) b=1;
> > +	else b=0;
> > +
> > +  for (; b < 0x100; ++b)
> > +    {
> > +    for (d = 0; d < 0x20; ++d)
> > +        {
> > +          for (f = 0; f < 0x8; ++f)
> > +            {
> > +
> > +              class = pci_conf_read16 (b, d, f, PCI_CLASS_DEVICE);
> > +              if (class != 0x700)
> > +                continue;;
> > +
> > +              bar0 = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);
> 
> Why would a serial port only be allowed to be on the port specified
> with BAR0? E.g. if you have a serial card with multiple ports, multiple
> BARs could be candidates.
> 
> Also, why would the first one found be it?

Because that is usually COM1. You can still do com1=115200,8n1,magic
com2=115200,8n1,0xd900,0 to have both of them available.

> 
> Jan
> 
> > +
> > +              /* Not IO */
> > +              if (!(bar0 & 1))
> > +                continue;
> > +
> > +              pci_conf_write32 (b, d, f, PCI_BASE_ADDRESS_0, 0xffffffff);
> > +              len = pci_conf_read32 (b, d, f, PCI_BASE_ADDRESS_0);
> > +              pci_conf_write32 (b, d, f, PCI_BASE_ADDRESS_0, bar0);
> > +
> > +              /* Not 8 bytes */
> > +              if ((len & 0xffff) != 0xfff9)
> > +                continue;
> > +
> > +              uart->io_base = bar0 & 0xfffe;
> > +              uart->irq = 0;
> 
> Hmm, I found that without use of an interrupt the output isn't very
> reliable. But yes, getting the proper number could be pretty difficult,
> especially this early.

Yes. The patch does not remove the option of doing 'com1=115200,8n1,5'
to use IRQ 5 for the standard port. But using the PCI serial interrupt
line then.. not so much.


> 
> Jan
> 
> > +
> > +              return 0;
> > +
> > +            }
> > +
> > +        }
> > +    }
> > +
> > +  if (!skip_amt)
> > +	return -1;
> > +
> > +  uart->io_base = 0x3f8;
> > +  uart->irq = 0;
> > +  uart->clock_hz  = UART_CLOCK_HZ;
> > +
> > +  return 0;
> > +}
> > +
> > +
> >  #define PARSE_ERR(_f, _a...)                 \
> >      do {                                     \
> >          printk( "ERROR: " _f "\n" , ## _a ); \
> > @@ -481,7 +541,18 @@ static void __init ns16550_parse_port_co
> >      if ( *conf == ',' )
> >      {
> >          conf++;
> > -        uart->io_base = simple_strtoul(conf, &conf, 0);
> > +        

Ugh. Looks like a big whitespace issue there
> > +        if ( strncmp(conf,"magic",5) == 0 ) {
> > +	    if (magic_uart_config(uart,1)) 

Ditto
> > +		return;
> > +	    conf+=5;
> > +        } else if ( strncmp(conf,"amt",3) == 0 ) {
> > +	    if (magic_uart_config(uart,0)) 

And here.

I can repost it with those fixed.
> > +		return;
> > +	    conf+=3;
> > +        } else {
> > +            uart->io_base = simple_strtoul(conf, &conf, 0);
> > +        }
> >  
> >          if ( *conf == ',' )
> >          {
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel