[PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrishnan]

Since it is an MMIO device, the code has been modified to accept MMIO based
devices as well. MMIO device settings are populated in the 'uart_config' table.
It also advertises 64 bit BAR. Therefore, code is reworked to account for 64
bit BAR and 64 bit MMIO lengths.

Some more quirks are - the need to shift the register offset by a specific
value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) bits before
transmitting data.

While testing, include com1=115200,8n1,pci,0 on the xen cmdline to observe
output on console using SoL.

Changes from V7:
  - per Jan's comments:
    - Moving pci_ro_device to ns16550_init_postirq() so that either
      one of pci_hide_device or pci_ro_device is done at one place
    - remove leading '0' from printk as absent segment identifier
      implies zero anyway.
  - per Ian's comments:
    - fixed issues that casued his build to fail.
    - cross-compiled for arm32 and arm64 after applying patch and
      build was successful on local machine.

Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Signed-off-by: Thomas Lendacky <Thomas.Lendacky@amd.com>
---
 xen/drivers/char/ns16550.c | 187 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 166 insertions(+), 21 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9c2cded..932d643 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -46,13 +46,14 @@ string_param("com2", opt_com2);
 static struct ns16550 {
     int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
     u64 io_base;   /* I/O port or memory-mapped I/O address. */
-    u32 io_size;
+    u64 io_size;
     int reg_shift; /* Bits to shift register offset by */
     int reg_width; /* Size of access to use, the registers
                     * themselves are still bytes */
     char __iomem *remapped_io_base;  /* Remapped virtual address of MMIO. */
     /* UART with IRQ line: interrupt-driven I/O. */
     struct irqaction irqaction;
+    u8 lsr_mask;
 #ifdef CONFIG_ARM
     struct vuart_info vuart;
 #endif
@@ -69,14 +70,50 @@ static struct ns16550 {
     bool_t pb_bdf_enable;   /* if =1, pb-bdf effective, port behind bridge */
     bool_t ps_bdf_enable;   /* if =1, ps_bdf effective, port on pci card */
     u32 bar;
+    u32 bar64;
     u16 cr;
     u8 bar_idx;
+    bool_t enable_ro; /* Make MMIO devices read only to Dom0 */
 #endif
 #ifdef HAS_DEVICE_TREE
     struct dt_irq dt_irq;
 #endif
 } ns16550_com[2] = { { 0 } };
 
+/* Defining uart config options for MMIO devices */
+struct ns16550_config_mmio {
+    u16 vendor_id;
+    u16 dev_id;
+    unsigned int reg_shift;
+    unsigned int reg_width;
+    unsigned int fifo_size;
+    u8 lsr_mask;
+    unsigned int max_bars;
+};
+
+
+#ifdef HAS_PCI
+/*
+ * 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,
+        .reg_shift = 2,
+        .reg_width = 1,
+        .fifo_size = 16,
+        .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
+        .max_bars = 1,
+    },
+};
+#endif
+
 static void ns16550_delayed_resume(void *data);
 
 static char ns_read_reg(struct ns16550 *uart, int reg)
@@ -134,7 +171,7 @@ static void ns16550_interrupt(
     while ( !(ns_read_reg(uart, UART_IIR) & UART_IIR_NOINT) )
     {
         char lsr = ns_read_reg(uart, UART_LSR);
-        if ( lsr & UART_LSR_THRE )
+        if ( (lsr & uart->lsr_mask) == uart->lsr_mask )
             serial_tx_interrupt(port, regs);
         if ( lsr & UART_LSR_DR )
             serial_rx_interrupt(port, regs);
@@ -160,7 +197,7 @@ static void __ns16550_poll(struct cpu_user_regs *regs)
         serial_rx_interrupt(port, regs);
     }
 
-    if ( ns_read_reg(uart, UART_LSR) & UART_LSR_THRE )
+    if ( ( ns_read_reg(uart, UART_LSR) & uart->lsr_mask ) == uart->lsr_mask )
         serial_tx_interrupt(port, regs);
 
 out:
@@ -183,7 +220,9 @@ static int ns16550_tx_ready(struct serial_port *port)
 
     if ( ns16550_ioport_invalid(uart) )
         return -EIO;
-    return ns_read_reg(uart, UART_LSR) & UART_LSR_THRE ? uart->fifo_size : 0;
+
+    return ( (ns_read_reg(uart, UART_LSR) &
+              uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
 }
 
 static void ns16550_putc(struct serial_port *port, char c)
@@ -354,8 +393,24 @@ static void __init ns16550_init_postirq(struct serial_port *port)
 
 #ifdef HAS_PCI
     if ( uart->bar || uart->ps_bdf_enable )
-        pci_hide_device(uart->ps_bdf[0], PCI_DEVFN(uart->ps_bdf[1],
-                                                   uart->ps_bdf[2]));
+    {
+        if ( !uart->enable_ro )
+            pci_hide_device(uart->ps_bdf[0], PCI_DEVFN(uart->ps_bdf[1],
+                            uart->ps_bdf[2]));
+        else
+        {
+            if ( rangeset_add_range(mmio_ro_ranges,
+                                    uart->io_base,
+                                    uart->io_base + uart->io_size - 1) )
+                printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n");
+
+            if ( pci_ro_device(0, uart->ps_bdf[0],
+                               PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2])) )
+                printk(XENLOG_INFO "Could not mark config space of %02x:%02x.%u read-only.\n",
+                                    uart->ps_bdf[0], uart->ps_bdf[1],
+                                    uart->ps_bdf[2]);
+        }
+    }
 #endif
 }
 
@@ -381,6 +436,13 @@ static void _ns16550_resume(struct serial_port *port)
     {
        pci_conf_write32(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2],
                         PCI_BASE_ADDRESS_0 + uart->bar_idx*4, uart->bar);
+
+        /* If 64 bit BAR, write higher 32 bits to BAR+4 */
+        if ( uart->bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
+            pci_conf_write32(0, uart->ps_bdf[0],
+                        uart->ps_bdf[1], uart->ps_bdf[2],
+                        PCI_BASE_ADDRESS_0 + (uart->bar_idx+1)*4, uart->bar64);
+
        pci_conf_write16(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2],
                         PCI_COMMAND, uart->cr);
     }
@@ -546,11 +608,13 @@ static int __init check_existence(struct ns16550 *uart)
 }
 
 #ifdef HAS_PCI
-static int
+static int __init
 pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
 {
-    uint32_t bar, len;
-    int b, d, f, nextf;
+    uint32_t bar, bar_64 = 0, len, len_64;
+    u64 size, mask;
+    unsigned int b, d, f, nextf, i;
+    u16 vendor, device;
 
     /* 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++ )
@@ -579,24 +643,98 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
                 bar = pci_conf_read32(0, b, d, f,
                                       PCI_BASE_ADDRESS_0 + bar_idx*4);
 
-                /* Not IO */
+                /* MMIO based */
                 if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
-                    continue;
-
-                pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0, ~0u);
-                len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0);
-                pci_conf_write32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
-
-                /* Not 8 bytes */
-                if ( (len & 0xffff) != 0xfff9 )
-                    continue;
+                {
+                    vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
+                    device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
+
+                    pci_conf_write32(0, b, d, f,
+                                     PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+                    len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0 + bar_idx*4);
+                    pci_conf_write32(0, b, d, f,
+                                     PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+
+                    /* Handle 64 bit BAR if found */
+                    if ( bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
+                    {
+                        bar_64 = pci_conf_read32(0, b, d, f,
+                                      PCI_BASE_ADDRESS_0 + (bar_idx+1)*4);
+                        pci_conf_write32(0, b, d, f,
+                                    PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, ~0u);
+                        len_64 = pci_conf_read32(0, b, d, f,
+                                    PCI_BASE_ADDRESS_0 + (bar_idx+1)*4);
+                        pci_conf_write32(0, b, d, f,
+                                    PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, bar_64);
+                        mask = ((u64)~0 << 32) | PCI_BASE_ADDRESS_MEM_MASK;
+                        size = (((u64)len_64 << 32) | len) & mask;
+                    }
+                    else
+                        size = len & PCI_BASE_ADDRESS_MEM_MASK;
+
+                    size &= -size;
+
+                    /* Check for quirks in uart_config lookup table */
+                    for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
+                    {
+                        if ( uart_config[i].vendor_id != vendor )
+                            continue;
+
+                        if ( uart_config[i].dev_id != device )
+                            continue;
+
+                        /*
+                         * Force length of mmio region to be at least
+                         * 8 bytes times (1 << reg_shift)
+                         */
+                        if ( size < (0x8 * (1 << uart_config[i].reg_shift)) )
+                            continue;
+
+                        if ( bar_idx >= uart_config[i].max_bars )
+                            continue;
+
+                        if ( uart_config[i].fifo_size )
+                            uart->fifo_size = uart_config[i].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->io_base = ((u64)bar_64 << 32) |
+                                        (bar & PCI_BASE_ADDRESS_MEM_MASK);
+                        /* Set device and MMIO region read only to Dom0 */
+                        uart->enable_ro = 1;
+                        break;
+                    }
+
+                    /* If we have an io_base, then we succeeded in the lookup */
+                    if ( !uart->io_base )
+                        continue;
+                }
+                /* IO based */
+                else
+                {
+                    pci_conf_write32(0, b, d, f,
+                                     PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+                    len = pci_conf_read32(0, b, d, f, PCI_BASE_ADDRESS_0);
+                    pci_conf_write32(0, b, d, f,
+                                     PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+                    size = len & PCI_BASE_ADDRESS_IO_MASK;
+                    size &= -size;
+
+                    /* Not 8 bytes */
+                    if ( size != 0x8 )
+                        continue;
+
+                    uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
+                }
 
                 uart->ps_bdf[0] = b;
                 uart->ps_bdf[1] = d;
                 uart->ps_bdf[2] = f;
-                uart->bar = bar;
                 uart->bar_idx = bar_idx;
-                uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
+                uart->bar = bar;
+                uart->bar64 = bar_64;
+                uart->io_size = size;
                 uart->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
                     pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
 
@@ -743,9 +881,16 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
     uart->reg_width = 1;
     uart->reg_shift = 0;
 
+#ifdef HAS_PCI
+    uart->enable_ro = 0;
+#endif
+
     /* Default is no transmit FIFO. */
     uart->fifo_size = 1;
 
+    /* Default lsr_mask = UART_LSR_THRE */
+    uart->lsr_mask = UART_LSR_THRE;
+
     ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
 }
 
-- 
1.8.1.2

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Jan Beulich]

>>> On 05.12.13 at 23:38, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:
> Since it is an MMIO device, the code has been modified to accept MMIO based
> devices as well. MMIO device settings are populated in the 'uart_config' 
> table.
> It also advertises 64 bit BAR. Therefore, code is reworked to account for 64
> bit BAR and 64 bit MMIO lengths.
> 
> Some more quirks are - the need to shift the register offset by a specific
> value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) bits before
> transmitting data.
> 
> While testing, include com1=115200,8n1,pci,0 on the xen cmdline to observe
> output on console using SoL.
> 
> Changes from V7:
>   - per Jan's comments:
>     - Moving pci_ro_device to ns16550_init_postirq() so that either
>       one of pci_hide_device or pci_ro_device is done at one place
>     - remove leading '0' from printk as absent segment identifier
>       implies zero anyway.
>   - per Ian's comments:
>     - fixed issues that casued his build to fail.
>     - cross-compiled for arm32 and arm64 after applying patch and
>       build was successful on local machine.
> 
> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> Signed-off-by: Thomas Lendacky <Thomas.Lendacky@amd.com>

I'm fine with this now, but I take it that you're not intending this
to go into 4.4, or else you'd have Cc-ed George explaining why
a freeze exception is being requested.

Jan

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrishnan]

