From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH V8] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips Date: Mon, 16 Dec 2013 14:29:09 +0000 Message-ID: <52AF0E35.7070000@citrix.com> References: <1386283126-2045-1-git-send-email-Aravind.Gopalakrishnan@amd.com> <52A19BC8020000780010ABFC@nat28.tlf.novell.com> <52A1F2B9.2070504@amd.com> <52A1F4AC.6020506@eu.citrix.com> <52A23410.6070906@amd.com> <52A58BCF020000780010B3F4@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <52A58BCF020000780010B3F4@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Aravind Gopalakrishnan , George Dunlap Cc: Thomas Lendacky , xen-devel@lists.xen.org, shurd@broadcom.com, Suravee Suthikulpanit , Sherry Hurwitz List-Id: xen-devel@lists.xenproject.org On 09/12/2013 08:22, Jan Beulich wrote: >>>> On 06.12.13 at 21:31, Aravind Gopalakrishnan 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 (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