diff mbox series

[v6,5/7] vfio/migration: Free resources when vfio_migration_realize fails

Message ID 20230703071510.160712-4-zhenzhong.duan@intel.com
State New
Headers show
Series VFIO migration related refactor and bug fix | expand

Commit Message

Duan, Zhenzhong July 3, 2023, 7:15 a.m. UTC 
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(-)

Comments

Joao Martins July 3, 2023, 3:23 p.m. UTC | #1 
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;
>          }
>      }
>
Avihai Horon July 3, 2023, 3:34 p.m. UTC | #2 
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
>
Cédric Le Goater July 3, 2023, 3:41 p.m. UTC | #3 
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
>>
>
Cédric Le Goater July 3, 2023, 3:44 p.m. UTC | #4 
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;
>           }
>       }
>
Duan, Zhenzhong July 4, 2023, 1:33 a.m. UTC | #5 
>-----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
Duan, Zhenzhong July 4, 2023, 1:56 a.m. UTC | #6 
>-----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 mbox series

Patch

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;
         }
     }