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

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

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.