| Message ID | 20171110173421.17904-3-lprosek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
On Mon, Nov 13, 2017 at 3:36 PM, Markus Armbruster <armbru@redhat.com> wrote: > 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> Thank you, I'll address your comments in v2.
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"); } } @@ -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. + */ + 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,
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(-)