[Qemu-devel] [PATCH 0/3] ivshmem: MSI bug fixes

[Qemu-devel] [PATCH 0/3] ivshmem: MSI bug fixes

[Ladi Prosek]

Fixes bugs in the ivshmem device implementation uncovered with the new
Windows ivshmem driver:
https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem

Ladi Prosek (3):
  ivshmem: Don't update non-existent MSI routes
  ivshmem: Always remove irqfd notifiers
  ivshmem: Improve MSI irqfd error handling

 hw/misc/ivshmem.c | 75 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 19 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 1/3] ivshmem: Don't update non-existent MSI routes

[Ladi Prosek]

As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
QEMU crashes with:

  kvm_irqchip_commit_routes: Assertion `ret == 0' failed.

if the ivshmem device is configured with more vectors than what the server
supports. This is caused by the ivshmem_vector_unmask() being called on
vectors that have not been initialized by ivshmem_add_kvm_msi_virq().

This commit fixes it by adding a simple check to the mask and unmask
callbacks.

Note that the opposite mismatch, if the server supplies more vectors than
what the device is configured for, is already handled and leads to output
like:

  Too many eventfd received, device has 1 vectors

Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/misc/ivshmem.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a5a46827fe..6e46669744 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -317,6 +317,10 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector,
     int ret;
 
     IVSHMEM_DPRINTF("vector unmask %p %d\n", dev, vector);
+    if (!v->pdev) {
+        error_report("ivshmem: vector %d route does not exist", vector);
+        return -EINVAL;
+    }
 
     ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev);
     if (ret < 0) {
@@ -331,12 +335,16 @@ static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector)
 {
     IVShmemState *s = IVSHMEM_COMMON(dev);
     EventNotifier *n = &s->peers[s->vm_id].eventfds[vector];
+    MSIVector *v = &s->msi_vectors[vector];
     int ret;
 
     IVSHMEM_DPRINTF("vector mask %p %d\n", dev, vector);
+    if (!v->pdev) {
+        error_report("ivshmem: vector %d route does not exist", vector);
+        return;
+    }
 
-    ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n,
-                                                s->msi_vectors[vector].virq);
+    ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq);
     if (ret != 0) {
         error_report("remove_irqfd_notifier_gsi failed");
     }
-- 
2.13.5

[Qemu-devel] [PATCH 2/3] ivshmem: Always remove irqfd notifiers

[Ladi Prosek]

As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
QEMU crashes with:

ivshmem: msix_set_vector_notifiers failed
msix_unset_vector_notifiers: Assertion `dev->msix_vector_use_notifier && dev->msix_vector_release_notifier' failed.

if MSI-X is repeatedly enabled and disabled on the ivshmem device, for example
by loading and unloading the Windows ivshmem driver. This is because
msix_unset_vector_notifiers() doesn't call any of the release notifier callbacks
since MSI-X is already disabled at that point (msix_enabled() returning false
is how this transition is detected in the first place). Thus ivshmem_vector_mask()
doesn't run and when MSI-X is subsequently enabled again ivshmem_vector_unmask()
fails.

This is fixed by keeping track of unmasked vectors and making sure that
ivshmem_vector_mask() always runs on MSI-X disable.

Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/misc/ivshmem.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 6e46669744..493a5030a1 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -77,6 +77,7 @@ typedef struct Peer {
 typedef struct MSIVector {
     PCIDevice *pdev;
     int virq;
+    bool unmasked;
 } MSIVector;
 
 typedef struct IVShmemState {
@@ -321,6 +322,7 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector,
         error_report("ivshmem: vector %d route does not exist", vector);
         return -EINVAL;
     }
+    assert(!v->unmasked);
 
     ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev);
     if (ret < 0) {
@@ -328,7 +330,11 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector,
     }
     kvm_irqchip_commit_routes(kvm_state);
 
-    return kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, v->virq);
+    ret = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, v->virq);
+    if (ret == 0) {
+        v->unmasked = true;
+    }
+    return ret;
 }
 
 static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector)
@@ -343,9 +349,12 @@ static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector)
         error_report("ivshmem: vector %d route does not exist", vector);
         return;
     }
+    assert(v->unmasked);
 
     ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq);
-    if (ret != 0) {
+    if (ret == 0) {
+        v->unmasked = false;
+    } else {
         error_report("remove_irqfd_notifier_gsi failed");
     }
 }
@@ -817,11 +826,20 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
     PCIDevice *pdev = PCI_DEVICE(s);
     int i;
 
-    for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
-        ivshmem_remove_kvm_msi_virq(s, i);
-    }
-
     msix_unset_vector_notifiers(pdev);
+
+    for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
+        /*
+         * MSI-X is already disabled here so msix_unset_vector_notifiers
+         * didn't call our release notifier. Do it now to keep our masks and
+         * unmasks balanced.
+         */
+        if (s->msi_vectors[i].unmasked) {
+            ivshmem_vector_mask(pdev, i);
+        }
+        ivshmem_remove_kvm_msi_virq(s, i);
+    }
+
 }
 
 static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
-- 
2.13.5

[Qemu-devel] [PATCH 3/3] ivshmem: Improve MSI irqfd error handling

[Ladi Prosek]

Adds a rollback path to ivshmem_enable_irqfd() and fixes
ivshmem_disable_irqfd() to bail if irqfd has not been enabled.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 hw/misc/ivshmem.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 493a5030a1..ff07a94691 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -784,6 +784,20 @@ static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
     return 0;
 }
 
+static void ivshmem_remove_kvm_msi_virq(IVShmemState *s, int vector)
+{
+    IVSHMEM_DPRINTF("ivshmem_remove_kvm_msi_virq vector:%d\n", vector);
+
+    if (s->msi_vectors[vector].pdev == NULL) {
+        return;
+    }
+
+    /* it was cleaned when masked in the frontend. */
+    kvm_irqchip_release_virq(kvm_state, s->msi_vectors[vector].virq);
+
+    s->msi_vectors[vector].pdev = NULL;
+}
+
 static void ivshmem_enable_irqfd(IVShmemState *s)
 {
     PCIDevice *pdev = PCI_DEVICE(s);
@@ -795,7 +809,7 @@ static void ivshmem_enable_irqfd(IVShmemState *s)
         ivshmem_add_kvm_msi_virq(s, i, &err);
         if (err) {
             error_report_err(err);
-            /* TODO do we need to handle the error? */
+            goto undo;
         }
     }
 
@@ -804,21 +818,14 @@ static void ivshmem_enable_irqfd(IVShmemState *s)
                                   ivshmem_vector_mask,
                                   ivshmem_vector_poll)) {
         error_report("ivshmem: msix_set_vector_notifiers failed");
+        goto undo;
     }
-}
+    return;
 
