[Qemu-devel] [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Qemu-devel] [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Michael S. Tsirkin]

vhost irqfd support: case where many vqs are
mapped to a single msix vector is currently broken.
Fix it up.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

This is on top of the qemu patchset, which is unchanged.
Fixes from v1:
	correct error handling 

 hw/msix.c       |   68 ++++++++++++++++++++++++++++++++++++++-----------------
 hw/msix.h       |    4 +-
 hw/pci.h        |    3 +-
 hw/virtio-pci.c |   56 ++++++++++++++++++++++++++++++++++++++-------
 4 files changed, 97 insertions(+), 34 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 3dd0456..3d4dd61 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -300,10 +300,8 @@ static void msix_mmio_writel(void *opaque, target_phys_addr_t addr,
     if (kvm_enabled() && kvm_irqchip_in_kernel()) {
         kvm_msix_update(dev, vector, was_masked, msix_is_masked(dev, vector));
     }
-    if (was_masked != msix_is_masked(dev, vector) &&
-        dev->msix_mask_notifier && dev->msix_mask_notifier_opaque[vector]) {
+    if (was_masked != msix_is_masked(dev, vector) && dev->msix_mask_notifier) {
         int r = dev->msix_mask_notifier(dev, vector,
-					dev->msix_mask_notifier_opaque[vector],
 					msix_is_masked(dev, vector));
         assert(r >= 0);
     }
@@ -351,9 +349,8 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
         int was_masked = msix_is_masked(dev, vector);
         dev->msix_table_page[offset] |= MSIX_VECTOR_MASK;
         if (was_masked != msix_is_masked(dev, vector) &&
-            dev->msix_mask_notifier && dev->msix_mask_notifier_opaque[vector]) {
+            dev->msix_mask_notifier) {
             r = dev->msix_mask_notifier(dev, vector,
-                                        dev->msix_mask_notifier_opaque[vector],
                                         msix_is_masked(dev, vector));
             assert(r >= 0);
         }
@@ -379,8 +376,6 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
                                             sizeof *dev->msix_irq_entries);
     }
 #endif
-    dev->msix_mask_notifier_opaque =
-        qemu_mallocz(nentries * sizeof *dev->msix_mask_notifier_opaque);
     dev->msix_mask_notifier = NULL;
     dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES *
                                         sizeof *dev->msix_entry_used);
@@ -444,8 +439,6 @@ int msix_uninit(PCIDevice *dev)
     dev->msix_entry_used = NULL;
     qemu_free(dev->msix_irq_entries);
     dev->msix_irq_entries = NULL;
-    qemu_free(dev->msix_mask_notifier_opaque);
-    dev->msix_mask_notifier_opaque = NULL;
     dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
     return 0;
 }
@@ -590,46 +583,79 @@ void msix_unuse_all_vectors(PCIDevice *dev)
     msix_free_irq_entries(dev);
 }
 
-int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque)
+static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
 {
     int r = 0;
     if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
         return 0;
 
     assert(dev->msix_mask_notifier);
-    assert(opaque);
-    assert(!dev->msix_mask_notifier_opaque[vector]);
 
     /* Unmask the new notifier unless vector is masked. */
     if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, opaque, false);
+        r = dev->msix_mask_notifier(dev, vector, false);
         if (r < 0) {
             return r;
         }
     }
-    dev->msix_mask_notifier_opaque[vector] = opaque;
     return r;
 }
 
-int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector)
+static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
 {
     int r = 0;
-    void *opaque;
     if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
         return 0;
 
-    opaque = dev->msix_mask_notifier_opaque[vector];
-
     assert(dev->msix_mask_notifier);
-    assert(opaque);
 
     /* Mask the old notifier unless it is already masked. */
     if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, opaque, true);
+        r = dev->msix_mask_notifier(dev, vector, true);
         if (r < 0) {
             return r;
         }
     }
-    dev->msix_mask_notifier_opaque[vector] = NULL;
+    return r;
+}
+
+int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
+{
+    int r, n;
+    assert(!dev->msix_mask_notifier);
+    dev->msix_mask_notifier = f;
+    for (n = 0; n < dev->msix_entries_nr; ++n) {
+        r = msix_set_mask_notifier_for_vector(dev, n);
+        if (r < 0) {
+            goto undo;
+        }
+    }
+    return 0;
+
+undo:
+    while (--n >= 0) {
+        msix_unset_mask_notifier_for_vector(dev, n);
+    }
+    dev->msix_mask_notifier = NULL;
+    return r;
+}
+
+int msix_unset_mask_notifier(PCIDevice *dev)
+{
+    int r, n;
+    assert(dev->msix_mask_notifier);
+    for (n = 0; n < dev->msix_entries_nr; ++n) {
+        r = msix_unset_mask_notifier_for_vector(dev, n);
+        if (r < 0) {
+            goto undo;
+        }
+    }
+    dev->msix_mask_notifier = NULL;
+    return 0;
+
+undo:
+    while (--n >= 0) {
+        msix_set_mask_notifier_for_vector(dev, n);
+    }
     return r;
 }