On 12/6/2013 2:41 AM, Jan Beulich wrote:
>>>> On 05.12.13 at 23:38, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:
>> Since it is an MMIO device, the code has been modified to accept MMIO based
>> devices as well. MMIO device settings are populated in the 'uart_config'
>> table.
>> It also advertises 64 bit BAR. Therefore, code is reworked to account for 64
>> bit BAR and 64 bit MMIO lengths.
>>
>> Some more quirks are - the need to shift the register offset by a specific
>> value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) bits before
>> transmitting data.
>>
>> While testing, include com1=115200,8n1,pci,0 on the xen cmdline to observe
>> output on console using SoL.
>>
>> Changes from V7:
>>    - per Jan's comments:
>>      - Moving pci_ro_device to ns16550_init_postirq() so that either
>>        one of pci_hide_device or pci_ro_device is done at one place
>>      - remove leading '0' from printk as absent segment identifier
>>        implies zero anyway.
>>    - per Ian's comments:
>>      - fixed issues that casued his build to fail.
>>      - cross-compiled for arm32 and arm64 after applying patch and
>>        build was successful on local machine.
>>
>> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> Signed-off-by: Thomas Lendacky <Thomas.Lendacky@amd.com>
> I'm fine with this now, but I take it that you're not intending this
> to go into 4.4, or else you'd have Cc-ed George explaining why
> a freeze exception is being requested.
>
>
Thanks Jan,

