| Message ID | 1451994098-6972-12-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 5 Jan 2016, Gerd Hoffmann wrote: > This patch moves igd-passthrough-isa-bridge creation out of the xen > passthrough code into machine init. It is triggered by the > igd-passthru=on machine option. Advantages: > > * This works for on both xen and kvm. > * It is activated for the pc machine type only, q35 has a real > isa bridge on 1f.0 and must be handled differently. The q35 > plan is https://lkml.org/lkml/2015/11/26/183 (should land in > the next merge window, i.e. linux 4.5). > * If we don't need it any more some day (intel is busy removing > chipset dependencies from the guest driver) we have a single > machine switch to just turn off all igd passthru chipset > tweaks. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/i386/pc_piix.c | 6 ++++++ > hw/xen/xen_pt.c | 14 -------------- > 2 files changed, 6 insertions(+), 14 deletions(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index f36222e..2afbbd3 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -281,6 +281,12 @@ static void pc_init1(MachineState *machine, > if (pcmc->pci_enabled) { > pc_pci_device_init(pci_bus); > } > + > +#ifdef CONFIG_LINUX > + if (machine->igd_gfx_passthru) { > + igd_passthrough_isa_bridge_create(pci_bus); > + } > +#endif One thing I don't like about this is that it is going to skip the checks done in xen_pt_initfn. For example it is going to create the isa bridge, even if there is going to be an error loading the vga bios or if the device specified is not even an Intel graphic card. > } > > /* Looking for a pc_compat_2_4() function? It doesn't exist. > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > index 18a7f72..5f626c9 100644 > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -685,17 +685,6 @@ static const MemoryListener xen_pt_io_listener = { > .priority = 10, > }; > > -static void > -xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s, > - XenHostPCIDevice *dev) > -{ > - uint16_t gpu_dev_id; > - PCIDevice *d = &s->dev; > - > - gpu_dev_id = dev->device_id; > - igd_passthrough_isa_bridge_create(d->bus); > -} > - > /* destroy. */ > static void xen_pt_destroy(PCIDevice *d) { > > @@ -810,9 +799,6 @@ static int xen_pt_initfn(PCIDevice *d) > xen_host_pci_device_put(&s->real_device); > return -1; > } > - > - /* Register ISA bridge for passthrough GFX. */ > - xen_igd_passthrough_isa_bridge_create(s, &s->real_device); > } > > /* Handle real device's MMIO/PIO BARs */ > -- > 1.8.3.1 >
Hi, > One thing I don't like about this is that it is going to skip the checks > done in xen_pt_initfn. Hmm? Those checks are still done when you assign a igd ... > For example it is going to create the isa bridge, > even if there is going to be an error loading the vga bios or if the > device specified is not even an Intel graphic card. Creating the special igd-isa-bridge is no longer tied to actually assigning a igd, but to the igd-passthru=on machine option being present (and machine type being 'pc'). xen_pt_initfn checks that igd-passthru=on is set in case it finds a igd device is assigned, that will make sure the igd-isa-bridge is present. But, yes, you can create a igd-isa-bridge now even when not assigning a igd device, either by specifying igd-passthru=on or using -device. I fail to see why this is a problem though, care to explain? Also note that moving this to machine init nicely handles the fact that the igd-isa-bridge is needed on 'pc' only, not on 'q35'. If you don't want create the igd-isa-bridge in machine init, what is your alternative suggestion to handle this? cheers, Gerd
CC'ing the Xen x86 maintainers On Thu, 7 Jan 2016, Gerd Hoffmann wrote: > Hi, > > > One thing I don't like about this is that it is going to skip the checks > > done in xen_pt_initfn. > > Hmm? Those checks are still done when you assign a igd ... Their failure doesn't affect the creation of the bridge. > > For example it is going to create the isa bridge, > > even if there is going to be an error loading the vga bios or if the > > device specified is not even an Intel graphic card. > > Creating the special igd-isa-bridge is no longer tied to actually > assigning a igd, but to the igd-passthru=on machine option being present > (and machine type being 'pc'). and machine type 'xenfv', unless I am mistaken? > xen_pt_initfn checks that igd-passthru=on is set in case it finds a igd > device is assigned, that will make sure the igd-isa-bridge is present. > > But, yes, you can create a igd-isa-bridge now even when not assigning a > igd device, either by specifying igd-passthru=on or using -device. I > fail to see why this is a problem though, care to explain? It is going to change the PCI layout of any virtual machines with a config file containing gfx_passthru="igd" and no pci config line. A Xen 4.7 user could add gfx_passthru="igd" to all her VM config files, because actually it does nothing unless an Intel graphic card is assigned to the VM. With this change she couldn't migrate their VMs from Xen 4.7 to Xen 4.8 safely because suddenly a new bridge appears. That said Xen 4.7 hasn't been released yet, so we could still change the intended behaviour. But it would require a bit of coordination (the qemu-xen tree in 4.7 is based on v2.4.1). > Also note that moving this to machine init nicely handles the fact that > the igd-isa-bridge is needed on 'pc' only, not on 'q35'. If you don't > want create the igd-isa-bridge in machine init, what is your alternative > suggestion to handle this? Maybe we could retain the check whether an Intel graphic card has been assigned?
On Do, 2016-01-07 at 13:10 +0000, Stefano Stabellini wrote: > CC'ing the Xen x86 maintainers > > On Thu, 7 Jan 2016, Gerd Hoffmann wrote: > > Hi, > > > > > One thing I don't like about this is that it is going to skip the checks > > > done in xen_pt_initfn. > > > > Hmm? Those checks are still done when you assign a igd ... > > Their failure doesn't affect the creation of the bridge. Doesn't their failure makes qemu throw a fatal error and exit? So the guest isn't going to run either way? > > > For example it is going to create the isa bridge, > > > even if there is going to be an error loading the vga bios or if the > > > device specified is not even an Intel graphic card. > > > > Creating the special igd-isa-bridge is no longer tied to actually > > assigning a igd, but to the igd-passthru=on machine option being present > > (and machine type being 'pc'). > > and machine type 'xenfv', unless I am mistaken? Yes, xenfv too (uses i440fx too and thus is a 'pc' derivate). > > xen_pt_initfn checks that igd-passthru=on is set in case it finds a igd > > device is assigned, that will make sure the igd-isa-bridge is present. > > > > But, yes, you can create a igd-isa-bridge now even when not assigning a > > igd device, either by specifying igd-passthru=on or using -device. I > > fail to see why this is a problem though, care to explain? > > It is going to change the PCI layout of any virtual machines with a > config file containing > > gfx_passthru="igd" > > and no pci config line. A Xen 4.7 user could add gfx_passthru="igd" to > all her VM config files, because actually it does nothing unless an > Intel graphic card is assigned to the VM. No. It changes the host bridge even when not passing through a igd, because that is linked to igd-passthru=on only. So making both host bridge tweak and isa bridge tweak triggered by igd-passthru=on brings more consistency to the whole thing. > > Also note that moving this to machine init nicely handles the fact that > > the igd-isa-bridge is needed on 'pc' only, not on 'q35'. If you don't > > want create the igd-isa-bridge in machine init, what is your alternative > > suggestion to handle this? > > Maybe we could retain the check whether an Intel graphic card has been > assigned? Should be possible, but is not that easy due to initialization order issues. cheers, Gerd
On Thu, 7 Jan 2016, Gerd Hoffmann wrote: > On Do, 2016-01-07 at 13:10 +0000, Stefano Stabellini wrote: > > CC'ing the Xen x86 maintainers > > > > On Thu, 7 Jan 2016, Gerd Hoffmann wrote: > > > Hi, > > > > > > > One thing I don't like about this is that it is going to skip the checks > > > > done in xen_pt_initfn. > > > > > > Hmm? Those checks are still done when you assign a igd ... > > > > Their failure doesn't affect the creation of the bridge. > > Doesn't their failure makes qemu throw a fatal error and exit? > So the guest isn't going to run either way? No, it doesn't. QEMU doesn't even print an error message. > > > > For example it is going to create the isa bridge, > > > > even if there is going to be an error loading the vga bios or if the > > > > device specified is not even an Intel graphic card. > > > > > > Creating the special igd-isa-bridge is no longer tied to actually > > > assigning a igd, but to the igd-passthru=on machine option being present > > > (and machine type being 'pc'). > > > > and machine type 'xenfv', unless I am mistaken? > > Yes, xenfv too (uses i440fx too and thus is a 'pc' derivate). Good > > > xen_pt_initfn checks that igd-passthru=on is set in case it finds a igd > > > device is assigned, that will make sure the igd-isa-bridge is present. > > > > > > But, yes, you can create a igd-isa-bridge now even when not assigning a > > > igd device, either by specifying igd-passthru=on or using -device. I > > > fail to see why this is a problem though, care to explain? > > > > It is going to change the PCI layout of any virtual machines with a > > config file containing > > > > gfx_passthru="igd" > > > > and no pci config line. A Xen 4.7 user could add gfx_passthru="igd" to > > all her VM config files, because actually it does nothing unless an > > Intel graphic card is assigned to the VM. > > No. It changes the host bridge even when not passing through a igd, > because that is linked to igd-passthru=on only. > > So making both host bridge tweak and isa bridge tweak triggered by > igd-passthru=on brings more consistency to the whole thing. That is true. Given that the only qemu-xen codebase with igd support is 4.7 and 4.7 hasn't been released yet, I am OK with changing the guest visible PCI layout. I might ask for your help in backporting the patches ;-) > > > Also note that moving this to machine init nicely handles the fact that > > > the igd-isa-bridge is needed on 'pc' only, not on 'q35'. If you don't > > > want create the igd-isa-bridge in machine init, what is your alternative > > > suggestion to handle this? > > > > Maybe we could retain the check whether an Intel graphic card has been > > assigned? > > Should be possible, but is not that easy due to initialization order > issues.
On Fri, 8 Jan 2016, Stefano Stabellini wrote: > > > > xen_pt_initfn checks that igd-passthru=on is set in case it finds a igd > > > > device is assigned, that will make sure the igd-isa-bridge is present. > > > > > > > > But, yes, you can create a igd-isa-bridge now even when not assigning a > > > > igd device, either by specifying igd-passthru=on or using -device. I > > > > fail to see why this is a problem though, care to explain? > > > > > > It is going to change the PCI layout of any virtual machines with a > > > config file containing > > > > > > gfx_passthru="igd" > > > > > > and no pci config line. A Xen 4.7 user could add gfx_passthru="igd" to > > > all her VM config files, because actually it does nothing unless an > > > Intel graphic card is assigned to the VM. > > > > No. It changes the host bridge even when not passing through a igd, > > because that is linked to igd-passthru=on only. > > > > So making both host bridge tweak and isa bridge tweak triggered by > > igd-passthru=on brings more consistency to the whole thing. > > That is true. Given that the only qemu-xen codebase with igd support is > 4.7 and 4.7 hasn't been released yet, I am OK with changing the guest > visible PCI layout. I might ask for your help in backporting the patches > ;-) One thing that I forgot to consider is that QEMU 2.5 has been released with igd passthrough too and Xen 4.6 + QEMU 2.5 is a combination we should support. However QEMU 2.5 has a serious bug (http://marc.info/?l=qemu-devel&m=145172165010604) which probably prevents igd passthrough from working at all. I asked Xudong to investigate. I am thinking that if the feature works in 2.5, we need to support it, therefore we cannot break migration by changing the PCI layout. Otherwise if the feature doesn't work, we could take the liberty to make the change. Do you agree?
Hi, > > That is true. Given that the only qemu-xen codebase with igd support is > > 4.7 and 4.7 hasn't been released yet, I am OK with changing the guest > > visible PCI layout. I might ask for your help in backporting the patches > > ;-) What are the 4.7 release plans btw? > One thing that I forgot to consider is that QEMU 2.5 has been released > with igd passthrough too and Xen 4.6 + QEMU 2.5 is a combination we > should support. > > However QEMU 2.5 has a serious bug > (http://marc.info/?l=qemu-devel&m=145172165010604) which probably > prevents igd passthrough from working at all. Stumbled over that one too, so my series has a (different) fix for it as well. > I asked Xudong to investigate. I am thinking that if the feature > works in 2.5, we need to support it, therefore we cannot break > migration by changing the PCI layout. I'd expect it is broken (at least for older guests). In case 2.5 works fine as-is we should be able to ditch TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE altogether b/c it is a no-op in 2.5 because of the bug. But lets wait for the test results ... > Otherwise if the feature doesn't work, we could take the liberty to > make the change. Do you agree? Yes. cheers, Gerd
On Fri, 8 Jan 2016, Gerd Hoffmann wrote: > Hi, > > > > That is true. Given that the only qemu-xen codebase with igd support is > > > 4.7 and 4.7 hasn't been released yet, I am OK with changing the guest > > > visible PCI layout. I might ask for your help in backporting the patches > > > ;-) > > What are the 4.7 release plans btw? This is the last development update from the release manager: http://marc.info/?l=qemu-devel&m=145172165010604 * Last posting date: March 18, 2016 * Hard code freeze: April 1, 2016 * RC1: TBD * Release: June 3, 2016
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index f36222e..2afbbd3 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -281,6 +281,12 @@ static void pc_init1(MachineState *machine, if (pcmc->pci_enabled) { pc_pci_device_init(pci_bus); } + +#ifdef CONFIG_LINUX + if (machine->igd_gfx_passthru) { + igd_passthrough_isa_bridge_create(pci_bus); + } +#endif } /* Looking for a pc_compat_2_4() function? It doesn't exist. diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index 18a7f72..5f626c9 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -685,17 +685,6 @@ static const MemoryListener xen_pt_io_listener = { .priority = 10, }; -static void -xen_igd_passthrough_isa_bridge_create(XenPCIPassthroughState *s, - XenHostPCIDevice *dev) -{ - uint16_t gpu_dev_id; - PCIDevice *d = &s->dev; - - gpu_dev_id = dev->device_id; - igd_passthrough_isa_bridge_create(d->bus); -} - /* destroy. */ static void xen_pt_destroy(PCIDevice *d) { @@ -810,9 +799,6 @@ static int xen_pt_initfn(PCIDevice *d) xen_host_pci_device_put(&s->real_device); return -1; } - - /* Register ISA bridge for passthrough GFX. */ - xen_igd_passthrough_isa_bridge_create(s, &s->real_device); } /* Handle real device's MMIO/PIO BARs */
This patch moves igd-passthrough-isa-bridge creation out of the xen passthrough code into machine init. It is triggered by the igd-passthru=on machine option. Advantages: * This works for on both xen and kvm. * It is activated for the pc machine type only, q35 has a real isa bridge on 1f.0 and must be handled differently. The q35 plan is https://lkml.org/lkml/2015/11/26/183 (should land in the next merge window, i.e. linux 4.5). * If we don't need it any more some day (intel is busy removing chipset dependencies from the guest driver) we have a single machine switch to just turn off all igd passthru chipset tweaks. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/i386/pc_piix.c | 6 ++++++ hw/xen/xen_pt.c | 14 -------------- 2 files changed, 6 insertions(+), 14 deletions(-)