diff --git a/hw/msix.h b/hw/msix.h
index 6b21ffb..5a81df5 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -33,6 +33,6 @@ void msix_reset(PCIDevice *dev);
 
 extern int msix_supported;
 
-int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque);
-int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector);
+int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func);
+int msix_unset_mask_notifier(PCIDevice *dev);
 #endif
diff --git a/hw/pci.h b/hw/pci.h
index ccb99d0..a40dc14 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -131,7 +131,7 @@ enum {
 #define PCI_CAPABILITY_CONFIG_MSIX_LENGTH 0x10
 
 typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
-				       void *opaque, int masked);
+				       int masked);
 
 struct PCIDevice {
     DeviceState qdev;
@@ -198,7 +198,6 @@ struct PCIDevice {
 
     struct kvm_irq_routing_entry *msix_irq_entries;
 
-    void **msix_mask_notifier_opaque;
     msix_mask_notifier_func msix_mask_notifier;
 
     /* Device capability configuration space */
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 232f943..c1204ce 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -428,11 +428,10 @@ static void virtio_pci_guest_notifier_read(void *opaque)
     }
 }
 
-static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
-                                    void *opaque, int masked)
+static int virtio_pci_mask_vq(PCIDevice *dev, unsigned vector,
+                              VirtQueue *vq, int masked)
 {
 #ifdef CONFIG_KVM
-    VirtQueue *vq = opaque;
     EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
     int r = kvm_set_irqfd(dev->msix_irq_entries[vector].gsi,
                           event_notifier_get_fd(notifier),
@@ -453,6 +452,34 @@ static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
 #endif
 }
 
+static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
+                                    int masked)
+{
+    VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
+    VirtIODevice *vdev = proxy->vdev;
+    int r, n;
+
+    for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
+        if (!virtio_queue_get_num(vdev, n)) {
+            break;
+        }
+        if (virtio_queue_vector(vdev, n) != vector) {
+            continue;
+        }
+        r = virtio_pci_mask_vq(dev, vector, virtio_get_queue(vdev, n), masked);
+        if (r < 0) {
+            goto undo;
+        }
+    }
+    return 0;
+undo:
+    while (--n >= 0) {
+        virtio_pci_mask_vq(dev, vector, virtio_get_queue(vdev, n), !masked);
+    }
+    return r;
+}
+
+
 static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
 {
     VirtIOPCIProxy *proxy = opaque;
@@ -466,11 +493,7 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
         }
         qemu_set_fd_handler(event_notifier_get_fd(notifier),
                             virtio_pci_guest_notifier_read, NULL, vq);
-        msix_set_mask_notifier(&proxy->pci_dev,
-                               virtio_queue_vector(proxy->vdev, n), vq);
     } else {
-        msix_unset_mask_notifier(&proxy->pci_dev,
-				 virtio_queue_vector(proxy->vdev, n));
         qemu_set_fd_handler(event_notifier_get_fd(notifier),
                             NULL, NULL, NULL);
         /* Test and clear notifier before closing it,
@@ -488,6 +511,13 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
     VirtIODevice *vdev = proxy->vdev;
     int r, n;
 
+    /* Must unset mask notifier while guest notifier
+     * is still assigned */
+    if (!assign) {
+	    r = msix_unset_mask_notifier(&proxy->pci_dev);
+            assert(r >= 0);
+    }
+
     for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
         if (!virtio_queue_get_num(vdev, n)) {
             break;
@@ -499,6 +529,16 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
         }
     }
 
+    /* Must set mask notifier after guest notifier
+     * has been assigned */
+    if (assign) {
+        r = msix_set_mask_notifier(&proxy->pci_dev,
+                                   virtio_pci_mask_notifier);
+        if (r < 0) {
+            goto assign_error;
+        }
+    }
+
     return 0;
 
 assign_error:
@@ -584,8 +624,6 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
 
     proxy->pci_dev.config_write = virtio_write_config;
 
-    proxy->pci_dev.msix_mask_notifier = virtio_pci_mask_notifier;
-
     size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len;
     if (size & (size-1))
         size = 1 << qemu_fls(size);
