qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [v4][PATCH 2/5] xen, gfx passthrough: create intel isa


From: George Dunlap
Subject: Re: [Qemu-devel] [v4][PATCH 2/5] xen, gfx passthrough: create intel isa bridge
Date: Tue, 3 Jun 2014 12:42:44 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]