|
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
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.
|
