[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 11/18/2013 2:04 AM, Jan Beulich wrote:
On 14.11.13 at 18:40, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@xxxxxxx> 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 
(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
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 
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..

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.