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

[Qemu-devel] [PATCH v3 0/4] 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

v1->v2:
* Patch 1 - added reproducer info to commit message (Markus)
* Patch 2 - restructured conditionals, fixed comment formatting (Markus)
* Patch 3 - added reproducer info to commit message (Markus)

v2->v3:
* Added patch 4

Ladi Prosek (4):
  ivshmem: Don't update non-existent MSI routes
  ivshmem: Always remove irqfd notifiers
  ivshmem: Improve MSI irqfd error handling
  ivshmem: Disable irqfd on device reset

 hw/misc/ivshmem.c | 101 ++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 71 insertions(+), 30 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v3 1/4] 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

To reproduce the assert, run:

  ivshmem-server -n 0

and QEMU with:

  -device ivshmem-doorbell,chardev=iv
  -chardev socket,path=/tmp/ivshmem_socket,id=iv

then load the Windows driver, at the time of writing available at:

https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem

The issue is believed to have been masked by other guest drivers, notably
Linux ones, not enabling MSI-X on the device.

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>
Reviewed-by: Markus Armbruster <armbru@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.6

[Qemu-devel] [PATCH v3 2/4] 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/ivshmem.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 6e46669744..91364d8364 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,13 @@ 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) {
+        return ret;
+    }
+    v->unmasked = true;
+
+    return 0;
 }
 
 static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector)
@@ -343,11 +351,14 @@ 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) {
         error_report("remove_irqfd_notifier_gsi failed");
+        return;
     }
+    v->unmasked = false;
 }
 
 static void ivshmem_vector_poll(PCIDevice *dev,
@@ -817,11 +828,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.6

[Qemu-devel] [PATCH v3 3/4] 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.

To reproduce, run:

  ivshmem-server -n 0

and QEMU with:

  -device ivshmem-doorbell,chardev=iv
  -chardev socket,path=/tmp/ivshmem_socket,id=iv

then load, unload, and load again the Windows driver, at the time of writing
available at:

https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem

The issue is believed to have been masked by other guest drivers, notably
Linux ones, not enabling MSI-X on the device.

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Reviewed-by: Markus Armbruster <armbru@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 91364d8364..d1bb246d12 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -786,6 +786,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);
@@ -797,7 +811,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;
         }
     }
 
@@ -806,21 +820,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)
@@ -828,6 +835,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.6

[Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset

[Ladi Prosek]

The effects of ivshmem_enable_irqfd() was not undone on device reset.

This manifested as:
ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.

when irqfd was enabled before reset and then enabled again after reset, making
ivshmem_enable_irqfd() run for the second time.

To reproduce, run:

  ivshmem-server

and QEMU with:

  -device ivshmem-doorbell,chardev=iv
  -chardev socket,path=/tmp/ivshmem_socket,id=iv

then install the Windows driver, at the time of writing available at:

https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem

and crash-reboot the guest by inducing a BSOD.

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

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index d1bb246d12..4be0d2627b 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
     }
 }
 
-static void ivshmem_reset(DeviceState *d)
-{
-    IVShmemState *s = IVSHMEM_COMMON(d);
-
-    s->intrstatus = 0;
-    s->intrmask = 0;
-    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
-        ivshmem_msix_vector_use(s);
-    }
-}
-
 static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
 {
     /* allocate QEMU callback data for receiving interrupts */
@@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
 
 }
 
