From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36968) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wrn7c-0006g7-HR for qemu-devel@nongnu.org; Tue, 03 Jun 2014 07:43:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wrn7W-0001wv-1v for qemu-devel@nongnu.org; Tue, 03 Jun 2014 07:43:16 -0400 Received: from smtp.citrix.com ([66.165.176.89]:9552) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wrn7V-0001wb-SC for qemu-devel@nongnu.org; Tue, 03 Jun 2014 07:43:10 -0400 Message-ID: <538DB4B4.6000602@eu.citrix.com> Date: Tue, 3 Jun 2014 12:42:44 +0100 From: George Dunlap MIME-Version: 1.0 References: <1401440369-29929-1-git-send-email-tiejun.chen@intel.com> <1401440369-29929-3-git-send-email-tiejun.chen@intel.com> <538D8B59.4070105@redhat.com> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v4][PATCH 2/5] xen, gfx passthrough: create intel isa bridge List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini , Paolo Bonzini Cc: peter.maydell@linaro.org, xen-devel@lists.xensource.com, Ian Campbell , mst@redhat.com, Ian Jackson , allen.m.kay@intel.com, Kelly.Zytaruk@amd.com, Jan Beulich , qemu-devel@nongnu.org, yang.z.zhang@intel.com, Tim Deegan , Anthony Liguori , anthony.perard@citrix.com, Tiejun Chen , George Dunlap , Konrad Rzeszutek Wilk On 06/03/2014 12:29 PM, Stefano Stabellini wrote: > On Tue, 3 Jun 2014, Paolo Bonzini wrote: >> Il 30/05/2014 10:59, Tiejun Chen ha scritto: >>> +static int create_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) >>> +{ >>> + struct PCIDevice *dev; >>> + >>> + char rid; >>> + >>> + dev = pci_create(bus, PCI_DEVFN(0x1f, 0), "intel-pch-isa-bridge"); >> This is really a huge hack. You're going to have two ISA bridge devices in >> the machine, with the BIOS imagining that the "good" one is at 1f.0 and the >> ACPI tables actually describing the other one. But the PCI device at 1f.0 >> remains there and a driver in the OS could catch it---not just >> intel_detect_pch---and if you want to add such a hack it should be done in the >> Xen management layers. >> >> If possible, the host bridge patches are even worse. If you change the vendor >> and device ID while keeping the registers of the i440FX you're going to get >> conflicts or break firmware badly; TianoCore and SeaBIOS both expect the >> normal i440FX vendor and device IDs, for example. >> >> The hardcoded list of offsets is also not acceptable. It is also not clear >> who is accessing the registers, whether the BIOS or the driver. For Linux, a >> cursory look at the driver shows that it only accesses 0x50/0x52 of the listed >> offsets, but also 0x44/0x48 ("MCH BAR"), what happens if that code path is >> encountered? >> >> The main problem with IGD passthrough is the incestuous (and that's a >> euphemism) relationship between the MCH, PCH and graphics driver. It may make >> sense at the hardware level, but for virtualization it doesn't. A >> virt-specific driver for GPU command passthrough (with aid from the kernel >> driver, but abstracting all the MCH/PCH-dependent details) would make much >> more sense. >> >> It's really not your fault, there's not much you can do given the hardware >> architecture. But I don't think this code can be accepted upstream, sorry. > Yeah, the code is not nice and it is not Tiejun's fault. > > Is there any way at all he could change the patch series to make it more > appealing to you? Or maybe we could having more clearly separated from > the rest of the codebase? > > > Otherwise I hate to have to diverge again from upstream QEMU but given > that we were already carrying these changes in the old > qemu-xen-traditional tree without issues, I feel that it would be unfair > for me not to merge them in the upstream based qemu-xen tree. > Unfortunately I imagine that the lack of this feature could be > considered a regression for us. > > Do the other Xen maintainters have any opinions on this? I would > appreciate your opinions. Well my very initial take is to say that it was a mistake to accept the IGD stuff into qemu-xen-traditional before making sure that it would be suitable for qemu-upstream. Avoiding having a fork again (or maintaining a set of out-of-tree patches) is more important than this one feature, IMHO. When Intel submitted this for qemu-xen-traditional, we should have recommended to them at that time to start with qemu-upstream; or, we should have made it clear that if they chose to submit it to qemu-xen-traditional first, they would be taking the risk of having it only be there. If we didn't warn them of that, that was a mistake on our part; but I don't think we can do anything other than apologize. (And of course see if there *is* a way to actually get it upstream.) -George