| Message ID | 20230703071510.160712-4-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VFIO migration related refactor and bug fix | expand |
On 03/07/2023 08:15, Zhenzhong Duan wrote: > When vfio_realize() succeeds, hot unplug will call vfio_exitfn() > to free resources allocated in vfio_realize(); when vfio_realize() > fails, vfio_exitfn() is never called and we need to free resources > in vfio_realize(). > > In the case that vfio_migration_realize() fails, > e.g: with -only-migratable & enable-migration=off, we see below: > > (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off > 0000:81:11.1: Migration disabled > Error: disallowing migration blocker (--only-migratable) for: 0000:81:11.1: Migration is disabled for VFIO device > > If we hotplug again we should see same log as above, but we see: > (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off > Error: vfio 0000:81:11.1: device is already attached > > That's because some references to VFIO device isn't released. > For resources allocated in vfio_migration_realize(), free them by > jumping to out_deinit path with calling a new function > vfio_migration_deinit(). For resources allocated in vfio_realize(), > free them by jumping to de-register path in vfio_realize(). > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Reviewed-by: Joao Martins <joao.m.martins@oracle.com> > --- > hw/vfio/migration.c | 33 +++++++++++++++++++++++---------- > hw/vfio/pci.c | 1 + > 2 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index e6e5e85f7580..e3954570c853 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -802,6 +802,17 @@ static int vfio_migration_init(VFIODevice *vbasedev) > return 0; > } > > +static void vfio_migration_deinit(VFIODevice *vbasedev) > +{ > + VFIOMigration *migration = vbasedev->migration; > + > + remove_migration_state_change_notifier(&migration->migration_state); > + qemu_del_vm_change_state_handler(migration->vm_state); > + unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); > + vfio_migration_free(vbasedev); > + vfio_unblock_multiple_devices_migration(); > +} > + > static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp) > { > int ret; > @@ -866,7 +877,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) > error_setg(&err, > "%s: VFIO device doesn't support device dirty tracking", > vbasedev->name); > - return vfio_block_migration(vbasedev, err, errp); > + goto add_blocker; > } > > warn_report("%s: VFIO device doesn't support device dirty tracking", > @@ -875,29 +886,31 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) > > ret = vfio_block_multiple_devices_migration(vbasedev, errp); > if (ret) { > - return ret; > + goto out_deinit; > } > > if (vfio_viommu_preset(vbasedev)) { > error_setg(&err, "%s: Migration is currently not supported " > "with vIOMMU enabled", vbasedev->name); > - return vfio_block_migration(vbasedev, err, errp); > + goto add_blocker; > } > > trace_vfio_migration_realize(vbasedev->name); > return 0; > + > +add_blocker: > + ret = vfio_block_migration(vbasedev, err, errp); > +out_deinit: > + if (ret) { > + vfio_migration_deinit(vbasedev); > + } > + return ret; > } > > void vfio_migration_exit(VFIODevice *vbasedev) > { > if (vbasedev->migration) { > - VFIOMigration *migration = vbasedev->migration; > - > - remove_migration_state_change_notifier(&migration->migration_state); > - qemu_del_vm_change_state_handler(migration->vm_state); > - unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); > - vfio_migration_free(vbasedev); > - vfio_unblock_multiple_devices_migration(); > + vfio_migration_deinit(vbasedev); > } > > if (vbasedev->migration_blocker) { > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index c2cf7454ece6..9154dd929d07 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; > } > } >
On 03/07/2023 10:15, Zhenzhong Duan wrote: > External email: Use caution opening links or attachments > > > When vfio_realize() succeeds, hot unplug will call vfio_exitfn() > to free resources allocated in vfio_realize(); when vfio_realize() > fails, vfio_exitfn() is never called and we need to free resources > in vfio_realize(). > > In the case that vfio_migration_realize() fails, > e.g: with -only-migratable & enable-migration=off, we see below: > > (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off > 0000:81:11.1: Migration disabled > Error: disallowing migration blocker (--only-migratable) for: 0000:81:11.1: Migration is disabled for VFIO device > > If we hotplug again we should see same log as above, but we see: > (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off > Error: vfio 0000:81:11.1: device is already attached > > That's because some references to VFIO device isn't released. > For resources allocated in vfio_migration_realize(), free them by > jumping to out_deinit path with calling a new function > vfio_migration_deinit(). For resources allocated in vfio_realize(), > free them by jumping to de-register path in vfio_realize(). Should we add Fixes tag? Thanks. > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/vfio/migration.c | 33 +++++++++++++++++++++++---------- > hw/vfio/pci.c | 1 + > 2 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index e6e5e85f7580..e3954570c853 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -802,6 +802,17 @@ static int vfio_migration_init(VFIODevice *vbasedev) > return 0; > } > > +static void vfio_migration_deinit(VFIODevice *vbasedev) > +{ > + VFIOMigration *migration = vbasedev->migration; > + > + remove_migration_state_change_notifier(&migration->migration_state); > + qemu_del_vm_change_state_handler(migration->vm_state); > + unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); > + vfio_migration_free(vbasedev); > + vfio_unblock_multiple_devices_migration(); > +} > + > static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp) > { > int ret; > @@ -866,7 +877,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) > error_setg(&err, > "%s: VFIO device doesn't support device dirty tracking", > vbasedev->name); > - return vfio_block_migration(vbasedev, err, errp); > + goto add_blocker; > } > > warn_report("%s: VFIO device doesn't support device dirty tracking", > @@ -875,29 +886,31 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) > > ret = vfio_block_multiple_devices_migration(vbasedev, errp); > if (ret) { > - return ret; > + goto out_deinit; > } > > if (vfio_viommu_preset(vbasedev)) { > error_setg(&err, "%s: Migration is currently not supported " > "with vIOMMU enabled", vbasedev->name); > - return vfio_block_migration(vbasedev, err, errp); > + goto add_blocker; > } > > trace_vfio_migration_realize(vbasedev->name); > return 0; > + > +add_blocker: > + ret = vfio_block_migration(vbasedev, err, errp); > +out_deinit: > + if (ret) { > + vfio_migration_deinit(vbasedev); > + } > + return ret; > } > > void vfio_migration_exit(VFIODevice *vbasedev) > { > if (vbasedev->migration) { > - VFIOMigration *migration = vbasedev->migration; > - > - remove_migration_state_change_notifier(&migration->migration_state); > - qemu_del_vm_change_state_handler(migration->vm_state); > - unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); > - vfio_migration_free(vbasedev); > - vfio_unblock_multiple_devices_migration(); > + vfio_migration_deinit(vbasedev); > } > > if (vbasedev->migration_blocker) { > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index c2cf7454ece6..9154dd929d07 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; > } > } > > -- > 2.34.1 >
On 7/3/23 17:34, Avihai Horon wrote: > > On 03/07/2023 10:15, Zhenzhong Duan wrote: >> External email: Use caution opening links or attachments >> >> >> When vfio_realize() succeeds, hot unplug will call vfio_exitfn() >> to free resources allocated in vfio_realize(); when vfio_realize() >> fails, vfio_exitfn() is never called and we need to free resources >> in vfio_realize(). >> >> In the case that vfio_migration_realize() fails, >> e.g: with -only-migratable & enable-migration=off, we see below: >> >> (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off >> 0000:81:11.1: Migration disabled >> Error: disallowing migration blocker (--only-migratable) for: 0000:81:11.1: Migration is disabled for VFIO device >> >> If we hotplug again we should see same log as above, but we see: >> (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off >> Error: vfio 0000:81:11.1: device is already attached >> >> That's because some references to VFIO device isn't released. >> For resources allocated in vfio_migration_realize(), free them by >> jumping to out_deinit path with calling a new function >> vfio_migration_deinit(). For resources allocated in vfio_realize(), >> free them by jumping to de-register path in vfio_realize(). > > Should we add Fixes tag? Yes. It would help downstream backports. No need to resend for that. Thanks, C. > > Thanks. > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> hw/vfio/migration.c | 33 +++++++++++++++++++++++---------- >> hw/vfio/pci.c | 1 + >> 2 files changed, 24 insertions(+), 10 deletions(-) >> >> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c >> index e6e5e85f7580..e3954570c853 100644 >> --- a/hw/vfio/migration.c >> +++ b/hw/vfio/migration.c >> @@ -802,6 +802,17 @@ static int vfio_migration_init(VFIODevice *vbasedev) >> return 0; >> } >> >> +static void vfio_migration_deinit(VFIODevice *vbasedev) >> +{ >> + VFIOMigration *migration = vbasedev->migration; >> + >> + remove_migration_state_change_notifier(&migration->migration_state); >> + qemu_del_vm_change_state_handler(migration->vm_state); >> + unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); >> + vfio_migration_free(vbasedev); >> + vfio_unblock_multiple_devices_migration(); >> +} >> + >> static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp) >> { >> int ret; >> @@ -866,7 +877,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) >> error_setg(&err, >> "%s: VFIO device doesn't support device dirty tracking", >> vbasedev->name); >> - return vfio_block_migration(vbasedev, err, errp); >> + goto add_blocker; >> } >> >> warn_report("%s: VFIO device doesn't support device dirty tracking", >> @@ -875,29 +886,31 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) >> >> ret = vfio_block_multiple_devices_migration(vbasedev, errp); >> if (ret) { >> - return ret; >> + goto out_deinit; >> } >> >> if (vfio_viommu_preset(vbasedev)) { >> error_setg(&err, "%s: Migration is currently not supported " >> "with vIOMMU enabled", vbasedev->name); >> - return vfio_block_migration(vbasedev, err, errp); >> + goto add_blocker; >> } >> >> trace_vfio_migration_realize(vbasedev->name); >> return 0; >> + >> +add_blocker: >> + ret = vfio_block_migration(vbasedev, err, errp); >> +out_deinit: >> + if (ret) { >> + vfio_migration_deinit(vbasedev); >> + } >> + return ret; >> } >> >> void vfio_migration_exit(VFIODevice *vbasedev) >> { >> if (vbasedev->migration) { >> - VFIOMigration *migration = vbasedev->migration; >> - >> - remove_migration_state_change_notifier(&migration->migration_state); >> - qemu_del_vm_change_state_handler(migration->vm_state); >> - unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); >> - vfio_migration_free(vbasedev); >> - vfio_unblock_multiple_devices_migration(); >> + vfio_migration_deinit(vbasedev); >> } >> >> if (vbasedev->migration_blocker) { >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index c2cf7454ece6..9154dd929d07 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; >> } >> } >> >> -- >> 2.34.1 >> >
On 7/3/23 09:15, Zhenzhong Duan wrote: > When vfio_realize() succeeds, hot unplug will call vfio_exitfn() > to free resources allocated in vfio_realize(); when vfio_realize() > fails, vfio_exitfn() is never called and we need to free resources > in vfio_realize(). > > In the case that vfio_migration_realize() fails, > e.g: with -only-migratable & enable-migration=off, we see below: > > (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off > 0000:81:11.1: Migration disabled > Error: disallowing migration blocker (--only-migratable) for: 0000:81:11.1: Migration is disabled for VFIO device > > If we hotplug again we should see same log as above, but we see: > (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off > Error: vfio 0000:81:11.1: device is already attached > > That's because some references to VFIO device isn't released. > For resources allocated in vfio_migration_realize(), free them by > jumping to out_deinit path with calling a new function > vfio_migration_deinit(). For resources allocated in vfio_realize(), > free them by jumping to de-register path in vfio_realize(). > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> The vfio_migration_realize() routine is somewhat difficult to follow but I don't see how to improve it. May be could move the viommu test at the beginning ? Anyhow, Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > --- > hw/vfio/migration.c | 33 +++++++++++++++++++++++---------- > hw/vfio/pci.c | 1 + > 2 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index e6e5e85f7580..e3954570c853 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -802,6 +802,17 @@ static int vfio_migration_init(VFIODevice *vbasedev) > return 0; > } > > +static void vfio_migration_deinit(VFIODevice *vbasedev) > +{ > + VFIOMigration *migration = vbasedev->migration; > + > + remove_migration_state_change_notifier(&migration->migration_state); > + qemu_del_vm_change_state_handler(migration->vm_state); > + unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); > + vfio_migration_free(vbasedev); > + vfio_unblock_multiple_devices_migration(); > +} > + > static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp) > { > int ret; > @@ -866,7 +877,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) > error_setg(&err, > "%s: VFIO device doesn't support device dirty tracking", > vbasedev->name); > - return vfio_block_migration(vbasedev, err, errp); > + goto add_blocker; > } > > warn_report("%s: VFIO device doesn't support device dirty tracking", > @@ -875,29 +886,31 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) > > ret = vfio_block_multiple_devices_migration(vbasedev, errp); > if (ret) { > - return ret; > + goto out_deinit; > } > > if (vfio_viommu_preset(vbasedev)) { > error_setg(&err, "%s: Migration is currently not supported " > "with vIOMMU enabled", vbasedev->name); > - return vfio_block_migration(vbasedev, err, errp); > + goto add_blocker; > } > > trace_vfio_migration_realize(vbasedev->name); > return 0; > + > +add_blocker: > + ret = vfio_block_migration(vbasedev, err, errp); > +out_deinit: > + if (ret) { > + vfio_migration_deinit(vbasedev); > + } > + return ret; > } > > void vfio_migration_exit(VFIODevice *vbasedev) > { > if (vbasedev->migration) { > - VFIOMigration *migration = vbasedev->migration; > - > - remove_migration_state_change_notifier(&migration->migration_state); > - qemu_del_vm_change_state_handler(migration->vm_state); > - unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); > - vfio_migration_free(vbasedev); > - vfio_unblock_multiple_devices_migration(); > + vfio_migration_deinit(vbasedev); > } > > if (vbasedev->migration_blocker) { > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index c2cf7454ece6..9154dd929d07 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; > } > } >
>-----Original Message----- >From: Duan, Zhenzhong <zhenzhong.duan@intel.com> >Sent: Monday, July 3, 2023 3:15 PM >To: qemu-devel@nongnu.org >Cc: alex.williamson@redhat.com; clg@redhat.com; Martins, Joao ><joao.m.martins@oracle.com>; avihaih@nvidia.com; Peng, Chao P ><chao.p.peng@intel.com> >Subject: [PATCH v6 5/7] vfio/migration: Free resources when >vfio_migration_realize fails > >When vfio_realize() succeeds, hot unplug will call vfio_exitfn() to free >resources allocated in vfio_realize(); when vfio_realize() fails, vfio_exitfn() is >never called and we need to free resources in vfio_realize(). > >In the case that vfio_migration_realize() fails, >e.g: with -only-migratable & enable-migration=off, we see below: > >(qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable- >migration=off >0000:81:11.1: Migration disabled >Error: disallowing migration blocker (--only-migratable) for: 0000:81:11.1: >Migration is disabled for VFIO device > >If we hotplug again we should see same log as above, but we see: >(qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable- >migration=off >Error: vfio 0000:81:11.1: device is already attached > >That's because some references to VFIO device isn't released. >For resources allocated in vfio_migration_realize(), free them by jumping to >out_deinit path with calling a new function vfio_migration_deinit(). For >resources allocated in vfio_realize(), free them by jumping to de-register path >in vfio_realize(). > Forgot fixes tag: Fixes: a22651053b59 ("vfio: Make vfio-pci device migration capable") Thanks Zhenzhong >Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >--- > hw/vfio/migration.c | 33 +++++++++++++++++++++++---------- > hw/vfio/pci.c | 1 + > 2 files changed, 24 insertions(+), 10 deletions(-) > >diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index >e6e5e85f7580..e3954570c853 100644 >--- a/hw/vfio/migration.c >+++ b/hw/vfio/migration.c >@@ -802,6 +802,17 @@ static int vfio_migration_init(VFIODevice *vbasedev) > return 0; > } > >+static void vfio_migration_deinit(VFIODevice *vbasedev) { >+ VFIOMigration *migration = vbasedev->migration; >+ >+ remove_migration_state_change_notifier(&migration->migration_state); >+ qemu_del_vm_change_state_handler(migration->vm_state); >+ unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); >+ vfio_migration_free(vbasedev); >+ vfio_unblock_multiple_devices_migration(); >+} >+ > static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error >**errp) { > int ret; >@@ -866,7 +877,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, >Error **errp) > error_setg(&err, > "%s: VFIO device doesn't support device dirty tracking", > vbasedev->name); >- return vfio_block_migration(vbasedev, err, errp); >+ goto add_blocker; > } > > warn_report("%s: VFIO device doesn't support device dirty tracking", >@@ -875,29 +886,31 @@ int vfio_migration_realize(VFIODevice *vbasedev, >Error **errp) > > ret = vfio_block_multiple_devices_migration(vbasedev, errp); > if (ret) { >- return ret; >+ goto out_deinit; > } > > if (vfio_viommu_preset(vbasedev)) { > error_setg(&err, "%s: Migration is currently not supported " > "with vIOMMU enabled", vbasedev->name); >- return vfio_block_migration(vbasedev, err, errp); >+ goto add_blocker; > } > > trace_vfio_migration_realize(vbasedev->name); > return 0; >+ >+add_blocker: >+ ret = vfio_block_migration(vbasedev, err, errp); >+out_deinit: >+ if (ret) { >+ vfio_migration_deinit(vbasedev); >+ } >+ return ret; > } > > void vfio_migration_exit(VFIODevice *vbasedev) { > if (vbasedev->migration) { >- VFIOMigration *migration = vbasedev->migration; >- >- remove_migration_state_change_notifier(&migration->migration_state); >- qemu_del_vm_change_state_handler(migration->vm_state); >- unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); >- vfio_migration_free(vbasedev); >- vfio_unblock_multiple_devices_migration(); >+ vfio_migration_deinit(vbasedev); > } > > if (vbasedev->migration_blocker) { >diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index c2cf7454ece6..9154dd929d07 >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; > } > } > >-- >2.34.1
>-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Sent: Monday, July 3, 2023 11:45 PM >Subject: Re: [PATCH v6 5/7] vfio/migration: Free resources when >vfio_migration_realize fails > >On 7/3/23 09:15, Zhenzhong Duan wrote: >> When vfio_realize() succeeds, hot unplug will call vfio_exitfn() to >> free resources allocated in vfio_realize(); when vfio_realize() fails, >> vfio_exitfn() is never called and we need to free resources in >> vfio_realize(). >> >> In the case that vfio_migration_realize() fails, >> e.g: with -only-migratable & enable-migration=off, we see below: >> >> (qemu) device_add >> vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off >> 0000:81:11.1: Migration disabled >> Error: disallowing migration blocker (--only-migratable) for: >> 0000:81:11.1: Migration is disabled for VFIO device >> >> If we hotplug again we should see same log as above, but we see: >> (qemu) device_add >> vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off >> Error: vfio 0000:81:11.1: device is already attached >> >> That's because some references to VFIO device isn't released. >> For resources allocated in vfio_migration_realize(), free them by >> jumping to out_deinit path with calling a new function >> vfio_migration_deinit(). For resources allocated in vfio_realize(), >> free them by jumping to de-register path in vfio_realize(). >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > >The vfio_migration_realize() routine is somewhat difficult to follow but I don't >see how to improve it. May be could move the viommu test at the beginning ? Is your purpose to remove vfio_unblock_multiple_devices_migration() from vfio_migration_deinit()? Or other benefit I misses? Thanks Zhenzhong
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index e6e5e85f7580..e3954570c853 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -802,6 +802,17 @@ static int vfio_migration_init(VFIODevice *vbasedev) return 0; } +static void vfio_migration_deinit(VFIODevice *vbasedev) +{ + VFIOMigration *migration = vbasedev->migration; + + remove_migration_state_change_notifier(&migration->migration_state); + qemu_del_vm_change_state_handler(migration->vm_state); + unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); + vfio_migration_free(vbasedev); + vfio_unblock_multiple_devices_migration(); +} + static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp) { int ret; @@ -866,7 +877,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) error_setg(&err, "%s: VFIO device doesn't support device dirty tracking", vbasedev->name); - return vfio_block_migration(vbasedev, err, errp); + goto add_blocker; } warn_report("%s: VFIO device doesn't support device dirty tracking", @@ -875,29 +886,31 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp) ret = vfio_block_multiple_devices_migration(vbasedev, errp); if (ret) { - return ret; + goto out_deinit; } if (vfio_viommu_preset(vbasedev)) { error_setg(&err, "%s: Migration is currently not supported " "with vIOMMU enabled", vbasedev->name); - return vfio_block_migration(vbasedev, err, errp); + goto add_blocker; } trace_vfio_migration_realize(vbasedev->name); return 0; + +add_blocker: + ret = vfio_block_migration(vbasedev, err, errp); +out_deinit: + if (ret) { + vfio_migration_deinit(vbasedev); + } + return ret; } void vfio_migration_exit(VFIODevice *vbasedev) { if (vbasedev->migration) { - VFIOMigration *migration = vbasedev->migration; - - remove_migration_state_change_notifier(&migration->migration_state); - qemu_del_vm_change_state_handler(migration->vm_state); - unregister_savevm(VMSTATE_IF(vbasedev->dev), "vfio", vbasedev); - vfio_migration_free(vbasedev); - vfio_unblock_multiple_devices_migration(); + vfio_migration_deinit(vbasedev); } if (vbasedev->migration_blocker) { diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index c2cf7454ece6..9154dd929d07 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 vfio_realize() succeeds, hot unplug will call vfio_exitfn() to free resources allocated in vfio_realize(); when vfio_realize() fails, vfio_exitfn() is never called and we need to free resources in vfio_realize(). In the case that vfio_migration_realize() fails, e.g: with -only-migratable & enable-migration=off, we see below: (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off 0000:81:11.1: Migration disabled Error: disallowing migration blocker (--only-migratable) for: 0000:81:11.1: Migration is disabled for VFIO device If we hotplug again we should see same log as above, but we see: (qemu) device_add vfio-pci,host=81:11.1,id=vfio1,bus=root1,enable-migration=off Error: vfio 0000:81:11.1: device is already attached That's because some references to VFIO device isn't released. For resources allocated in vfio_migration_realize(), free them by jumping to out_deinit path with calling a new function vfio_migration_deinit(). For resources allocated in vfio_realize(), free them by jumping to de-register path in vfio_realize(). Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/vfio/migration.c | 33 +++++++++++++++++++++++---------- hw/vfio/pci.c | 1 + 2 files changed, 24 insertions(+), 10 deletions(-)