-static void ivshmem_remove_kvm_msi_virq(IVShmemState *s, int vector)
-{
-    IVSHMEM_DPRINTF("ivshmem_remove_kvm_msi_virq vector:%d\n", vector);
-
-    if (s->msi_vectors[vector].pdev == NULL) {
-        return;
+undo:
+    while (--i >= 0) {
+        ivshmem_remove_kvm_msi_virq(s, i);
     }
-
-    /* it was cleaned when masked in the frontend. */
-    kvm_irqchip_release_virq(kvm_state, s->msi_vectors[vector].virq);
-
-    s->msi_vectors[vector].pdev = NULL;
 }
 
 static void ivshmem_disable_irqfd(IVShmemState *s)
@@ -826,6 +833,10 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
     PCIDevice *pdev = PCI_DEVICE(s);
     int i;
 
+    if (!pdev->msix_vector_use_notifier) {
+        return;
+    }
+
     msix_unset_vector_notifiers(pdev);
 
     for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
-- 
2.13.5

Re: [Qemu-devel] [PATCH 1/3] ivshmem: Don't update non-existent MSI routes

[Marc-André Lureau]

----- Original Message -----
> As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
> QEMU crashes with:
> 
>   kvm_irqchip_commit_routes: Assertion `ret == 0' failed.
> 
> if the ivshmem device is configured with more vectors than what the server
> supports. This is caused by the ivshmem_vector_unmask() being called on
> vectors that have not been initialized by ivshmem_add_kvm_msi_virq().
> 
> This commit fixes it by adding a simple check to the mask and unmask
> callbacks.
> 
> Note that the opposite mismatch, if the server supplies more vectors than
> what the device is configured for, is already handled and leads to output
> like:
> 
>   Too many eventfd received, device has 1 vectors
> 
> Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/misc/ivshmem.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index a5a46827fe..6e46669744 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -317,6 +317,10 @@ static int ivshmem_vector_unmask(PCIDevice *dev,
> unsigned vector,
>      int ret;
>  
>      IVSHMEM_DPRINTF("vector unmask %p %d\n", dev, vector);
> +    if (!v->pdev) {
> +        error_report("ivshmem: vector %d route does not exist", vector);
> +        return -EINVAL;
> +    }
>  
>      ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev);
>      if (ret < 0) {
> @@ -331,12 +335,16 @@ static void ivshmem_vector_mask(PCIDevice *dev,
> unsigned vector)
>  {
>      IVShmemState *s = IVSHMEM_COMMON(dev);
>      EventNotifier *n = &s->peers[s->vm_id].eventfds[vector];
> +    MSIVector *v = &s->msi_vectors[vector];
>      int ret;
>  
>      IVSHMEM_DPRINTF("vector mask %p %d\n", dev, vector);
> +    if (!v->pdev) {
> +        error_report("ivshmem: vector %d route does not exist", vector);
> +        return;
> +    }
>  
> -    ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n,
> -
> s->msi_vectors[vector].virq);
> +    ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq);
>      if (ret != 0) {
>          error_report("remove_irqfd_notifier_gsi failed");
>      }
> --
> 2.13.5
> 
> 

Re: [Qemu-devel] [PATCH 1/3] ivshmem: Don't update non-existent MSI routes

[geoff]

Thanks Ladi, I had not yet had time to dig into these, this patch set 
resolves all issues I was aware of.

Tested-by: Geoffrey McRae <geoff@hostfission.com>

On 2017-11-11 04:34, Ladi Prosek wrote:
> As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi 
> notifications"),
> QEMU crashes with:
> 
>   kvm_irqchip_commit_routes: Assertion `ret == 0' failed.
> 
> if the ivshmem device is configured with more vectors than what the 
> server
> supports. This is caused by the ivshmem_vector_unmask() being called on
> vectors that have not been initialized by ivshmem_add_kvm_msi_virq().
> 
> This commit fixes it by adding a simple check to the mask and unmask
> callbacks.
> 
> Note that the opposite mismatch, if the server supplies more vectors 
> than
> what the device is configured for, is already handled and leads to 
> output
> like:
> 
>   Too many eventfd received, device has 1 vectors
> 
> Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/misc/ivshmem.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index a5a46827fe..6e46669744 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -317,6 +317,10 @@ static int ivshmem_vector_unmask(PCIDevice *dev,
> unsigned vector,
>      int ret;
> 
>      IVSHMEM_DPRINTF("vector unmask %p %d\n", dev, vector);
> +    if (!v->pdev) {
> +        error_report("ivshmem: vector %d route does not exist", 
> vector);
> +        return -EINVAL;
> +    }
> 
>      ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev);
>      if (ret < 0) {
> @@ -331,12 +335,16 @@ static void ivshmem_vector_mask(PCIDevice *dev,
> unsigned vector)
>  {
>      IVShmemState *s = IVSHMEM_COMMON(dev);
>      EventNotifier *n = &s->peers[s->vm_id].eventfds[vector];
> +    MSIVector *v = &s->msi_vectors[vector];
>      int ret;
> 
>      IVSHMEM_DPRINTF("vector mask %p %d\n", dev, vector);
> +    if (!v->pdev) {
> +        error_report("ivshmem: vector %d route does not exist", 
> vector);
> +        return;
> +    }
> 
> -    ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n,
> -                                                
> s->msi_vectors[vector].virq);
> +    ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, 
> v->virq);
>      if (ret != 0) {
>          error_report("remove_irqfd_notifier_gsi failed");
>      }

Re: [Qemu-devel] [PATCH 1/3] ivshmem: Don't update non-existent MSI routes

[Markus Armbruster]

Ladi Prosek <lprosek@redhat.com> writes:

> As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
> QEMU crashes with:
>
>   kvm_irqchip_commit_routes: Assertion `ret == 0' failed.
>
> if the ivshmem device is configured with more vectors than what the server
> supports. This is caused by the ivshmem_vector_unmask() being called on
> vectors that have not been initialized by ivshmem_add_kvm_msi_virq().
>
> This commit fixes it by adding a simple check to the mask and unmask
> callbacks.
>
> Note that the opposite mismatch, if the server supplies more vectors than
> what the device is configured for, is already handled and leads to output
> like:
>
>   Too many eventfd received, device has 1 vectors
>
> Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>

I think I understand your description of what's wrong.  Not obvious to
me is how it can happen.  The cover letter mentions a Windows ivshmem
driver.  Is this a device bug a driver can trigger?  If yes, how?

Re: [Qemu-devel] [PATCH 2/3] ivshmem: Always remove irqfd notifiers

[Markus Armbruster]

Ladi Prosek <lprosek@redhat.com> writes:

> As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
> QEMU crashes with:
>
> ivshmem: msix_set_vector_notifiers failed
> msix_unset_vector_notifiers: Assertion `dev->msix_vector_use_notifier && dev->msix_vector_release_notifier' failed.
>
> if MSI-X is repeatedly enabled and disabled on the ivshmem device, for example
> by loading and unloading the Windows ivshmem driver. This is because
> msix_unset_vector_notifiers() doesn't call any of the release notifier callbacks
> since MSI-X is already disabled at that point (msix_enabled() returning false
> is how this transition is detected in the first place). Thus ivshmem_vector_mask()
> doesn't run and when MSI-X is subsequently enabled again ivshmem_vector_unmask()
> fails.
>
> This is fixed by keeping track of unmasked vectors and making sure that
> ivshmem_vector_mask() always runs on MSI-X disable.
>
> Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/misc/ivshmem.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 6e46669744..493a5030a1 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -77,6 +77,7 @@ typedef struct Peer {
>  typedef struct MSIVector {
>      PCIDevice *pdev;
>      int virq;
> +    bool unmasked;
>  } MSIVector;
>  
>  typedef struct IVShmemState {
> @@ -321,6 +322,7 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector,
>          error_report("ivshmem: vector %d route does not exist", vector);
>          return -EINVAL;
>      }
> +    assert(!v->unmasked);
>  
>      ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev);
>      if (ret < 0) {
> @@ -328,7 +330,11 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector,
>      }
>      kvm_irqchip_commit_routes(kvm_state);
>  
> -    return kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, v->virq);
> +    ret = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, v->virq);
> +    if (ret == 0) {
> +        v->unmasked = true;
> +    }
> +    return ret;
>  }
>  
>  static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector)
> @@ -343,9 +349,12 @@ static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector)
>          error_report("ivshmem: vector %d route does not exist", vector);
>          return;
>      }
> +    assert(v->unmasked);
>  
>      ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq);
> -    if (ret != 0) {
> +    if (ret == 0) {
> +        v->unmasked = false;
> +    } else {
>          error_report("remove_irqfd_notifier_gsi failed");
>      }
>  }