(Now cc-ing George..)
Please do consider this patch for 4.4 as it is a customer request for 
the AMD Open Compute project.

Thanks,
-Aravind.

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[George Dunlap]

On 12/06/2013 03:52 PM, Aravind Gopalakrishnan wrote:
> On 12/6/2013 2:41 AM, Jan Beulich wrote:
>>>>> On 05.12.13 at 23:38, Aravind Gopalakrishnan 
>>>>> <Aravind.Gopalakrishnan@amd.com> wrote:
>>> Since it is an MMIO device, the code has been modified to accept 
>>> MMIO based
>>> devices as well. MMIO device settings are populated in the 
>>> 'uart_config'
>>> table.
>>> It also advertises 64 bit BAR. Therefore, code is reworked to 
>>> account for 64
>>> bit BAR and 64 bit MMIO lengths.
>>>
>>> Some more quirks are - the need to shift the register offset by a 
>>> specific
>>> value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) 
>>> bits before
>>> transmitting data.
>>>
>>> While testing, include com1=115200,8n1,pci,0 on the xen cmdline to 
>>> observe
>>> output on console using SoL.
>>>
>>> Changes from V7:
>>>    - per Jan's comments:
>>>      - Moving pci_ro_device to ns16550_init_postirq() so that either
>>>        one of pci_hide_device or pci_ro_device is done at one place
>>>      - remove leading '0' from printk as absent segment identifier
>>>        implies zero anyway.
>>>    - per Ian's comments:
>>>      - fixed issues that casued his build to fail.
>>>      - cross-compiled for arm32 and arm64 after applying patch and
>>>        build was successful on local machine.
>>>
>>> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>> Signed-off-by: Thomas Lendacky <Thomas.Lendacky@amd.com>
>> I'm fine with this now, but I take it that you're not intending this
>> to go into 4.4, or else you'd have Cc-ed George explaining why
>> a freeze exception is being requested.
>>
>>
> Thanks Jan,
>
> (Now cc-ing George..)
> Please do consider this patch for 4.4 as it is a customer request for 
> the AMD Open Compute project.

Can you take a look at the guidelines linked below, think about the 
questions there, and then give a brief summary of the benefits and 
potential risks?

http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_code_freeze

Thanks,
  -George

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrishnan]

On 12/6/2013 10:00 AM, George Dunlap wrote:
> On 12/06/2013 03:52 PM, Aravind Gopalakrishnan wrote:
>> On 12/6/2013 2:41 AM, Jan Beulich wrote:
>>>>>> On 05.12.13 at 23:38, Aravind Gopalakrishnan 
>>>>>> <Aravind.Gopalakrishnan@amd.com> wrote:
>>>> Since it is an MMIO device, the code has been modified to accept 
>>>> MMIO based
>>>> devices as well. MMIO device settings are populated in the 
>>>> 'uart_config'
>>>> table.
>>>> It also advertises 64 bit BAR. Therefore, code is reworked to 
>>>> account for 64
>>>> bit BAR and 64 bit MMIO lengths.
>>>>
>>>> Some more quirks are - the need to shift the register offset by a 
>>>> specific
>>>> value and we also need to verify (UART_LSR_THRE && UART_LSR_TEMT) 
>>>> bits before
>>>> transmitting data.
>>>>
>>>> While testing, include com1=115200,8n1,pci,0 on the xen cmdline to 
>>>> observe
>>>> output on console using SoL.
>>>>
>>>> Changes from V7:
>>>>    - per Jan's comments:
>>>>      - Moving pci_ro_device to ns16550_init_postirq() so that either
>>>>        one of pci_hide_device or pci_ro_device is done at one place
>>>>      - remove leading '0' from printk as absent segment identifier
>>>>        implies zero anyway.
>>>>    - per Ian's comments:
>>>>      - fixed issues that casued his build to fail.
>>>>      - cross-compiled for arm32 and arm64 after applying patch and
>>>>        build was successful on local machine.
>>>>
>>>> Signed-off-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
>>>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>>> Signed-off-by: Thomas Lendacky <Thomas.Lendacky@amd.com>
>>> I'm fine with this now, but I take it that you're not intending this
>>> to go into 4.4, or else you'd have Cc-ed George explaining why
>>> a freeze exception is being requested.
>>>
>>>
>> Thanks Jan,
>>
>> (Now cc-ing George..)
>> Please do consider this patch for 4.4 as it is a customer request for 
>> the AMD Open Compute project.
>
> Can you take a look at the guidelines linked below, think about the 
> questions there, and then give a brief summary of the benefits and 
> potential risks?
>
> http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_code_freeze 
>
>
To answer some of the questions-
- What functionality is being fixed / enabled by this patch?
This patch enables the UART present in Broadcom TruManage capable 
NetXtreme 5725 chip.
This chip is used in the Open Compute platform offering by AMD and is a 
feature
request from the customer who would like to use SoL while using Xen 
virtualization.
This platform does not have any other serial ports that can be used.

