Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups

[Alex Williamson]

On Mon, 2012-10-08 at 23:40 +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 08, 2012 at 01:27:33PM -0600, Alex Williamson wrote:
> > On Mon, 2012-10-08 at 22:15 +0200, Michael S. Tsirkin wrote:
> > > On Mon, Oct 08, 2012 at 09:58:32AM -0600, Alex Williamson wrote:
> > > > Michael, Jan,
> > > > 
> > > > Any comments on these?  I'd like to make the PCI changes before I update
> > > > vfio-pci to make use of the new resampling irqfd in kvm.  We don't have
> > > > anyone officially listed as maintainer of pci-assign since it's been
> > > > moved to qemu.  I could include the pci-assign patches in my tree if you
> > > > prefer.  Thanks,
> > > > 
> > > > Alex
> > > 
> > > Patches themselves look fine, but I'd like to
> > > better understand why do we want the INTx fallback.
> > > Isn't it easier to add intx routing support?
> > 
> > vfio-pci can work with or without intx routing support.  Its presence is
> > just one requirement to enable kvm accelerated intx support.  Regardless
> > of whether it's easy or hard to implement intx routing in a given
> > chipset, I currently can't probe for it and make useful decisions about
> > whether or not to enable kvm support without potentially hitting an
> > assert.  It's arguable how important intx acceleration is for specific
> > applications, so while I'd like all chipsets to implement it, I don't
> > know that it should be a gating factor to chipset integration.  Thanks,
> > 
> > Alex
> 
> Yes but there's nothing kvm specific in the routing API,
> and IIRC it actually works fine without kvm.

Correct, but intx routing isn't very useful without kvm.

> As I see it, if some chipset does not expose it, it's a bug, and the
> reason for lack of support is because no one cares about supporting
> device assignment there.

Should we not have a more robust response to bugs than to kill the VM?
Especially when it's as trivial as using the non-accelerated intx mode
(vfio-pci) or having device init fail (pci-assign).  Calling assert is
lazy.

> So this API is not something devices should probe for.
> How about just assuming it works?

If that's the case, why test for any capabilities?  Let's just assume
everything is there and litter the code with asserts rather than
robustly deal with errors. </sarcasm>

> Otherwise, you are adding code and API that will become dead code
> when everyone supports the required API.

So q35 is going to be the last ever chipset?  It may be the next one to
implement intx routing, but hopefully not the last.

We're talking about this:

 hw/pci.c |    8 ++++++--
 hw/pci.h |    1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

Is this somehow tipping us over the edge and creating an unmaintainable
API?  Does it somehow prevent a driver from being just as lazy and doing
assert(intx.mode == PCI_INTX_NOROUTE)?  The API is obviously not
complete as is if I have to assume it's there, call into it and hope for
the best.  Thanks,

Alex

> > > > On Tue, 2012-10-02 at 13:21 -0600, Alex Williamson wrote:
> > > > > A few cleanups that I'll also apply to vfio-pci.  First make intx
> > > > > route checking non-fatal.  vfio-pci has a fallback INTx mechanism
> > > > > that doesn't rely on this, so we can already run on q35, but not if
> > > > > we can't even probe for intx routing w/o blowing up.  Next, both
> > > > > vfio-pci and pci-assign test whether INTx routing has changed using
> > > > > similar functions.  Make this common.  Finally, expose a way to
> > > > > get the MSI message for an MSI vector.  Again, both pci-assign and
> > > > > vfio-pci need to do this to program the vector for KVM injection.
> > > > > Thanks,
> > > > > 
> > > > > Alex
> > > > > 
> > > > > ---
> > > > > 
> > > > > Alex Williamson (6):
> > > > >       pci-assign: Use msi_get_message()
> > > > >       msi: Add msi_get_message()
> > > > >       pci-assign: Use pci_intx_route_changed()
> > > > >       pci: Helper function for testing if an INTx route changed
> > > > >       pci-assign: Add support for no-route
> > > > >       pci: Add INTx no-route option
> > > > > 
> > > > > 
> > > > >  hw/kvm/pci-assign.c |   14 ++++++++------
> > > > >  hw/msi.c            |   45 
> > > > > +++++++++++++++++++++++++++++----------------
> > > > >  hw/msi.h            |    1 +
> > > > >  hw/pci.c            |   13 +++++++++++--
> > > > >  hw/pci.h            |    2 ++
> > > > >  5 files changed, 51 insertions(+), 24 deletions(-)
> > > > 
> > > > 
> > 
> >