-- 
1.7.3-rc1

[Qemu-devel] Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Alex Williamson]

On Wed, 2010-10-06 at 16:56 +0200, Michael S. Tsirkin wrote:
> vhost irqfd support: case where many vqs are
> mapped to a single msix vector is currently broken.
> Fix it up.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> This is on top of the qemu patchset, which is unchanged.
> Fixes from v1:
> 	correct error handling 
> 
>  hw/msix.c       |   68 ++++++++++++++++++++++++++++++++++++++-----------------
>  hw/msix.h       |    4 +-
>  hw/pci.h        |    3 +-
>  hw/virtio-pci.c |   56 ++++++++++++++++++++++++++++++++++++++-------
>  4 files changed, 97 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 3dd0456..3d4dd61 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -300,10 +300,8 @@ static void msix_mmio_writel(void *opaque, target_phys_addr_t addr,
>      if (kvm_enabled() && kvm_irqchip_in_kernel()) {
>          kvm_msix_update(dev, vector, was_masked, msix_is_masked(dev, vector));
>      }
> -    if (was_masked != msix_is_masked(dev, vector) &&
> -        dev->msix_mask_notifier && dev->msix_mask_notifier_opaque[vector]) {
> +    if (was_masked != msix_is_masked(dev, vector) && dev->msix_mask_notifier) {
>          int r = dev->msix_mask_notifier(dev, vector,
> -					dev->msix_mask_notifier_opaque[vector],
>  					msix_is_masked(dev, vector));
>          assert(r >= 0);
>      }
> @@ -351,9 +349,8 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>          int was_masked = msix_is_masked(dev, vector);
>          dev->msix_table_page[offset] |= MSIX_VECTOR_MASK;
>          if (was_masked != msix_is_masked(dev, vector) &&
> -            dev->msix_mask_notifier && dev->msix_mask_notifier_opaque[vector]) {
> +            dev->msix_mask_notifier) {
>              r = dev->msix_mask_notifier(dev, vector,
> -                                        dev->msix_mask_notifier_opaque[vector],
>                                          msix_is_masked(dev, vector));
>              assert(r >= 0);
>          }
> @@ -379,8 +376,6 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>                                              sizeof *dev->msix_irq_entries);
>      }
>  #endif
> -    dev->msix_mask_notifier_opaque =
> -        qemu_mallocz(nentries * sizeof *dev->msix_mask_notifier_opaque);
>      dev->msix_mask_notifier = NULL;
>      dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES *
>                                          sizeof *dev->msix_entry_used);
> @@ -444,8 +439,6 @@ int msix_uninit(PCIDevice *dev)
>      dev->msix_entry_used = NULL;
>      qemu_free(dev->msix_irq_entries);
>      dev->msix_irq_entries = NULL;
> -    qemu_free(dev->msix_mask_notifier_opaque);
> -    dev->msix_mask_notifier_opaque = NULL;
>      dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>      return 0;
>  }
> @@ -590,46 +583,79 @@ void msix_unuse_all_vectors(PCIDevice *dev)
>      msix_free_irq_entries(dev);
>  }
>  
> -int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque)
> +static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
>  {
>      int r = 0;
>      if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
>          return 0;
>  
>      assert(dev->msix_mask_notifier);
> -    assert(opaque);
> -    assert(!dev->msix_mask_notifier_opaque[vector]);
>  
>      /* Unmask the new notifier unless vector is masked. */
>      if (!msix_is_masked(dev, vector)) {
> -        r = dev->msix_mask_notifier(dev, vector, opaque, false);
> +        r = dev->msix_mask_notifier(dev, vector, false);
>          if (r < 0) {
>              return r;
>          }
>      }
> -    dev->msix_mask_notifier_opaque[vector] = opaque;
>      return r;
>  }
>  
> -int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector)
> +static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
>  {
>      int r = 0;
> -    void *opaque;
>      if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
>          return 0;
>  
> -    opaque = dev->msix_mask_notifier_opaque[vector];
> -
>      assert(dev->msix_mask_notifier);
> -    assert(opaque);
>  
>      /* Mask the old notifier unless it is already masked. */
>      if (!msix_is_masked(dev, vector)) {
> -        r = dev->msix_mask_notifier(dev, vector, opaque, true);
> +        r = dev->msix_mask_notifier(dev, vector, true);
>          if (r < 0) {
>              return r;
>          }
>      }
> -    dev->msix_mask_notifier_opaque[vector] = NULL;
> +    return r;
> +}

The above need to be combined to a single function now since the only
difference is s/true/false.

Alex

