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

[Markus Armbruster <armbru@redhat.com>]

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>