On Mon, Nov 13, 2017 at 3:36 PM, Markus Armbruster <arm...@redhat.com> wrote:
> 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>
Thank you, I'll address your comments in v2.
