[Qemu-devel] [PATCH v2] vfio/igd: handle q35 machine type

[Qemu-devel] [PATCH v2] vfio/igd: handle q35 machine type

[Gerd Hoffmann]

Basically skip the lpc quirks with -M q35.
Applies on top of the vfio-igd patch series by alex.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/vfio/pci-quirks.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 5828362..1757e3d 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -12,6 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/nvram/fw_cfg.h"
+#include "hw/i386/ich9.h"
 #include "pci.h"
 #include "trace.h"
 #include "qemu/range.h"
@@ -1410,6 +1411,7 @@ int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
                            struct vfio_region_info *region)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(vdev);
+    PCIBus *bus;
     PCIDevice *lpc_bridge;
     int ret;
 
@@ -1423,6 +1425,19 @@ int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
 
     dc->hotpluggable = false;
 
+    bus = pci_device_root_bus(&vdev->pdev);
+    lpc_bridge = pci_find_device(bus, 0, PCI_DEVFN(0x1f, 0));
+    if (lpc_bridge) {
+        const char *type = object_get_typename(OBJECT(lpc_bridge));
+        if (strcmp(type, TYPE_ICH9_LPC_DEVICE) == 0) {
+            /*
+             * q35 lpc present, leave it as-is.
+             * linux kernel 4.5+ can deal with this.
+             */
+            return 0;
+        }
+    }
+
     lpc_bridge = pci_create_simple(pci_device_root_bus(&vdev->pdev),
                                    PCI_DEVFN(0x1f, 0),
                                    "vfio-pci-igd-lpc-bridge");
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] vfio/igd: handle q35 machine type

[Alex Williamson]

On Fri,  4 Mar 2016 09:41:53 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> Basically skip the lpc quirks with -M q35.
> Applies on top of the vfio-igd patch series by alex.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/vfio/pci-quirks.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index 5828362..1757e3d 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -12,6 +12,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/nvram/fw_cfg.h"
> +#include "hw/i386/ich9.h"
>  #include "pci.h"
>  #include "trace.h"
>  #include "qemu/range.h"
> @@ -1410,6 +1411,7 @@ int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
>                             struct vfio_region_info *region)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(vdev);
> +    PCIBus *bus;
>      PCIDevice *lpc_bridge;
>      int ret;
>  
> @@ -1423,6 +1425,19 @@ int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
>  
>      dc->hotpluggable = false;
>  
> +    bus = pci_device_root_bus(&vdev->pdev);
> +    lpc_bridge = pci_find_device(bus, 0, PCI_DEVFN(0x1f, 0));
> +    if (lpc_bridge) {
> +        const char *type = object_get_typename(OBJECT(lpc_bridge));
> +        if (strcmp(type, TYPE_ICH9_LPC_DEVICE) == 0) {
> +            /*
> +             * q35 lpc present, leave it as-is.
> +             * linux kernel 4.5+ can deal with this.

What about anything older?  I assume we want to support current
distributions.  Based on this patch it seems like we also want to test
first if the lpc device is present, create it if not, then probably
copy host data into it.  Thanks,

Alex

> +             */
> +            return 0;
> +        }
> +    }
> +
>      lpc_bridge = pci_create_simple(pci_device_root_bus(&vdev->pdev),
>                                     PCI_DEVFN(0x1f, 0),
>                                     "vfio-pci-igd-lpc-bridge");

Re: [Qemu-devel] [PATCH v2] vfio/igd: handle q35 machine type

[Gerd Hoffmann]

