| Message ID | 20230621080204.420723-2-zhenzhong.duan@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | VFIO migration related refactor and bug fix | expand |
On 21/06/2023 09:02, Zhenzhong Duan wrote: > When adding migration blocker failed in vfio_migration_realize(), > hotplug will fail and we see below: > > (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,x-enable-migration=true > 0000:81:11.1: VFIO migration is not supported in kernel > Error: disallowing migration blocker (--only-migratable) for: VFIO device doesn't support migration > > If we hotplug again we should see same log as above, but we see: > (qemu) device_add vfio-pci,host=81:11.0,id=vfio0,bus=root0,x-enable-migration=true > Error: vfio 0000:81:11.0: device is already attached > > That's because some references to VFIO device isn't released, > we should check return value of vfio_migration_realize() and > release the references, then VFIO device will be truely > released when hotplug failed. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/vfio/pci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 73874a94de12..c71b0955d81c 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -3210,6 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > ret = vfio_migration_realize(vbasedev, errp); > if (ret) { > error_report("%s: Migration disabled", vbasedev->name); > + goto out_deregister; > } > } > This doesn't look right. This means that failure to support migration will deregister the device. Migration "realize" should not condition as to whether your device finishes the realize. Maybe the fix is elsewhere?
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Sent: Wednesday, June 21, 2023 7:08 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 1/3] vfio/pci: Fix resource leak in vfio_realize > > > >On 21/06/2023 09:02, Zhenzhong Duan wrote: >> When adding migration blocker failed in vfio_migration_realize(), >> hotplug will fail and we see below: >> >> (qemu) device_add >> vfio-pci,host=81:11.1,id=vfio1,bus=root1,x-enable-migration=true >> 0000:81:11.1: VFIO migration is not supported in kernel >> Error: disallowing migration blocker (--only-migratable) for: VFIO >> device doesn't support migration >> >> If we hotplug again we should see same log as above, but we see: >> (qemu) device_add >> vfio-pci,host=81:11.0,id=vfio0,bus=root0,x-enable-migration=true >> Error: vfio 0000:81:11.0: device is already attached >> >> That's because some references to VFIO device isn't released, we >> should check return value of vfio_migration_realize() and release the >> references, then VFIO device will be truely released when hotplug >> failed. >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> hw/vfio/pci.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index >> 73874a94de12..c71b0955d81c 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -3210,6 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> ret = vfio_migration_realize(vbasedev, errp); >> if (ret) { >> error_report("%s: Migration disabled", vbasedev->name); >> + goto out_deregister; >> } >> } >> >This doesn't look right. This means that failure to support migration will >deregister the device. In my understanding, failure to support migration but success to add migration blocker will not deregister device. vfio_migration_realize() is successful in this case. But failure to add migration blocker should deregister device, because vfio_exitfn() is never called in this case(errp set), jumping to out_deregister is the best choice. Then vfio_migration_realize() should return failure in this case. > Migration "realize" should not condition as to whether >your device finishes the realize. What's the impact if we condition it? Thanks Zhenzhong
>-----Original Message----- >From: Duan, Zhenzhong >Sent: Sunday, June 25, 2023 2:01 PM >To: Joao Martins <joao.m.martins@oracle.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 1/3] vfio/pci: Fix resource leak in vfio_realize > >>-----Original Message----- >>From: Joao Martins <joao.m.martins@oracle.com> >>Sent: Wednesday, June 21, 2023 7:08 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 1/3] vfio/pci: Fix resource leak in vfio_realize >> >> >> >>On 21/06/2023 09:02, Zhenzhong Duan wrote: >>> When adding migration blocker failed in vfio_migration_realize(), >>> hotplug will fail and we see below: >>> >>> (qemu) device_add >>> vfio-pci,host=81:11.1,id=vfio1,bus=root1,x-enable-migration=true >>> 0000:81:11.1: VFIO migration is not supported in kernel >>> Error: disallowing migration blocker (--only-migratable) for: VFIO >>> device doesn't support migration >>> >>> If we hotplug again we should see same log as above, but we see: >>> (qemu) device_add >>> vfio-pci,host=81:11.0,id=vfio0,bus=root0,x-enable-migration=true >>> Error: vfio 0000:81:11.0: device is already attached >>> >>> That's because some references to VFIO device isn't released, we >>> should check return value of vfio_migration_realize() and release the >>> references, then VFIO device will be truely released when hotplug >>> failed. >>> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> hw/vfio/pci.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index >>> 73874a94de12..c71b0955d81c 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -3210,6 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error >>**errp) >>> ret = vfio_migration_realize(vbasedev, errp); >>> if (ret) { >>> error_report("%s: Migration disabled", vbasedev->name); >>> + goto out_deregister; >>> } >>> } >>> >>This doesn't look right. This means that failure to support migration >>will deregister the device. > >In my understanding, failure to support migration but success to add >migration blocker will not deregister device. vfio_migration_realize() is >successful in this case. >But failure to add migration blocker should deregister device, because >vfio_exitfn() is never called in this case(errp set), jumping to out_deregister is >the best choice. Then vfio_migration_realize() should return failure in this >case. I just realized the error path in vfio_realize() isn't adequate. We need to add more deregister code just as vfio_exitfn(), see below. I'll re-post after we are consensus on this and get some comments of PATCH3. --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -3210,7 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) ret = vfio_migration_realize(vbasedev, errp); if (ret) { error_report("%s: Migration disabled", vbasedev->name); - goto out_deregister; + goto out_vfio_migration; } } @@ -3220,11 +3220,17 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) return; +out_vfio_migration: + vfio_migration_exit(vbasedev); out_deregister: pci_device_set_intx_routing_notifier(&vdev->pdev, NULL); 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); + } out_teardown: vfio_teardown_msi(vdev); vfio_bars_exit(vdev); Thanks Zhenzhong
On 26/06/2023 08:02, Duan, Zhenzhong wrote: >> -----Original Message----- >> From: Duan, Zhenzhong >> Sent: Sunday, June 25, 2023 2:01 PM >> To: Joao Martins <joao.m.martins@oracle.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 1/3] vfio/pci: Fix resource leak in vfio_realize >> >>> -----Original Message----- >>> From: Joao Martins <joao.m.martins@oracle.com> >>> Sent: Wednesday, June 21, 2023 7:08 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 1/3] vfio/pci: Fix resource leak in vfio_realize >>> >>> >>> >>> On 21/06/2023 09:02, Zhenzhong Duan wrote: >>>> When adding migration blocker failed in vfio_migration_realize(), >>>> hotplug will fail and we see below: >>>> >>>> (qemu) device_add >>>> vfio-pci,host=81:11.1,id=vfio1,bus=root1,x-enable-migration=true >>>> 0000:81:11.1: VFIO migration is not supported in kernel >>>> Error: disallowing migration blocker (--only-migratable) for: VFIO >>>> device doesn't support migration >>>> >>>> If we hotplug again we should see same log as above, but we see: >>>> (qemu) device_add >>>> vfio-pci,host=81:11.0,id=vfio0,bus=root0,x-enable-migration=true >>>> Error: vfio 0000:81:11.0: device is already attached >>>> >>>> That's because some references to VFIO device isn't released, we >>>> should check return value of vfio_migration_realize() and release the >>>> references, then VFIO device will be truely released when hotplug >>>> failed. >>>> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> hw/vfio/pci.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index >>>> 73874a94de12..c71b0955d81c 100644 >>>> --- a/hw/vfio/pci.c >>>> +++ b/hw/vfio/pci.c >>>> @@ -3210,6 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error >>> **errp) >>>> ret = vfio_migration_realize(vbasedev, errp); >>>> if (ret) { >>>> error_report("%s: Migration disabled", vbasedev->name); >>>> + goto out_deregister; >>>> } >>>> } >>>> >>> This doesn't look right. This means that failure to support migration >>> will deregister the device. >> >> In my understanding, failure to support migration but success to add >> migration blocker will not deregister device. vfio_migration_realize() is >> successful in this case. >> But failure to add migration blocker should deregister device, because >> vfio_exitfn() is never called in this case(errp set), jumping to out_deregister is >> the best choice. Then vfio_migration_realize() should return failure in this >> case. > I was checking other devices in the tree, and I think you are right. Failure to add the migration blocker results in a failure of device realize. Which IIUC only happens in 'only-migratable' setups and during snapshots. Maybe also including a sentence explainer in the commit message is useful too. > I just realized the error path in vfio_realize() isn't adequate. We need to add more deregister code just as vfio_exitfn(), see below. I'll re-post after we are consensus on this and get some comments of PATCH3. > > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -3210,7 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > ret = vfio_migration_realize(vbasedev, errp); > if (ret) { > error_report("%s: Migration disabled", vbasedev->name); > - goto out_deregister; > + goto out_vfio_migration; > } > } > > @@ -3220,11 +3220,17 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > > return; > > +out_vfio_migration: > + vfio_migration_exit(vbasedev); This belongs in this patch from the looks > out_deregister: > pci_device_set_intx_routing_notifier(&vdev->pdev, NULL); > 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); > + } But this one suggests another one as it looks a pre-existing issue? > out_teardown: > vfio_teardown_msi(vdev); > vfio_bars_exit(vdev); > > Thanks > Zhenzhong
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Sent: Monday, June 26, 2023 6:08 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 1/3] vfio/pci: Fix resource leak in vfio_realize > >On 26/06/2023 08:02, Duan, Zhenzhong wrote: >>> -----Original Message----- >>> From: Duan, Zhenzhong >>> Sent: Sunday, June 25, 2023 2:01 PM >>> To: Joao Martins <joao.m.martins@oracle.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 1/3] vfio/pci: Fix resource leak in >>> vfio_realize >>> >>>> -----Original Message----- >>>> From: Joao Martins <joao.m.martins@oracle.com> >>>> Sent: Wednesday, June 21, 2023 7:08 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 1/3] vfio/pci: Fix resource leak in >>>> vfio_realize >>>> >>>> >>>> >>>> On 21/06/2023 09:02, Zhenzhong Duan wrote: >>>>> When adding migration blocker failed in vfio_migration_realize(), >>>>> hotplug will fail and we see below: >>>>> >>>>> (qemu) device_add >>>>> vfio-pci,host=81:11.1,id=vfio1,bus=root1,x-enable-migration=true >>>>> 0000:81:11.1: VFIO migration is not supported in kernel >>>>> Error: disallowing migration blocker (--only-migratable) for: VFIO >>>>> device doesn't support migration >>>>> >>>>> If we hotplug again we should see same log as above, but we see: >>>>> (qemu) device_add >>>>> vfio-pci,host=81:11.0,id=vfio0,bus=root0,x-enable-migration=true >>>>> Error: vfio 0000:81:11.0: device is already attached >>>>> >>>>> That's because some references to VFIO device isn't released, we >>>>> should check return value of vfio_migration_realize() and release >>>>> the references, then VFIO device will be truely released when >>>>> hotplug failed. >>>>> >>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>> --- >>>>> hw/vfio/pci.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index >>>>> 73874a94de12..c71b0955d81c 100644 >>>>> --- a/hw/vfio/pci.c >>>>> +++ b/hw/vfio/pci.c >>>>> @@ -3210,6 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, >>>>> Error >>>> **errp) >>>>> ret = vfio_migration_realize(vbasedev, errp); >>>>> if (ret) { >>>>> error_report("%s: Migration disabled", >>>>> vbasedev->name); >>>>> + goto out_deregister; >>>>> } >>>>> } >>>>> >>>> This doesn't look right. This means that failure to support >>>> migration will deregister the device. >>> >>> In my understanding, failure to support migration but success to add >>> migration blocker will not deregister device. >>> vfio_migration_realize() is successful in this case. >>> But failure to add migration blocker should deregister device, >>> because >>> vfio_exitfn() is never called in this case(errp set), jumping to >>> out_deregister is the best choice. Then vfio_migration_realize() >>> should return failure in this case. >> > >I was checking other devices in the tree, and I think you are right. Failure to >add the migration blocker results in a failure of device realize. Which IIUC only >happens in 'only-migratable' setups and during snapshots. Yes. > >Maybe also including a sentence explainer in the commit message is useful >too. Sure, will do. > >> I just realized the error path in vfio_realize() isn't adequate. We need to add >more deregister code just as vfio_exitfn(), see below. I'll re-post after we are >consensus on this and get some comments of PATCH3. >> >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -3210,7 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error >**errp) >> ret = vfio_migration_realize(vbasedev, errp); >> if (ret) { >> error_report("%s: Migration disabled", vbasedev->name); >> - goto out_deregister; >> + goto out_vfio_migration; >> } >> } >> >> @@ -3220,11 +3220,17 @@ static void vfio_realize(PCIDevice *pdev, >> Error **errp) >> >> return; >> >> +out_vfio_migration: >> + vfio_migration_exit(vbasedev); > >This belongs in this patch from the looks Yes, I plan to merge above change to this patch. > >> out_deregister: >> pci_device_set_intx_routing_notifier(&vdev->pdev, NULL); >> 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); >> + } > >But this one suggests another one as it looks a pre-existing issue? Yes, it's another resource leak I just found. Not sure if it's better to also merge above change to this patch which is targeting resource leak issues, or to PATCH2 which is targeting out_deregister path, or to create a new one. Any suggestion? Thanks Zhenzhong
>>> out_deregister: >>> pci_device_set_intx_routing_notifier(&vdev->pdev, NULL); >>> 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); >>> + } >> >> But this one suggests another one as it looks a pre-existing issue? > Yes, it's another resource leak I just found. > Not sure if it's better to also merge above change to this patch which is targeting resource leak issues, > or to PATCH2 which is targeting out_deregister path, or to create a new one. > Any suggestion? In general they are all bugs in the same deregistration path, but each resource is not being teardown correctly. I tend to prefer 'logical change' per commit, so there's would be a fix the irqchip_change notifier and another one for mmap_timer teardown. Both with the Fixes tags that introduced each bug. Unless everything was introduced by the same change in which case you would do everything in one patch.
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Sent: Tuesday, June 27, 2023 6:22 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 1/3] vfio/pci: Fix resource leak in vfio_realize > >>>> out_deregister: >>>> pci_device_set_intx_routing_notifier(&vdev->pdev, NULL); >>>> 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); >>>> + } >>> >>> But this one suggests another one as it looks a pre-existing issue? >> Yes, it's another resource leak I just found. >> Not sure if it's better to also merge above change to this patch which >> is targeting resource leak issues, or to PATCH2 which is targeting >out_deregister path, or to create a new one. >> Any suggestion? > >In general they are all bugs in the same deregistration path, but each resource >is not being teardown correctly. I tend to prefer 'logical change' per commit, >so there's would be a fix the irqchip_change notifier and another one for >mmap_timer teardown. Both with the Fixes tags that introduced each bug. >Unless everything was introduced by the same change in which case you >would do everything in one patch. Clear. Thanks Zhenzhong
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 73874a94de12..c71b0955d81c 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -3210,6 +3210,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) ret = vfio_migration_realize(vbasedev, errp); if (ret) { error_report("%s: Migration disabled", vbasedev->name); + goto out_deregister; } }
When adding migration blocker failed in vfio_migration_realize(), hotplug will fail and we see below: (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,x-enable-migration=true 0000:81:11.1: VFIO migration is not supported in kernel Error: disallowing migration blocker (--only-migratable) for: VFIO device doesn't support migration If we hotplug again we should see same log as above, but we see: (qemu) device_add vfio-pci,host=81:11.0,id=vfio0,bus=root0,x-enable-migration=true Error: vfio 0000:81:11.0: device is already attached That's because some references to VFIO device isn't released, we should check return value of vfio_migration_realize() and release the references, then VFIO device will be truely released when hotplug failed. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/vfio/pci.c | 1 + 1 file changed, 1 insertion(+)