| Message ID | 20230621080204.420723-3-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VFIO migration related refactor and bug fix | expand |
On 21/06/2023 09:02, Zhenzhong Duan wrote: > In case irqchip_change_notifier isn't added, removing it triggers segfault. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.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 c71b0955d81c..82c4cf4f7609 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -3222,7 +3222,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > > out_deregister: > 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); > + } If the first patch ends up being pursued (which I am not quite sure) it should be folded in the previous patch, as the out_deregister is used starting your patch 1. > out_teardown: > vfio_teardown_msi(vdev); > vfio_bars_exit(vdev);
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Sent: Wednesday, June 21, 2023 7:09 PM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >Cc: alex.williamson@redhat.com; clg@redhat.com; qemu-devel@nongnu.org; >avihaih@nvidia.com; Peng, Chao P <chao.p.peng@intel.com> >Subject: Re: [PATCH v3 2/3] vfio/pci: Fix a segfault in vfio_realize > > > >On 21/06/2023 09:02, Zhenzhong Duan wrote: >> In case irqchip_change_notifier isn't added, removing it triggers segfault. >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.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 >> c71b0955d81c..82c4cf4f7609 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -3222,7 +3222,9 @@ static void vfio_realize(PCIDevice *pdev, Error >> **errp) >> >> out_deregister: >> 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); >> + } > >If the first patch ends up being pursued (which I am not quite sure) it should >be folded in the previous patch, as the out_deregister is used starting your >patch 1. Sorry for late response, just back from vacation. out_deregister isn't only for vfio migration, there are some other jump sites to out_deregister in vfio_realize. Take below code for example: if (vdev->display_xres || vdev->display_yres) { if (vdev->dpy == NULL) { error_setg(errp, "xres and yres properties require display=on"); goto out_deregister; } I can reproduce a segmentation fault when hotplug a vfio device using below cmd: (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,xres=1 Connection closed by foreign host. After fix: (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,xres=1 Error: vfio 0000:81:11.1: xres and yres properties require display=on (qemu) Thanks Zhenzhong
On 25/06/2023 07:01, Duan, Zhenzhong wrote: >> -----Original Message----- >> From: Joao Martins <joao.m.martins@oracle.com> >> Sent: Wednesday, June 21, 2023 7:09 PM >> To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> Cc: alex.williamson@redhat.com; clg@redhat.com; qemu-devel@nongnu.org; >> avihaih@nvidia.com; Peng, Chao P <chao.p.peng@intel.com> >> Subject: Re: [PATCH v3 2/3] vfio/pci: Fix a segfault in vfio_realize >> >> >> >> On 21/06/2023 09:02, Zhenzhong Duan wrote: >>> In case irqchip_change_notifier isn't added, removing it triggers segfault. >>> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.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 >>> c71b0955d81c..82c4cf4f7609 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -3222,7 +3222,9 @@ static void vfio_realize(PCIDevice *pdev, Error >>> **errp) >>> >>> out_deregister: >>> 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); >>> + } >> >> If the first patch ends up being pursued (which I am not quite sure) it should >> be folded in the previous patch, as the out_deregister is used starting your >> patch 1. > Sorry for late response, just back from vacation. > > out_deregister isn't only for vfio migration, there are some other jump sites to out_deregister in vfio_realize. Take below code for example: > > if (vdev->display_xres || vdev->display_yres) { > if (vdev->dpy == NULL) { > error_setg(errp, "xres and yres properties require display=on"); > goto out_deregister; > } > > I can reproduce a segmentation fault when hotplug a vfio device using below cmd: > (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,xres=1 Connection closed by foreign host. > > After fix: > (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,xres=1 > Error: vfio 0000:81:11.1: xres and yres properties require display=on > (qemu) > Makes sense. Let's keep it separate then. > Thanks > Zhenzhong > >
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index c71b0955d81c..82c4cf4f7609 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -3222,7 +3222,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) out_deregister: 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); + } out_teardown: vfio_teardown_msi(vdev); vfio_bars_exit(vdev);
In case irqchip_change_notifier isn't added, removing it triggers segfault. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/vfio/pci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)