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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/12/2013 08:22, Jan Beulich wrote:
>>>> On 06.12.13 at 21:31, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
>> On 12/6/2013 10:00 AM, George Dunlap wrote:
>>> 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_c 
>> ode_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.
> Honestly, if I'm asked - I'm not convinced. To me this boils down to
> low risk low benefit, with the risk analysis part apparently heavily
> biased towards "the patch appears to be bug free", whereas from
> a patch history perspective this clearly wasn't the case from the
> beginning, and hence there's a fair chance that some aspect was
> still overlooked in the latest review round. Furthermore we're not
> talking about something that was on the feature list for 4.4.
>
> Jan
>

It turns out that we have some of this hardware in our testing pool.

Therefore,

Tested-by: Andrew Cooper <andrew.cooper3@citrix.com> (by way of backport
to 4.3)

I would however agree that on the whole, it is probably too high-risk /
low-reward  for inclusion in 4.4, but it should be fine for accepting as
soon as the 4.5 window opens.

~Andrew