* Re: [PATCH] vfio/pci: Don't remove irqchip notifier if not registered
2019-12-31 13:39 [PATCH] vfio/pci: Don't remove irqchip notifier if not registered Peter Xu
@ 2020-01-02 2:19 ` David Gibson
2020-01-06 9:17 ` Greg Kurz
2020-01-06 22:13 ` Alex Williamson
2 siblings, 0 replies; 5+ messages in thread
From: David Gibson @ 2020-01-02 2:19 UTC (permalink / raw)
To: Peter Xu; +Cc: yanghliu, Alex Williamson, qemu-devel, Eduardo Habkost
[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] vfio/pci: Don't remove irqchip notifier if not registered
2019-12-31 13:39 [PATCH] vfio/pci: Don't remove irqchip notifier if not registered Peter Xu
2020-01-02 2:19 ` David Gibson
@ 2020-01-06 9:17 ` Greg Kurz
2020-01-06 22:13 ` Alex Williamson
2 siblings, 0 replies; 5+ messages in thread
From: Greg Kurz @ 2020-01-06 9:17 UTC (permalink / raw)
To: Peter Xu
Cc: Eduardo Habkost, qemu-devel, qemu-stable, Alex Williamson,
yanghliu, David Gibson
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);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] vfio/pci: Don't remove irqchip notifier if not registered
2019-12-31 13:39 [PATCH] vfio/pci: Don't remove irqchip notifier if not registered Peter Xu
2020-01-02 2:19 ` David Gibson
2020-01-06 9:17 ` Greg Kurz
@ 2020-01-06 22:13 ` Alex Williamson
2020-01-07 13:33 ` Peter Xu
2 siblings, 1 reply; 5+ messages in thread
From: Alex Williamson @ 2020-01-06 22:13 UTC (permalink / raw)
To: Peter Xu; +Cc: yanghliu, Greg Kurz, David Gibson, qemu-devel, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] vfio/pci: Don't remove irqchip notifier if not registered
2020-01-06 22:13 ` Alex Williamson
@ 2020-01-07 13:33 ` Peter Xu
0 siblings, 0 replies; 5+ messages in thread
From: Peter Xu @ 2020-01-07 13:33 UTC (permalink / raw)
To: Alex Williamson
Cc: yanghliu, Greg Kurz, David Gibson, qemu-devel, Eduardo Habkost
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
^ permalink raw reply [flat|nested] 5+ messages in thread