On Tue, 2012-10-16 at 17:08 +0200, Michael S. Tsirkin wrote:
> On Tue, Oct 16, 2012 at 08:48:04AM -0600, Alex Williamson wrote:
> > On Tue, 2012-10-16 at 16:14 +0200, Michael S. Tsirkin wrote:
> > > On Tue, Oct 16, 2012 at 07:51:43AM -0600, Alex Williamson wrote:
> > > > On Tue, 2012-10-16 at 08:39 +0200, Michael S. Tsirkin wrote:
> > > > > On Mon, Oct 15, 2012 at 02:28:15PM -0600, Alex Williamson wrote:
> > > > > > This makes use of the new level irqfd support enabling bypass of
> > > > > > qemu userspace both on INTx injection and unmask. This significantly
> > > > > > boosts the performance of devices making use of legacy interrupts.
> > > > > >
> > > > > > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > > > > > ---
> > > > > >
> > > > > > My INTx routing workaround below will probably raise some eyebrows,
> > > > > > but I don't feel it's worth subjecting users to core dumps if they
> > > > > > want to try vfio-pci on new platforms. INTx routing is part of some
> > > > > > larger plan, but until that plan materializes we have to try to avoid
> > > > > > the API unless we think there's a good chance it might be there.
> > > > > > I'll accept the maintenance of updating a whitelist in the interim.
> > > > > > Thanks,
> > > > > >
> > > > > > Alex
> > > > > >
> > > > > > hw/vfio_pci.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > 1 file changed, 224 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/vfio_pci.c b/hw/vfio_pci.c
> > > > > > index 639371e..777a5f8 100644
> > > > > > --- a/hw/vfio_pci.c
> > > > > > +++ b/hw/vfio_pci.c
> > > > > > @@ -154,6 +154,53 @@ static uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> > > > > > static void vfio_mmap_set_enabled(VFIODevice *vdev, bool enabled);
> > > > > >
> > > > > > /*
> > > > > > + * PCI code refuses to make it possible to probe whether the chipset
> > > > > > + * supports pci_device_route_intx_to_irq() and booby traps the call
> > > > > > + * to assert if doesn't. For us, this is just an optimization, so
> > > > > > + * only enable it when we know it's present. Unfortunately PCIBus is
> > > > > > + * private, so we can't just look at the function pointer.
> > > > > > + */
> > > > > > +static bool vfio_pci_bus_has_intx_route(PCIDevice *pdev)
> > > > > > +{
> > > > > > +#ifdef CONFIG_KVM
> > > > > > + BusState *bus = qdev_get_parent_bus(&pdev->qdev);
> > > > > > + DeviceState *dev;
> > > > > > +
> > > > > > + if (!kvm_irqchip_in_kernel() ||
> > > > > > + !kvm_check_extension(kvm_state, KVM_CAP_IRQFD_RESAMPLE)) {
> > > > > > + return false;
> > > > > > + }
> > > > >
> > > > >
> > > > > Shouldn't we update linux-headers/ to get KVM_CAP_IRQFD_RESAMPLE?
> > > > > Also for KVM_IRQFD_FLAG_RESAMPLE.
> > > >
> > > > I posted the patch for that separately yesterday. I'll only request a
> > > > pull once that's in.
> > >
> > > OK missed that. In the future, might be a good idea to note dependencies
> > > in the patchset or repost them as part of patchset with appropriate
> > > tagging.
> > >
> > > > > > +
> > > > > > + for (; bus->parent; bus = qdev_get_parent_bus(dev)) {
> > > > > > +
> > > > > > + dev = bus->parent;
> > > > > > +
> > > > > > + if (!strncmp("i440FX-pcihost", object_get_typename(OBJECT(dev)), 14)) {
> > > > > > + return true;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + error_report("vfio-pci: VM chipset does not support INTx routing, "
> > > > > > + "using slow INTx mode\n");
> > > > >
> > > > > When does this code trigger? It seems irqchip implies piix ATM -
> > > > > is this just dead code?
> > > >
> > > > Unused, but not unnecessary. Another chipset is under development,
> > > > which means very quickly irqchip will not imply piix.
> > >
> > > So this is for purposes of an out of tree stuff, let's
> > > keep these hacks out of tree too. My guess is
> > > q35 can just be added with pci_device_route_intx straight away.
> > >
> > > > Likewise irqfd
> > > > support is being added to other architectures, so I don't know how long
> > > > the kvm specific tests will hold up. Testing for a specific chipset
> > > > could of course be avoided if we were willing to support:
> > > >
> > > > bool pci_device_intx_route_supported(PCIDevice *pdev)
> > > >
> > > > or the NOROUTE option I posted previously.
> > >
> > > This is just moving the pain to users who will
> > > get bad performance and will have to debug it. Injecting
> > > through userspace is slow, new architectures should
> > > simply add irqfd straight away instead of adding
> > > work arounds in userspace.
> > >
> > > This is exactly why we have the assert in pci core.
> >
> > Let's compare user experience:
> >
> > As coded here:
> >
> > - Error message, only runs slower if the driver actually uses INTx.
> > Result: file bug, continue using vfio-pci, maybe do something useful,
> > maybe find new bugs to file.
> >
> > Blindly calling PCI code w/ assert:
> >
> > - Assert. Result: file bug, full stop.
> >
> > Maybe I do too much kernel programming, but I don't take asserts lightly
> > and prefer they be saved for "something is really broken and I can't
> > safely continue". This is not such a case.
>
> There's no chance we ship e.g. q35 by mistake without this API: since
> there is no way this specific assert can be missed in even basic
> testing:
>
> So I see it differently:
>
> As coded here:
> chipset authors get lazy and do not implement API.
> bad performance for all users.
>
> With assert:
> chipset authors implement necessary API.
> good performance for all users.
I prefer a carrot, not a whip. Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