> +
> +int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
> +{
> +    int r, n;
> +    assert(!dev->msix_mask_notifier);
> +    dev->msix_mask_notifier = f;
> +    for (n = 0; n < dev->msix_entries_nr; ++n) {
> +        r = msix_set_mask_notifier_for_vector(dev, n);
> +        if (r < 0) {
> +            goto undo;
> +        }
> +    }
> +    return 0;
> +
> +undo:
> +    while (--n >= 0) {
> +        msix_unset_mask_notifier_for_vector(dev, n);
> +    }
> +    dev->msix_mask_notifier = NULL;
> +    return r;
> +}
> +
> +int msix_unset_mask_notifier(PCIDevice *dev)
> +{
> +    int r, n;
> +    assert(dev->msix_mask_notifier);
> +    for (n = 0; n < dev->msix_entries_nr; ++n) {
> +        r = msix_unset_mask_notifier_for_vector(dev, n);
> +        if (r < 0) {
> +            goto undo;
> +        }
> +    }
> +    dev->msix_mask_notifier = NULL;
> +    return 0;
> +
> +undo:
> +    while (--n >= 0) {
> +        msix_set_mask_notifier_for_vector(dev, n);
> +    }
>      return r;
>  }
> diff --git a/hw/msix.h b/hw/msix.h
> index 6b21ffb..5a81df5 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -33,6 +33,6 @@ void msix_reset(PCIDevice *dev);
>  
>  extern int msix_supported;
>  
> -int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque);
> -int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector);
> +int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func);
> +int msix_unset_mask_notifier(PCIDevice *dev);
>  #endif
> diff --git a/hw/pci.h b/hw/pci.h
> index ccb99d0..a40dc14 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -131,7 +131,7 @@ enum {
>  #define PCI_CAPABILITY_CONFIG_MSIX_LENGTH 0x10
>  
>  typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
> -				       void *opaque, int masked);
> +				       int masked);
>  
>  struct PCIDevice {
>      DeviceState qdev;
> @@ -198,7 +198,6 @@ struct PCIDevice {
>  
>      struct kvm_irq_routing_entry *msix_irq_entries;
>  
> -    void **msix_mask_notifier_opaque;
>      msix_mask_notifier_func msix_mask_notifier;
>  
>      /* Device capability configuration space */
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 232f943..c1204ce 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -428,11 +428,10 @@ static void virtio_pci_guest_notifier_read(void *opaque)
>      }
>  }
>  
> -static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
> -                                    void *opaque, int masked)
> +static int virtio_pci_mask_vq(PCIDevice *dev, unsigned vector,
> +                              VirtQueue *vq, int masked)
>  {
>  #ifdef CONFIG_KVM
> -    VirtQueue *vq = opaque;
>      EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
>      int r = kvm_set_irqfd(dev->msix_irq_entries[vector].gsi,
>                            event_notifier_get_fd(notifier),
> @@ -453,6 +452,34 @@ static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
>  #endif
>  }
>  
> +static int virtio_pci_mask_notifier(PCIDevice *dev, unsigned vector,
> +                                    int masked)
> +{
> +    VirtIOPCIProxy *proxy = container_of(dev, VirtIOPCIProxy, pci_dev);
> +    VirtIODevice *vdev = proxy->vdev;
> +    int r, n;
> +
> +    for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
> +        if (!virtio_queue_get_num(vdev, n)) {
> +            break;
> +        }
> +        if (virtio_queue_vector(vdev, n) != vector) {
> +            continue;
> +        }
> +        r = virtio_pci_mask_vq(dev, vector, virtio_get_queue(vdev, n), masked);
> +        if (r < 0) {
> +            goto undo;
> +        }
> +    }
> +    return 0;
> +undo:
> +    while (--n >= 0) {
> +        virtio_pci_mask_vq(dev, vector, virtio_get_queue(vdev, n), !masked);
> +    }
> +    return r;
> +}
> +
> +
>  static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
>  {
>      VirtIOPCIProxy *proxy = opaque;
> @@ -466,11 +493,7 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
>          }
>          qemu_set_fd_handler(event_notifier_get_fd(notifier),
>                              virtio_pci_guest_notifier_read, NULL, vq);
> -        msix_set_mask_notifier(&proxy->pci_dev,
> -                               virtio_queue_vector(proxy->vdev, n), vq);
>      } else {
> -        msix_unset_mask_notifier(&proxy->pci_dev,
> -				 virtio_queue_vector(proxy->vdev, n));
>          qemu_set_fd_handler(event_notifier_get_fd(notifier),
>                              NULL, NULL, NULL);
>          /* Test and clear notifier before closing it,
> @@ -488,6 +511,13 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
>      VirtIODevice *vdev = proxy->vdev;
>      int r, n;
>  
> +    /* Must unset mask notifier while guest notifier
> +     * is still assigned */
> +    if (!assign) {
> +	    r = msix_unset_mask_notifier(&proxy->pci_dev);
> +            assert(r >= 0);
> +    }
> +
>      for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
>          if (!virtio_queue_get_num(vdev, n)) {
>              break;
> @@ -499,6 +529,16 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
>          }
>      }
>  
> +    /* Must set mask notifier after guest notifier
> +     * has been assigned */
> +    if (assign) {
> +        r = msix_set_mask_notifier(&proxy->pci_dev,
> +                                   virtio_pci_mask_notifier);
> +        if (r < 0) {
> +            goto assign_error;
> +        }
> +    }
> +
>      return 0;
>  
>  assign_error:
> @@ -584,8 +624,6 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
>  
>      proxy->pci_dev.config_write = virtio_write_config;
>  
> -    proxy->pci_dev.msix_mask_notifier = virtio_pci_mask_notifier;
> -
>      size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev) + vdev->config_len;
>      if (size & (size-1))
>          size = 1 << qemu_fls(size);

[Qemu-devel] Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Michael S. Tsirkin]

On Wed, Oct 06, 2010 at 10:48:44AM -0600, Alex Williamson wrote:
> > -int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector)
> > +static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> >  {
> >      int r = 0;
> > -    void *opaque;
> >      if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> >          return 0;
> >  
> > -    opaque = dev->msix_mask_notifier_opaque[vector];
> > -
> >      assert(dev->msix_mask_notifier);
> > -    assert(opaque);
> >  
> >      /* Mask the old notifier unless it is already masked. */
> >      if (!msix_is_masked(dev, vector)) {
> > -        r = dev->msix_mask_notifier(dev, vector, opaque, true);
> > +        r = dev->msix_mask_notifier(dev, vector, true);
> >          if (r < 0) {
> >              return r;
> >          }
> >      }
> > -    dev->msix_mask_notifier_opaque[vector] = NULL;
> > +    return r;
> > +}
> 
> The above need to be combined to a single function now since the only
> difference is s/true/false.
> 
> Alex

This is the way it was in the past, and it turned out to be very
confusing to read since both variables: mask and assign are bool but
polarity is reversed.

Unrolled it seems easier to grok.

-- 
MST

[Qemu-devel] Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Alex Williamson]

On Wed, 2010-10-06 at 19:02 +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 06, 2010 at 10:48:44AM -0600, Alex Williamson wrote:
> > > -int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector)
> > > +static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> > >  {
> > >      int r = 0;
> > > -    void *opaque;
> > >      if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> > >          return 0;
> > >  
> > > -    opaque = dev->msix_mask_notifier_opaque[vector];
> > > -
> > >      assert(dev->msix_mask_notifier);
> > > -    assert(opaque);
> > >  
> > >      /* Mask the old notifier unless it is already masked. */
> > >      if (!msix_is_masked(dev, vector)) {
> > > -        r = dev->msix_mask_notifier(dev, vector, opaque, true);
> > > +        r = dev->msix_mask_notifier(dev, vector, true);
> > >          if (r < 0) {
> > >              return r;
> > >          }
> > >      }
> > > -    dev->msix_mask_notifier_opaque[vector] = NULL;
> > > +    return r;
> > > +}
> > 
> > The above need to be combined to a single function now since the only
> > difference is s/true/false.
> > 
> > Alex
> 
> This is the way it was in the past, and it turned out to be very
> confusing to read since both variables: mask and assign are bool but
> polarity is reversed.
> 
> Unrolled it seems easier to grok.

You could always keep the functions as separate wrapper callers of the
common function so you only need to keep true = unset, false = set
straight in one place.  Thanks,

Alex

[Qemu-devel] Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Michael S. Tsirkin]

