Patchwork virtio-pci: guest notifier mask without non-irqfd

login
register
mail settings
Submitter Michael S. Tsirkin
Date March 13, 2013, 7:46 p.m.
Message ID <20130313194643.GA9717@redhat.com>
Download mbox | patch
Permalink /patch/227340/
State New
Headers show

Comments

Michael S. Tsirkin - March 13, 2013, 7:46 p.m.
non-irqfd setups are currently broken with vhost:
we start up masked and nothing unmasks the interrupts.
Fix by using mask notifiers, same as the irqfd path.

Sharing irqchip/non irqchip code is always a good thing,
in this case it will help non irqchip benefit
from backend masking optimization.

Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Alexey, the following is a clean way to fix the
problem that you reported "Re: QEMU -netdev vhost=on + -device
virtio-net-pci  bug" (previous patch was a quick hack but
not I think a good fix).
Lightly tested on x86/kvm and still under test, could you please try it
out and report whether it works for you?

 hw/virtio-pci.c | 79 ++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 44 insertions(+), 35 deletions(-)
Alexey Kardashevskiy - March 14, 2013, 12:23 a.m.
Michael,

yes, that works fine on ppc64 with vhost=on. Thanks!


On 14/03/13 06:46, Michael S. Tsirkin wrote:
> non-irqfd setups are currently broken with vhost:
> we start up masked and nothing unmasks the interrupts.
> Fix by using mask notifiers, same as the irqfd path.
>
> Sharing irqchip/non irqchip code is always a good thing,
> in this case it will help non irqchip benefit
> from backend masking optimization.
>
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Alexey, the following is a clean way to fix the
> problem that you reported "Re: QEMU -netdev vhost=on + -device
> virtio-net-pci  bug" (previous patch was a quick hack but
> not I think a good fix).
> Lightly tested on x86/kvm and still under test, could you please try it
> out and report whether it works for you?
>
>   hw/virtio-pci.c | 79 ++++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index ba56ab2..4f8a9cf 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -609,20 +609,23 @@ static void kvm_virtio_pci_vector_release(VirtIOPCIProxy *proxy, int nvqs)
>       }
>   }
>
> -static int kvm_virtio_pci_vq_vector_unmask(VirtIOPCIProxy *proxy,
> -                                        unsigned int queue_no,
> -                                        unsigned int vector,
> -                                        MSIMessage msg)
> +static int virtio_pci_vq_vector_unmask(VirtIOPCIProxy *proxy,
> +                                       unsigned int queue_no,
> +                                       unsigned int vector,
> +                                       MSIMessage msg)
>   {
>       VirtQueue *vq = virtio_get_queue(proxy->vdev, queue_no);
>       EventNotifier *n = virtio_queue_get_guest_notifier(vq);
> -    VirtIOIRQFD *irqfd = &proxy->vector_irqfd[vector];
> +    VirtIOIRQFD *irqfd;
>       int ret = 0;
>
> -    if (irqfd->msg.data != msg.data || irqfd->msg.address != msg.address) {
> -        ret = kvm_irqchip_update_msi_route(kvm_state, irqfd->virq, msg);
> -        if (ret < 0) {
> -            return ret;
> +    if (proxy->vector_irqfd) {
> +        irqfd = &proxy->vector_irqfd[vector];
> +        if (irqfd->msg.data != msg.data || irqfd->msg.address != msg.address) {
> +            ret = kvm_irqchip_update_msi_route(kvm_state, irqfd->virq, msg);
> +            if (ret < 0) {
> +                return ret;
> +            }
>           }
>       }
>
> @@ -642,7 +645,7 @@ static int kvm_virtio_pci_vq_vector_unmask(VirtIOPCIProxy *proxy,
>       return ret;
>   }
>
> -static void kvm_virtio_pci_vq_vector_mask(VirtIOPCIProxy *proxy,
> +static void virtio_pci_vq_vector_mask(VirtIOPCIProxy *proxy,
>                                                unsigned int queue_no,
>                                                unsigned int vector)
>   {
> @@ -656,8 +659,8 @@ static void kvm_virtio_pci_vq_vector_mask(VirtIOPCIProxy *proxy,
>       }
>   }
>
> -static int kvm_virtio_pci_vector_unmask(PCIDevice *dev, unsigned vector,
> -                                     MSIMessage msg)
> +static int virtio_pci_vector_unmask(PCIDevice *dev, unsigned vector,
> +                                    MSIMessage msg)
>   {
>       VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
>       VirtIODevice *vdev = proxy->vdev;
> @@ -670,7 +673,7 @@ static int kvm_virtio_pci_vector_unmask(PCIDevice *dev, unsigned vector,
>           if (virtio_queue_vector(vdev, queue_no) != vector) {
>               continue;
>           }
> -        ret = kvm_virtio_pci_vq_vector_unmask(proxy, queue_no, vector, msg);
> +        ret = virtio_pci_vq_vector_unmask(proxy, queue_no, vector, msg);
>           if (ret < 0) {
>               goto undo;
>           }
> @@ -682,12 +685,12 @@ undo:
>           if (virtio_queue_vector(vdev, queue_no) != vector) {
>               continue;
>           }
> -        kvm_virtio_pci_vq_vector_mask(proxy, queue_no, vector);
> +        virtio_pci_vq_vector_mask(proxy, queue_no, vector);
>       }
>       return ret;
>   }
>
> -static void kvm_virtio_pci_vector_mask(PCIDevice *dev, unsigned vector)
> +static void virtio_pci_vector_mask(PCIDevice *dev, unsigned vector)
>   {
>       VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
>       VirtIODevice *vdev = proxy->vdev;
> @@ -700,13 +703,13 @@ static void kvm_virtio_pci_vector_mask(PCIDevice *dev, unsigned vector)
>           if (virtio_queue_vector(vdev, queue_no) != vector) {
>               continue;
>           }
> -        kvm_virtio_pci_vq_vector_mask(proxy, queue_no, vector);
> +        virtio_pci_vq_vector_mask(proxy, queue_no, vector);
>       }
>   }
>
> -static void kvm_virtio_pci_vector_poll(PCIDevice *dev,
> -                                       unsigned int vector_start,
> -                                       unsigned int vector_end)
> +static void virtio_pci_vector_poll(PCIDevice *dev,
> +                                   unsigned int vector_start,
> +                                   unsigned int vector_end)
>   {
>       VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
>       VirtIODevice *vdev = proxy->vdev;
> @@ -781,11 +784,13 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
>       proxy->nvqs_with_notifiers = nvqs;
>
>       /* Must unset vector notifier while guest notifier is still assigned */
> -    if (proxy->vector_irqfd && !assign) {
> +    if ((proxy->vector_irqfd || vdev->guest_notifier_mask) && !assign) {
>           msix_unset_vector_notifiers(&proxy->pci_dev);
> -        kvm_virtio_pci_vector_release(proxy, nvqs);
> -        g_free(proxy->vector_irqfd);
> -        proxy->vector_irqfd = NULL;
> +        if (proxy->vector_irqfd) {
> +            kvm_virtio_pci_vector_release(proxy, nvqs);
> +            g_free(proxy->vector_irqfd);
> +            proxy->vector_irqfd = NULL;
> +        }
>       }
>
>       for (n = 0; n < nvqs; n++) {
> @@ -801,18 +806,20 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
>       }
>
>       /* Must set vector notifier after guest notifier has been assigned */
> -    if (with_irqfd && assign) {
> -        proxy->vector_irqfd =
> -            g_malloc0(sizeof(*proxy->vector_irqfd) *
> -                      msix_nr_vectors_allocated(&proxy->pci_dev));
> -        r = kvm_virtio_pci_vector_use(proxy, nvqs);
> -        if (r < 0) {
> -            goto assign_error;
> +    if ((with_irqfd || vdev->guest_notifier_mask) && assign) {
> +        if (with_irqfd) {
> +            proxy->vector_irqfd =
> +                g_malloc0(sizeof(*proxy->vector_irqfd) *
> +                          msix_nr_vectors_allocated(&proxy->pci_dev));
> +            r = kvm_virtio_pci_vector_use(proxy, nvqs);
> +            if (r < 0) {
> +                goto assign_error;
> +            }
>           }
>           r = msix_set_vector_notifiers(&proxy->pci_dev,
> -                                      kvm_virtio_pci_vector_unmask,
> -                                      kvm_virtio_pci_vector_mask,
> -                                      kvm_virtio_pci_vector_poll);
> +                                      virtio_pci_vector_unmask,
> +                                      virtio_pci_vector_mask,
> +                                      virtio_pci_vector_poll);
>           if (r < 0) {
>               goto notifiers_error;
>           }
> @@ -821,8 +828,10 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
>       return 0;
>
>   notifiers_error:
> -    assert(assign);
> -    kvm_virtio_pci_vector_release(proxy, nvqs);
> +    if (with_irqfd) {
> +        assert(assign);
> +        kvm_virtio_pci_vector_release(proxy, nvqs);
> +    }
>
>   assign_error:
>       /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */
>

Patch

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index ba56ab2..4f8a9cf 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -609,20 +609,23 @@  static void kvm_virtio_pci_vector_release(VirtIOPCIProxy *proxy, int nvqs)
     }
 }
 
-static int kvm_virtio_pci_vq_vector_unmask(VirtIOPCIProxy *proxy,
-                                        unsigned int queue_no,
-                                        unsigned int vector,
-                                        MSIMessage msg)
+static int virtio_pci_vq_vector_unmask(VirtIOPCIProxy *proxy,
+                                       unsigned int queue_no,
+                                       unsigned int vector,
+                                       MSIMessage msg)
 {
     VirtQueue *vq = virtio_get_queue(proxy->vdev, queue_no);
     EventNotifier *n = virtio_queue_get_guest_notifier(vq);
-    VirtIOIRQFD *irqfd = &proxy->vector_irqfd[vector];
+    VirtIOIRQFD *irqfd;
     int ret = 0;
 
-    if (irqfd->msg.data != msg.data || irqfd->msg.address != msg.address) {
-        ret = kvm_irqchip_update_msi_route(kvm_state, irqfd->virq, msg);
-        if (ret < 0) {
-            return ret;
+    if (proxy->vector_irqfd) {
+        irqfd = &proxy->vector_irqfd[vector];
+        if (irqfd->msg.data != msg.data || irqfd->msg.address != msg.address) {
+            ret = kvm_irqchip_update_msi_route(kvm_state, irqfd->virq, msg);
+            if (ret < 0) {
+                return ret;
+            }
         }
     }
 
@@ -642,7 +645,7 @@  static int kvm_virtio_pci_vq_vector_unmask(VirtIOPCIProxy *proxy,
     return ret;
 }
 
-static void kvm_virtio_pci_vq_vector_mask(VirtIOPCIProxy *proxy,
+static void virtio_pci_vq_vector_mask(VirtIOPCIProxy *proxy,
                                              unsigned int queue_no,
                                              unsigned int vector)
 {
@@ -656,8 +659,8 @@  static void kvm_virtio_pci_vq_vector_mask(VirtIOPCIProxy *proxy,
     }
 }
 
-static int kvm_virtio_pci_vector_unmask(PCIDevice *dev, unsigned vector,
-                                     MSIMessage msg)
+static int virtio_pci_vector_unmask(PCIDevice *dev, unsigned vector,
+                                    MSIMessage msg)
 {
     VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
     VirtIODevice *vdev = proxy->vdev;
@@ -670,7 +673,7 @@  static int kvm_virtio_pci_vector_unmask(PCIDevice *dev, unsigned vector,
         if (virtio_queue_vector(vdev, queue_no) != vector) {
             continue;
         }
-        ret = kvm_virtio_pci_vq_vector_unmask(proxy, queue_no, vector, msg);
+        ret = virtio_pci_vq_vector_unmask(proxy, queue_no, vector, msg);
         if (ret < 0) {
             goto undo;
         }
@@ -682,12 +685,12 @@  undo:
         if (virtio_queue_vector(vdev, queue_no) != vector) {
             continue;
         }
-        kvm_virtio_pci_vq_vector_mask(proxy, queue_no, vector);
+        virtio_pci_vq_vector_mask(proxy, queue_no, vector);
     }
     return ret;
 }
 
-static void kvm_virtio_pci_vector_mask(PCIDevice *dev, unsigned vector)
+static void virtio_pci_vector_mask(PCIDevice *dev, unsigned vector)
 {
     VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
     VirtIODevice *vdev = proxy->vdev;
@@ -700,13 +703,13 @@  static void kvm_virtio_pci_vector_mask(PCIDevice *dev, unsigned vector)
         if (virtio_queue_vector(vdev, queue_no) != vector) {
             continue;
         }
-        kvm_virtio_pci_vq_vector_mask(proxy, queue_no, vector);
+        virtio_pci_vq_vector_mask(proxy, queue_no, vector);
     }
 }
 
-static void kvm_virtio_pci_vector_poll(PCIDevice *dev,
-                                       unsigned int vector_start,
-                                       unsigned int vector_end)
+static void virtio_pci_vector_poll(PCIDevice *dev,
+                                   unsigned int vector_start,
+                                   unsigned int vector_end)
 {
     VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
     VirtIODevice *vdev = proxy->vdev;
@@ -781,11 +784,13 @@  static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
     proxy->nvqs_with_notifiers = nvqs;
 
     /* Must unset vector notifier while guest notifier is still assigned */
-    if (proxy->vector_irqfd && !assign) {
+    if ((proxy->vector_irqfd || vdev->guest_notifier_mask) && !assign) {
         msix_unset_vector_notifiers(&proxy->pci_dev);
-        kvm_virtio_pci_vector_release(proxy, nvqs);
-        g_free(proxy->vector_irqfd);
-        proxy->vector_irqfd = NULL;
+        if (proxy->vector_irqfd) {
+            kvm_virtio_pci_vector_release(proxy, nvqs);
+            g_free(proxy->vector_irqfd);
+            proxy->vector_irqfd = NULL;
+        }
     }
 
     for (n = 0; n < nvqs; n++) {
@@ -801,18 +806,20 @@  static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
     }
 
     /* Must set vector notifier after guest notifier has been assigned */
-    if (with_irqfd && assign) {
-        proxy->vector_irqfd =
-            g_malloc0(sizeof(*proxy->vector_irqfd) *
-                      msix_nr_vectors_allocated(&proxy->pci_dev));
-        r = kvm_virtio_pci_vector_use(proxy, nvqs);
-        if (r < 0) {
-            goto assign_error;
+    if ((with_irqfd || vdev->guest_notifier_mask) && assign) {
+        if (with_irqfd) {
+            proxy->vector_irqfd =
+                g_malloc0(sizeof(*proxy->vector_irqfd) *
+                          msix_nr_vectors_allocated(&proxy->pci_dev));
+            r = kvm_virtio_pci_vector_use(proxy, nvqs);
+            if (r < 0) {
+                goto assign_error;
+            }
         }
         r = msix_set_vector_notifiers(&proxy->pci_dev,
-                                      kvm_virtio_pci_vector_unmask,
-                                      kvm_virtio_pci_vector_mask,
-                                      kvm_virtio_pci_vector_poll);
+                                      virtio_pci_vector_unmask,
+                                      virtio_pci_vector_mask,
+                                      virtio_pci_vector_poll);
         if (r < 0) {
             goto notifiers_error;
         }
@@ -821,8 +828,10 @@  static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
     return 0;
 
 notifiers_error:
-    assert(assign);
-    kvm_virtio_pci_vector_release(proxy, nvqs);
+    if (with_irqfd) {
+        assert(assign);
+        kvm_virtio_pci_vector_release(proxy, nvqs);
+    }
 
 assign_error:
     /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */