Ladi Prosek <lpro...@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 <lpro...@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 <arm...@redhat.com>
