1. Patchew
  2. QEMU
  3. View series

[PATCH] vfio/pci: Don't remove irqchip notifier if not registered

Peter Xu posted 1 patch 4 years, 4 months ago
Test asan failed
Test checkpatch failed
Test FreeBSD failed
Test docker-mingw@fedora failed
Test docker-clang@ubuntu failed
Test docker-quick@centos7 failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191231133915.115363-1-peterx@redhat.com
Maintainers: Alex Williamson <alex.williamson@redhat.com>
hw/vfio/pci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[PATCH] vfio/pci: Don't remove irqchip notifier if not registered
Posted by Peter Xu 4 years, 4 months ago 
The kvm irqchip notifier is only registered if the device supports
INTx, however it's unconditionally removed.  If the assigned device
does not support INTx, this will cause QEMU to crash when unplugging
the device from the system.  Change it to conditionally remove the
notifier only if the notify hook is setup.

CC: Eduardo Habkost <ehabkost@redhat.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Alex Williamson <alex.williamson@redhat.com>
Reported-by: yanghliu@redhat.com
Fixes: c5478fea27 ("vfio/pci: Respond to KVM irqchip change notifier", 2019-11-26)
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1782678
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/vfio/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 2d40b396f2..337a173ce7 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3076,7 +3076,9 @@ static void vfio_exitfn(PCIDevice *pdev)
     vfio_unregister_req_notifier(vdev);
     vfio_unregister_err_notifier(vdev);
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
-    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
+    if (vdev->irqchip_change_notifier.notify) {
+        kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
+    }
     vfio_disable_interrupts(vdev);
     if (vdev->intx.mmap_timer) {
         timer_free(vdev->intx.mmap_timer);
-- 
2.24.1
Re: [PATCH] vfio/pci: Don't remove irqchip notifier if not registered
Posted by David Gibson 4 years, 4 months ago 
On Tue, Dec 31, 2019 at 08:39:15AM -0500, Peter Xu wrote:
> The kvm irqchip notifier is only registered if the device supports
> INTx, however it's unconditionally removed.  If the assigned device
> does not support INTx, this will cause QEMU to crash when unplugging
> the device from the system.  Change it to conditionally remove the
> notifier only if the notify hook is setup.
> 
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Alex Williamson <alex.williamson@redhat.com>
> Reported-by: yanghliu@redhat.com
> Fixes: c5478fea27 ("vfio/pci: Respond to KVM irqchip change notifier", 2019-11-26)
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1782678
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Mea culpa.

> ---
>  hw/vfio/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 2d40b396f2..337a173ce7 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3076,7 +3076,9 @@ static void vfio_exitfn(PCIDevice *pdev)
>      vfio_unregister_req_notifier(vdev);
>      vfio_unregister_err_notifier(vdev);
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
> -    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +    if (vdev->irqchip_change_notifier.notify) {
> +        kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +    }
>      vfio_disable_interrupts(vdev);
>      if (vdev->intx.mmap_timer) {
>          timer_free(vdev->intx.mmap_timer);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [PATCH] vfio/pci: Don't remove irqchip notifier if not registered
Posted by Greg Kurz 4 years, 4 months ago 
On Tue, 31 Dec 2019 08:39:15 -0500
Peter Xu <peterx@redhat.com> wrote:

> The kvm irqchip notifier is only registered if the device supports
> INTx, however it's unconditionally removed.  If the assigned device
> does not support INTx, this will cause QEMU to crash when unplugging
> the device from the system.  Change it to conditionally remove the
> notifier only if the notify hook is setup.
> 
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Alex Williamson <alex.williamson@redhat.com>
> Reported-by: yanghliu@redhat.com
> Fixes: c5478fea27 ("vfio/pci: Respond to KVM irqchip change notifier", 2019-11-26)
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1782678
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Good catch... sorry for missing this during review :-\

Cc'ing stable since we certainly want this fix in 4.2.1 as well.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/vfio/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 2d40b396f2..337a173ce7 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3076,7 +3076,9 @@ static void vfio_exitfn(PCIDevice *pdev)
>      vfio_unregister_req_notifier(vdev);
>      vfio_unregister_err_notifier(vdev);
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
> -    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +    if (vdev->irqchip_change_notifier.notify) {
> +        kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +    }
>      vfio_disable_interrupts(vdev);
>      if (vdev->intx.mmap_timer) {
>          timer_free(vdev->intx.mmap_timer);
Re: [PATCH] vfio/pci: Don't remove irqchip notifier if not registered
Posted by Alex Williamson 4 years, 4 months ago 
On Tue, 31 Dec 2019 08:39:15 -0500
Peter Xu <peterx@redhat.com> wrote:

> The kvm irqchip notifier is only registered if the device supports
> INTx, however it's unconditionally removed.  If the assigned device
> does not support INTx, this will cause QEMU to crash when unplugging
> the device from the system.  Change it to conditionally remove the
> notifier only if the notify hook is setup.
> 
> CC: Eduardo Habkost <ehabkost@redhat.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Alex Williamson <alex.williamson@redhat.com>
> Reported-by: yanghliu@redhat.com
> Fixes: c5478fea27 ("vfio/pci: Respond to KVM irqchip change notifier", 2019-11-26)
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1782678
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/vfio/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 2d40b396f2..337a173ce7 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3076,7 +3076,9 @@ static void vfio_exitfn(PCIDevice *pdev)
>      vfio_unregister_req_notifier(vdev);
>      vfio_unregister_err_notifier(vdev);
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
> -    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +    if (vdev->irqchip_change_notifier.notify) {
> +        kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> +    }
>      vfio_disable_interrupts(vdev);
>      if (vdev->intx.mmap_timer) {
>          timer_free(vdev->intx.mmap_timer);


Thanks, Peter!  Sent a pull request with David and Greg's R-b, stable
tag, and debug credit to Eduardo as I think he spotted the issue first.
Thanks,

Alex
Re: [PATCH] vfio/pci: Don't remove irqchip notifier if not registered
Posted by Peter Xu 4 years, 4 months ago 
On Mon, Jan 06, 2020 at 03:13:24PM -0700, Alex Williamson wrote:
> On Tue, 31 Dec 2019 08:39:15 -0500
> Peter Xu <peterx@redhat.com> wrote:
> 
> > The kvm irqchip notifier is only registered if the device supports
> > INTx, however it's unconditionally removed.  If the assigned device
> > does not support INTx, this will cause QEMU to crash when unplugging
> > the device from the system.  Change it to conditionally remove the
> > notifier only if the notify hook is setup.
> > 
> > CC: Eduardo Habkost <ehabkost@redhat.com>
> > CC: David Gibson <david@gibson.dropbear.id.au>
> > CC: Alex Williamson <alex.williamson@redhat.com>
> > Reported-by: yanghliu@redhat.com
> > Fixes: c5478fea27 ("vfio/pci: Respond to KVM irqchip change notifier", 2019-11-26)
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1782678
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/vfio/pci.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 2d40b396f2..337a173ce7 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -3076,7 +3076,9 @@ static void vfio_exitfn(PCIDevice *pdev)
> >      vfio_unregister_req_notifier(vdev);
> >      vfio_unregister_err_notifier(vdev);
> >      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
> > -    kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> > +    if (vdev->irqchip_change_notifier.notify) {
> > +        kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
> > +    }
> >      vfio_disable_interrupts(vdev);
> >      if (vdev->intx.mmap_timer) {
> >          timer_free(vdev->intx.mmap_timer);
> 
> 
> Thanks, Peter!  Sent a pull request with David and Greg's R-b, stable
> tag, and debug credit to Eduardo as I think he spotted the issue first.
> Thanks,

Yes thanks!  I wished Eduardo had even updated the bz when he debugged
so I won't be needinfo-ed and did it twice without notice :)

-- 
Peter Xu

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.