Patchwork [PULL,4/5] vfio-pci: Add debug config options to disable MSI/X KVM support

login
register
mail settings
Submitter Alex Williamson
Date Dec. 6, 2013, 8:48 p.m.
Message ID <20131206204842.16731.29998.stgit@bling.home>
Download mbox | patch
Permalink /patch/298260/
State New
Headers show

Comments

Alex Williamson - Dec. 6, 2013, 8:48 p.m.
It's sometimes useful to be able to verify interrupts are passing
through correctly.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/misc/vfio.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)
Paolo Bonzini - Dec. 6, 2013, 10:06 p.m.
Il 06/12/2013 21:48, Alex Williamson ha scritto:
>  /* Extra debugging, trap acceleration paths for more logging */
>  #define VFIO_ALLOW_MMAP 1
>  #define VFIO_ALLOW_KVM_INTX 1
> +#define VFIO_ALLOW_KVM_MSI 1
> +#define VFIO_ALLOW_KVM_MSIX 1

Why not make these device properties instead?

Paolo
Alex Williamson - Dec. 6, 2013, 10:36 p.m.
On Fri, 2013-12-06 at 23:06 +0100, Paolo Bonzini wrote:
> Il 06/12/2013 21:48, Alex Williamson ha scritto:
> >  /* Extra debugging, trap acceleration paths for more logging */
> >  #define VFIO_ALLOW_MMAP 1
> >  #define VFIO_ALLOW_KVM_INTX 1
> > +#define VFIO_ALLOW_KVM_MSI 1
> > +#define VFIO_ALLOW_KVM_MSIX 1
> 
> Why not make these device properties instead?

Honestly, I don't think they're that useful to the average or even to
the advanced user.  Each of these disables an acceleration/bypass path
and gives more logging output when debug is enabled.  Otherwise we don't
get debugging to see when mmap'd BARs are accessed and we can't tell
when interrupts happen relative to other accesses.  For instance it was
pretty obvious to find the Nvidia MSI ACK when seeing a config space
write immediately following an MSI injection.  Without debugging
enabled, these just slow things down and I don't know of any cases where
a device behaves better with these features disabled.  Thanks,

Alex
Paolo Bonzini - Dec. 9, 2013, 9:48 a.m.
Il 06/12/2013 23:36, Alex Williamson ha scritto:
>>> > >  #define VFIO_ALLOW_MMAP 1
>>> > >  #define VFIO_ALLOW_KVM_INTX 1
>>> > > +#define VFIO_ALLOW_KVM_MSI 1
>>> > > +#define VFIO_ALLOW_KVM_MSIX 1
>> > 
>> > Why not make these device properties instead?
> Honestly, I don't think they're that useful to the average or even to
> the advanced user.  Each of these disables an acceleration/bypass path
> and gives more logging output when debug is enabled.  Otherwise we don't
> get debugging to see when mmap'd BARs are accessed and we can't tell
> when interrupts happen relative to other accesses.  For instance it was
> pretty obvious to find the Nvidia MSI ACK when seeing a config space
> write immediately following an MSI injection.  Without debugging
> enabled, these just slow things down and I don't know of any cases where
> a device behaves better with these features disabled.

Yeah, behaving better is unlikely.  I was thinking of them as a
debugging features.

As to debug messages, have you considered replacing them with tracepoints?

Paolo

Patch

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 09bdddf..f367537 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -52,6 +52,8 @@ 
 /* Extra debugging, trap acceleration paths for more logging */
 #define VFIO_ALLOW_MMAP 1
 #define VFIO_ALLOW_KVM_INTX 1
+#define VFIO_ALLOW_KVM_MSI 1
+#define VFIO_ALLOW_KVM_MSIX 1
 
 struct VFIODevice;
 
@@ -590,9 +592,21 @@  static void vfio_msi_interrupt(void *opaque)
         return;
     }
 
-    DPRINTF("%s(%04x:%02x:%02x.%x) vector %d\n", __func__,
+#ifdef VFIO_DEBUG
+    MSIMessage msg;
+
+    if (vdev->interrupt == VFIO_INT_MSIX) {
+        msg = msi_get_message(&vdev->pdev, nr);
+    } else if (vdev->interrupt == VFIO_INT_MSI) {
+        msg = msix_get_message(&vdev->pdev, nr);
+    } else {
+        abort();
+    }
+
+    DPRINTF("%s(%04x:%02x:%02x.%x) vector %d 0x%"PRIx64"/0x%x\n", __func__,
             vdev->host.domain, vdev->host.bus, vdev->host.slot,
-            vdev->host.function, nr);
+            vdev->host.function, nr, msg.address, msg.data);
+#endif
 
     if (vdev->interrupt == VFIO_INT_MSIX) {
         msix_notify(&vdev->pdev, nr);
@@ -660,7 +674,8 @@  static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
      * Attempt to enable route through KVM irqchip,
      * default to userspace handling if unavailable.
      */
-    vector->virq = msg ? kvm_irqchip_add_msi_route(kvm_state, *msg) : -1;
+    vector->virq = msg && VFIO_ALLOW_KVM_MSIX ?
+                   kvm_irqchip_add_msi_route(kvm_state, *msg) : -1;
     if (vector->virq < 0 ||
         kvm_irqchip_add_irqfd_notifier(kvm_state, &vector->interrupt,
                                        NULL, vector->virq) < 0) {
@@ -827,7 +842,8 @@  retry:
          * Attempt to enable route through KVM irqchip,
          * default to userspace handling if unavailable.
          */
-        vector->virq = kvm_irqchip_add_msi_route(kvm_state, vector->msg);
+        vector->virq = VFIO_ALLOW_KVM_MSI ?
+                       kvm_irqchip_add_msi_route(kvm_state, vector->msg) : -1;
         if (vector->virq < 0 ||
             kvm_irqchip_add_irqfd_notifier(kvm_state, &vector->interrupt,
                                            NULL, vector->virq) < 0) {