On Wed, Oct 06, 2010 at 11:24:24AM -0600, Alex Williamson wrote:
> On Wed, 2010-10-06 at 19:02 +0200, Michael S. Tsirkin wrote:
> > On Wed, Oct 06, 2010 at 10:48:44AM -0600, Alex Williamson wrote:
> > > > -int msix_unset_mask_notifier(PCIDevice *dev, unsigned vector)
> > > > +static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> > > >  {
> > > >      int r = 0;
> > > > -    void *opaque;
> > > >      if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> > > >          return 0;
> > > >  
> > > > -    opaque = dev->msix_mask_notifier_opaque[vector];
> > > > -
> > > >      assert(dev->msix_mask_notifier);
> > > > -    assert(opaque);
> > > >  
> > > >      /* Mask the old notifier unless it is already masked. */
> > > >      if (!msix_is_masked(dev, vector)) {
> > > > -        r = dev->msix_mask_notifier(dev, vector, opaque, true);
> > > > +        r = dev->msix_mask_notifier(dev, vector, true);
> > > >          if (r < 0) {
> > > >              return r;
> > > >          }
> > > >      }
> > > > -    dev->msix_mask_notifier_opaque[vector] = NULL;
> > > > +    return r;
> > > > +}
> > > 
> > > The above need to be combined to a single function now since the only
> > > difference is s/true/false.
> > > 
> > > Alex
> > 
> > This is the way it was in the past, and it turned out to be very
> > confusing to read since both variables: mask and assign are bool but
> > polarity is reversed.
> > 
> > Unrolled it seems easier to grok.
> 
> You could always keep the functions as separate wrapper callers of the
> common function so you only need to keep true = unset, false = set
> straight in one place.  Thanks,
> 
> Alex

wrappers still make this confusing.
we had so many bugs here, I feel minor duplication
is worth it.

-- 
MST

[Qemu-devel] Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Michael S. Tsirkin]

On Wed, Oct 06, 2010 at 11:24:24AM -0600, Alex Williamson wrote:
> You could always keep the functions as separate wrapper callers of the
> common function so you only need to keep true = unset, false = set
> straight in one place.  Thanks,


Just to show why it does not work, I did exactly this: as you see the
code is shorter but the true/false magic gets spread: it was in 2
places, (set/unset) now it is in 4 places and it is within the loop, in
code that is more complex.

So I think I'll stick to the original version and we can
patch it up later if there's a will.


diff --git a/hw/msix.c b/hw/msix.c
index 3d4dd61..4b705a0 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -583,40 +583,15 @@ void msix_unuse_all_vectors(PCIDevice *dev)
     msix_free_irq_entries(dev);
 }
 
-static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
+/* Invoke the notifier if vector entry is used and unmasked. */
+static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, bool masked)
 {
     int r = 0;
-    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
+    if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
         return 0;
-
-    assert(dev->msix_mask_notifier);
-
-    /* Unmask the new notifier unless vector is masked. */
-    if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, false);
-        if (r < 0) {
-            return r;
-        }
     }
-    return r;
-}
-
-static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
-{
-    int r = 0;
-    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
-        return 0;
-
     assert(dev->msix_mask_notifier);
-
-    /* Mask the old notifier unless it is already masked. */
-    if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, true);
-        if (r < 0) {
-            return r;
-        }
-    }
-    return r;
+    return dev->msix_mask_notifier(dev, vector, masked);
 }
 
 int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
@@ -625,7 +600,7 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
     assert(!dev->msix_mask_notifier);
     dev->msix_mask_notifier = f;
     for (n = 0; n < dev->msix_entries_nr; ++n) {
-        r = msix_set_mask_notifier_for_vector(dev, n);
+        r = msix_notify_if_unmasked(dev, n, false);
         if (r < 0) {
             goto undo;
         }
@@ -634,7 +609,7 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
 
 undo:
     while (--n >= 0) {
-        msix_unset_mask_notifier_for_vector(dev, n);
+        msix_notify_if_unmasked(dev, n, true);
     }
     dev->msix_mask_notifier = NULL;
     return r;
@@ -645,7 +620,7 @@ int msix_unset_mask_notifier(PCIDevice *dev)
     int r, n;
     assert(dev->msix_mask_notifier);
     for (n = 0; n < dev->msix_entries_nr; ++n) {
-        r = msix_unset_mask_notifier_for_vector(dev, n);
+        r = msix_notify_if_unmasked(dev, n, true);
         if (r < 0) {
             goto undo;
         }
@@ -655,7 +630,7 @@ int msix_unset_mask_notifier(PCIDevice *dev)
 
 undo:
     while (--n >= 0) {
-        msix_set_mask_notifier_for_vector(dev, n);
+        msix_notify_if_unmasked(dev, n, false);
     }
     return r;
 }

[Qemu-devel] Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Alex Williamson]

On Wed, 2010-10-06 at 23:44 +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 06, 2010 at 11:24:24AM -0600, Alex Williamson wrote:
> > You could always keep the functions as separate wrapper callers of the
> > common function so you only need to keep true = unset, false = set
> > straight in one place.  Thanks,
> 
> 
> Just to show why it does not work, I did exactly this: as you see the
> code is shorter but the true/false magic gets spread: it was in 2
> places, (set/unset) now it is in 4 places and it is within the loop, in
> code that is more complex.