+static void ivshmem_reset(DeviceState *d)
+{
+    IVShmemState *s = IVSHMEM_COMMON(d);
+
+    ivshmem_disable_irqfd(s);
+
+    s->intrstatus = 0;
+    s->intrmask = 0;
+    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
+        ivshmem_msix_vector_use(s);
+    }
+}
+
 static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
                                  uint32_t val, int len)
 {
-- 
2.13.6

Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset

[Markus Armbruster]

Ladi Prosek <lprosek@redhat.com> writes:

> The effects of ivshmem_enable_irqfd() was not undone on device reset.
>
> This manifested as:
> ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.
>
> when irqfd was enabled before reset and then enabled again after reset, making
> ivshmem_enable_irqfd() run for the second time.
>
> To reproduce, run:
>
>   ivshmem-server
>
> and QEMU with:
>
>   -device ivshmem-doorbell,chardev=iv
>   -chardev socket,path=/tmp/ivshmem_socket,id=iv
>
> then install the Windows driver, at the time of writing available at:
>
> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem
>
> and crash-reboot the guest by inducing a BSOD.
>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>  hw/misc/ivshmem.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index d1bb246d12..4be0d2627b 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
>      }
>  }
>  
> -static void ivshmem_reset(DeviceState *d)
> -{
> -    IVShmemState *s = IVSHMEM_COMMON(d);
> -
> -    s->intrstatus = 0;
> -    s->intrmask = 0;
> -    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> -        ivshmem_msix_vector_use(s);
> -    }
> -}
> -
>  static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>  {
>      /* allocate QEMU callback data for receiving interrupts */
> @@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>  
>  }
>  
> +static void ivshmem_reset(DeviceState *d)
> +{
> +    IVShmemState *s = IVSHMEM_COMMON(d);
> +
> +    ivshmem_disable_irqfd(s);
> +
> +    s->intrstatus = 0;
> +    s->intrmask = 0;
> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> +        ivshmem_msix_vector_use(s);
> +    }
> +}
> +
>  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>                                   uint32_t val, int len)
>  {

Why are you moving ivshmem_reset()?  Makes the actual change harder to
see than necessary.

Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset

[Ladi Prosek]

On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Ladi Prosek <lprosek@redhat.com> writes:
>
>> The effects of ivshmem_enable_irqfd() was not undone on device reset.
>>
>> This manifested as:
>> ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.
>>
>> when irqfd was enabled before reset and then enabled again after reset, making
>> ivshmem_enable_irqfd() run for the second time.
>>
>> To reproduce, run:
>>
>>   ivshmem-server
>>
>> and QEMU with:
>>
>>   -device ivshmem-doorbell,chardev=iv
>>   -chardev socket,path=/tmp/ivshmem_socket,id=iv
>>
>> then install the Windows driver, at the time of writing available at:
>>
>> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem
>>
>> and crash-reboot the guest by inducing a BSOD.
>>
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> ---
>>  hw/misc/ivshmem.c | 24 +++++++++++++-----------
>>  1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>> index d1bb246d12..4be0d2627b 100644
>> --- a/hw/misc/ivshmem.c
>> +++ b/hw/misc/ivshmem.c
>> @@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
>>      }
>>  }
>>
>> -static void ivshmem_reset(DeviceState *d)
>> -{
>> -    IVShmemState *s = IVSHMEM_COMMON(d);
>> -
>> -    s->intrstatus = 0;
>> -    s->intrmask = 0;
>> -    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>> -        ivshmem_msix_vector_use(s);
>> -    }
>> -}
>> -
>>  static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>  {
>>      /* allocate QEMU callback data for receiving interrupts */
>> @@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>>
>>  }
>>
>> +static void ivshmem_reset(DeviceState *d)
>> +{
>> +    IVShmemState *s = IVSHMEM_COMMON(d);
>> +
>> +    ivshmem_disable_irqfd(s);
>> +
>> +    s->intrstatus = 0;
>> +    s->intrmask = 0;
>> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>> +        ivshmem_msix_vector_use(s);
>> +    }
>> +}
>> +
>>  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>>                                   uint32_t val, int len)
>>  {
>
> Why are you moving ivshmem_reset()?  Makes the actual change harder to
> see than necessary.

ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
source file. I generally prefer to order static functions
topologically if possible. If you'd prefer adding a forward decl
instead (fewer lines touched, easier to bisect?) I can certainly do
that. Thanks!

Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On 12/08/2017 07:44 AM, Ladi Prosek wrote:

>>>  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>>>                                   uint32_t val, int len)
>>>  {
>>
>> Why are you moving ivshmem_reset()?  Makes the actual change harder to
>> see than necessary.
> 
> ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
> source file. I generally prefer to order static functions
> topologically if possible. If you'd prefer adding a forward decl
> instead (fewer lines touched, easier to bisect?) I can certainly do
> that. Thanks!

That, or split it into two patches (one doing just the code motion, the
other making the semantic change).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset

[Markus Armbruster]

Ladi Prosek <lprosek@redhat.com> writes:

> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Ladi Prosek <lprosek@redhat.com> writes:
>>
>>> The effects of ivshmem_enable_irqfd() was not undone on device reset.
>>>
>>> This manifested as:
>>> ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.
>>>
>>> when irqfd was enabled before reset and then enabled again after reset, making
>>> ivshmem_enable_irqfd() run for the second time.
>>>
>>> To reproduce, run:
>>>
>>>   ivshmem-server
>>>
>>> and QEMU with:
>>>
>>>   -device ivshmem-doorbell,chardev=iv
>>>   -chardev socket,path=/tmp/ivshmem_socket,id=iv
>>>
>>> then install the Windows driver, at the time of writing available at:
>>>
>>> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem
>>>
>>> and crash-reboot the guest by inducing a BSOD.
>>>
>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>> ---
>>>  hw/misc/ivshmem.c | 24 +++++++++++++-----------
>>>  1 file changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>> index d1bb246d12..4be0d2627b 100644
>>> --- a/hw/misc/ivshmem.c
>>> +++ b/hw/misc/ivshmem.c
>>> @@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
>>>      }
>>>  }
>>>
>>> -static void ivshmem_reset(DeviceState *d)
>>> -{
>>> -    IVShmemState *s = IVSHMEM_COMMON(d);
>>> -
>>> -    s->intrstatus = 0;
>>> -    s->intrmask = 0;
>>> -    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>> -        ivshmem_msix_vector_use(s);
>>> -    }
>>> -}
>>> -
>>>  static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>>  {
>>>      /* allocate QEMU callback data for receiving interrupts */
>>> @@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>>>
>>>  }
>>>
>>> +static void ivshmem_reset(DeviceState *d)
>>> +{
>>> +    IVShmemState *s = IVSHMEM_COMMON(d);
>>> +
>>> +    ivshmem_disable_irqfd(s);
>>> +
>>> +    s->intrstatus = 0;
>>> +    s->intrmask = 0;
>>> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>> +        ivshmem_msix_vector_use(s);
>>> +    }
>>> +}
>>> +
>>>  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>>>                                   uint32_t val, int len)
>>>  {
>>
>> Why are you moving ivshmem_reset()?  Makes the actual change harder to
>> see than necessary.
>
> ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
> source file. I generally prefer to order static functions
> topologically if possible. If you'd prefer adding a forward decl
> instead (fewer lines touched, easier to bisect?) I can certainly do
> that. Thanks!

Well, it compiles before your patch, your patch doesn't add any calls,
so I can't quite see why a forward declaration would be needed.

Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset

[Ladi Prosek]

On Fri, Dec 8, 2017 at 6:28 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Ladi Prosek <lprosek@redhat.com> writes:
>
>> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Ladi Prosek <lprosek@redhat.com> writes:
>>>
>>>> The effects of ivshmem_enable_irqfd() was not undone on device reset.
>>>>
>>>> This manifested as:
>>>> ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.
>>>>
>>>> when irqfd was enabled before reset and then enabled again after reset, making
>>>> ivshmem_enable_irqfd() run for the second time.
>>>>
>>>> To reproduce, run:
>>>>
>>>>   ivshmem-server
>>>>
>>>> and QEMU with:
>>>>
>>>>   -device ivshmem-doorbell,chardev=iv
>>>>   -chardev socket,path=/tmp/ivshmem_socket,id=iv
>>>>
>>>> then install the Windows driver, at the time of writing available at:
>>>>
>>>> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem
>>>>
>>>> and crash-reboot the guest by inducing a BSOD.
>>>>
>>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>>> ---
>>>>  hw/misc/ivshmem.c | 24 +++++++++++++-----------
>>>>  1 file changed, 13 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>>> index d1bb246d12..4be0d2627b 100644
>>>> --- a/hw/misc/ivshmem.c
>>>> +++ b/hw/misc/ivshmem.c
>>>> @@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
>>>>      }
>>>>  }
>>>>
>>>> -static void ivshmem_reset(DeviceState *d)
>>>> -{
>>>> -    IVShmemState *s = IVSHMEM_COMMON(d);
>>>> -
>>>> -    s->intrstatus = 0;
>>>> -    s->intrmask = 0;
>>>> -    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>>> -        ivshmem_msix_vector_use(s);
>>>> -    }
>>>> -}
>>>> -
>>>>  static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>>>  {
>>>>      /* allocate QEMU callback data for receiving interrupts */
>>>> @@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>>>>
>>>>  }
>>>>
>>>> +static void ivshmem_reset(DeviceState *d)
>>>> +{
>>>> +    IVShmemState *s = IVSHMEM_COMMON(d);
>>>> +
>>>> +    ivshmem_disable_irqfd(s);
>>>> +
>>>> +    s->intrstatus = 0;
>>>> +    s->intrmask = 0;
>>>> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>>> +        ivshmem_msix_vector_use(s);
>>>> +    }
>>>> +}
>>>> +
>>>>  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>>>>                                   uint32_t val, int len)
>>>>  {
>>>
>>> Why are you moving ivshmem_reset()?  Makes the actual change harder to
>>> see than necessary.
>>
>> ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
>> source file. I generally prefer to order static functions
>> topologically if possible. If you'd prefer adding a forward decl
>> instead (fewer lines touched, easier to bisect?) I can certainly do
>> that. Thanks!
>
> Well, it compiles before your patch, your patch doesn't add any calls,
> so I can't quite see why a forward declaration would be needed.

It adds a call to ivshmem_disable_irqfd().

Re: [Qemu-devel] [PATCH v3 4/4] ivshmem: Disable irqfd on device reset

[Markus Armbruster]

Ladi Prosek <lprosek@redhat.com> writes:

> On Fri, Dec 8, 2017 at 6:28 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Ladi Prosek <lprosek@redhat.com> writes:
>>
>>> On Fri, Dec 8, 2017 at 2:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Ladi Prosek <lprosek@redhat.com> writes:
>>>>
>>>>> The effects of ivshmem_enable_irqfd() was not undone on device reset.
>>>>>
>>>>> This manifested as:
>>>>> ivshmem_add_kvm_msi_virq: Assertion `!s->msi_vectors[vector].pdev' failed.
>>>>>
>>>>> when irqfd was enabled before reset and then enabled again after reset, making
>>>>> ivshmem_enable_irqfd() run for the second time.
>>>>>
>>>>> To reproduce, run:
>>>>>
>>>>>   ivshmem-server
>>>>>
>>>>> and QEMU with:
>>>>>
>>>>>   -device ivshmem-doorbell,chardev=iv
>>>>>   -chardev socket,path=/tmp/ivshmem_socket,id=iv
>>>>>
>>>>> then install the Windows driver, at the time of writing available at:
>>>>>
>>>>> https://github.com/virtio-win/kvm-guest-drivers-windows/tree/master/ivshmem
>>>>>
>>>>> and crash-reboot the guest by inducing a BSOD.
>>>>>
>>>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>>>> ---
>>>>>  hw/misc/ivshmem.c | 24 +++++++++++++-----------
>>>>>  1 file changed, 13 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
>>>>> index d1bb246d12..4be0d2627b 100644
>>>>> --- a/hw/misc/ivshmem.c
>>>>> +++ b/hw/misc/ivshmem.c
>>>>> @@ -758,17 +758,6 @@ static void ivshmem_msix_vector_use(IVShmemState *s)
>>>>>      }
>>>>>  }
>>>>>
>>>>> -static void ivshmem_reset(DeviceState *d)
>>>>> -{
>>>>> -    IVShmemState *s = IVSHMEM_COMMON(d);
>>>>> -
>>>>> -    s->intrstatus = 0;
>>>>> -    s->intrmask = 0;
>>>>> -    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>>>> -        ivshmem_msix_vector_use(s);
>>>>> -    }
>>>>> -}
>>>>> -
>>>>>  static int ivshmem_setup_interrupts(IVShmemState *s, Error **errp)
>>>>>  {
>>>>>      /* allocate QEMU callback data for receiving interrupts */
>>>>> @@ -855,6 +844,19 @@ static void ivshmem_disable_irqfd(IVShmemState *s)
>>>>>
>>>>>  }
>>>>>
>>>>> +static void ivshmem_reset(DeviceState *d)
>>>>> +{
>>>>> +    IVShmemState *s = IVSHMEM_COMMON(d);
>>>>> +
>>>>> +    ivshmem_disable_irqfd(s);
>>>>> +
>>>>> +    s->intrstatus = 0;
>>>>> +    s->intrmask = 0;
>>>>> +    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
>>>>> +        ivshmem_msix_vector_use(s);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>>>>>                                   uint32_t val, int len)
>>>>>  {
>>>>
>>>> Why are you moving ivshmem_reset()?  Makes the actual change harder to
>>>> see than necessary.
>>>
>>> ivshmem_disable_irqfd() is declared after ivshmem_reset() in the
>>> source file. I generally prefer to order static functions
>>> topologically if possible. If you'd prefer adding a forward decl
>>> instead (fewer lines touched, easier to bisect?) I can certainly do
>>> that. Thanks!
>>
>> Well, it compiles before your patch, your patch doesn't add any calls,
>> so I can't quite see why a forward declaration would be needed.
>
> It adds a call to ivshmem_disable_irqfd().

Note to self: patch review on Friday late afternoon is a bad idea.

I'd prefer the forward declaration.  Thanks!