I generally prefer to put the error case in the conditional, and keep
the normal case out of it, like this:

       ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq);
       if (ret < 0) {
           error_report("remove_irqfd_notifier_gsi failed");
       }
       v->unmasked = false;

However, that makes ivshmem_vector_mask() and ivshmem_vector_unmask()
even more asymmetric.  Hmm.

> @@ -817,11 +826,20 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>      PCIDevice *pdev = PCI_DEVICE(s);
>      int i;
>  
> -    for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
> -        ivshmem_remove_kvm_msi_virq(s, i);
> -    }
> -
>      msix_unset_vector_notifiers(pdev);
> +
> +    for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
> +        /*
> +         * MSI-X is already disabled here so msix_unset_vector_notifiers
> +         * didn't call our release notifier. Do it now to keep our masks and
> +         * unmasks balanced.
> +         */

For consistency with other comments in this file, put () after the
function name, and two spaces after the sentence-ending period.

> +        if (s->msi_vectors[i].unmasked) {
> +            ivshmem_vector_mask(pdev, i);
> +        }
> +        ivshmem_remove_kvm_msi_virq(s, i);
> +    }
> +
>  }
>  
>  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,

Only nit-picking, so:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] ivshmem: Don't update non-existent MSI routes

[Ladi Prosek]

On Mon, Nov 13, 2017 at 3:22 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Ladi Prosek <lprosek@redhat.com> writes:
>
>> As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
>> QEMU crashes with:
>>
>>   kvm_irqchip_commit_routes: Assertion `ret == 0' failed.
>>
>> if the ivshmem device is configured with more vectors than what the server
>> supports. This is caused by the ivshmem_vector_unmask() being called on
>> vectors that have not been initialized by ivshmem_add_kvm_msi_virq().
>>
>> This commit fixes it by adding a simple check to the mask and unmask
>> callbacks.
>>
>> Note that the opposite mismatch, if the server supplies more vectors than
>> what the device is configured for, is already handled and leads to output
>> like:
>>
>>   Too many eventfd received, device has 1 vectors
>>
>> Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>
> I think I understand your description of what's wrong.  Not obvious to
> me is how it can happen.  The cover letter mentions a Windows ivshmem
> driver.  Is this a device bug a driver can trigger?  If yes, how?

I don't have a Linux guest handy but this code has existed for quite a
long time so yes, I think it's safe to assume that it can't be
(easily) triggered by the Linux driver.

The reproducer is as simple as:

ivshmem-server -n 0
qemu ... -device ivshmem-doorbell,chardev=iv -chardev
socket,path=/tmp/ivshmem_socket,id=iv

and load the Windows driver in the guest.

Maybe Linux won't enable MSI-X on the device?

Thanks!
Ladi

Re: [Qemu-devel] [PATCH 2/3] ivshmem: Always remove irqfd notifiers

[Ladi Prosek]

On Mon, Nov 13, 2017 at 3:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Ladi Prosek <lprosek@redhat.com> writes:
>
>> As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
>> QEMU crashes with:
>>
>> ivshmem: msix_set_vector_notifiers failed
>> msix_unset_vector_notifiers: Assertion `dev->msix_vector_use_notifier && dev->msix_vector_release_notifier' failed.
>>
>> if MSI-X is repeatedly enabled and disabled on the ivshmem device, for example
>> by loading and unloading the Windows ivshmem driver. This is because
>> msix_unset_vector_notifiers() doesn't call any of the release notifier callbacks
>> since MSI-X is already disabled at that point (msix_enabled() returning false
>> is how this transition is detected in the first place). Thus ivshmem_vector_mask()
>> doesn't run and when MSI-X is subsequently enabled again ivshmem_vector_unmask()
>> fails.
>>
>> This is fixed by keeping track of unmasked vectors and making sure that
>> ivshmem_vector_mask() always runs on MSI-X disable.
>>
>> Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> ---
>>  hw/misc/ivshmem.c | 30 ++++++++++++++++++++++++------
>>  1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index 6e46669744..493a5030a1 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -77,6 +77,7 @@ typedef struct Peer {
>>  typedef struct MSIVector {
>>      PCIDevice *pdev;
>>      int virq;
>> +    bool unmasked;
>>  } MSIVector;
>>
>>  typedef struct IVShmemState {
>> @@ -321,6 +322,7 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector,
>>          error_report("ivshmem: vector %d route does not exist", vector);
>>          return -EINVAL;
>>      }
>> +    assert(!v->unmasked);
>>
>>      ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev);
>>      if (ret < 0) {
>> @@ -328,7 +330,11 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector,
>>      }
>>      kvm_irqchip_commit_routes(kvm_state);
>>
>> -    return kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, v->virq);
>> +    ret = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL, v->virq);
>> +    if (ret == 0) {
>> +        v->unmasked = true;
>> +    }
>> +    return ret;
>>  }
>>
>>  static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector)
>> @@ -343,9 +349,12 @@ static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector)
>>          error_report("ivshmem: vector %d route does not exist", vector);
>>          return;
>>      }
>> +    assert(v->unmasked);
>>
>>      ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq);
>> -    if (ret != 0) {
>> +    if (ret == 0) {
>> +        v->unmasked = false;
>> +    } else {
>>          error_report("remove_irqfd_notifier_gsi failed");
>>      }
>>  }
>
> I generally prefer to put the error case in the conditional, and keep
> the normal case out of it, like this:
>
>        ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq);
>        if (ret < 0) {
>            error_report("remove_irqfd_notifier_gsi failed");
>        }
>        v->unmasked = false;
>
> However, that makes ivshmem_vector_mask() and ivshmem_vector_unmask()
> even more asymmetric.  Hmm.
>
>> @@ -817,11 +826,20 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>>      PCIDevice *pdev = PCI_DEVICE(s);
>>      int i;
>>
>> -    for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
>> -        ivshmem_remove_kvm_msi_virq(s, i);
>> -    }
>> -
>>      msix_unset_vector_notifiers(pdev);
>> +
>> +    for (i = 0; i < s->peers[s->vm_id].nb_eventfds; i++) {
>> +        /*
>> +         * MSI-X is already disabled here so msix_unset_vector_notifiers
>> +         * didn't call our release notifier. Do it now to keep our masks and
>> +         * unmasks balanced.
>> +         */
>
> For consistency with other comments in this file, put () after the
> function name, and two spaces after the sentence-ending period.
>
>> +        if (s->msi_vectors[i].unmasked) {
>> +            ivshmem_vector_mask(pdev, i);
>> +        }
>> +        ivshmem_remove_kvm_msi_virq(s, i);
>> +    }
>> +
>>  }
>>
>>  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>
> Only nit-picking, so:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thank you, I'll address your comments in v2.