You seem to have missed the wrapper function.  I'm simply suggesting
something like this:

static int __do_msix_mask_notifier_for_vector(PCIDevice *dev, unsigned vector, bool mask)
{
    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
        return 0;

    assert(dev->msix_mask_notifier);

    /* Set the new notifier unless vector is masked. */
    if (!msix_is_masked(dev, vector)) {
        return dev->msix_mask_notifier(dev, vector, mask);
    }
    return 0;
}

static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
{
    return __do_msix_mask_notifier_for_vector(dev, vector, false);
}

static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
{
    return __do_msix_mask_notifier_for_vector(dev, vector, true);
}

Which then doesn't go on to complicate the callers like the below does.
Thanks,

Alex


> So I think I'll stick to the original version and we can
> patch it up later if there's a will.
> 
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 3d4dd61..4b705a0 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -583,40 +583,15 @@ void msix_unuse_all_vectors(PCIDevice *dev)
>      msix_free_irq_entries(dev);
>  }
>  
> -static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +/* Invoke the notifier if vector entry is used and unmasked. */
> +static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, bool masked)
>  {
>      int r = 0;
> -    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> +    if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
>          return 0;
> -
> -    assert(dev->msix_mask_notifier);
> -
> -    /* Unmask the new notifier unless vector is masked. */
> -    if (!msix_is_masked(dev, vector)) {
> -        r = dev->msix_mask_notifier(dev, vector, false);
> -        if (r < 0) {
> -            return r;
> -        }
>      }
> -    return r;
> -}
> -
> -static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> -{
> -    int r = 0;
> -    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> -        return 0;
> -
>      assert(dev->msix_mask_notifier);
> -
> -    /* Mask the old notifier unless it is already masked. */
> -    if (!msix_is_masked(dev, vector)) {
> -        r = dev->msix_mask_notifier(dev, vector, true);
> -        if (r < 0) {
> -            return r;
> -        }
> -    }
> -    return r;
> +    return dev->msix_mask_notifier(dev, vector, masked);
>  }
>  
>  int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
> @@ -625,7 +600,7 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
>      assert(!dev->msix_mask_notifier);
>      dev->msix_mask_notifier = f;
>      for (n = 0; n < dev->msix_entries_nr; ++n) {
> -        r = msix_set_mask_notifier_for_vector(dev, n);
> +        r = msix_notify_if_unmasked(dev, n, false);
>          if (r < 0) {
>              goto undo;
>          }
> @@ -634,7 +609,7 @@ int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
>  
>  undo:
>      while (--n >= 0) {
> -        msix_unset_mask_notifier_for_vector(dev, n);
> +        msix_notify_if_unmasked(dev, n, true);
>      }
>      dev->msix_mask_notifier = NULL;
>      return r;
> @@ -645,7 +620,7 @@ int msix_unset_mask_notifier(PCIDevice *dev)
>      int r, n;
>      assert(dev->msix_mask_notifier);
>      for (n = 0; n < dev->msix_entries_nr; ++n) {
> -        r = msix_unset_mask_notifier_for_vector(dev, n);
> +        r = msix_notify_if_unmasked(dev, n, true);
>          if (r < 0) {
>              goto undo;
>          }
> @@ -655,7 +630,7 @@ int msix_unset_mask_notifier(PCIDevice *dev)
>  
>  undo:
>      while (--n >= 0) {
> -        msix_set_mask_notifier_for_vector(dev, n);
> +        msix_notify_if_unmasked(dev, n, false);
>      }
>      return r;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Qemu-devel] Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Michael S. Tsirkin]

On Wed, Oct 06, 2010 at 04:05:38PM -0600, Alex Williamson wrote:
> On Wed, 2010-10-06 at 23:44 +0200, Michael S. Tsirkin wrote:
> > On Wed, Oct 06, 2010 at 11:24:24AM -0600, Alex Williamson wrote:
> > > You could always keep the functions as separate wrapper callers of the
> > > common function so you only need to keep true = unset, false = set
> > > straight in one place.  Thanks,
> > 
> > 
> > Just to show why it does not work, I did exactly this: as you see the
> > code is shorter but the true/false magic gets spread: it was in 2
> > places, (set/unset) now it is in 4 places and it is within the loop, in
> > code that is more complex.
> 
> You seem to have missed the wrapper function.  I'm simply suggesting
> something like this:

Good idea. I tweaked the code a bit more and came up with this:
I think I will keep this as an incremental patch just to
keep diffs small.

msix: factor out mask notifier code

Move some common code handling msix mask notifiers to a function.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


