[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups |
Date: |
Mon, 8 Oct 2012 23:50:08 +0200 |
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. </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.
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(-)
> > > > >
> > > > >
> > >
> > >
>
>
- [Qemu-devel] [PATCH 3/6] pci: Helper function for testing if an INTx route changed, (continued)
- [Qemu-devel] [PATCH 3/6] pci: Helper function for testing if an INTx route changed, Alex Williamson, 2012/10/02
- [Qemu-devel] [PATCH 4/6] pci-assign: Use pci_intx_route_changed(), Alex Williamson, 2012/10/02
- [Qemu-devel] [PATCH 5/6] msi: Add msi_get_message(), Alex Williamson, 2012/10/02
- [Qemu-devel] [PATCH 6/6] pci-assign: Use msi_get_message(), Alex Williamson, 2012/10/02
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Alex Williamson, 2012/10/08
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Jan Kiszka, 2012/10/08
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Michael S. Tsirkin, 2012/10/08
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Alex Williamson, 2012/10/08
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Michael S. Tsirkin, 2012/10/08
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Alex Williamson, 2012/10/08
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Alex Williamson, 2012/10/08
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Michael S. Tsirkin, 2012/10/08
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Alex Williamson, 2012/10/08
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Michael S. Tsirkin, 2012/10/11
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Jan Kiszka, 2012/10/09
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Alex Williamson, 2012/10/10
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Michael S. Tsirkin, 2012/10/11
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Alex Williamson, 2012/10/11
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Michael S. Tsirkin, 2012/10/11
- Re: [Qemu-devel] [PATCH 0/6] Misc PCI cleanups, Alex Williamson, 2012/10/11