- If bug exists, what could be broken?/ Probability of the bug:
The patch ensures that the existing functionality of the ns16550 code is 
not affected in
any manner. The existing code only supports IO-based UARTS and I have 
verified Xen serial console
to work fine with IO-based serial devices (after applying patch). The 
only part of patch that
touches/changes existing code is the line that does a check of the 
'size' of the address space
exposed by the device-

/* Not 8 bytes */
if ( size != 0x8 )
     continue;

This too is not changing original behavior, but merely modifying the 
code to calculate
the 'size' before we check for it. Previously,it was

/* Not 8 bytes */
if ( (len & 0xffff) != 0xfff9 )
     continue;

which does same thing, only a little more implicitly.

Since the UART in this BCM chip is MMIO based, and has 64-bit BAR, 
additions have been made to
account for the lack of support in existing serial code in Xen. 
Moreover, the patch is
careful to only support this particular MMIO based UART. If we detect 
anything else,
the code falls back to default (existing) behavior of ignoring the device.

Problems will arise if we try to use interrupts. (Undefined behaviour)
But to avoid those, we will document to the customer to add 
com1=115200,8n1,pci,0
on xen cmdline to observe output on console. Googling on 'Serial over 
Lan on Xen'
indicates this is an existing restriction for other SoL devices.

We are also making this PCI device read only to Dom0. We cannot hide it 
entirely as Dom0
is supposed to always see the device. For this reason,  we use 
pci_ro_device and add the
MMIO region to mmio_ro_ranges to prevent write access by Dom0 (thus 
protecting any malicious
Dom0 access to the address space)

If bugs arise, then I am inclined to think that it would break only this 
specific BCM chip
and not existing functionality. (probability is low as I have tested it 
against the chip and it
works fine)

Also, tested cross-compiling to arm32 and arm64 and verified that build 
does not break.

- Given the above benefit and risk, is this patch worth it?
Given the customer desire to use Xen on this platform in the 4.4 
timeframe, and the low
probability of regression on other devices, we would request this be 
applied against 4.4.

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Jan Beulich]

