[PATCH v2] Enable serial output for Oxford Semiconductor PCIe cards and fixes.

[PATCH v2] Enable serial output for Oxford Semiconductor PCIe cards and fixes.

[Konrad Rzeszutek Wilk]

Since v1 [http://mid.gmane.org/1394040334-16278-1-git-send-email-konrad.wilk@oracle.com]:
 - Update it per Jan and Andrew comments
 - Tackle on two bug-fixes.


Hey,

This is the second, posting of the changes to the ns16550 driver
to also support other chipsets - like the Oxford 16C950 one - popular on
the inexpensive Rosewill RC-300E.

There are also two bug-fixes that I discovered while testing my code.
One of them was found through code inspection while the other was
when I was testing my code.

Of interest is:
 [PATCH v2 3/7] serial: Support OXPCIe952 aka Oxford Semiconductor

 that enables the functionality on the PCIe card I have - while the rest
of the patches flesh out the code to support more of PCI and PCIe type cards.

 xen/arch/x86/oprofile/op_model_athlon.c |    5 +-
 xen/arch/x86/x86_64/mmconf-fam10h.c     |    1 +
 xen/arch/x86/x86_64/mmconfig-shared.c   |    1 +
 xen/arch/x86/x86_64/mmconfig.h          |    7 -
 xen/drivers/char/ns16550.c              |  254 ++++++++++++++++++++++++++++--
 xen/include/xen/pci_ids.h               |   12 ++
 6 files changed, 253 insertions(+), 27 deletions(-)

Konrad Rzeszutek Wilk (7):
      serial: Skip over PCIe device which have no quirks (fix AMT regression).
      serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.
      serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138)
      serial: Seperate the PCI device ids and parameters
      serial: Use #defines for PCI vendors.
      serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART
      pci: Put all PCI device vendor and models in one file.

[PATCH v2 1/7] serial: Skip over PCIe device which have no quirks (fix AMT regression).

[Konrad Rzeszutek Wilk]

The "ns16550: Add support for UART present in Broadcom TruManage
capable NetXtreme chips" implies that only devices that are have
an MMIO BAR and are in the quirks table should be processed.

Even the comment at the end says so:

 If we have an io_base, then we succeeded in the lookup

But the code was checking for the !io_base - which is to say if
the io_base was 0 then we would skip scanning. But io_base
always has a value - it is set by 'ns16550_init' to a default
value - so it would never hit the 'continue' path.

This means that if we have an communication device followed by
a serial AMT device we would pick the communication device instead
of the AMT device.

See:
00:16.0 Communication controller: Intel Corporation Cougar Point HECI Controller #1 (rev 04)
        Subsystem: Intel Corporation Device 2008
        Flags: bus master, fast devsel, latency 0, IRQ 11
        Memory at fb12a000 (64-bit, non-prefetchable) [size=16]
00:16.3 Serial controller: Intel Corporation Cougar Point KT Controller (rev 04) (prog-if 02 [16550])
        Subsystem: Intel Corporation Device 2008
        Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 17
        I/O ports at f0e0 [size=8]
        Memory at fb129000 (32-bit, non-prefetchable) [size=4K]

pci 0000:00:16.0: [8086:1c3a] type 00 class 0x078000
pci 0000:00:16.3: [8086:1c3d] type 00 class 0x070002

And Xen picks 00:16.0 as its console when using 'com1=115200,8n1,amt'.

This patch fixes it and allows us to use AMT again by zeroing
out io_base to zero. If the scan did not work, the io_base is
set back to a default value (the 'pci_uart_config' does that
already at its end).

CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Thomas Lendacky <Thomas.Lendacky@amd.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Keir Fraser <keir@xen.org>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 53e49a1..2fded08 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -616,6 +616,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
     unsigned int b, d, f, nextf, i;
     u16 vendor, device;
 
+    uart->io_base = 0;
     /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
     for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
     {
-- 
1.7.7.6

[PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Konrad Rzeszutek Wilk]

The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
in __setup_xen. Then we call 'ns16550_init' which copies those in
the appropiate uart, which then calls 'ns16550_parse_port_config'
to deal with parameter parsing. If the 'pci' or 'amt' parameter
has been specified we further call 'pci_uart_config code' which
scans the PCI bus. If it does not find any relevant devices
it will overwrite io_base with 0x3f8 regardless whether this is
COM1 or COM2. The overwrite is a way to set it back to the
failsafe defaults - except for COM2 of course.

This in theory (as I don't have a machine with two COM ports
readily available) means that if the user specified 'com2=9600,8n1,pci'
and the device did not have an PCI serial card, instead of using 0x2f8
for the io_base it ends up using 0x3f8 - and we don't get the
output on COM2.

Fix it by saving the original io_base before starting the
scan of the PCI bus. If we don't find an serial PCI device then
assign the original io_base value back.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 2fded08..c2a25da 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -612,10 +612,11 @@ static int __init
 pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
 {
     uint32_t bar, bar_64 = 0, len, len_64;
-    u64 size, mask;
+    u64 size, mask, orig_base;
     unsigned int b, d, f, nextf, i;
     u16 vendor, device;
 
+    orig_base = uart->io_base;
     uart->io_base = 0;
     /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
     for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
@@ -747,7 +748,8 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
     if ( !skip_amt )
         return -1;
 
-    uart->io_base = 0x3f8;
+    if ( !uart->io_base )
+        uart->io_base = orig_base;
     uart->irq = 0;
     uart->clock_hz  = UART_CLOCK_HZ;
 
-- 
1.7.7.6

[PATCH v2 3/7] serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138)

[Konrad Rzeszutek Wilk]

Because they are PCIe and machine nowadys have those instead of
PCI, and they are inexpensive.

Tested with 1415:c138. Should also work on 0xc11f, 0xc11b models
of that chip.

Also on  OXPCIe200 1 Native UART 1415: 0xc40b, 0xc40f, 0xc41b,
0xc41f, 0xc42b, 0xc42f, 0xc43b, 0xc43f, 0xc44b, 0xc44f, 0xc45b
0xc45f, 0xc46b, 0xc46f, 0xc47b, 0xc47f, 0xc48b, 0xc48f, 0xc49b
0xc49f, 0xc4ab, 0xc4af, 0xc4bb, 0xc4bf, 0xc4cb, 0xc4cf

but since I don't have any of those cards this patch does not
enable it.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: Init for ARM and add offset to virt addr]
[v2: Remove the offset usage]
---
 xen/drivers/char/ns16550.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index c2a25da..2a2c3cb 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -89,6 +89,9 @@ struct ns16550_config_mmio {
     unsigned int fifo_size;
     u8 lsr_mask;
     unsigned int max_bars;
+    unsigned int base_baud;
+    unsigned int uart_offset;
+    unsigned int first_offset;
 };
 
 
@@ -111,6 +114,19 @@ static struct ns16550_config_mmio __initdata uart_config[] =
         .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
         .max_bars = 1,
     },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = 0x1415,
+        .dev_id = 0xc138,
+        .base_baud = 4000000,
+        .uart_offset = 0x200,
+        .first_offset = 0x1000,
+        .reg_width = 1,
+        .reg_shift = 0,
+        .fifo_size = 16,
+        .lsr_mask = UART_LSR_THRE,
+        .max_bars = 1, /* It can do more, but we would need more custom code.*/
+    }
 };
 #endif
 
@@ -703,6 +719,10 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
                         uart->lsr_mask = uart_config[i].lsr_mask;
                         uart->io_base = ((u64)bar_64 << 32) |
                                         (bar & PCI_BASE_ADDRESS_MEM_MASK);
+                        uart->io_base += uart_config[i].first_offset;
+                        uart->io_base += bar_idx * uart_config[i].uart_offset;
+                        if ( uart_config[i].base_baud )
+                            uart->clock_hz = uart_config[i].base_baud * 16;
                         /* Set device and MMIO region read only to Dom0 */
                         uart->enable_ro = 1;
                         break;
