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

[Alex Williamson]

On Tue, 2012-10-09 at 00:44 +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 08, 2012 at 04:09:48PM -0600, Alex Williamson wrote:
> > On Mon, 2012-10-08 at 23:50 +0200, Michael S. Tsirkin wrote:
> > > On Mon, Oct 08, 2012 at 03:11:37PM -0600, Alex Williamson wrote:
> > > > 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.
> > > 
> > > This API has nothing to do with acceleration that I can see.
> > 
> > Ok, let's build on that...
> > 
> > > > > 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.
> > > 
> > > Everyone should just implement same APIs.  This was exactly why I asked
> > > Jan to change routing so everyone does the same thing, instead of this
> > > bolted-on hack just for assignment.
> > 
> > Do you have any idea how to implement intx routing on a g3beige
> > platform?  Me neither, but vfio-pci works just fine on it if I don't
> > call pci_device_route_intx_to_irq on that platform.  I could test kvm
> > support before calling it, but we both agree that intx routing isn't
> > tied to kvm support, same for kvm irq chip support.  Now I'm in a pickle
> > of what can I test for to see if intx routing is in place that won't
> > break some day if we run g3beige on a ppc host w/ kvm acceleration, and
> > heaven forbid that it should ever implement something we call an
> > irqchip.
> > 
> > > I merged this anyway so we can stop maintaining two trees which is even
> > > more painful.
> > > 
> > > > 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?
> > > 
> > > 
> > > One has to draw a line in the sand somewhere, and I think it's a good
> > > idea to stop now before this metastazes all over the code.
> > 
> > I think the API is broken w/o this, so...
> > 
> > > > Does it somehow prevent a driver from being just as lazy and doing
> > > > assert(intx.mode == PCI_INTX_NOROUTE)?
> > > 
> > > What does this return code mean? How can there be no route?
> > 
> > The function returns PCIINTxRoute, so I can't very well return -1 or
> > -ENODEV if we don't know how to generate the route.  Obviously the route
> > exists somewhere in the depths of qemu_irq, but we don't know how to get
> > it.  Therefore, we return No Route.
> 
> In other words it means there's a chipset bug. Why would
> a device want to assert on this? Let chipset assert where
> there's a chance to debug it.

Is it a chipset bug to support a feature that has no value on that
platform?  Is it useful to assert on a known unimplemented feature?  I
say no to both.

> >  It's far more obvious than trying
> > to figure out what INVERTED means.
> > 
> > > > 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
> > > 
> > > Why can't all chipsets implement this? You are just working
> > > around broken chipsets in your device, this is wrong.
> > 
> > Like I asked, do you know how to make intx route work on g3beige or
> > define a test that doesn't pull in unrelated features to avoid calling
> > it there?  Implementing intx routing on a chipset that has no hopes of
> > qemu bypass is wasted effort,
> > therefore we need an API that allows us to
> > test it.  Thanks,
> > 
> > Alex
> > 
> 
> And this is why intx API is a wrong thing to keep long term.
> We end up with code like
>       if (fast thing works) {
>               do fast thing
>       } else
>               do slow thing
> in many devices which is not where I want the complexity
> to be, I want to keep it in the core.
>
> I plan to rework this, to make all devices do something like what Avi's
> memory API does, flattening the hierarchy at registration time, and
> prefer not to add more dead code that will need to be removed.

While I agree qemu_irq needs a re-write, I have a hard time seeing
anything other than device assignment that needs this particular
feature, so I don't think there are "many devices" that will be doing
the above.  Even if there were, it's pretty common to test features and
enable fast paths based on availability.  So long as there are easy ways
to test both paths, which there are with vfio-pci, I don't see a problem
with that.

> Is there any rush to use vfio on g3beige?  If yes I could merge
> something like this patch as a temporary measure but I prefer not to.

Yes, I already use it as a non-x86 test platform and I'd like to not
break it or create tests on non-related features to avoid intx routing
code.  I'd also like to enable vfio-pci on any other platforms that
support PCI and don't want to deal with this on every chipset.  Thanks,

Alex