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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

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.