Xen devel - Nov 2013 - [PATCH V2] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips

There are few quirks regarding the chip:
Firstly, it is an MMIO device. Therefore, the code has been modified to
accept MMIO based devices as well. Settings particular to such devices are
populated  in the table ''uart_config''. Currently, we only
support BCM5725
TruManage chip.

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, we ned to include com1=115200,8n1,pci,0 on the xen cmdline to
be able to observe output using SOL.

While at it, change types of fifo_size, reg_shift, reg_width to u32.

Changes from V1:
  - Change types of fifo_size, reg_shift, reg_width to u32
  - Remove if (!bar) condition
  - Remove spurious whitespaces

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 | 101 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 86 insertions(+), 15 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9c2cded..58c3d95 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -44,15 +44,17 @@ string_param("com1", opt_com1);
 string_param("com2", opt_com2);
 
 static struct ns16550 {
-    int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
+    int baud, clock_hz, data_bits, parity, stop_bits, irq;
     u64 io_base;   /* I/O port or memory-mapped I/O address. */
+    u32 fifo_size;
     u32 io_size;
-    int reg_shift; /* Bits to shift register offset by */
-    int reg_width; /* Size of access to use, the registers
+    u32 reg_shift; /* Bits to shift register offset by */
+    u32 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;
+    u32 lsr_mask;
 #ifdef CONFIG_ARM
     struct vuart_info vuart;
 #endif
@@ -77,6 +79,37 @@ 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;
+    u32 reg_shift;
+    u32 reg_width; 
+    u32 fifo_size;
+    u32 lsr_mask;
+    u32 max_bars;
+};
+
+/*
+ * 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 uart_config[] +{
+    /* Broadcom TruManage device */
+    { 
+        .vendor_id = 0x14e4,
+        .dev_id = 0x160a,
+        .reg_shift = 2,
+        .reg_width = 1,
+        .fifo_size = 64,
+        .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
+        .max_bars = 1,
+    },
+};
+
 static void ns16550_delayed_resume(void *data);
 
 static char ns_read_reg(struct ns16550 *uart, int reg)
@@ -134,7 +167,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 +193,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 +216,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)
@@ -550,7 +585,8 @@ static int
 pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
 {
     uint32_t bar, len;
-    int b, d, f, nextf;
+    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++ )
@@ -581,22 +617,54 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int
bar_idx)
 
                 /* Not IO */
                 if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
-                    continue;
+                {
+                    vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
+                    device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
+
+                    /* 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;
+
+                        if ( bar_idx >= uart_config[i].max_bars )
+                            break;
+
+                        uart->reg_shift = uart_config[i].reg_shift;
+                        uart->reg_width = uart_config[i].reg_width;
+                        uart->fifo_size = uart_config[i].fifo_size;
+                        uart->lsr_mask = uart_config[i].lsr_mask;
+                        uart->io_base = bar & PCI_BASE_ADDRESS_MEM_MASK;
+                        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, ~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);
 
-                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;
 
-                /* Not 8 bytes */
-                if ( (len & 0xffff) != 0xfff9 )
-                    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->irq = pci_conf_read8(0, b, d, f, PCI_INTERRUPT_PIN) ?
                     pci_conf_read8(0, b, d, f, PCI_INTERRUPT_LINE) : 0;
 
@@ -746,6 +814,9 @@ void __init ns16550_init(int index, struct ns16550_defaults
*defaults)
     /* 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

>>> On 14.11.13 at 18:40, Aravind Gopalakrishnan
<Aravind.Gopalakrishnan@amd.com> wrote:
>  static struct ns16550 {
> -    int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
> +    int baud, clock_hz, data_bits, parity, stop_bits, irq;
>      u64 io_base;   /* I/O port or memory-mapped I/O address. */
> +    u32 fifo_size;
Is this in any way related to the main purpose of the patch here?
And if there is (i.e. in response to Andrew''s comment on v1), then
I don''t see why most/all of the other fields shouldn''t follow
suit.
>      u32 io_size;
> -    int reg_shift; /* Bits to shift register offset by */
> -    int reg_width; /* Size of access to use, the registers
> +    u32 reg_shift; /* Bits to shift register offset by */
> +    u32 reg_width; /* Size of access to use, the registers
As much as for fifo_size above, there''s nothing here requiring
them to be fixed-size 32 bits. Please use unsigned int instead.
>                      * 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;
> +    u32 lsr_mask;
And this one, if I''m not mistaken, would be u8 if meant to be fixed
size. Or else once again unsigned int.
> +static struct ns16550_config_mmio uart_config[] > +{
> +    /* Broadcom TruManage device */
> +    { 
> +        .vendor_id = 0x14e4,
> +        .dev_id = 0x160a,
> +        .reg_shift = 2,
> +        .reg_width = 1,
> +        .fifo_size = 64,
> +        .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
> +        .max_bars = 1,
> +    },
> +};
Looks like this is used by pci_uart_config() only. Hence it could
be __initdata (and pci_uart_config() would then for consistency
also need to be marked __init - I don''t know why it isn''t
already).
>                  /* Not IO */
>                  if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
> -                    continue;
> +                {
> +                    vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
> +                    device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
> +
> +                    /* 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;
> +
> +                        if ( bar_idx >= uart_config[i].max_bars )
> +                            break;
Did you not mean "continue" here?
> +
> +                        uart->reg_shift = uart_config[i].reg_shift;
> +                        uart->reg_width = uart_config[i].reg_width;
> +                        uart->fifo_size = uart_config[i].fifo_size;
> +                        uart->lsr_mask = uart_config[i].lsr_mask;
> +                        uart->io_base = bar &
PCI_BASE_ADDRESS_MEM_MASK;
> +                        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, ~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);
>  
> -                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;
I think this size checking actually should also be done in the MMIO
case.

Jan

On 11/15/2013 3:31 AM, Jan Beulich wrote:
>>>> On 14.11.13 at 18:40, Aravind Gopalakrishnan
<Aravind.Gopalakrishnan@amd.com> wrote:
>>   static struct ns16550 {
>> -    int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
>> +    int baud, clock_hz, data_bits, parity, stop_bits, irq;
>>       u64 io_base;   /* I/O port or memory-mapped I/O address. */
>> +    u32 fifo_size;
> Is this in any way related to the main purpose of the patch here?
> And if there is (i.e. in response to Andrew''s comment on v1), then
> I don''t see why most/all of the other fields shouldn''t
follow suit.
I don''t mind changing them all in one patch.. But -

Since it is not directly related to the main purpose of the patch, would 
it be okay
if I did this in a follow up patch and go back to using ''int''
for now?

>>       u32 io_size;
>> -    int reg_shift; /* Bits to shift register offset by */
>> -    int reg_width; /* Size of access to use, the registers
>> +    u32 reg_shift; /* Bits to shift register offset by */
>> +    u32 reg_width; /* Size of access to use, the registers
> As much as for fifo_size above, there''s nothing here requiring
> them to be fixed-size 32 bits. Please use unsigned int instead.
>
>>                       * 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;
>> +    u32 lsr_mask;
> And this one, if I''m not mistaken, would be u8 if meant to be
fixed
> size. Or else once again unsigned int.
>
>> +static struct ns16550_config_mmio uart_config[] >> +{
>> +    /* Broadcom TruManage device */
>> +    {
>> +        .vendor_id = 0x14e4,
>> +        .dev_id = 0x160a,
>> +        .reg_shift = 2,
>> +        .reg_width = 1,
>> +        .fifo_size = 64,
>> +        .lsr_mask = (UART_LSR_THRE | UART_LSR_TEMT),
>> +        .max_bars = 1,
>> +    },
>> +};
> Looks like this is used by pci_uart_config() only. Hence it could
> be __initdata (and pci_uart_config() would then for consistency
> also need to be marked __init - I don''t know why it isn''t
already).
Will fix this.
>>                   /* Not IO */
>>                   if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
>> -                    continue;
>> +                {
>> +                    vendor = pci_conf_read16(0, b, d, f,
PCI_VENDOR_ID);
>> +                    device = pci_conf_read16(0, b, d, f,
PCI_DEVICE_ID);
>> +
>> +                    /* 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;
>> +
>> +                        if ( bar_idx >= uart_config[i].max_bars )
>> +                            break;
> Did you not mean "continue" here?
No.. break is fine here since we only support bar0 for this specific 
device..
(By the time we reach this condition, we have already verified
''vendor''
and ''device'' from
uart_config table anyway.. it is needless to iterate through other 
devices in the table)
>> +
>> +                        uart->reg_shift = uart_config[i].reg_shift;
>> +                        uart->reg_width = uart_config[i].reg_width;
>> +                        uart->fifo_size = uart_config[i].fifo_size;
>> +                        uart->lsr_mask = uart_config[i].lsr_mask;
>> +                        uart->io_base = bar &
PCI_BASE_ADDRESS_MEM_MASK;
>> +                        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,
~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);
>>   
>> -                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;
> I think this size checking actually should also be done in the MMIO
> case.
>
> Jan
This check would not work for this device as the length of region is 64K.

But if we really want to force a length check for MMIO cases as well,
we can define another field in struct uart_config and verify if length 
matches..
Do let me know what you make of it..

Meanwhile, I will fix other issues and have a V3 ready..

Thanks,
-Aravind.

>>> On 15.11.13 at 18:51, Aravind Gopalakrioshnan
<aravind.gopalakrishnan@amd.com> wrote:
> On 11/15/2013 3:31 AM, Jan Beulich wrote:
>>>>> On 14.11.13 at 18:40, Aravind Gopalakrishnan
<Aravind.Gopalakrishnan@amd.com> wrote:
>>>   static struct ns16550 {
>>> -    int baud, clock_hz, data_bits, parity, stop_bits, fifo_size,
irq;
>>> +    int baud, clock_hz, data_bits, parity, stop_bits, irq;
>>>       u64 io_base;   /* I/O port or memory-mapped I/O address. */
>>> +    u32 fifo_size;
>> Is this in any way related to the main purpose of the patch here?
>> And if there is (i.e. in response to Andrew''s comment on v1),
then
>> I don''t see why most/all of the other fields
shouldn''t follow suit.
> 
> I don''t mind changing them all in one patch.. But -
> 
> Since it is not directly related to the main purpose of the patch, would 
> it be okay
> if I did this in a follow up patch and go back to using
''int'' for now?
"Go back" is probably the wrong term - in the new code you add
you should strive to use unsigned int where possible. And in a
second patch, converting the bogus uses of plain int to unsigned
would be desirable.
>>>                   /* Not IO */
>>>                   if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
>>> -                    continue;
>>> +                {
>>> +                    vendor = pci_conf_read16(0, b, d, f,
PCI_VENDOR_ID);
>>> +                    device = pci_conf_read16(0, b, d, f,
PCI_DEVICE_ID);
>>> +
>>> +                    /* 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;
>>> +
>>> +                        if ( bar_idx >= uart_config[i].max_bars
)
>>> +                            break;
>> Did you not mean "continue" here?
> 
> No.. break is fine here since we only support bar0 for this specific 
> device..
> (By the time we reach this condition, we have already verified
''vendor''
> and ''device'' from
> uart_config table anyway.. it is needless to iterate through other 
> devices in the table)
Here you make assumptions on the single entry you know the list
is currently holding. But you should consider the general case,
and I don''t see why there couldn''t be two flavors of a device
having the same (vendor, device) pair but different max_bars.
>>> +                    /* Not 8 bytes */
>>> +                    if ( (len & 0xffff) != 0xfff9 )
>>> +                        continue;
>> I think this size checking actually should also be done in the MMIO
>> case.
> 
> This check would not work for this device as the length of region is 64K.
I didn''t mean you to make the exact same check; but I do expect
you to do some similar checking.
> But if we really want to force a length check for MMIO cases as well,
> we can define another field in struct uart_config and verify if length 
> matches..
> Do let me know what you make of it..
If you think an exact match check is necessary, then yes, such a
new field might be needed. But since I don''t see Xen depending
on the exact size, but just on it being at least a certain size, doing
just that check would seem fine to me.

But then you''d need to carefully consider what the remainder of
the MMIO range is used for - if any of that could conflict with
what Xen needs to fully control the device, then you should also
hide any PAGE_SIZE chunks from all domains (including Dom0).
(Unfortunately we can''t currently hide sub-page chunks in an
effective manner.)

Jan

On 11/18/2013 2:04 AM, Jan Beulich wrote:
>>>>>> On 14.11.13 at 18:40, Aravind Gopalakrishnan
<Aravind.Gopalakrishnan@amd.com> wrote:
>>>>    static struct ns16550 {
>>>> -    int baud, clock_hz, data_bits, parity, stop_bits,
fifo_size, irq;
>>>> +    int baud, clock_hz, data_bits, parity, stop_bits, irq;
>>>>        u64 io_base;   /* I/O port or memory-mapped I/O address.
*/
>>>> +    u32 fifo_size;
>>> Is this in any way related to the main purpose of the patch here?
>>> And if there is (i.e. in response to Andrew''s comment on
v1), then
>>> I don''t see why most/all of the other fields
shouldn''t follow suit.
>> I don''t mind changing them all in one patch.. But -
>>
>> Since it is not directly related to the main purpose of the patch,
would
>> it be okay
>> if I did this in a follow up patch and go back to using
''int'' for now?
> "Go back" is probably the wrong term - in the new code you add
> you should strive to use unsigned int where possible. And in a
> second patch, converting the bogus uses of plain int to unsigned
> would be desirable.
Okay, Will do..
>>>>                    /* Not IO */
>>>>                    if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO)
)
>>>> -                    continue;
>>>> +                {
>>>> +                    vendor = pci_conf_read16(0, b, d, f,
PCI_VENDOR_ID);
>>>> +                    device = pci_conf_read16(0, b, d, f,
PCI_DEVICE_ID);
>>>> +
>>>> +                    /* 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;
>>>> +
>>>> +                        if ( bar_idx >=
uart_config[i].max_bars )
>>>> +                            break;
>>> Did you not mean "continue" here?
>> No.. break is fine here since we only support bar0 for this specific
>> device..
>> (By the time we reach this condition, we have already verified
''vendor''
>> and ''device'' from
>> uart_config table anyway.. it is needless to iterate through other
>> devices in the table)
> Here you make assumptions on the single entry you know the list
> is currently holding. But you should consider the general case,
> and I don''t see why there couldn''t be two flavors of a
device
> having the same (vendor, device) pair but different max_bars.
Okay, I''ll make it ''continue'' here too..

Although, to differentaite between two ''flavors'' of device
having same
(vendor, device) id''s,
It might be worthwhile(in the future) to add a new field in 
''uart_config'' table that clearly identifies the flavors..
>>>> +                    /* Not 8 bytes */
>>>> +                    if ( (len & 0xffff) != 0xfff9 )
>>>> +                        continue;
>>> I think this size checking actually should also be done in the MMIO
>>> case.
>> This check would not work for this device as the length of region is
64K.
> I didn''t mean you to make the exact same check; but I do expect
> you to do some similar checking.
>
>> But if we really want to force a length check for MMIO cases as well,
>> we can define another field in struct uart_config and verify if length
>> matches..
>> Do let me know what you make of it..
> If you think an exact match check is necessary, then yes, such a
> new field might be needed. But since I don''t see Xen depending
> on the exact size, but just on it being at least a certain size, doing
> just that check would seem fine to me.
Okay, Will fix this.
> But then you''d need to carefully consider what the remainder of
> the MMIO range is used for - if any of that could conflict with
> what Xen needs to fully control the device, then you should also
> hide any PAGE_SIZE chunks from all domains (including Dom0).
> (Unfortunately we can''t currently hide sub-page chunks in an
> effective manner.)
With respect to this,
I am not seeing any untoward side effects to Xen from letting the code 
run as-is.
I have tested the patch with the Broadcom 5725 UART chip and a IO based 
UART as well
and both seem to function fine..

I did try using ''iomem_deny_access'' to hide the MMIO range
from dom0.
But I don''t see an effect.
Not sure if I am missing something or is there a different way to hide 
PAGE_SIZE chunks?

I will spin out a V3 now as you can verify the changes I make..

Thanks,
-Aravind.


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

>>> On 19.11.13 at 17:40, Aravind Gopalakrioshnan
<aravind.gopalakrishnan@amd.com> wrote:
> On 11/18/2013 2:04 AM, Jan Beulich wrote:
>> But then you''d need to carefully consider what the remainder
of
>> the MMIO range is used for - if any of that could conflict with
>> what Xen needs to fully control the device, then you should also
>> hide any PAGE_SIZE chunks from all domains (including Dom0).
>> (Unfortunately we can''t currently hide sub-page chunks in an
>> effective manner.)
> 
> With respect to this,
> I am not seeing any untoward side effects to Xen from letting the code 
> run as-is.
> I have tested the patch with the Broadcom 5725 UART chip and a IO based 
> UART as well
> and both seem to function fine..
Sure, because you surely used a well behaved Dom0. But you
should (a) protect Xen from a bug in Dom0 and (b) either prevent
the device from being assigned to a guest, or make sure a
malicious guest can''t interfere with Xen''s operation.
> I did try using ''iomem_deny_access'' to hide the MMIO
range from dom0.
> But I don''t see an effect.
> Not sure if I am missing something or is there a different way to hide 
> PAGE_SIZE chunks?
No, what you say you did is correct afaict.

Jan

On 11/19/2013 10:47 AM, Jan Beulich wrote:
>>>> On 19.11.13 at 17:40, Aravind Gopalakrioshnan
<aravind.gopalakrishnan@amd.com> wrote:
>> On 11/18/2013 2:04 AM, Jan Beulich wrote:
>>> But then you''d need to carefully consider what the
remainder of
>>> the MMIO range is used for - if any of that could conflict with
>>> what Xen needs to fully control the device, then you should also
>>> hide any PAGE_SIZE chunks from all domains (including Dom0).
>>> (Unfortunately we can''t currently hide sub-page chunks in
an
>>> effective manner.)
>> With respect to this,
>> I am not seeing any untoward side effects to Xen from letting the code
>> run as-is.
>> I have tested the patch with the Broadcom 5725 UART chip and a IO based
>> UART as well
>> and both seem to function fine..
> Sure, because you surely used a well behaved Dom0. But you
> should (a) protect Xen from a bug in Dom0 and (b) either prevent
> the device from being assigned to a guest, or make sure a
> malicious guest can''t interfere with Xen''s operation.
>
>> I did try using ''iomem_deny_access'' to hide the MMIO
range from dom0.
>> But I don''t see an effect.
>> Not sure if I am missing something or is there a different way to hide
>> PAGE_SIZE chunks?
> No, what you say you did is correct afaict.
>
>
Looks like the arguments I was passing in to
''iomem_deny_access'' was
incorrect before (apologies)
(I just had to use paddr_to_pfn() to get PAGE_SHIFT-ed value)
I tried with the proper (page shifted) values, but it breaks Xen 
throwing the message:

(XEN) mm.c:785:d0 Non-privileged (0) attempt to map I/O space

The reason for this is -  dom0 sees the UART device and tries to 
configure it at the bar value (which is blocked by Xen)
which means pci_hide_device() is not functioning as expected..(again, 
not sure if I am missing something..). But-
Could this be due to the fact that this is a multifunction device and 
the UART is only a subfunction?

Meanwhile, I am sending a V3 with the other changes..

Thanks,
Aravind.

>>> On 20.11.13 at 01:53, Aravind Gopalakrishnan
<aravind.gopalakrishnan@amd.com> wrote:
> Looks like the arguments I was passing in to
''iomem_deny_access'' was
> incorrect before (apologies)
> (I just had to use paddr_to_pfn() to get PAGE_SHIFT-ed value)
> I tried with the proper (page shifted) values, but it breaks Xen 
> throwing the message:
> 
> (XEN) mm.c:785:d0 Non-privileged (0) attempt to map I/O space
Xen should not be affected by this message appearing; Dom0
likely would be in one way or another.
> The reason for this is -  dom0 sees the UART device and tries to 
> configure it at the bar value (which is blocked by Xen)
> which means pci_hide_device() is not functioning as expected..(again, 
> not sure if I am missing something..).
Then you didn''t understand the purpose of pci_hide_device(),
yet I would have expected you to have looked at commit e46ea4d4
("PCI: don''t allow guest assignment of devices used by Xen")
in this
context: Such devices are unavailable for assignment to a DomU,
but visible as usual to Dom0 (and nothing prevents Dom0 from
assigning the device e.g. new BAR values - pci_ro_device() would -,
and hence using iomem_deny_access() is pointless/wrong).
> But-
> Could this be due to the fact that this is a multifunction device and 
> the UART is only a subfunction?
Multi-function in the usual sense? If so, all the BARs on that
function are only to be used by that function... Or do you perhaps
mean a function in the PCI sense providing more functionality than
just the serial port (as in many other combined serial/parallel or
multi-port serial add in cards)?

Jan

On 11/20/2013 2:10 AM, Jan Beulich wrote:
>>>> On 20.11.13 at 01:53, Aravind Gopalakrishnan
<aravind.gopalakrishnan@amd.com> wrote:
>> Looks like the arguments I was passing in to
''iomem_deny_access'' was
>> incorrect before (apologies)
>> (I just had to use paddr_to_pfn() to get PAGE_SHIFT-ed value)
>> I tried with the proper (page shifted) values, but it breaks Xen
>> throwing the message:
>>
>> (XEN) mm.c:785:d0 Non-privileged (0) attempt to map I/O space
> Xen should not be affected by this message appearing; Dom0
> likely would be in one way or another.
>
>> The reason for this is -  dom0 sees the UART device and tries to
>> configure it at the bar value (which is blocked by Xen)
>> which means pci_hide_device() is not functioning as expected..(again,
>> not sure if I am missing something..).
> Then you didn''t understand the purpose of pci_hide_device(),
> yet I would have expected you to have looked at commit e46ea4d4
> ("PCI: don''t allow guest assignment of devices used by
Xen") in this
> context: Such devices are unavailable for assignment to a DomU,
> but visible as usual to Dom0 (and nothing prevents Dom0 from
> assigning the device e.g. new BAR values - pci_ro_device() would -,
> and hence using iomem_deny_access() is pointless/wrong).
>
>> But-
>> Could this be due to the fact that this is a multifunction device and
>> the UART is only a subfunction?
> Multi-function in the usual sense? If so, all the BARs on that
> function are only to be used by that function... Or do you perhaps
> mean a function in the PCI sense providing more functionality than
> just the serial port (as in many other combined serial/parallel or
> multi-port serial add in cards)?
>
> Jan
>
>
I meant multi-function as in latter sense (provides more than a serial 
port). Here is snapshot of lspci for clarity -

02:00.0 Ethernet controller [0200]: Broadcom Corporation NetXtreme 
BCM5725 Gigabit Ethernet PCIe [14e4:1643] (rev 10)
02:00.5 Communication controller [0780]: Broadcom Corporation Device 
[14e4:160a] (rev 01)
02:00.6 IPMI SMIC interface [0c07]: Broadcom Corporation Device 
[14e4:16b9] (rev 10)

Anyway, I have now reworked the code such that Xen hides the MMIO region 
from (io_base+PAGE_SIZE) to end of the region.
([    0.000000] Xen: [mem 0x00000000d0031000-0x00000000d003ffff] reserved)
This would (in some extent) alleviate conflicts in case Dom0 interferes 
with Xen''s control of the device as we are hiding all
possible PAGE_SIZE''d chunks of the MMIO region..
Since we can''t hide the device from dom0, dom0 sees the device and
tries
to ''ioremap'' at the above said regions, but fails.
Although it does not break Xen, dom0 throws a stack trace with a warning 
message. dom0 continues to boot fine..

Also, to your comments on V3 - (Sorry I have to do this here as my mail 
client seems to have lost the V3 thread..)
I have fixed the code in accordance to your comments except for the 
''(-len)'' suggestion. Reason being -
It does not work all the time. Example: (for IO case)
after applying (len &= PCI_BASE_ADDRESS_IO_MASK),
len = fff8;
len = -(len) would give you ffff0008 and you will need to mask off 
higher 16 bits again..

Instead, I have kept length calculation consistent with what linux does 
and extended that to IO case..

Sending the changes out as V4.
(Tested the code changes with BCM5725 and an IO based UART and verified 
to be working correctly..)

-Aravind.

On Thu, Nov 21, 2013 at 04:49:33PM -0600, Aravind Gopalakrishnan
wrote:
> On 11/20/2013 2:10 AM, Jan Beulich wrote:
> >>>>On 20.11.13 at 01:53, Aravind Gopalakrishnan
<aravind.gopalakrishnan@amd.com> wrote:
> >>Looks like the arguments I was passing in to
''iomem_deny_access'' was
> >>incorrect before (apologies)
> >>(I just had to use paddr_to_pfn() to get PAGE_SHIFT-ed value)
> >>I tried with the proper (page shifted) values, but it breaks Xen
> >>throwing the message:
> >>
> >>(XEN) mm.c:785:d0 Non-privileged (0) attempt to map I/O space
> >Xen should not be affected by this message appearing; Dom0
> >likely would be in one way or another.
> >
> >>The reason for this is -  dom0 sees the UART device and tries to
> >>configure it at the bar value (which is blocked by Xen)
> >>which means pci_hide_device() is not functioning as
expected..(again,
> >>not sure if I am missing something..).
> >Then you didn''t understand the purpose of pci_hide_device(),
> >yet I would have expected you to have looked at commit e46ea4d4
> >("PCI: don''t allow guest assignment of devices used by
Xen") in this
> >context: Such devices are unavailable for assignment to a DomU,
> >but visible as usual to Dom0 (and nothing prevents Dom0 from
> >assigning the device e.g. new BAR values - pci_ro_device() would -,
> >and hence using iomem_deny_access() is pointless/wrong).
> >
> >>But-
> >>Could this be due to the fact that this is a multifunction device
and
> >>the UART is only a subfunction?
> >Multi-function in the usual sense? If so, all the BARs on that
> >function are only to be used by that function... Or do you perhaps
> >mean a function in the PCI sense providing more functionality than
> >just the serial port (as in many other combined serial/parallel or
> >multi-port serial add in cards)?
> >
> >Jan
> >
> >
> 
> I meant multi-function as in latter sense (provides more than a
> serial port). Here is snapshot of lspci for clarity -
> 
> 02:00.0 Ethernet controller [0200]: Broadcom Corporation NetXtreme
> BCM5725 Gigabit Ethernet PCIe [14e4:1643] (rev 10)
> 02:00.5 Communication controller [0780]: Broadcom Corporation Device
> [14e4:160a] (rev 01)
> 02:00.6 IPMI SMIC interface [0c07]: Broadcom Corporation Device
> [14e4:16b9] (rev 10)
> 
> Anyway, I have now reworked the code such that Xen hides the MMIO
> region from (io_base+PAGE_SIZE) to end of the region.
> ([    0.000000] Xen: [mem 0x00000000d0031000-0x00000000d003ffff] reserved)
> This would (in some extent) alleviate conflicts in case Dom0
> interferes with Xen''s control of the device as we are hiding all
> possible PAGE_SIZE''d chunks of the MMIO region..
> Since we can''t hide the device from dom0, dom0 sees the device and
> tries to ''ioremap'' at the above said regions, but fails.
> Although it does not break Xen, dom0 throws a stack trace with a
> warning message. dom0 continues to boot fine..
What is the stack trace? So that at least I know what to look for
if somebody mentions this?

Thanks
> 
> Also, to your comments on V3 - (Sorry I have to do this here as my
> mail client seems to have lost the V3 thread..)
> I have fixed the code in accordance to your comments except for the
> ''(-len)'' suggestion. Reason being -
> It does not work all the time. Example: (for IO case)
> after applying (len &= PCI_BASE_ADDRESS_IO_MASK),
> len = fff8;
> len = -(len) would give you ffff0008 and you will need to mask off
> higher 16 bits again..
> 
> Instead, I have kept length calculation consistent with what linux
> does and extended that to IO case..
> 
> Sending the changes out as V4.
> (Tested the code changes with BCM5725 and an IO based UART and
> verified to be working correctly..)
> 
> -Aravind.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Nov 26, 2013, at 10:37 AM, Konrad Rzeszutek Wilk 
<konrad.wilk@oracle.com> wrote:
> On Thu, Nov 21, 2013 at 04:49:33PM -0600, Aravind Gopalakrishnan wrote:
>> On 11/20/2013 2:10 AM, Jan Beulich wrote:
>>>>>> On 20.11.13 at 01:53, Aravind Gopalakrishnan 
>>>>>> <aravind.gopalakrishnan@amd.com> wrote:
>>>> Looks like the arguments I was passing in to
''iomem_deny_access'' was
>>>> incorrect before (apologies)
>>>> (I just had to use paddr_to_pfn() to get PAGE_SHIFT-ed value)
>>>> I tried with the proper (page shifted) values, but it breaks
Xen
>>>> throwing the message:
>>>>
>>>> (XEN) mm.c:785:d0 Non-privileged (0) attempt to map I/O space
>>> Xen should not be affected by this message appearing; Dom0
>>> likely would be in one way or another.
>>>
>>>> The reason for this is -  dom0 sees the UART device and tries
to
>>>> configure it at the bar value (which is blocked by Xen)
>>>> which means pci_hide_device() is not functioning as
expected..(again,
>>>> not sure if I am missing something..).
>>> Then you didn''t understand the purpose of
pci_hide_device(),
>>> yet I would have expected you to have looked at commit e46ea4d4
>>> ("PCI: don''t allow guest assignment of devices used
by Xen") in this
>>> context: Such devices are unavailable for assignment to a DomU,
>>> but visible as usual to Dom0 (and nothing prevents Dom0 from
>>> assigning the device e.g. new BAR values - pci_ro_device() would -,
>>> and hence using iomem_deny_access() is pointless/wrong).
>>>
>>>> But-
>>>> Could this be due to the fact that this is a multifunction
device and
>>>> the UART is only a subfunction?
>>> Multi-function in the usual sense? If so, all the BARs on that
>>> function are only to be used by that function... Or do you perhaps
>>> mean a function in the PCI sense providing more functionality than
>>> just the serial port (as in many other combined serial/parallel or
>>> multi-port serial add in cards)?
>>>
>>> Jan
>>>
>>>
>>
>> I meant multi-function as in latter sense (provides more than a
>> serial port). Here is snapshot of lspci for clarity -
>>
>> 02:00.0 Ethernet controller [0200]: Broadcom Corporation NetXtreme
>> BCM5725 Gigabit Ethernet PCIe [14e4:1643] (rev 10)
>> 02:00.5 Communication controller [0780]: Broadcom Corporation Device
>> [14e4:160a] (rev 01)
>> 02:00.6 IPMI SMIC interface [0c07]: Broadcom Corporation Device
>> [14e4:16b9] (rev 10)
>>
>> Anyway, I have now reworked the code such that Xen hides the MMIO
>> region from (io_base+PAGE_SIZE) to end of the region.
>> ([    0.000000] Xen: [mem 0x00000000d0031000-0x00000000d003ffff] 
>> reserved)
>> This would (in some extent) alleviate conflicts in case Dom0
>> interferes with Xen''s control of the device as we are hiding
all
>> possible PAGE_SIZE''d chunks of the MMIO region..
>> Since we can''t hide the device from dom0, dom0 sees the device
and
>> tries to ''ioremap'' at the above said regions, but
fails.
>> Although it does not break Xen, dom0 throws a stack trace with a
>> warning message. dom0 continues to boot fine..
>
> What is the stack trace? So that at least I know what to look for
> if somebody mentions this?
>
> Thanks
>>
>>
(per Jan''s commets), I have reworked the patch to make the pci device 
read only and also included the MMIO range to mmio_ro_ranges. This seems 
to work fine and does not throw any dom0 stack trace either.. (Am 
sending this out as V6)

In case you are still curious, here is the stack trace from dom0 when I 
used iomem_deny_access -

(XEN) mm.c:785:d0 Non-privileged (0) attempt to map I/O space 000d003f
[ 13.726130] resource map sanity check conflict: 0xd0030000 0xd003ffff 
0xd0031000 0xd003ffff reserved
[ 13.758008] ------------[ cut here ]------------
[ 13.758231] WARNING: CPU: 31 PID: 1 at arch/x86/mm/ioremap.c:171 
__ioremap_caller+0x2f2/0x380()
[ 13.759200] Info: mapping multiple BARs. Your kernel is fine.
[ 13.760048] Modules linked in:
[ 13.790037] CPU: 31 PID: 1 Comm: swapper/0 Not tainted 3.12.0-rc5+ #7
[ 13.790984] Hardware name: empty empty/ S8237 , BIOS V1.0B 07/23/2013
[ 13.791214]  0000000000000009 ffff880828cbf948 ffffffff816f867d 
ffff880828cbf990
[ 13.792269]  ffff880828cbf980 ffffffff81067a3c ffffc900036a0000 
00000000d0040000
[ 13.823332]  00000000d0030000 00000000000d0040 0000000000010000 
ffff880828cbf9e0
[ 13.824280] Call Trace:
[ 13.824987]  [<ffffffff816f867d>] dump_stack+0x45/0x56
[ 13.825183]  [<ffffffff81067a3c>] warn_slowpath_common+0x8c/0xc0
[ 13.855176]  [<ffffffff81067b2c>] warn_slowpath_fmt+0x4c/0x50
[ 13.856003]  [<ffffffff810580c2>] __ioremap_caller+0x2f2/0x380
[ 13.856218]  [<ffffffff810581a7>] ioremap_nocache+0x17/0x20
[ 13.888203]  [<ffffffff8146598f>] setup_port+0x10f/0x130
[ 13.888403]  [<ffffffff81465c01>] pci_default_setup+0x91/0xb0
[ 13.889244]  [<ffffffff81465c32>] pci_brcm_trumanage_setup+0x12/0x30
[ 13.890151]  [<ffffffff81466f47>] pciserial_init_ports+0x147/0x210
[ 13.913649]  [<ffffffff814670e9>] pciserial_init_one+0xd9/0x1e0
[ 13.913870]  [<ffffffff813a21cb>] local_pci_probe+0x4b/0x80
[ 13.915128]  [<ffffffff813a2421>] pci_device_probe+0x111/0x120
[ 13.915370]  [<ffffffff81484e37>] driver_probe_device+0x77/0x240
[ 13.915632]  [<ffffffff814850ab>] __driver_attach+0xab/0xb0
[ 13.916893]  [<ffffffff81485000>] ? driver_probe_device+0x240/0x240
[ 13.917122]  [<ffffffff81482f3d>] bus_for_each_dev+0x5d/0xa0
[ 13.918376]  [<ffffffff8148491e>] driver_attach+0x1e/0x20
[ 13.918579]  [<ffffffff81484434>] bus_add_driver+0x104/0x290
[ 13.918788]  [<ffffffff814857c4>] driver_register+0x64/0xf0
[ 13.920076]  [<ffffffff81d60e66>] ? early_serial_setup+0x129/0x129
[ 13.920330]  [<ffffffff813a12fc>] __pci_register_driver+0x4c/0x50
[ 13.921617]  [<ffffffff81d60e7f>] serial_pci_driver_init+0x19/0x1b
[ 13.921844]  [<ffffffff8100217a>] do_one_initcall+0x12a/0x190
[ 13.922093]  [<ffffffff81088ffb>] ? parse_args+0x1fb/0x350
[ 13.923385]  [<ffffffff81d1a047>] kernel_init_freeable+0x139/0x1cb
[ 13.923611]  [<ffffffff81d19815>] ? loglevel+0x31/0x31
[ 13.924843]  [<ffffffff816e8e70>] ? rest_init+0x80/0x80
[ 13.925040]  [<ffffffff816e8e7e>] kernel_init+0xe/0xf0
[ 13.925262]  [<ffffffff8170803c>] ret_from_fork+0x7c/0xb0
[ 13.925468]  [<ffffffff816e8e70>] ? rest_init+0x80/0x80
[ 13.926690] ---[ end trace 6d7c17d875f132f5 ]---

(Apologies for the late reply.. )



Thanks,
Aravind
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org <mailto:Xen-devel@lists.xen.org>
>> http://lists.xen.org/xen-devel



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