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

[George Dunlap <george.dunlap@eu.citrix.com>]

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