On Mo, 2016-03-07 at 14:41 -0700, Alex Williamson wrote:
> On Fri,  4 Mar 2016 09:41:53 +0100
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > Basically skip the lpc quirks with -M q35.
> > Applies on top of the vfio-igd patch series by alex.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  hw/vfio/pci-quirks.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> > index 5828362..1757e3d 100644
> > --- a/hw/vfio/pci-quirks.c
> > +++ b/hw/vfio/pci-quirks.c
> > @@ -12,6 +12,7 @@
> >  
> >  #include "qemu/osdep.h"
> >  #include "hw/nvram/fw_cfg.h"
> > +#include "hw/i386/ich9.h"
> >  #include "pci.h"
> >  #include "trace.h"
> >  #include "qemu/range.h"
> > @@ -1410,6 +1411,7 @@ int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
> >                             struct vfio_region_info *region)
> >  {
> >      DeviceClass *dc = DEVICE_GET_CLASS(vdev);
> > +    PCIBus *bus;
> >      PCIDevice *lpc_bridge;
> >      int ret;
> >  
> > @@ -1423,6 +1425,19 @@ int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
> >  
> >      dc->hotpluggable = false;
> >  
> > +    bus = pci_device_root_bus(&vdev->pdev);
> > +    lpc_bridge = pci_find_device(bus, 0, PCI_DEVFN(0x1f, 0));
> > +    if (lpc_bridge) {
> > +        const char *type = object_get_typename(OBJECT(lpc_bridge));
> > +        if (strcmp(type, TYPE_ICH9_LPC_DEVICE) == 0) {
> > +            /*
> > +             * q35 lpc present, leave it as-is.
> > +             * linux kernel 4.5+ can deal with this.
> 
> What about anything older?  I assume we want to support current
> distributions.  Based on this patch it seems like we also want to test
> first if the lpc device is present, create it if not, then probably
> copy host data into it.  Thanks,

q35 already has a isa bridge @ 1f.0, and it's not a dummy device but
emulates the ich9 lpc.  Just overwriting the identity of that device
isn't a good idea I think.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] vfio/igd: handle q35 machine type

[Alex Williamson]

On Tue, 08 Mar 2016 09:04:50 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mo, 2016-03-07 at 14:41 -0700, Alex Williamson wrote:
> > On Fri,  4 Mar 2016 09:41:53 +0100
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   
> > > Basically skip the lpc quirks with -M q35.
> > > Applies on top of the vfio-igd patch series by alex.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > ---
> > >  hw/vfio/pci-quirks.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> > > index 5828362..1757e3d 100644
> > > --- a/hw/vfio/pci-quirks.c
> > > +++ b/hw/vfio/pci-quirks.c
> > > @@ -12,6 +12,7 @@
> > >  
> > >  #include "qemu/osdep.h"
> > >  #include "hw/nvram/fw_cfg.h"
> > > +#include "hw/i386/ich9.h"
> > >  #include "pci.h"
> > >  #include "trace.h"
> > >  #include "qemu/range.h"
> > > @@ -1410,6 +1411,7 @@ int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
> > >                             struct vfio_region_info *region)
> > >  {
> > >      DeviceClass *dc = DEVICE_GET_CLASS(vdev);
> > > +    PCIBus *bus;
> > >      PCIDevice *lpc_bridge;
> > >      int ret;
> > >  
> > > @@ -1423,6 +1425,19 @@ int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
> > >  
> > >      dc->hotpluggable = false;
> > >  
> > > +    bus = pci_device_root_bus(&vdev->pdev);
> > > +    lpc_bridge = pci_find_device(bus, 0, PCI_DEVFN(0x1f, 0));
> > > +    if (lpc_bridge) {
> > > +        const char *type = object_get_typename(OBJECT(lpc_bridge));
> > > +        if (strcmp(type, TYPE_ICH9_LPC_DEVICE) == 0) {
> > > +            /*
> > > +             * q35 lpc present, leave it as-is.
> > > +             * linux kernel 4.5+ can deal with this.  
> > 
> > What about anything older?  I assume we want to support current
> > distributions.  Based on this patch it seems like we also want to test
> > first if the lpc device is present, create it if not, then probably
> > copy host data into it.  Thanks,  
> 
> q35 already has a isa bridge @ 1f.0, and it's not a dummy device but
> emulates the ich9 lpc.  Just overwriting the identity of that device
> isn't a good idea I think.

It's not a good idea in practice or in theory?  It's exactly what we're
proposing to do for the host bridge.  Do we know for certain that any of
the emulated ich9 registers do not align with newer chipsets?  It seems
quite limiting of igd assignment on q35 VMs if we're going to remove
collateral device modification.  Should we only claim "legacy" igd
support on 440FX and support only universal passthrough on q35?  Thanks,

Alex

Re: [Qemu-devel] [PATCH v2] vfio/igd: handle q35 machine type

[Gerd Hoffmann]

  Hi,

> > q35 already has a isa bridge @ 1f.0, and it's not a dummy device but
> > emulates the ich9 lpc.  Just overwriting the identity of that device
> > isn't a good idea I think.
> 
> It's not a good idea in practice or in theory?  It's exactly what we're
> proposing to do for the host bridge.

We don't change the pci ids for the host bridge.  It's still a
i440fx/q35 host bridge, only the subsystem ID is changed (although I'm
sometimes wondering why), and the gfx registers added of course.

> Do we know for certain that any of
> the emulated ich9 registers do not align with newer chipsets?

See drivers/mfd/lpc_ich.c (lpc_chipset_info array).

Lots of different versions exist for GPIO wiring.  Which probably isn't
that much of a problem as I think we don't emulate gpio.

For iTCO there are tree different versions too, and we *do* emulate
that.

> It seems
> quite limiting of igd assignment on q35 VMs if we're going to remove
> collateral device modification.  Should we only claim "legacy" igd
> support on 440FX and support only universal passthrough on q35?  Thanks,

I'll go test this a bit more.  Possibly we can drop the whole lpc
tweaking.  At least when looking at the linux driver code it doesn't
seem to be very important.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] vfio/igd: handle q35 machine type

[Gerd Hoffmann]

  Hi,

[ context for igvt list: how do we deal with the ich9 lpc emulated by
  q35 machine type best, when pci-assigning a igd? ]

> > It seems
> > quite limiting of igd assignment on q35 VMs if we're going to remove
> > collateral device modification.  Should we only claim "legacy" igd
> > support on 440FX and support only universal passthrough on q35?  Thanks,
> 
> I'll go test this a bit more.  Possibly we can drop the whole lpc
> tweaking.  At least when looking at the linux driver code it doesn't
> seem to be very important.

Ok, we can't, it's actually checked in quite a few places.  The checks
are hidden in a macro though which I initially didn't spot.  The bios is
unhappy too if the lpc @ 1f.0 goes away.

But just copying over the pci ids doesn't work either, it breaks
firmware q35 initialization.  Which in turn breaks acpi (partly), so
some things like poweroff stop working.  And I have some irq routing
errors in the kernel log too.

So, q35 works (without lpc tweaks) with 4.5-rc6+ guest kernel (should be
4.4.5+ soon) and bios rom disabled.  That is at least a start.

To get the bios and older kernels working we have to fiddle with the
lpc.  Copying from the host looks bad to me, we have to maintain tons of
pci ids in the firmware then.  At least the intel driver only needs to
know which pch generation is used, so we might be able to get away with
one pch per igd generation, and hopefully can stop doing that long term,
when intel untangles igd and pch in hardware.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] vfio/igd: handle q35 machine type

[Gerd Hoffmann]

  Hi,

> We don't change the pci ids for the host bridge.  It's still a
> i440fx/q35 host bridge, only the subsystem ID is changed (although I'm
> sometimes wondering why), and the gfx registers added of course.

Hmm, not changing the subsystem id (and only adding the gfx regs) seems
to have no bad side effects.  bios, old and new kernels are all still
happy.

cheers,
  Gerd

Re: [Qemu-devel] [iGVT-g] [PATCH v2] vfio/igd: handle q35 machine type

[Tian, Kevin]

> From: Gerd Hoffmann
> Sent: Wednesday, March 09, 2016 11:01 PM
> 
>   Hi,
> 
> [ context for igvt list: how do we deal with the ich9 lpc emulated by
>   q35 machine type best, when pci-assigning a igd? ]
> 
> > > It seems
> > > quite limiting of igd assignment on q35 VMs if we're going to remove
> > > collateral device modification.  Should we only claim "legacy" igd
> > > support on 440FX and support only universal passthrough on q35?  Thanks,
> >
> > I'll go test this a bit more.  Possibly we can drop the whole lpc
> > tweaking.  At least when looking at the linux driver code it doesn't
> > seem to be very important.
> 
> Ok, we can't, it's actually checked in quite a few places.  The checks
> are hidden in a macro though which I initially didn't spot.  The bios is
> unhappy too if the lpc @ 1f.0 goes away.
> 
> But just copying over the pci ids doesn't work either, it breaks
> firmware q35 initialization.  Which in turn breaks acpi (partly), so
> some things like poweroff stop working.  And I have some irq routing
> errors in the kernel log too.
> 
> So, q35 works (without lpc tweaks) with 4.5-rc6+ guest kernel (should be
> 4.4.5+ soon) and bios rom disabled.  That is at least a start.
> 
> To get the bios and older kernels working we have to fiddle with the
> lpc.  Copying from the host looks bad to me, we have to maintain tons of
> pci ids in the firmware then.  At least the intel driver only needs to
> know which pch generation is used, so we might be able to get away with
> one pch per igd generation, and hopefully can stop doing that long term,
> when intel untangles igd and pch in hardware.
> 

Yes, in the future we can expect sort of default PCH generation based on
PCIID, when no lpc is exposed.

Thanks
Kevin