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

[Cédric Le Goater]

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