Re: [Qemu-devel] [PATCH 1/3] ivshmem: Don't update non-existent MSI routes

[Markus Armbruster]

Ladi Prosek <lprosek@redhat.com> writes:

> On Mon, Nov 13, 2017 at 3:22 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Ladi Prosek <lprosek@redhat.com> writes:
>>
>>> As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"),
>>> QEMU crashes with:
>>>
>>>   kvm_irqchip_commit_routes: Assertion `ret == 0' failed.
>>>
>>> if the ivshmem device is configured with more vectors than what the server
>>> supports. This is caused by the ivshmem_vector_unmask() being called on
>>> vectors that have not been initialized by ivshmem_add_kvm_msi_virq().
>>>
>>> This commit fixes it by adding a simple check to the mask and unmask
>>> callbacks.
>>>
>>> Note that the opposite mismatch, if the server supplies more vectors than
>>> what the device is configured for, is already handled and leads to output
>>> like:
>>>
>>>   Too many eventfd received, device has 1 vectors
>>>
>>> Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications")
>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>
>> I think I understand your description of what's wrong.  Not obvious to
>> me is how it can happen.  The cover letter mentions a Windows ivshmem
>> driver.  Is this a device bug a driver can trigger?  If yes, how?
>
> I don't have a Linux guest handy but this code has existed for quite a
> long time so yes, I think it's safe to assume that it can't be
> (easily) triggered by the Linux driver.
>
> The reproducer is as simple as:
>
> ivshmem-server -n 0
> qemu ... -device ivshmem-doorbell,chardev=iv -chardev
> socket,path=/tmp/ivshmem_socket,id=iv
>
> and load the Windows driver in the guest.
>
> Maybe Linux won't enable MSI-X on the device?

Please work your reproducer into the commit message.  Make sure to note
that you're using "the Windows driver" (ideally with a pointer), and why
you assume the Linux driver doesn't trigger it.  With that, you can add
my

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] ivshmem: Improve MSI irqfd error handling

[Markus Armbruster]

Ladi Prosek <lprosek@redhat.com> writes:

> Adds a rollback path to ivshmem_enable_irqfd() and fixes
> ivshmem_disable_irqfd() to bail if irqfd has not been enabled.
>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>

Is this a theoretical bug, or can you trigger it?

Re: [Qemu-devel] [PATCH 3/3] ivshmem: Improve MSI irqfd error handling

[geoff]

On 2017-11-14 04:27, Markus Armbruster wrote:
> Ladi Prosek <lprosek@redhat.com> writes:
> 
>> Adds a rollback path to ivshmem_enable_irqfd() and fixes
>> ivshmem_disable_irqfd() to bail if irqfd has not been enabled.
>> 
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> 
> Is this a theoretical bug, or can you trigger it?

It is reproducible, I can trigger it by simply unloading the windows 
driver and then attempting to re-load it.

-Geoff

Re: [Qemu-devel] [PATCH 3/3] ivshmem: Improve MSI irqfd error handling

[Markus Armbruster]

geoff--- via Qemu-devel <qemu-devel@nongnu.org> writes:

> On 2017-11-14 04:27, Markus Armbruster wrote:
>> Ladi Prosek <lprosek@redhat.com> writes:
>>
>>> Adds a rollback path to ivshmem_enable_irqfd() and fixes
>>> ivshmem_disable_irqfd() to bail if irqfd has not been enabled.
>>>
>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>
>> Is this a theoretical bug, or can you trigger it?
>
> It is reproducible, I can trigger it by simply unloading the windows
> driver and then attempting to re-load it.

Adding that to the commit message would be nice.  You then can add my

Reviewed-by: Markus Armbruster <armbru@redhat.com>