>>> On 06.12.13 at 21:31, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
> On 12/6/2013 10:00 AM, George Dunlap wrote:
>> Can you take a look at the guidelines linked below, think about the 
>> questions there, and then give a brief summary of the benefits and 
>> potential risks?
>>
>> 
> http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_c 
> ode_freeze 
>>
>>
> To answer some of the questions-
> - What functionality is being fixed / enabled by this patch?
> This patch enables the UART present in Broadcom TruManage capable 
> NetXtreme 5725 chip.
> This chip is used in the Open Compute platform offering by AMD and is a 
> feature
> request from the customer who would like to use SoL while using Xen 
> virtualization.
> This platform does not have any other serial ports that can be used.
> 
> - If bug exists, what could be broken?/ Probability of the bug:
> The patch ensures that the existing functionality of the ns16550 code is 
> not affected in
> any manner. The existing code only supports IO-based UARTS and I have 
> verified Xen serial console
> to work fine with IO-based serial devices (after applying patch). The 
> only part of patch that
> touches/changes existing code is the line that does a check of the 
> 'size' of the address space
> exposed by the device-
> 
> /* Not 8 bytes */
> if ( size != 0x8 )
>      continue;
> 
> This too is not changing original behavior, but merely modifying the 
> code to calculate
> the 'size' before we check for it. Previously,it was
> 
> /* Not 8 bytes */
> if ( (len & 0xffff) != 0xfff9 )
>      continue;
> 
> which does same thing, only a little more implicitly.
> 
> Since the UART in this BCM chip is MMIO based, and has 64-bit BAR, 
> additions have been made to
> account for the lack of support in existing serial code in Xen. 
> Moreover, the patch is
> careful to only support this particular MMIO based UART. If we detect 
> anything else,
> the code falls back to default (existing) behavior of ignoring the device.
> 
> Problems will arise if we try to use interrupts. (Undefined behaviour)
> But to avoid those, we will document to the customer to add 
> com1=115200,8n1,pci,0
> on xen cmdline to observe output on console. Googling on 'Serial over 
> Lan on Xen'
> indicates this is an existing restriction for other SoL devices.
> 
> We are also making this PCI device read only to Dom0. We cannot hide it 
> entirely as Dom0
> is supposed to always see the device. For this reason,  we use 
> pci_ro_device and add the
> MMIO region to mmio_ro_ranges to prevent write access by Dom0 (thus 
> protecting any malicious
> Dom0 access to the address space)
> 
> If bugs arise, then I am inclined to think that it would break only this 
> specific BCM chip
> and not existing functionality. (probability is low as I have tested it 
> against the chip and it
> works fine)
> 
> Also, tested cross-compiling to arm32 and arm64 and verified that build 
> does not break.
> 
> - Given the above benefit and risk, is this patch worth it?
> Given the customer desire to use Xen on this platform in the 4.4 
> timeframe, and the low
> probability of regression on other devices, we would request this be 
> applied against 4.4.

Honestly, if I'm asked - I'm not convinced. To me this boils down to
low risk low benefit, with the risk analysis part apparently heavily
biased towards "the patch appears to be bug free", whereas from
a patch history perspective this clearly wasn't the case from the
beginning, and hence there's a fair chance that some aspect was
still overlooked in the latest review round. Furthermore we're not
talking about something that was on the feature list for 4.4.

Jan

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Andrew Cooper]

On 09/12/2013 08:22, Jan Beulich wrote:
>>>> On 06.12.13 at 21:31, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
>> On 12/6/2013 10:00 AM, George Dunlap wrote:
>>> Can you take a look at the guidelines linked below, think about the 
>>> questions there, and then give a brief summary of the benefits and 
>>> potential risks?
>>>
>>>
>> http://wiki.xen.org/wiki/Xen_Roadmap/4.4#Exception_guidelines_for_after_the_c 
>> ode_freeze 
>>>
>> To answer some of the questions-
>> - What functionality is being fixed / enabled by this patch?
>> This patch enables the UART present in Broadcom TruManage capable 
>> NetXtreme 5725 chip.
>> This chip is used in the Open Compute platform offering by AMD and is a 
>> feature
>> request from the customer who would like to use SoL while using Xen 
>> virtualization.
>> This platform does not have any other serial ports that can be used.
>>
>> - If bug exists, what could be broken?/ Probability of the bug:
>> The patch ensures that the existing functionality of the ns16550 code is 
>> not affected in
>> any manner. The existing code only supports IO-based UARTS and I have 
>> verified Xen serial console
>> to work fine with IO-based serial devices (after applying patch). The 
>> only part of patch that
>> touches/changes existing code is the line that does a check of the 
>> 'size' of the address space
>> exposed by the device-
>>
>> /* Not 8 bytes */
>> if ( size != 0x8 )
>>      continue;
>>
>> This too is not changing original behavior, but merely modifying the 
>> code to calculate
>> the 'size' before we check for it. Previously,it was
>>
>> /* Not 8 bytes */
>> if ( (len & 0xffff) != 0xfff9 )
>>      continue;
>>
>> which does same thing, only a little more implicitly.
>>
>> Since the UART in this BCM chip is MMIO based, and has 64-bit BAR, 
>> additions have been made to
>> account for the lack of support in existing serial code in Xen. 
>> Moreover, the patch is
>> careful to only support this particular MMIO based UART. If we detect 
>> anything else,
>> the code falls back to default (existing) behavior of ignoring the device.
>>
>> Problems will arise if we try to use interrupts. (Undefined behaviour)
>> But to avoid those, we will document to the customer to add 
>> com1=115200,8n1,pci,0
>> on xen cmdline to observe output on console. Googling on 'Serial over 
>> Lan on Xen'
>> indicates this is an existing restriction for other SoL devices.
>>
>> We are also making this PCI device read only to Dom0. We cannot hide it 
>> entirely as Dom0
>> is supposed to always see the device. For this reason,  we use 
>> pci_ro_device and add the
>> MMIO region to mmio_ro_ranges to prevent write access by Dom0 (thus 
>> protecting any malicious
>> Dom0 access to the address space)
>>
>> If bugs arise, then I am inclined to think that it would break only this 
>> specific BCM chip
>> and not existing functionality. (probability is low as I have tested it 
>> against the chip and it
>> works fine)
>>
>> Also, tested cross-compiling to arm32 and arm64 and verified that build 
>> does not break.
>>
>> - Given the above benefit and risk, is this patch worth it?
>> Given the customer desire to use Xen on this platform in the 4.4 
>> timeframe, and the low
>> probability of regression on other devices, we would request this be 
>> applied against 4.4.
> Honestly, if I'm asked - I'm not convinced. To me this boils down to
> low risk low benefit, with the risk analysis part apparently heavily
> biased towards "the patch appears to be bug free", whereas from
> a patch history perspective this clearly wasn't the case from the
> beginning, and hence there's a fair chance that some aspect was
> still overlooked in the latest review round. Furthermore we're not
> talking about something that was on the feature list for 4.4.
>
> Jan
>

