| Message ID | 1457080913-30018-1-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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");
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
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
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
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
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
> 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
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");
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(+)