-- 
1.7.7.6

[PATCH v2 4/7] serial: Seperate the PCI device ids and parameters

[Konrad Rzeszutek Wilk]

This will allow us to re-use the parameters for multiple PCI
devices.

No functional change.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |   67 +++++++++++++++++++++++++++++---------------
 1 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 2a2c3cb..b191a90 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -80,10 +80,14 @@ static struct ns16550 {
 #endif
 } ns16550_com[2] = { { 0 } };
 
-/* Defining uart config options for MMIO devices */
 struct ns16550_config_mmio {
     u16 vendor_id;
     u16 dev_id;
+    unsigned int param;
+};
+
+/* Defining uart config options for MMIO devices */
+struct ns16550_config_param {
     unsigned int reg_shift;
     unsigned int reg_width;
     unsigned int fifo_size;
@@ -96,28 +100,27 @@ struct ns16550_config_mmio {
 
 
 #ifdef HAS_PCI
+enum ns16550_config_param_nr {
+    param_default = 0,
+    param_trumanage,
+    param_oxford,
+};
 /*
  * Create lookup tables for specific MMIO devices..
  * It is assumed that if the device found is MMIO,
  * then you have indexed it here. Else, the driver
  * does nothing.
  */
-static struct ns16550_config_mmio __initdata uart_config[] =
-{
-    /* Broadcom TruManage device */
-    {
-        .vendor_id = 0x14e4,
-        .dev_id = 0x160a,
+static struct ns16550_config_param __initdata uart_param[] = {
+    [param_default] = { }, /* Ignored. */
+    [param_trumanage] = {
         .reg_shift = 2,
         .reg_width = 1,
         .fifo_size = 16,
         .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
         .max_bars = 1,
     },
-    /* OXPCIe952 1 Native UART  */
-    {
-        .vendor_id = 0x1415,
-        .dev_id = 0xc138,
+    [param_oxford] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
@@ -128,6 +131,21 @@ static struct ns16550_config_mmio __initdata uart_config[] =
         .max_bars = 1, /* It can do more, but we would need more custom code.*/
     }
 };
+static struct ns16550_config_mmio __initdata uart_config[] =
+{
+    /* Broadcom TruManage device */
+    {
+        .vendor_id = 0x14e4,
+        .dev_id = 0x160a,
+        .param = param_trumanage,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = 0x1415,
+        .dev_id = 0xc138,
+        .param = param_oxford,
+    }
+};
 #endif
 
 static void ns16550_delayed_resume(void *data);
@@ -692,37 +710,40 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
 
                     size &= -size;
 
-                    /* Check for quirks in uart_config lookup table */
+                    /* Check for params in uart_config lookup table */
                     for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
                     {
+                        unsigned int p;
+
                         if ( uart_config[i].vendor_id != vendor )
                             continue;
 
                         if ( uart_config[i].dev_id != device )
                             continue;
 
+                        p = uart_config[i].param;
                         /*
                          * Force length of mmio region to be at least
                          * 8 bytes times (1 << reg_shift)
                          */
-                        if ( size < (0x8 * (1 << uart_config[i].reg_shift)) )
+                        if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
                             continue;
 
-                        if ( bar_idx >= uart_config[i].max_bars )
+                        if ( bar_idx >= uart_param[p].max_bars )
                             continue;
 
-                        if ( uart_config[i].fifo_size )
-                            uart->fifo_size = uart_config[i].fifo_size;
+                        if ( uart_param[p].fifo_size )
+                            uart->fifo_size = uart_param[p].fifo_size;
 
-                        uart->reg_shift = uart_config[i].reg_shift;
-                        uart->reg_width = uart_config[i].reg_width;
-                        uart->lsr_mask = uart_config[i].lsr_mask;
+                        uart->reg_shift = uart_param[p].reg_shift;
+                        uart->reg_width = uart_param[p].reg_width;
+                        uart->lsr_mask = uart_param[p].lsr_mask;
                         uart->io_base = ((u64)bar_64 << 32) |
                                         (bar & PCI_BASE_ADDRESS_MEM_MASK);
-                        uart->io_base += uart_config[i].first_offset;
-                        uart->io_base += bar_idx * uart_config[i].uart_offset;
-                        if ( uart_config[i].base_baud )
-                            uart->clock_hz = uart_config[i].base_baud * 16;
+                        uart->io_base += uart_param[p].first_offset;
+                        uart->io_base += bar_idx * uart_param[p].uart_offset;
+                        if ( uart_param[p].base_baud )
+                            uart->clock_hz = uart_param[p].base_baud * 16;
                         /* Set device and MMIO region read only to Dom0 */
                         uart->enable_ro = 1;
                         break;
-- 
1.7.7.6

[PATCH v2 5/7] serial: Use #defines for PCI vendors.

[Konrad Rzeszutek Wilk]

Instead of having hard-coded values. We only do PCI vendors
as Jan requested.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index b191a90..b3331b6 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -131,17 +131,20 @@ static struct ns16550_config_param __initdata uart_param[] = {
         .max_bars = 1, /* It can do more, but we would need more custom code.*/
     }
 };
+
+#define PCI_VENDOR_ID_BROADCOM           0x14e4
+#define PCI_VENDOR_ID_OXSEMI             0x1415
 static struct ns16550_config_mmio __initdata uart_config[] =
 {
     /* Broadcom TruManage device */
     {
-        .vendor_id = 0x14e4,
+        .vendor_id = PCI_VENDOR_ID_BROADCOM,
         .dev_id = 0x160a,
         .param = param_trumanage,
     },
     /* OXPCIe952 1 Native UART  */
     {
-        .vendor_id = 0x1415,
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
         .dev_id = 0xc138,
         .param = param_oxford,
     }
-- 
1.7.7.6

[PATCH v2 6/7] serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART

[Konrad Rzeszutek Wilk]

This covers all of the OXPCIe952 1 Native UART and
OXPCIe200 1 Native UART chipsets.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/drivers/char/ns16550.c |  174 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 174 insertions(+), 0 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index b3331b6..eb604c0 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -145,8 +145,182 @@ static struct ns16550_config_mmio __initdata uart_config[] =
     /* OXPCIe952 1 Native UART  */
     {
         .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc11b,
+        .param = param_oxford,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc11f,
+        .param = param_oxford,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
         .dev_id = 0xc138,
         .param = param_oxford,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc13d,
+        .param = param_oxford,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc40b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc40f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc41b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc41f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc42b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc42f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc43b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc43f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc44b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc44f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc45b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc45f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc46b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc46f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc47b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc47f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc48b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc48f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc49b,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc49f,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4ab,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4af,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4bb,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4bf,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4cb,
+        .param = param_oxford,
+    },
+    /* OXPCIe200 1 Native UART  */
+    {
+        .vendor_id = PCI_VENDOR_ID_OXSEMI,
+        .dev_id = 0xc4cf,
+        .param = param_oxford,
     }
 };
 #endif
-- 
1.7.7.6

[PATCH v2 7/7] pci: Put all PCI device vendor and models in one file.

[Konrad Rzeszutek Wilk]

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/oprofile/op_model_athlon.c |    5 ++---
 xen/arch/x86/x86_64/mmconf-fam10h.c     |    1 +
 xen/arch/x86/x86_64/mmconfig-shared.c   |    1 +
 xen/arch/x86/x86_64/mmconfig.h          |    7 -------
 xen/drivers/char/ns16550.c              |    3 +--
 xen/include/xen/pci_ids.h               |   12 ++++++++++++
 6 files changed, 17 insertions(+), 12 deletions(-)
 create mode 100644 xen/include/xen/pci_ids.h

diff --git a/xen/arch/x86/oprofile/op_model_athlon.c b/xen/arch/x86/oprofile/op_model_athlon.c
index e784018..e1df1f2 100644
--- a/xen/arch/x86/oprofile/op_model_athlon.c
+++ b/xen/arch/x86/oprofile/op_model_athlon.c
@@ -20,7 +20,8 @@
 #include <asm/current.h>
 #include <asm/hvm/support.h>
 #include <xen/pci_regs.h>
- 
+#include <xen/pci_ids.h>
+
 #include "op_x86_model.h"
 #include "op_counter.h"
 
@@ -445,8 +446,6 @@ static inline void __init init_ibs_nmi_per_cpu(void *arg)
 	apic_write(reg, APIC_EILVT_MSG_NMI << 8);
 }
 
-#define PCI_VENDOR_ID_AMD               0x1022
-#define PCI_DEVICE_ID_AMD_10H_NB_MISC   0x1203
 #define IBSCTL                          0x1cc
 static int __init init_ibs_nmi(void)
 {
diff --git a/xen/arch/x86/x86_64/mmconf-fam10h.c b/xen/arch/x86/x86_64/mmconf-fam10h.c
index 1373acb..65260f6 100644
--- a/xen/arch/x86/x86_64/mmconf-fam10h.c
+++ b/xen/arch/x86/x86_64/mmconf-fam10h.c
@@ -6,6 +6,7 @@
 #include <xen/acpi.h>
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
+#include <xen/pci_ids.h>
 #include <xen/init.h>
 #include <xen/dmi.h>
 #include <asm/amd.h>
diff --git a/xen/arch/x86/x86_64/mmconfig-shared.c b/xen/arch/x86/x86_64/mmconfig-shared.c
index 7589b64..742bc18 100644
--- a/xen/arch/x86/x86_64/mmconfig-shared.c
+++ b/xen/arch/x86/x86_64/mmconfig-shared.c
@@ -19,6 +19,7 @@
 #include <xen/xmalloc.h>
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
+#include <xen/pci_ids.h>
 #include <asm/e820.h>
 #include <asm/msr.h>
 #include <asm/msr-index.h>
diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h
index 36e0448..5125997 100644
--- a/xen/arch/x86/x86_64/mmconfig.h
+++ b/xen/arch/x86/x86_64/mmconfig.h
@@ -17,10 +17,6 @@
  * Author: Allen Kay <allen.m.kay@intel.com> - adapted from linux
  */
 
-#define PCI_VENDOR_ID_INTEL        0x8086
-#define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
-#define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
-
 /* ioport ends */
 #define PCI_PROBE_BIOS        0x0001
 #define PCI_PROBE_CONF1        0x0002
@@ -29,11 +25,8 @@
 #define PCI_PROBE_MASK        0x000f
 #define PCI_PROBE_NOEARLY    0x0010
 
-#define PCI_VENDOR_ID_AMD             0x1022
 #define PCI_CHECK_ENABLE_AMD_MMCONF     0x20000
 
-#define PCI_VENDOR_ID_NVIDIA       0x10de
-
 extern unsigned int pci_probe;
 
 /*
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index eb604c0..2f98e70 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -19,6 +19,7 @@
 #ifdef HAS_PCI
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
+#include <xen/pci_ids.h>
 #endif
 #include <xen/8250-uart.h>
 #include <xen/vmap.h>
@@ -132,8 +133,6 @@ static struct ns16550_config_param __initdata uart_param[] = {
     }
 };
 
-#define PCI_VENDOR_ID_BROADCOM           0x14e4
-#define PCI_VENDOR_ID_OXSEMI             0x1415
 static struct ns16550_config_mmio __initdata uart_config[] =
 {
     /* Broadcom TruManage device */
diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
new file mode 100644
index 0000000..a6219be
--- /dev/null
+++ b/xen/include/xen/pci_ids.h
@@ -0,0 +1,12 @@
+#define PCI_VENDOR_ID_AMD                0x1022
+#define PCI_DEVICE_ID_AMD_10H_NB_MISC    0x1203
+
+#define PCI_VENDOR_ID_BROADCOM           0x14e4
+
+#define PCI_VENDOR_ID_INTEL              0x8086
+#define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
+#define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
+
+#define PCI_VENDOR_ID_NVIDIA             0x10de
+
+#define PCI_VENDOR_ID_OXSEMI             0x1415
-- 
1.7.7.6

Re: [PATCH v2] Enable serial output for Oxford Semiconductor PCIe cards and fixes.

[Aravind Gopalakrishnan]

On 3/7/2014 1:01 PM, Konrad Rzeszutek Wilk wrote:
> Since v1 [http://mid.gmane.org/1394040334-16278-1-git-send-email-konrad.wilk@oracle.com]:
>   - Update it per Jan and Andrew comments
>   - Tackle on two bug-fixes.
>
>
> Hey,
>
> This is the second, posting of the changes to the ns16550 driver
> to also support other chipsets - like the Oxford 16C950 one - popular on
> the inexpensive Rosewill RC-300E.
>
> There are also two bug-fixes that I discovered while testing my code.
> One of them was found through code inspection while the other was
> when I was testing my code.
>
> Of interest is:
>   [PATCH v2 3/7] serial: Support OXPCIe952 aka Oxford Semiconductor
>
>   that enables the functionality on the PCIe card I have - while the rest
> of the patches flesh out the code to support more of PCI and PCIe type cards.
>
>   xen/arch/x86/oprofile/op_model_athlon.c |    5 +-
>   xen/arch/x86/x86_64/mmconf-fam10h.c     |    1 +
>   xen/arch/x86/x86_64/mmconfig-shared.c   |    1 +
>   xen/arch/x86/x86_64/mmconfig.h          |    7 -
>   xen/drivers/char/ns16550.c              |  254 ++++++++++++++++++++++++++++--
>   xen/include/xen/pci_ids.h               |   12 ++
>   6 files changed, 253 insertions(+), 27 deletions(-)
>
> Konrad Rzeszutek Wilk (7):
>        serial: Skip over PCIe device which have no quirks (fix AMT regression).
>        serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.
>        serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138)
>        serial: Seperate the PCI device ids and parameters
>        serial: Use #defines for PCI vendors.
>        serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART
>        pci: Put all PCI device vendor and models in one file.
>
>

Tested the patch set with Broadcom Trumanage MMIO device just to make 
sure there are no regressions and can verify that it works fine..

Thanks,
-Aravind.

Re: [PATCH v2] Enable serial output for Oxford Semiconductor PCIe cards and fixes.

[Konrad Rzeszutek Wilk]

On Fri, Mar 07, 2014 at 03:27:35PM -0600, Aravind Gopalakrishnan wrote:
> On 3/7/2014 1:01 PM, Konrad Rzeszutek Wilk wrote:
> >Since v1 [http://mid.gmane.org/1394040334-16278-1-git-send-email-konrad.wilk@oracle.com]:
> >  - Update it per Jan and Andrew comments
> >  - Tackle on two bug-fixes.
> >
> >
> >Hey,
> >
> >This is the second, posting of the changes to the ns16550 driver
> >to also support other chipsets - like the Oxford 16C950 one - popular on
> >the inexpensive Rosewill RC-300E.
> >
> >There are also two bug-fixes that I discovered while testing my code.
> >One of them was found through code inspection while the other was
> >when I was testing my code.
> >
> >Of interest is:
> >  [PATCH v2 3/7] serial: Support OXPCIe952 aka Oxford Semiconductor
> >
> >  that enables the functionality on the PCIe card I have - while the rest
> >of the patches flesh out the code to support more of PCI and PCIe type cards.
> >
> >  xen/arch/x86/oprofile/op_model_athlon.c |    5 +-
> >  xen/arch/x86/x86_64/mmconf-fam10h.c     |    1 +
> >  xen/arch/x86/x86_64/mmconfig-shared.c   |    1 +
> >  xen/arch/x86/x86_64/mmconfig.h          |    7 -
> >  xen/drivers/char/ns16550.c              |  254 ++++++++++++++++++++++++++++--
> >  xen/include/xen/pci_ids.h               |   12 ++
> >  6 files changed, 253 insertions(+), 27 deletions(-)
> >
> >Konrad Rzeszutek Wilk (7):
> >       serial: Skip over PCIe device which have no quirks (fix AMT regression).
> >       serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.
> >       serial: Support OXPCIe952 aka Oxford Semiconductor Ltd Device c138 (1415:c138)
> >       serial: Seperate the PCI device ids and parameters
> >       serial: Use #defines for PCI vendors.
> >       serial: Expand the PCI serial quirks for OXPCIe200 and OXPCIe952 1 Native UART
> >       pci: Put all PCI device vendor and models in one file.
> >
> >
> 
> Tested the patch set with Broadcom Trumanage MMIO device just to
> make sure there are no regressions and can verify that it works
> fine..

Thank you!
> 
> Thanks,
> -Aravind.
> 

Re: [PATCH v2 1/7] serial: Skip over PCIe device which have no quirks (fix AMT regression).

[Jan Beulich]

>>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> The "ns16550: Add support for UART present in Broadcom TruManage
> capable NetXtreme chips" implies that only devices that are have
> an MMIO BAR and are in the quirks table should be processed.
> 
> Even the comment at the end says so:
> 
>  If we have an io_base, then we succeeded in the lookup
> 
> But the code was checking for the !io_base - which is to say if
> the io_base was 0 then we would skip scanning. But io_base
> always has a value - it is set by 'ns16550_init' to a default
> value - so it would never hit the 'continue' path.
> 
> This means that if we have an communication device followed by
> a serial AMT device we would pick the communication device instead
> of the AMT device.
> 
> See:
> 00:16.0 Communication controller: Intel Corporation Cougar Point HECI 
> Controller #1 (rev 04)
>         Subsystem: Intel Corporation Device 2008
>         Flags: bus master, fast devsel, latency 0, IRQ 11
>         Memory at fb12a000 (64-bit, non-prefetchable) [size=16]
> 00:16.3 Serial controller: Intel Corporation Cougar Point KT Controller (rev 
> 04) (prog-if 02 [16550])
>         Subsystem: Intel Corporation Device 2008
>         Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 17
>         I/O ports at f0e0 [size=8]
>         Memory at fb129000 (32-bit, non-prefetchable) [size=4K]
> 
> pci 0000:00:16.0: [8086:1c3a] type 00 class 0x078000
> pci 0000:00:16.3: [8086:1c3d] type 00 class 0x070002
> 
> And Xen picks 00:16.0 as its console when using 'com1=115200,8n1,amt'.
> 
> This patch fixes it and allows us to use AMT again by zeroing
> out io_base to zero. If the scan did not work, the io_base is
> set back to a default value (the 'pci_uart_config' does that
> already at its end).
> 
> CC: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> CC: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Thomas Lendacky <Thomas.Lendacky@amd.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Keir Fraser <keir@xen.org>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> ---
>  xen/drivers/char/ns16550.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 53e49a1..2fded08 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -616,6 +616,7 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>      unsigned int b, d, f, nextf, i;
>      u16 vendor, device;
>  
> +    uart->io_base = 0;
>      /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
>      for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
>      {
> -- 
> 1.7.7.6

Re: [PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Jan Beulich]

>>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
> in __setup_xen. Then we call 'ns16550_init' which copies those in
> the appropiate uart, which then calls 'ns16550_parse_port_config'
> to deal with parameter parsing. If the 'pci' or 'amt' parameter
> has been specified we further call 'pci_uart_config code' which
> scans the PCI bus. If it does not find any relevant devices
> it will overwrite io_base with 0x3f8 regardless whether this is
> COM1 or COM2. The overwrite is a way to set it back to the
> failsafe defaults - except for COM2 of course.
> 
> This in theory (as I don't have a machine with two COM ports
> readily available) means that if the user specified 'com2=9600,8n1,pci'
> and the device did not have an PCI serial card, instead of using 0x2f8
> for the io_base it ends up using 0x3f8 - and we don't get the
> output on COM2.
> 
> Fix it by saving the original io_base before starting the
> scan of the PCI bus. If we don't find an serial PCI device then
> assign the original io_base value back.

While reviewing patch 1 I was specifically thinking of why you didn't
do this at once. But then I realized that this is done only in the AMT
case, and was assuming that you, when originally adding AMT
support, specifically did it that way knowing that if anything it could
only sit on port 0x3f8. If that wasn't the original intention, the
change is fine, but the description should be updated to say that
this only affects the AMT case.

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -612,10 +612,11 @@ static int __init
>  pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>  {
>      uint32_t bar, bar_64 = 0, len, len_64;
> -    u64 size, mask;
> +    u64 size, mask, orig_base;
>      unsigned int b, d, f, nextf, i;
>      u16 vendor, device;
>  
> +    orig_base = uart->io_base;
>      uart->io_base = 0;
>      /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
>      for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
> @@ -747,7 +748,8 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>      if ( !skip_amt )
>          return -1;
>  
> -    uart->io_base = 0x3f8;
> +    if ( !uart->io_base )
> +        uart->io_base = orig_base;

I don't think io_base can be zero when getting here. And even if it
could, what would be there would be bogus/wrong. Hence I think
you will want to unconditionally restore the original value.

>      uart->irq = 0;

I further wonder whether that's not really suffering from the same
issue: Shouldn't you save/restore this too? Or rather, looking at the
flow, simply delete the line, leaving the old value in place?

Jan

Re: [PATCH v2 4/7] serial: Seperate the PCI device ids and parameters

[Jan Beulich]

>>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> @@ -96,28 +100,27 @@ struct ns16550_config_mmio {
>  
>  
>  #ifdef HAS_PCI
> +enum ns16550_config_param_nr {

Perhaps better e.g. _kind or _idx rather than _nr? But in the end
you don't use the enum tag anyway, so you could as well leave out
the tag altogether.

> +    param_default = 0,
> +    param_trumanage,
> +    param_oxford,
> +};
>  /*
>   * Create lookup tables for specific MMIO devices..
>   * It is assumed that if the device found is MMIO,
>   * then you have indexed it here. Else, the driver
>   * does nothing.
>   */
> -static struct ns16550_config_mmio __initdata uart_config[] =
> -{
> -    /* Broadcom TruManage device */
> -    {
> -        .vendor_id = 0x14e4,
> -        .dev_id = 0x160a,
> +static struct ns16550_config_param __initdata uart_param[] = {

If you need to touch this anyway, please make both this ...

> +    [param_default] = { }, /* Ignored. */
> +    [param_trumanage] = {
>          .reg_shift = 2,
>          .reg_width = 1,
>          .fifo_size = 16,
>          .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
>          .max_bars = 1,
>      },
> -    /* OXPCIe952 1 Native UART  */
> -    {
> -        .vendor_id = 0x1415,
> -        .dev_id = 0xc138,
> +    [param_oxford] = {
>          .base_baud = 4000000,
>          .uart_offset = 0x200,
>          .first_offset = 0x1000,
> @@ -128,6 +131,21 @@ static struct ns16550_config_mmio __initdata uart_config[] =
>          .max_bars = 1, /* It can do more, but we would need more custom 
> code.*/
>      }
>  };
> +static struct ns16550_config_mmio __initdata uart_config[] =

... and this it "const ... __initconst" now that we have the latter.
(Sorry for not noticing the first time through.)

Jan

Re: [PATCH v2 5/7] serial: Use #defines for PCI vendors.

[Jan Beulich]

>>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> Instead of having hard-coded values. We only do PCI vendors
> as Jan requested.

I think this isn't worth it as a separate patch. Either move it ahead
to the one introducing the support for the Oxford device, or merge
it with the patch introducing pci_ids.h (and then in order to not
needlessly bloat it, merge that one here, i.e. ahead of patch 6).

Jan

Re: [PATCH v2 7/7] pci: Put all PCI device vendor and models in one file.

[Jan Beulich]

>>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:

So in patch 5 you were following the advice and moved only the
vendor IDs. Yet here you move device IDs too. Please be consistent
(preferably, as suggested, only putting vendor IDs in the global
header).

Jan

Re: [PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Konrad Rzeszutek Wilk]

On Mon, Mar 10, 2014 at 09:35:16AM +0000, Jan Beulich wrote:
> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
> > in __setup_xen. Then we call 'ns16550_init' which copies those in
> > the appropiate uart, which then calls 'ns16550_parse_port_config'
> > to deal with parameter parsing. If the 'pci' or 'amt' parameter
> > has been specified we further call 'pci_uart_config code' which
> > scans the PCI bus. If it does not find any relevant devices
> > it will overwrite io_base with 0x3f8 regardless whether this is
> > COM1 or COM2. The overwrite is a way to set it back to the
> > failsafe defaults - except for COM2 of course.
> > 
> > This in theory (as I don't have a machine with two COM ports
> > readily available) means that if the user specified 'com2=9600,8n1,pci'
> > and the device did not have an PCI serial card, instead of using 0x2f8
> > for the io_base it ends up using 0x3f8 - and we don't get the
> > output on COM2.
> > 
> > Fix it by saving the original io_base before starting the
> > scan of the PCI bus. If we don't find an serial PCI device then
> > assign the original io_base value back.
> 
> While reviewing patch 1 I was specifically thinking of why you didn't
> do this at once. But then I realized that this is done only in the AMT
> case, and was assuming that you, when originally adding AMT
> support, specifically did it that way knowing that if anything it could
> only sit on port 0x3f8. If that wasn't the original intention, the
> change is fine, but the description should be updated to say that
> this only affects the AMT case.
> 
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -612,10 +612,11 @@ static int __init
> >  pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
> >  {
> >      uint32_t bar, bar_64 = 0, len, len_64;
> > -    u64 size, mask;
> > +    u64 size, mask, orig_base;
> >      unsigned int b, d, f, nextf, i;
> >      u16 vendor, device;
> >  
> > +    orig_base = uart->io_base;
> >      uart->io_base = 0;
> >      /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
> >      for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
> > @@ -747,7 +748,8 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
> >      if ( !skip_amt )
> >          return -1;
> >  
> > -    uart->io_base = 0x3f8;
> > +    if ( !uart->io_base )
> > +        uart->io_base = orig_base;
> 
> I don't think io_base can be zero when getting here. And even if it
> could, what would be there would be bogus/wrong. Hence I think
> you will want to unconditionally restore the original value.

It can (with the "serial: Skip over PCIe device which have no quirks
(fix AMT regression)." patch.

If an user specified 'com1=115200,8n1,amt' on a machine without
AMT but with the old-fashioned COM1, this would still work.

The scan of the PCI bus would naturally fail, so the io_base would
be zero (as we just had set it to zero at the start of the loop).

But as you mentioned - if we are done with the loop and hadn't
found anything - we might as well uncondionally set it to the
original value. I was being a bit conservative here.

> 
> >      uart->irq = 0;
> 
> I further wonder whether that's not really suffering from the same
> issue: Shouldn't you save/restore this too? Or rather, looking at the
> flow, simply delete the line, leaving the old value in place?

Yes.
<nods>

Let me do it per you suggestion and just do this:

>From 5519bf9cfcaf6ec99af146825ae53f7f53deb37c Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 7 Mar 2014 10:45:04 -0500
Subject: [PATCH] serial: Fix COM1 assumption if pci_uart_config did not find
 the PCI serial card.

The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
in __setup_xen. Then we call 'ns16550_init' which copies those in
the appropiate uart, which then calls 'ns16550_parse_port_config'
to deal with parameter parsing. If the 'pci' or 'amt' parameter
has been specified we further call 'pci_uart_config code' which
scans the PCI bus. If it does not find any relevant devices
it will overwrite io_base with 0x3f8 regardless whether this is
COM1 or COM2. The overwrite is a way to set it back to the
failsafe defaults - except for COM2 of course.

This in theory (as I don't have a machine with two COM ports
readily available) means that if the user specified 'com2=9600,8n1,pci'
and the device did not have an PCI serial card, instead of using 0x2f8
for the io_base it ends up using 0x3f8 - and we don't get the
output on COM2.

Fix it by saving the original io_base before starting the
scan of the PCI bus. If we don't find an serial PCI device then
assign the original io_base value back.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: Also remove the irq overwrite]
---
 xen/drivers/char/ns16550.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 2fded08..b440e01 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -612,10 +612,11 @@ static int __init
 pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
 {
     uint32_t bar, bar_64 = 0, len, len_64;
-    u64 size, mask;
+    u64 size, mask, orig_base;
     unsigned int b, d, f, nextf, i;
     u16 vendor, device;
 
+    orig_base = uart->io_base;
     uart->io_base = 0;
     /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
     for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
@@ -747,8 +748,8 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
     if ( !skip_amt )
         return -1;
 
-    uart->io_base = 0x3f8;
-    uart->irq = 0;
+    /* No PCI or PCIe found, fallback to the defaults. */
+    uart->io_base = orig_base;
     uart->clock_hz  = UART_CLOCK_HZ;
 
     return 0;
-- 
1.7.7.6

Re: [PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Konrad Rzeszutek Wilk]

On Mon, Mar 10, 2014 at 09:35:16AM +0000, Jan Beulich wrote:
> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
> > in __setup_xen. Then we call 'ns16550_init' which copies those in
> > the appropiate uart, which then calls 'ns16550_parse_port_config'
> > to deal with parameter parsing. If the 'pci' or 'amt' parameter
> > has been specified we further call 'pci_uart_config code' which
> > scans the PCI bus. If it does not find any relevant devices
> > it will overwrite io_base with 0x3f8 regardless whether this is
> > COM1 or COM2. The overwrite is a way to set it back to the
> > failsafe defaults - except for COM2 of course.
> > 
> > This in theory (as I don't have a machine with two COM ports
> > readily available) means that if the user specified 'com2=9600,8n1,pci'
> > and the device did not have an PCI serial card, instead of using 0x2f8
> > for the io_base it ends up using 0x3f8 - and we don't get the
> > output on COM2.
> > 
> > Fix it by saving the original io_base before starting the
> > scan of the PCI bus. If we don't find an serial PCI device then
> > assign the original io_base value back.
> 
> While reviewing patch 1 I was specifically thinking of why you didn't
> do this at once. But then I realized that this is done only in the AMT
> case, and was assuming that you, when originally adding AMT
> support, specifically did it that way knowing that if anything it could
> only sit on port 0x3f8. If that wasn't the original intention, the

That (AMT support) was eons ago. I sadly don't remember why it was
done that way :-(

> change is fine, but the description should be updated to say that
> this only affects the AMT case.

Yes, let me update the commit description. Since I already had sent an
updated patch, let me just copy in the new description:

	serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card (AMT one).

	The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
	in __setup_xen. Then we call 'ns16550_init' which copies those in
	the appropiate uart, which then calls 'ns16550_parse_port_config'
	to deal with parameter parsing. If the 'pci' or 'amt' parameter
	has been specified we further call 'pci_uart_config code' which
	scans the PCI bus. If it does not find any relevant devices
	it will overwrite io_base with 0x3f8 regardless whether this is
	COM1 or COM2. The overwrite is a way to set it back to the
	failsafe defaults - except for COM2 of course.

	This in theory (as I don't have a machine with two COM ports
	readily available) means that if the user specified 'com2=9600,8n1,pci'
	or 'com2=9600,8n1,amt' and the device did not have an PCI serial card
	(or AMT), instead of using 0x2f8 for the io_base it ends up using 0x3f8
	- and we don't get the output on COM2.

	Worst yet, we also uncondionally reset the IRQ value - so we would
	never get the proper interrupt when falling back to the legacy
	0x3f8 and 0x2f8 COM ports.

	This bug was introduced with the original AMT support and I cannot
	recall why it was done that way - it is a bug.

	Fix it by saving the original io_base before starting the
	scan of the PCI bus. If we don't find an serial PCI device (because
	we did not exit out of the loop using return) then
	assign the original io_base value back.

	Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
	[v1: Also remove the irq override spotted by Jan]

Re: [PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Jan Beulich]

>>> On 10.03.14 at 17:07, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Mar 10, 2014 at 09:35:16AM +0000, Jan Beulich wrote:
>> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
>> > The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
>> > in __setup_xen. Then we call 'ns16550_init' which copies those in
>> > the appropiate uart, which then calls 'ns16550_parse_port_config'
>> > to deal with parameter parsing. If the 'pci' or 'amt' parameter
>> > has been specified we further call 'pci_uart_config code' which
>> > scans the PCI bus. If it does not find any relevant devices
>> > it will overwrite io_base with 0x3f8 regardless whether this is
>> > COM1 or COM2. The overwrite is a way to set it back to the
>> > failsafe defaults - except for COM2 of course.
>> > 
>> > This in theory (as I don't have a machine with two COM ports
>> > readily available) means that if the user specified 'com2=9600,8n1,pci'
>> > and the device did not have an PCI serial card, instead of using 0x2f8
>> > for the io_base it ends up using 0x3f8 - and we don't get the
>> > output on COM2.
>> > 
>> > Fix it by saving the original io_base before starting the
>> > scan of the PCI bus. If we don't find an serial PCI device then
>> > assign the original io_base value back.
>> 
>> While reviewing patch 1 I was specifically thinking of why you didn't
>> do this at once. But then I realized that this is done only in the AMT
>> case, and was assuming that you, when originally adding AMT
>> support, specifically did it that way knowing that if anything it could
>> only sit on port 0x3f8. If that wasn't the original intention, the
>> change is fine, but the description should be updated to say that
>> this only affects the AMT case.
>> 
>> > --- a/xen/drivers/char/ns16550.c
>> > +++ b/xen/drivers/char/ns16550.c
>> > @@ -612,10 +612,11 @@ static int __init
>> >  pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
>> >  {
>> >      uint32_t bar, bar_64 = 0, len, len_64;
>> > -    u64 size, mask;
>> > +    u64 size, mask, orig_base;
>> >      unsigned int b, d, f, nextf, i;
>> >      u16 vendor, device;
>> >  
>> > +    orig_base = uart->io_base;
>> >      uart->io_base = 0;
>> >      /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. 
> */
>> >      for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
>> > @@ -747,7 +748,8 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int 
> bar_idx)
>> >      if ( !skip_amt )
>> >          return -1;
>> >  
>> > -    uart->io_base = 0x3f8;
>> > +    if ( !uart->io_base )
>> > +        uart->io_base = orig_base;
>> 
>> I don't think io_base can be zero when getting here. And even if it
>> could, what would be there would be bogus/wrong. Hence I think
>> you will want to unconditionally restore the original value.
> 
> It can (with the "serial: Skip over PCIe device which have no quirks
> (fix AMT regression)." patch.

And I realize that there was a "non-" missing in front of the "zero"
in my reply.

> Let me do it per you suggestion and just do this:

Looks good to me.

Jan

Re: [PATCH v2 4/7] serial: Seperate the PCI device ids and parameters

[Konrad Rzeszutek Wilk]

On Mon, Mar 10, 2014 at 09:41:40AM +0000, Jan Beulich wrote:
> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > @@ -96,28 +100,27 @@ struct ns16550_config_mmio {
> >  
> >  
> >  #ifdef HAS_PCI
> > +enum ns16550_config_param_nr {
> 
> Perhaps better e.g. _kind or _idx rather than _nr? But in the end
> you don't use the enum tag anyway, so you could as well leave out
> the tag altogether.

OK, will replace it with #defines.

> 
> > +    param_default = 0,
> > +    param_trumanage,
> > +    param_oxford,
> > +};
> >  /*
> >   * Create lookup tables for specific MMIO devices..
> >   * It is assumed that if the device found is MMIO,
> >   * then you have indexed it here. Else, the driver
> >   * does nothing.
> >   */
> > -static struct ns16550_config_mmio __initdata uart_config[] =
> > -{
> > -    /* Broadcom TruManage device */
> > -    {
> > -        .vendor_id = 0x14e4,
> > -        .dev_id = 0x160a,
> > +static struct ns16550_config_param __initdata uart_param[] = {
> 
> If you need to touch this anyway, please make both this ...
> 
> > +    [param_default] = { }, /* Ignored. */
> > +    [param_trumanage] = {
> >          .reg_shift = 2,
> >          .reg_width = 1,
> >          .fifo_size = 16,
> >          .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
> >          .max_bars = 1,
> >      },
> > -    /* OXPCIe952 1 Native UART  */
> > -    {
> > -        .vendor_id = 0x1415,
> > -        .dev_id = 0xc138,
> > +    [param_oxford] = {
> >          .base_baud = 4000000,
> >          .uart_offset = 0x200,
> >          .first_offset = 0x1000,
> > @@ -128,6 +131,21 @@ static struct ns16550_config_mmio __initdata uart_config[] =
> >          .max_bars = 1, /* It can do more, but we would need more custom 
> > code.*/
> >      }
> >  };
> > +static struct ns16550_config_mmio __initdata uart_config[] =
> 
> ... and this it "const ... __initconst" now that we have the latter.
> (Sorry for not noticing the first time through.)

How does that look to you?

>From d85150bae12257fca7576261e4cc2bedb05e0cf1 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 7 Mar 2014 12:44:30 -0500
Subject: [PATCH] serial: Seperate the PCI device ids and parameters

This will allow us to re-use the parameters for multiple PCI
devices.

No functional change.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: Use #defines instead of enum, use __initconst and const by Jan's review]
---
 xen/drivers/char/ns16550.c |   69 +++++++++++++++++++++++++++++--------------
 1 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index ac509f9..6003331 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -80,10 +80,14 @@ static struct ns16550 {
 #endif
 } ns16550_com[2] = { { 0 } };
 
-/* Defining uart config options for MMIO devices */
 struct ns16550_config_mmio {
     u16 vendor_id;
     u16 dev_id;
+    unsigned int param;
+};
+
+/* Defining uart config options for MMIO devices */
+struct ns16550_config_param {
     unsigned int reg_shift;
     unsigned int reg_width;
     unsigned int fifo_size;
@@ -96,28 +100,26 @@ struct ns16550_config_mmio {
 
 
 #ifdef HAS_PCI
+#define PARAM_DEFAULT   0
+#define PARAM_TRUMANAGE 1
+#define PARAM_OXFORD    2
+
 /*
  * Create lookup tables for specific MMIO devices..
  * It is assumed that if the device found is MMIO,
  * then you have indexed it here. Else, the driver
  * does nothing.
  */
-static struct ns16550_config_mmio __initdata uart_config[] =
-{
-    /* Broadcom TruManage device */
-    {
-        .vendor_id = 0x14e4,
-        .dev_id = 0x160a,
+static const struct  ns16550_config_param __initconst uart_param[] = {
+    [PARAM_DEFAULT] = { }, /* Ignored. */
+    [PARAM_TRUMANAGE] = {
         .reg_shift = 2,
         .reg_width = 1,
         .fifo_size = 16,
         .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
         .max_bars = 1,
     },
-    /* OXPCIe952 1 Native UART  */
-    {
-        .vendor_id = 0x1415,
-        .dev_id = 0xc138,
+    [PARAM_OXFORD] = {
         .base_baud = 4000000,
         .uart_offset = 0x200,
         .first_offset = 0x1000,
@@ -128,6 +130,24 @@ static struct ns16550_config_mmio __initdata uart_config[] =
         .max_bars = 1, /* It can do more, but we would need more custom code.*/
     }
 };
+static const struct ns16550_config_mmio __initconst uart_config[] =
+{
+    /* Broadcom TruManage device */
+    {
+        .vendor_id = 0x14e4,
+        .dev_id = 0x160a,
+        .param = PARAM_TRUMANAGE,
+    },
+    /* OXPCIe952 1 Native UART  */
+    {
+        .vendor_id = 0x1415,
+        .dev_id = 0xc138,
+        .param = PARAM_OXFORD,
+    }
+};
+#undef PARAM_DEFAULT
+#undef PARAM_TRUMANAGE
+#undef PARAM_OXFORD
 #endif
 
 static void ns16550_delayed_resume(void *data);
@@ -692,37 +712,40 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
 
                     size &= -size;
 
-                    /* Check for quirks in uart_config lookup table */
+                    /* Check for params in uart_config lookup table */
                     for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
                     {
+                        unsigned int p;
+
                         if ( uart_config[i].vendor_id != vendor )
                             continue;
 
                         if ( uart_config[i].dev_id != device )
                             continue;
 
+                        p = uart_config[i].param;
                         /*
                          * Force length of mmio region to be at least
                          * 8 bytes times (1 << reg_shift)
                          */
-                        if ( size < (0x8 * (1 << uart_config[i].reg_shift)) )
+                        if ( size < (0x8 * (1 << uart_param[p].reg_shift)) )
                             continue;
 
-                        if ( bar_idx >= uart_config[i].max_bars )
+                        if ( bar_idx >= uart_param[p].max_bars )
                             continue;
 
-                        if ( uart_config[i].fifo_size )
-                            uart->fifo_size = uart_config[i].fifo_size;
+                        if ( uart_param[p].fifo_size )
+                            uart->fifo_size = uart_param[p].fifo_size;
 
-                        uart->reg_shift = uart_config[i].reg_shift;
-                        uart->reg_width = uart_config[i].reg_width;
-                        uart->lsr_mask = uart_config[i].lsr_mask;
+                        uart->reg_shift = uart_param[p].reg_shift;
+                        uart->reg_width = uart_param[p].reg_width;
+                        uart->lsr_mask = uart_param[p].lsr_mask;
                         uart->io_base = ((u64)bar_64 << 32) |
                                         (bar & PCI_BASE_ADDRESS_MEM_MASK);
-                        uart->io_base += uart_config[i].first_offset;
-                        uart->io_base += bar_idx * uart_config[i].uart_offset;
-                        if ( uart_config[i].base_baud )
-                            uart->clock_hz = uart_config[i].base_baud * 16;
+                        uart->io_base += uart_param[p].first_offset;
+                        uart->io_base += bar_idx * uart_param[p].uart_offset;
+                        if ( uart_param[p].base_baud )
+                            uart->clock_hz = uart_param[p].base_baud * 16;
                         /* Set device and MMIO region read only to Dom0 */
                         uart->enable_ro = 1;
                         break;
-- 
1.7.7.6

Re: [PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Jan Beulich]

>>> On 10.03.14 at 17:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Mar 10, 2014 at 09:35:16AM +0000, Jan Beulich wrote:
>> change is fine, but the description should be updated to say that
>> this only affects the AMT case.
> 
> Yes, let me update the commit description. Since I already had sent an
> updated patch, let me just copy in the new description:
> 
> 	serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card (AMT one).
> 
> 	The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
> 	in __setup_xen. Then we call 'ns16550_init' which copies those in
> 	the appropiate uart, which then calls 'ns16550_parse_port_config'
> 	to deal with parameter parsing. If the 'pci' or 'amt' parameter
> 	has been specified we further call 'pci_uart_config code' which
> 	scans the PCI bus. If it does not find any relevant devices
> 	it will overwrite io_base with 0x3f8 regardless whether this is
> 	COM1 or COM2. The overwrite is a way to set it back to the
> 	failsafe defaults - except for COM2 of course.

But this still gives the (false) impression that the overwrite is
happening in the PCI and AMT cases.

> 	This in theory (as I don't have a machine with two COM ports
> 	readily available) means that if the user specified 'com2=9600,8n1,pci'
> 	or 'com2=9600,8n1,amt' and the device did not have an PCI serial card
> 	(or AMT), instead of using 0x2f8 for the io_base it ends up using 0x3f8
> 	- and we don't get the output on COM2.
> 
> 	Worst yet, we also uncondionally reset the IRQ value - so we would
> 	never get the proper interrupt when falling back to the legacy
> 	0x3f8 and 0x2f8 COM ports.

Which isn't _that_ bad - we'd run in polling mode.

Jan

Re: [PATCH v2 4/7] serial: Seperate the PCI device ids and parameters

[Jan Beulich]

>>> On 10.03.14 at 17:23, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Mar 10, 2014 at 09:41:40AM +0000, Jan Beulich wrote:
>> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
>> > @@ -96,28 +100,27 @@ struct ns16550_config_mmio {
>> >  
>> >  
>> >  #ifdef HAS_PCI
>> > +enum ns16550_config_param_nr {
>> 
>> Perhaps better e.g. _kind or _idx rather than _nr? But in the end
>> you don't use the enum tag anyway, so you could as well leave out
>> the tag altogether.
> 
> OK, will replace it with #defines.

But that wasn't what I meant.

> How does that look to you?

Quite okay. I think that using an enum here is quite appropriate/
desirable here, but I also don't heavily object the #define approach.

Jan

Re: [PATCH v2 4/7] serial: Seperate the PCI device ids and parameters

[Konrad Rzeszutek Wilk]

On Mon, Mar 10, 2014 at 04:30:44PM +0000, Jan Beulich wrote:
> >>> On 10.03.14 at 17:23, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > On Mon, Mar 10, 2014 at 09:41:40AM +0000, Jan Beulich wrote:
> >> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> >> > @@ -96,28 +100,27 @@ struct ns16550_config_mmio {
> >> >  
> >> >  
> >> >  #ifdef HAS_PCI
> >> > +enum ns16550_config_param_nr {
> >> 
> >> Perhaps better e.g. _kind or _idx rather than _nr? But in the end
> >> you don't use the enum tag anyway, so you could as well leave out
> >> the tag altogether.
> > 
> > OK, will replace it with #defines.
> 
> But that wasn't what I meant.

My apologies.

My brain parsed this statement: 'don't use the enum tag.. leave out the tag altogether'
as the 'enum' not being useful. But you meant the '_nr'.
> 
> > How does that look to you?
> 
> Quite okay. I think that using an enum here is quite appropriate/
> desirable here, but I also don't heavily object the #define approach.

Heh. Let me revert it back to the enum as it will give future developers
a clear path of how to expand the list of parameters for other serial
devices.

> 
> Jan
> 

Re: [PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Konrad Rzeszutek Wilk]

On Mon, Mar 10, 2014 at 04:23:32PM +0000, Jan Beulich wrote:
> >>> On 10.03.14 at 17:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > On Mon, Mar 10, 2014 at 09:35:16AM +0000, Jan Beulich wrote:
> >> change is fine, but the description should be updated to say that
> >> this only affects the AMT case.
> > 
> > Yes, let me update the commit description. Since I already had sent an
> > updated patch, let me just copy in the new description:
> > 
> > 	serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card (AMT one).
> > 
> > 	The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
> > 	in __setup_xen. Then we call 'ns16550_init' which copies those in
> > 	the appropiate uart, which then calls 'ns16550_parse_port_config'
> > 	to deal with parameter parsing. If the 'pci' or 'amt' parameter
> > 	has been specified we further call 'pci_uart_config code' which
> > 	scans the PCI bus. If it does not find any relevant devices
> > 	it will overwrite io_base with 0x3f8 regardless whether this is
> > 	COM1 or COM2. The overwrite is a way to set it back to the
> > 	failsafe defaults - except for COM2 of course.
> 
> But this still gives the (false) impression that the overwrite is
> happening in the PCI and AMT cases.
> 
> > 	This in theory (as I don't have a machine with two COM ports
> > 	readily available) means that if the user specified 'com2=9600,8n1,pci'
> > 	or 'com2=9600,8n1,amt' and the device did not have an PCI serial card
> > 	(or AMT), instead of using 0x2f8 for the io_base it ends up using 0x3f8
> > 	- and we don't get the output on COM2.
> > 
> > 	Worst yet, we also uncondionally reset the IRQ value - so we would
> > 	never get the proper interrupt when falling back to the legacy
> > 	0x3f8 and 0x2f8 COM ports.
> 
> Which isn't _that_ bad - we'd run in polling mode.

Ah, right. How does this update look to you? I tried as best to make it clear
what works and what does not.

	serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card (or AMT one).

	The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
	in __setup_xen. Then we call 'ns16550_init' which copies those in
	the appropiate uart, which then calls 'ns16550_parse_port_config'
	to deal with parameter parsing. If the 'pci' or 'amt' parameter
	has been specified we further call 'pci_uart_config code' which
	scans the PCI bus. If it does not find any relevant devices
	(no PCI/PCIe or AMT devices are found) it will overwrite io_base with
	0x3f8 regardless whether this is COM1 or COM2 - but only if 'pci'
	or 'amt' parameter had been specified.

	The overwrite is a way to set it back to the failsafe defaults -
	except for COM2 of course.

	Note again - if an PCI or AMT card is found, this over-write
	will not happen.

	This in theory (as I don't have a machine with two COM ports
	readily available) means that if the user specified 'com2=9600,8n1,pci'
	or 'com2=9600,8n1,amt' and the device did not have an PCI serial card
	(or AMT), instead of using 0x2f8 for the io_base it ends up using 0x3f8
	- and we don't get the output on COM2. If the user had done
	'com2=9600,8n1' we would never get in this path so this bug would
	never manifset itself (because we don't end up scanning for AMT or
	PCI serial devices).

	We also uncondionally reset the IRQ value - so we would never get the
	proper interrupt when falling back to the legacy 0x3f8 and 0x2f8 COM ports.
	That is OK - as we would end up using the polling mode - while 
	not the best - it still would work. 

	This bug was introduced with the original AMT support and I cannot
	recall why it was done that way - it is a bug.

	Fix it by saving the original io_base before starting the
	scan of the PCI bus. If we don't find an serial PCI device (because
	we did not exit out of the loop using return) then
	assign the original io_base value back.

	Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
	[v1: Also remove the irq override spotted by Jan]
	[v2: Add more details to the commit description]
> 
> Jan
> 

Re: [PATCH v2 4/7] serial: Seperate the PCI device ids and parameters

[Konrad Rzeszutek Wilk]

On Mon, Mar 10, 2014 at 12:38:54PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 10, 2014 at 04:30:44PM +0000, Jan Beulich wrote:
> > >>> On 10.03.14 at 17:23, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > > On Mon, Mar 10, 2014 at 09:41:40AM +0000, Jan Beulich wrote:
> > >> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> > >> > @@ -96,28 +100,27 @@ struct ns16550_config_mmio {
> > >> >  
> > >> >  
> > >> >  #ifdef HAS_PCI
> > >> > +enum ns16550_config_param_nr {
> > >> 
> > >> Perhaps better e.g. _kind or _idx rather than _nr? But in the end
> > >> you don't use the enum tag anyway, so you could as well leave out
> > >> the tag altogether.
> > > 
> > > OK, will replace it with #defines.
> > 
> > But that wasn't what I meant.
> 
> My apologies.
> 
> My brain parsed this statement: 'don't use the enum tag.. leave out the tag altogether'
> as the 'enum' not being useful. But you meant the '_nr'.

But then of course the moment I changed it to 'ns16550_config_param' I realized
why I had tacked on _nr to it. We already have the 'struct ns16550_config_param'
so we get name space collision.

I will go ahead and make the 'enum' as ns16550_config_param_idx as you suggested.

Re: [PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Jan Beulich]

>>> On 10.03.14 at 17:45, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Mar 10, 2014 at 04:23:32PM +0000, Jan Beulich wrote:
>> >>> On 10.03.14 at 17:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> > On Mon, Mar 10, 2014 at 09:35:16AM +0000, Jan Beulich wrote:
>> >> change is fine, but the description should be updated to say that
>> >> this only affects the AMT case.
>> > 
>> > Yes, let me update the commit description. Since I already had sent an
>> > updated patch, let me just copy in the new description:
>> > 
>> > 	serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial 
> card (AMT one).
>> > 
>> > 	The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
>> > 	in __setup_xen. Then we call 'ns16550_init' which copies those in
>> > 	the appropiate uart, which then calls 'ns16550_parse_port_config'
>> > 	to deal with parameter parsing. If the 'pci' or 'amt' parameter
>> > 	has been specified we further call 'pci_uart_config code' which
>> > 	scans the PCI bus. If it does not find any relevant devices
>> > 	it will overwrite io_base with 0x3f8 regardless whether this is
>> > 	COM1 or COM2. The overwrite is a way to set it back to the
>> > 	failsafe defaults - except for COM2 of course.
>> 
>> But this still gives the (false) impression that the overwrite is
>> happening in the PCI and AMT cases.
>> 
>> > 	This in theory (as I don't have a machine with two COM ports
>> > 	readily available) means that if the user specified 'com2=9600,8n1,pci'
>> > 	or 'com2=9600,8n1,amt' and the device did not have an PCI serial card
>> > 	(or AMT), instead of using 0x2f8 for the io_base it ends up using 0x3f8
>> > 	- and we don't get the output on COM2.
>> > 
>> > 	Worst yet, we also uncondionally reset the IRQ value - so we would
>> > 	never get the proper interrupt when falling back to the legacy
>> > 	0x3f8 and 0x2f8 COM ports.
>> 
>> Which isn't _that_ bad - we'd run in polling mode.
> 
> Ah, right. How does this update look to you? I tried as best to make it 
> clear
> what works and what does not.
> 
> 	serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial 
> card (or AMT one).
> 
> 	The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
> 	in __setup_xen. Then we call 'ns16550_init' which copies those in
> 	the appropiate uart, which then calls 'ns16550_parse_port_config'
> 	to deal with parameter parsing. If the 'pci' or 'amt' parameter
> 	has been specified we further call 'pci_uart_config code' which
> 	scans the PCI bus. If it does not find any relevant devices
> 	(no PCI/PCIe or AMT devices are found) it will overwrite io_base with
> 	0x3f8 regardless whether this is COM1 or COM2 - but only if 'pci'
> 	or 'amt' parameter had been specified.

But you still say "'pci' or 'amt'" here, when the overwite happens
only for 'amt'.

Jan

Re: [PATCH v2 4/7] serial: Seperate the PCI device ids and parameters

[Jan Beulich]

>>> On 10.03.14 at 17:51, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Mon, Mar 10, 2014 at 12:38:54PM -0400, Konrad Rzeszutek Wilk wrote:
>> On Mon, Mar 10, 2014 at 04:30:44PM +0000, Jan Beulich wrote:
>> > >>> On 10.03.14 at 17:23, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> > > On Mon, Mar 10, 2014 at 09:41:40AM +0000, Jan Beulich wrote:
>> > >> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
>> > >> > @@ -96,28 +100,27 @@ struct ns16550_config_mmio {
>> > >> >  
>> > >> >  
>> > >> >  #ifdef HAS_PCI
>> > >> > +enum ns16550_config_param_nr {
>> > >> 
>> > >> Perhaps better e.g. _kind or _idx rather than _nr? But in the end
>> > >> you don't use the enum tag anyway, so you could as well leave out
>> > >> the tag altogether.
>> > > 
>> > > OK, will replace it with #defines.
>> > 
>> > But that wasn't what I meant.
>> 
>> My apologies.
>> 
>> My brain parsed this statement: 'don't use the enum tag.. leave out the tag 
> altogether'
>> as the 'enum' not being useful. But you meant the '_nr'.
> 
> But then of course the moment I changed it to 'ns16550_config_param' I 
> realized
> why I had tacked on _nr to it. We already have the 'struct 
> ns16550_config_param'
> so we get name space collision.
> 
> I will go ahead and make the 'enum' as ns16550_config_param_idx as you 
> suggested.

Still - why not leave off the tag altogether - you don't really need
it to be named, as you never refer to it.

Jan