[3/3] ivshmem: Improve MSI irqfd error handling

Commit Message

Ladi Prosek Nov. 10, 2017, 5:34 p.m. UTC

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 <[e-mail]>
---
 hw/misc/ivshmem.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

Comments

Markus Armbruster Nov. 13, 2017, 5:27 p.m. UTC | #1

Ladi Prosek <[e-mail]> 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 <[e-mail]>

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

Zhijian Li (Fujitsu)" via Nov. 13, 2017, 5:47 p.m. UTC | #2

On 2017-11-14 04:27, Markus Armbruster wrote:
> Ladi Prosek <[e-mail]> 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 <[e-mail]>
> 
> 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

Markus Armbruster Nov. 14, 2017, 7:37 a.m. UTC | #3

geoff--- via Qemu-devel <[e-mail]> writes:

> On 2017-11-14 04:27, Markus Armbruster wrote:
>> Ladi Prosek <[e-mail]> 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 <[e-mail]>
>>
>> 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 <[e-mail]>

Patch

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++) {