diff --git a/hw/msix.c b/hw/msix.c
index 3d4dd61..5f66d20 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -583,40 +583,26 @@ void msix_unuse_all_vectors(PCIDevice *dev)
     msix_free_irq_entries(dev);
 }
 
-static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
+/* Invoke the notifier if vector entry is used and unmasked. */
+static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, int masked)
 {
-    int r = 0;
-    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
-        return 0;
-
     assert(dev->msix_mask_notifier);
-
-    /* Unmask the new notifier unless vector is masked. */
-    if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, false);
-        if (r < 0) {
-            return r;
-        }
+    if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
+        return 0;
     }
-    return r;
+    return dev->msix_mask_notifier(dev, vector, masked);
 }
 
-static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
+static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
 {
-    int r = 0;
-    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
-        return 0;
-
-    assert(dev->msix_mask_notifier);
+	/* Notifier has been set. Invoke it on unmasked vectors. */
+	return msix_notify_if_unmasked(dev, vector, 0); 
+}
 
-    /* Mask the old notifier unless it is already masked. */
-    if (!msix_is_masked(dev, vector)) {
-        r = dev->msix_mask_notifier(dev, vector, true);
-        if (r < 0) {
-            return r;
-        }
-    }
-    return r;
+static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
+{
+	/* Notifier will be unset. Invoke it to mask unmasked entries. */
+	return msix_notify_if_unmasked(dev, vector, 1); 
 }
 
 int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)

[Qemu-devel] Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Alex Williamson]

On Thu, 2010-10-07 at 11:57 +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 06, 2010 at 04:05:38PM -0600, Alex Williamson wrote:
> > On Wed, 2010-10-06 at 23:44 +0200, Michael S. Tsirkin wrote:
> > > On Wed, Oct 06, 2010 at 11:24:24AM -0600, Alex Williamson wrote:
> > > > You could always keep the functions as separate wrapper callers of the
> > > > common function so you only need to keep true = unset, false = set
> > > > straight in one place.  Thanks,
> > > 
> > > 
> > > Just to show why it does not work, I did exactly this: as you see the
> > > code is shorter but the true/false magic gets spread: it was in 2
> > > places, (set/unset) now it is in 4 places and it is within the loop, in
> > > code that is more complex.
> > 
> > You seem to have missed the wrapper function.  I'm simply suggesting
> > something like this:
> 
> Good idea. I tweaked the code a bit more and came up with this:
> I think I will keep this as an incremental patch just to
> keep diffs small.
> 
> msix: factor out mask notifier code
> 
> Move some common code handling msix mask notifiers to a function.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index 3d4dd61..5f66d20 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -583,40 +583,26 @@ void msix_unuse_all_vectors(PCIDevice *dev)
>      msix_free_irq_entries(dev);
>  }
>  
> -static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +/* Invoke the notifier if vector entry is used and unmasked. */
> +static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, int masked)
>  {
> -    int r = 0;
> -    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> -        return 0;
> -
>      assert(dev->msix_mask_notifier);
> -
> -    /* Unmask the new notifier unless vector is masked. */
> -    if (!msix_is_masked(dev, vector)) {
> -        r = dev->msix_mask_notifier(dev, vector, false);
> -        if (r < 0) {
> -            return r;
> -        }
> +    if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) {
> +        return 0;
>      }
> -    return r;
> +    return dev->msix_mask_notifier(dev, vector, masked);
>  }
>  
> -static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
>  {
> -    int r = 0;
> -    if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
> -        return 0;
> -
> -    assert(dev->msix_mask_notifier);
> +	/* Notifier has been set. Invoke it on unmasked vectors. */
> +	return msix_notify_if_unmasked(dev, vector, 0); 
> +}

Looks fine except for the extra white space here and copied below.
Thanks,

Alex
 
> -    /* Mask the old notifier unless it is already masked. */
> -    if (!msix_is_masked(dev, vector)) {
> -        r = dev->msix_mask_notifier(dev, vector, true);
> -        if (r < 0) {
> -            return r;
> -        }
> -    }
> -    return r;
> +static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector)
> +{
> +	/* Notifier will be unset. Invoke it to mask unmasked entries. */
> +	return msix_notify_if_unmasked(dev, vector, 1); 
>  }
>  
>  int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)

[Qemu-devel] Re: [PATCHv2] qemu-kvm/vhost: fix up irqfd support

[Michael S. Tsirkin]

> Looks fine except for the extra white space here and copied below.
> Thanks,
> 
> Alex

OK, I fixed this up and queued the patch with your ack.
Thanks for the review!

-- 
MST