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

[George Dunlap]

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