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

[Ladi Prosek]

On Mon, Nov 13, 2017 at 3:36 PM, Markus Armbruster <address@hidden> wrote:
> Ladi Prosek <address@hidden> 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 <address@hidden>
>> ---
>>  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 <address@hidden>

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