From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57500) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLLBU-0002Jl-Kg for qemu-devel@nongnu.org; Mon, 08 Oct 2012 17:48:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TLLBT-0004Ju-9X for qemu-devel@nongnu.org; Mon, 08 Oct 2012 17:48:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8984) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TLLBT-0004Jq-11 for qemu-devel@nongnu.org; Mon, 08 Oct 2012 17:48:19 -0400 Date: Mon, 8 Oct 2012 23:50:08 +0200 From: "Michael S. Tsirkin" Message-ID: <20121008215008.GA19208@redhat.com> References: <20121002191609.31100.77382.stgit@bling.home> <1349711912.2759.96.camel@ul30vt.home> <20121008201539.GC17303@redhat.com> <1349724453.2759.163.camel@ul30vt.home> <20121008214052.GC17820@redhat.com> <1349730697.2759.211.camel@ul30vt.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1349730697.2759.211.camel@ul30vt.home> Subject: Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: jan.kiszka@siemens.com, qemu-devel@nongnu.org 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. > > 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. > > > 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. 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. > 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 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. > > > > > 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(-) > > > > > > > > > > > > > > > > > >