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.d...@intel.com>
Reviewed-by: Joao Martins <joao.m.mart...@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;
> }
> }
>
