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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.12.13 at 21:31, Aravind Gopalakrishnan 
>>> <aravind.gopalakrishnan@xxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel