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. 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