It turns out that we have some of this hardware in our testing pool.

Therefore,

Tested-by: Andrew Cooper <andrew.cooper3@citrix.com> (by way of backport
to 4.3)

I would however agree that on the whole, it is probably too high-risk /
low-reward  for inclusion in 4.4, but it should be fine for accepting as
soon as the 4.5 window opens.

~Andrew

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrishnan]

On 12/16/2013 8:29 AM, Andrew Cooper wrote:
>
> It turns out that we have some of this hardware in our testing pool.
>
> Therefore,
>
> Tested-by: Andrew Cooper <andrew.cooper3@citrix.com> (by way of backport
> to 4.3)
>
> I would however agree that on the whole, it is probably too high-risk /
> low-reward  for inclusion in 4.4, but it should be fine for accepting as
> soon as the 4.5 window opens.
>
>

Ping..

Jan (or George), could you please clarify if (hopefully) there are no 
other concerns regarding the patch, it would be picked up automatically 
for 4.5?


Thanks,
-Aravind.

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Jan Beulich]

>>> On 24.02.14 at 17:12, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
> On 12/16/2013 8:29 AM, Andrew Cooper wrote:
>>
>> It turns out that we have some of this hardware in our testing pool.
>>
>> Therefore,
>>
>> Tested-by: Andrew Cooper <andrew.cooper3@citrix.com> (by way of backport
>> to 4.3)
>>
>> I would however agree that on the whole, it is probably too high-risk /
>> low-reward  for inclusion in 4.4, but it should be fine for accepting as
>> soon as the 4.5 window opens.
>>
>>
> 
> Ping..
> 
> Jan (or George), could you please clarify if (hopefully) there are no 
> other concerns regarding the patch, it would be picked up automatically 
> for 4.5?

It's been well over two months since you posted this, and it's still not
having Keir's ack. The best way therefore is for you to re-post, with
Keir properly Cc-ed, and with Andrew's Tested-by added.

Jan

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Aravind Gopalakrishnan]

On 2/24/2014 10:26 AM, Jan Beulich wrote:
>>>   
>>> It's been well over two months since you posted this, and it's still not
>>> having Keir's ack. The best way therefore is for you to re-post, with
>>> Keir properly Cc-ed, and with Andrew's Tested-by added.
>>>
>>> Jan
>>>
>>>

Ok, will do..

Shall I retain the version number or start from scratch?

-Aravind.

Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

[Jan Beulich]

>>> On 24.02.14 at 19:08, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
> On 2/24/2014 10:26 AM, Jan Beulich wrote:
>>>>   
>>>> It's been well over two months since you posted this, and it's still not
>>>> having Keir's ack. The best way therefore is for you to re-post, with
>>>> Keir properly Cc-ed, and with Andrew's Tested-by added.
> 
> Ok, will do..
> 
> Shall I retain the version number or start from scratch?

If there's no change, retain the version number and add "RESEND".

Jan