| Message ID | 20171110173421.17904-2-lprosek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
----- Original Message ----- > As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"), > QEMU crashes with: > > kvm_irqchip_commit_routes: Assertion `ret == 0' failed. > > if the ivshmem device is configured with more vectors than what the server > supports. This is caused by the ivshmem_vector_unmask() being called on > vectors that have not been initialized by ivshmem_add_kvm_msi_virq(). > > This commit fixes it by adding a simple check to the mask and unmask > callbacks. > > Note that the opposite mismatch, if the server supplies more vectors than > what the device is configured for, is already handled and leads to output > like: > > Too many eventfd received, device has 1 vectors > > Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications") > Signed-off-by: Ladi Prosek <lprosek@redhat.com> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/misc/ivshmem.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index a5a46827fe..6e46669744 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -317,6 +317,10 @@ static int ivshmem_vector_unmask(PCIDevice *dev, > unsigned vector, > int ret; > > IVSHMEM_DPRINTF("vector unmask %p %d\n", dev, vector); > + if (!v->pdev) { > + error_report("ivshmem: vector %d route does not exist", vector); > + return -EINVAL; > + } > > ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev); > if (ret < 0) { > @@ -331,12 +335,16 @@ static void ivshmem_vector_mask(PCIDevice *dev, > unsigned vector) > { > IVShmemState *s = IVSHMEM_COMMON(dev); > EventNotifier *n = &s->peers[s->vm_id].eventfds[vector]; > + MSIVector *v = &s->msi_vectors[vector]; > int ret; > > IVSHMEM_DPRINTF("vector mask %p %d\n", dev, vector); > + if (!v->pdev) { > + error_report("ivshmem: vector %d route does not exist", vector); > + return; > + } > > - ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, > - > s->msi_vectors[vector].virq); > + ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq); > if (ret != 0) { > error_report("remove_irqfd_notifier_gsi failed"); > } > -- > 2.13.5 > >
Thanks Ladi, I had not yet had time to dig into these, this patch set resolves all issues I was aware of. Tested-by: Geoffrey McRae <geoff@hostfission.com> On 2017-11-11 04:34, Ladi Prosek wrote: > As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi > notifications"), > QEMU crashes with: > > kvm_irqchip_commit_routes: Assertion `ret == 0' failed. > > if the ivshmem device is configured with more vectors than what the > server > supports. This is caused by the ivshmem_vector_unmask() being called on > vectors that have not been initialized by ivshmem_add_kvm_msi_virq(). > > This commit fixes it by adding a simple check to the mask and unmask > callbacks. > > Note that the opposite mismatch, if the server supplies more vectors > than > what the device is configured for, is already handled and leads to > output > like: > > Too many eventfd received, device has 1 vectors > > Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications") > Signed-off-by: Ladi Prosek <lprosek@redhat.com> > --- > hw/misc/ivshmem.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index a5a46827fe..6e46669744 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -317,6 +317,10 @@ static int ivshmem_vector_unmask(PCIDevice *dev, > unsigned vector, > int ret; > > IVSHMEM_DPRINTF("vector unmask %p %d\n", dev, vector); > + if (!v->pdev) { > + error_report("ivshmem: vector %d route does not exist", > vector); > + return -EINVAL; > + } > > ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev); > if (ret < 0) { > @@ -331,12 +335,16 @@ static void ivshmem_vector_mask(PCIDevice *dev, > unsigned vector) > { > IVShmemState *s = IVSHMEM_COMMON(dev); > EventNotifier *n = &s->peers[s->vm_id].eventfds[vector]; > + MSIVector *v = &s->msi_vectors[vector]; > int ret; > > IVSHMEM_DPRINTF("vector mask %p %d\n", dev, vector); > + if (!v->pdev) { > + error_report("ivshmem: vector %d route does not exist", > vector); > + return; > + } > > - ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, > - > s->msi_vectors[vector].virq); > + ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, > v->virq); > if (ret != 0) { > error_report("remove_irqfd_notifier_gsi failed"); > }
Ladi Prosek <lprosek@redhat.com> writes: > As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"), > QEMU crashes with: > > kvm_irqchip_commit_routes: Assertion `ret == 0' failed. > > if the ivshmem device is configured with more vectors than what the server > supports. This is caused by the ivshmem_vector_unmask() being called on > vectors that have not been initialized by ivshmem_add_kvm_msi_virq(). > > This commit fixes it by adding a simple check to the mask and unmask > callbacks. > > Note that the opposite mismatch, if the server supplies more vectors than > what the device is configured for, is already handled and leads to output > like: > > Too many eventfd received, device has 1 vectors > > Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications") > Signed-off-by: Ladi Prosek <lprosek@redhat.com> I think I understand your description of what's wrong. Not obvious to me is how it can happen. The cover letter mentions a Windows ivshmem driver. Is this a device bug a driver can trigger? If yes, how?
On Mon, Nov 13, 2017 at 3:22 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: >> >> kvm_irqchip_commit_routes: Assertion `ret == 0' failed. >> >> if the ivshmem device is configured with more vectors than what the server >> supports. This is caused by the ivshmem_vector_unmask() being called on >> vectors that have not been initialized by ivshmem_add_kvm_msi_virq(). >> >> This commit fixes it by adding a simple check to the mask and unmask >> callbacks. >> >> Note that the opposite mismatch, if the server supplies more vectors than >> what the device is configured for, is already handled and leads to output >> like: >> >> Too many eventfd received, device has 1 vectors >> >> Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications") >> Signed-off-by: Ladi Prosek <lprosek@redhat.com> > > I think I understand your description of what's wrong. Not obvious to > me is how it can happen. The cover letter mentions a Windows ivshmem > driver. Is this a device bug a driver can trigger? If yes, how? I don't have a Linux guest handy but this code has existed for quite a long time so yes, I think it's safe to assume that it can't be (easily) triggered by the Linux driver. The reproducer is as simple as: ivshmem-server -n 0 qemu ... -device ivshmem-doorbell,chardev=iv -chardev socket,path=/tmp/ivshmem_socket,id=iv and load the Windows driver in the guest. Maybe Linux won't enable MSI-X on the device? Thanks! Ladi
Ladi Prosek <lprosek@redhat.com> writes: > On Mon, Nov 13, 2017 at 3:22 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: >>> >>> kvm_irqchip_commit_routes: Assertion `ret == 0' failed. >>> >>> if the ivshmem device is configured with more vectors than what the server >>> supports. This is caused by the ivshmem_vector_unmask() being called on >>> vectors that have not been initialized by ivshmem_add_kvm_msi_virq(). >>> >>> This commit fixes it by adding a simple check to the mask and unmask >>> callbacks. >>> >>> Note that the opposite mismatch, if the server supplies more vectors than >>> what the device is configured for, is already handled and leads to output >>> like: >>> >>> Too many eventfd received, device has 1 vectors >>> >>> Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications") >>> Signed-off-by: Ladi Prosek <lprosek@redhat.com> >> >> I think I understand your description of what's wrong. Not obvious to >> me is how it can happen. The cover letter mentions a Windows ivshmem >> driver. Is this a device bug a driver can trigger? If yes, how? > > I don't have a Linux guest handy but this code has existed for quite a > long time so yes, I think it's safe to assume that it can't be > (easily) triggered by the Linux driver. > > The reproducer is as simple as: > > ivshmem-server -n 0 > qemu ... -device ivshmem-doorbell,chardev=iv -chardev > socket,path=/tmp/ivshmem_socket,id=iv > > and load the Windows driver in the guest. > > Maybe Linux won't enable MSI-X on the device? Please work your reproducer into the commit message. Make sure to note that you're using "the Windows driver" (ideally with a pointer), and why you assume the Linux driver doesn't trigger it. With that, you can add my Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index a5a46827fe..6e46669744 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -317,6 +317,10 @@ static int ivshmem_vector_unmask(PCIDevice *dev, unsigned vector, int ret; IVSHMEM_DPRINTF("vector unmask %p %d\n", dev, vector); + if (!v->pdev) { + error_report("ivshmem: vector %d route does not exist", vector); + return -EINVAL; + } ret = kvm_irqchip_update_msi_route(kvm_state, v->virq, msg, dev); if (ret < 0) { @@ -331,12 +335,16 @@ static void ivshmem_vector_mask(PCIDevice *dev, unsigned vector) { IVShmemState *s = IVSHMEM_COMMON(dev); EventNotifier *n = &s->peers[s->vm_id].eventfds[vector]; + MSIVector *v = &s->msi_vectors[vector]; int ret; IVSHMEM_DPRINTF("vector mask %p %d\n", dev, vector); + if (!v->pdev) { + error_report("ivshmem: vector %d route does not exist", vector); + return; + } - ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, - s->msi_vectors[vector].virq); + ret = kvm_irqchip_remove_irqfd_notifier_gsi(kvm_state, n, v->virq); if (ret != 0) { error_report("remove_irqfd_notifier_gsi failed"); }
As of commit 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications"), QEMU crashes with: kvm_irqchip_commit_routes: Assertion `ret == 0' failed. if the ivshmem device is configured with more vectors than what the server supports. This is caused by the ivshmem_vector_unmask() being called on vectors that have not been initialized by ivshmem_add_kvm_msi_virq(). This commit fixes it by adding a simple check to the mask and unmask callbacks. Note that the opposite mismatch, if the server supplies more vectors than what the device is configured for, is already handled and leads to output like: Too many eventfd received, device has 1 vectors Fixes: 660c97eef6f8 ("ivshmem: use kvm irqfd for msi notifications") Signed-off-by: Ladi Prosek <lprosek@redhat.com> --- hw/misc/ivshmem.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)