[PATCH v6 0/7] VFIO migration related refactor and bug fix

[PATCH v6 0/7] VFIO migration related refactor and bug fix

[Zhenzhong Duan]

Hello,

PATCH4,6,7 refactors the VFIO migration blocker related code based on
suggestions from Joao and Cedric, so that code is simpler and
redundant "Migration disabled" isn't printed.

But before that works, also found some hotplug bugs when testing
blocker adding failed case. PATCH1-3,5 fix them.

See patch description for details.

v6: Update patch description with Joao suggested words
    Move rename patch to last and keep int return type for internal
    functions per Joao
    Move a check out from vfio_migration_deinit()
    Add Reviewed-by
v5: PATCH1-2 are not sent in v5 as Cedric already picked them
    Minor adjust on PATCH3 per Joao and Cedric
    Move PATCH4 after refactor patch per Joao
    Split refactor patch to three patches, PATCH4,5,7 per Cedric

v4: Rebased on [1] which contains Avihai's patchset [2]
    Add more patches to fix resource leak issue, split based on
    different fix TAG per Joao
    Change to not print "Migration disabled" with explicit
    enable-migration=off per Avihai
    Rename vfio_block_giommu_migration to vfio_viommu_preset per Joao

v3: Add PATCH1,2 to fix hotplug bug
    Fix bugs in PATCH3 Avihai and Joao pointed out

Tested vfio hotplug/unplug with vfio migration supported and unsupported cases,
and different param of enable-migration=[on/off/auto] and -only-migratable.

[1] https://github.com/legoater/qemu/tree/vfio-next
[2] https://lists.nongnu.org/archive/html/qemu-devel/2023-06/msg06117.html

Thanks

Zhenzhong Duan (5):
  vfio/pci: Disable INTx in vfio_realize error path
  vfio/migration: Change vIOMMU blocker from global to per device
  vfio/migration: Free resources when vfio_migration_realize fails
  vfio/migration: Remove print of "Migration disabled"
  vfio/migration: Return bool type for vfio_migration_realize()

 hw/vfio/common.c              | 51 ++---------------------------------
 hw/vfio/migration.c           | 51 ++++++++++++++++++++++++-----------
 hw/vfio/pci.c                 |  9 ++++---
 include/hw/vfio/vfio-common.h |  5 ++--
 4 files changed, 44 insertions(+), 72 deletions(-)

-- 
2.34.1

[PATCH v6 3/7] vfio/pci: Disable INTx in vfio_realize error path

[Zhenzhong Duan]

When vfio realize fails, INTx isn't disabled if it has been enabled.
This may confuse host side with unhandled interrupt report.

Fixes: c5478fea27ac ("vfio/pci: Respond to KVM irqchip change notifier")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ab6645ba60af..31c4ab250fbe 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3220,6 +3220,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     return;
 
 out_deregister:
+    if (vdev->interrupt == VFIO_INT_INTx) {
+        vfio_intx_disable(vdev);
+    }
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     if (vdev->irqchip_change_notifier.notify) {
         kvm_irqchip_remove_change_notifier(&vdev->irqchip_change_notifier);
-- 
2.34.1

[PATCH v6 4/7] vfio/migration: Change vIOMMU blocker from global to per device

[Zhenzhong Duan]

Contrary to multiple device blocker which needs to consider already-attached
devices to unblock/block dynamically, the vIOMMU migration blocker is a device
specific config. Meaning it only needs to know whether the device is bypassing
or not the vIOMMU (via machine property, or per pxb-pcie::bypass_iommu), and
does not need the state of currently present devices. For this reason, the
vIOMMU global migration blocker can be consolidated into the per-device
migration blocker, allowing us to remove some unnecessary code.

This change also makes vfio_mig_active() more accurate as it doesn't check for
global blocker.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
---
 hw/vfio/common.c              | 51 ++---------------------------------
 hw/vfio/migration.c           |  7 ++---
 hw/vfio/pci.c                 |  1 -
 include/hw/vfio/vfio-common.h |  3 +--
 4 files changed, 7 insertions(+), 55 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 77e2ee0e5c6e..9aac21abb76e 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -362,7 +362,6 @@ bool vfio_mig_active(void)
 }
 
 static Error *multiple_devices_migration_blocker;
-static Error *giommu_migration_blocker;
 
 static unsigned int vfio_migratable_device_num(void)
 {
@@ -420,55 +419,9 @@ void vfio_unblock_multiple_devices_migration(void)
     multiple_devices_migration_blocker = NULL;
 }
 
-static bool vfio_viommu_preset(void)
+bool vfio_viommu_preset(VFIODevice *vbasedev)
 {
-    VFIOAddressSpace *space;
-
-    QLIST_FOREACH(space, &vfio_address_spaces, list) {
-        if (space->as != &address_space_memory) {
-            return true;
-        }
-    }
-
-    return false;
-}
-
-int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
-{
-    int ret;
-
-    if (giommu_migration_blocker ||
-        !vfio_viommu_preset()) {
-        return 0;
-    }
-
-    if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
-        error_setg(errp,
-                   "Migration is currently not supported with vIOMMU enabled");
-        return -EINVAL;
-    }
-
-    error_setg(&giommu_migration_blocker,
-               "Migration is currently not supported with vIOMMU enabled");
-    ret = migrate_add_blocker(giommu_migration_blocker, errp);
-    if (ret < 0) {
-        error_free(giommu_migration_blocker);
-        giommu_migration_blocker = NULL;
-    }
-
-    return ret;
-}
-
-void vfio_migration_finalize(void)
-{
-    if (!giommu_migration_blocker ||
-        vfio_viommu_preset()) {
-        return;
-    }
-
-    migrate_del_blocker(giommu_migration_blocker);
-    error_free(giommu_migration_blocker);
-    giommu_migration_blocker = NULL;
+    return vbasedev->group->container->space->as != &address_space_memory;
 }
 
 static void vfio_set_migration_error(int err)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 1db7d52ab2c1..e6e5e85f7580 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -878,9 +878,10 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
         return ret;
     }
 
-    ret = vfio_block_giommu_migration(vbasedev, errp);
-    if (ret) {
-        return ret;
+    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);
     }
 
     trace_vfio_migration_realize(vbasedev->name);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 31c4ab250fbe..c2cf7454ece6 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3255,7 +3255,6 @@ static void vfio_instance_finalize(Object *obj)
      */
     vfio_put_device(vdev);
     vfio_put_group(group);
-    vfio_migration_finalize();
 }
 
 static void vfio_exitfn(PCIDevice *pdev)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 93429b9abba0..45167c8a8a54 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -227,7 +227,7 @@ extern VFIOGroupList vfio_group_list;
 bool vfio_mig_active(void);
 int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
 void vfio_unblock_multiple_devices_migration(void);
-int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp);
+bool vfio_viommu_preset(VFIODevice *vbasedev);
 int64_t vfio_mig_bytes_transferred(void);
 void vfio_reset_bytes_transferred(void);
 
@@ -254,6 +254,5 @@ int vfio_spapr_remove_window(VFIOContainer *container,
 
 int vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
 void vfio_migration_exit(VFIODevice *vbasedev);
-void vfio_migration_finalize(void);
 
 #endif /* HW_VFIO_VFIO_COMMON_H */
-- 
2.34.1

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

[Zhenzhong Duan]

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(-)

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

[PATCH v6 6/7] vfio/migration: Remove print of "Migration disabled"

[Zhenzhong Duan]

Property enable_migration supports [on/off/auto].
In ON mode, error pointer is passed to errp and logged.
In OFF mode, we doesn't need to log "Migration disabled" as it's intentional.
In AUTO mode, we should only ever see errors or warnings if the device
supports migration and an error or incompatibility occurs while further
probing or configuring it. Lack of support for migration shoundn't
generate an error or warning.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
---
 hw/vfio/pci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 9154dd929d07..eefd4ec330d9 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3209,7 +3209,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     if (!pdev->failover_pair_id) {
         ret = vfio_migration_realize(vbasedev, errp);
         if (ret) {
-            error_report("%s: Migration disabled", vbasedev->name);
             goto out_deregister;
         }
     }
-- 
2.34.1

[PATCH v6 7/7] vfio/migration: Return bool type for vfio_migration_realize()

[Zhenzhong Duan]

Make vfio_migration_realize() adhere to the convention of other realize()
callbacks(like qdev_realize) by returning bool instead of int.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Suggested-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/migration.c           | 15 ++++++++++-----
 hw/vfio/pci.c                 |  3 +--
 include/hw/vfio/vfio-common.h |  2 +-
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index e3954570c853..2674f4bc472d 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -846,7 +846,12 @@ void vfio_reset_bytes_transferred(void)
     bytes_transferred = 0;
 }
 
-int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
+/*
+ * Return true when either migration initialized or blocker registered.
+ * Currently only return false when adding blocker fails which will
+ * de-register vfio device.
+ */
+bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
 {
     Error *err = NULL;
     int ret;
@@ -854,7 +859,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
     if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
         error_setg(&err, "%s: Migration is disabled for VFIO device",
                    vbasedev->name);
-        return vfio_block_migration(vbasedev, err, errp);
+        return !vfio_block_migration(vbasedev, err, errp);
     }
 
     ret = vfio_migration_init(vbasedev);
@@ -869,7 +874,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
                        vbasedev->name, ret, strerror(-ret));
         }
 
-        return vfio_block_migration(vbasedev, err, errp);
+        return !vfio_block_migration(vbasedev, err, errp);
     }
 
     if (!vbasedev->dirty_pages_supported) {
@@ -896,7 +901,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
     }
 
     trace_vfio_migration_realize(vbasedev->name);
-    return 0;
+    return true;
 
 add_blocker:
     ret = vfio_block_migration(vbasedev, err, errp);
@@ -904,7 +909,7 @@ out_deinit:
     if (ret) {
         vfio_migration_deinit(vbasedev);
     }
-    return ret;
+    return !ret;
 }
 
 void vfio_migration_exit(VFIODevice *vbasedev)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index eefd4ec330d9..68dd99283620 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3207,8 +3207,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     }
 
     if (!pdev->failover_pair_id) {
-        ret = vfio_migration_realize(vbasedev, errp);
-        if (ret) {
+        if (!vfio_migration_realize(vbasedev, errp)) {
             goto out_deregister;
         }
     }
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 45167c8a8a54..da43d273524e 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -252,7 +252,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
 int vfio_spapr_remove_window(VFIOContainer *container,
                              hwaddr offset_within_address_space);
 
-int vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
+bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
 void vfio_migration_exit(VFIODevice *vbasedev);
 
 #endif /* HW_VFIO_VFIO_COMMON_H */
-- 
2.34.1

Re: [PATCH v6 4/7] vfio/migration: Change vIOMMU blocker from global to per device

[Cédric Le Goater]

On 7/3/23 09:15, Zhenzhong Duan wrote:
> Contrary to multiple device blocker which needs to consider already-attached
> devices to unblock/block dynamically, the vIOMMU migration blocker is a device
> specific config. Meaning it only needs to know whether the device is bypassing
> or not the vIOMMU (via machine property, or per pxb-pcie::bypass_iommu), and
> does not need the state of currently present devices. For this reason, the
> vIOMMU global migration blocker can be consolidated into the per-device
> migration blocker, allowing us to remove some unnecessary code.
> 
> This change also makes vfio_mig_active() more accurate as it doesn't check for
> global blocker.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Reviewed-by: Joao Martins <joao.m.martins@oracle.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/common.c              | 51 ++---------------------------------
>   hw/vfio/migration.c           |  7 ++---
>   hw/vfio/pci.c                 |  1 -
>   include/hw/vfio/vfio-common.h |  3 +--
>   4 files changed, 7 insertions(+), 55 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 77e2ee0e5c6e..9aac21abb76e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -362,7 +362,6 @@ bool vfio_mig_active(void)
>   }
>   
>   static Error *multiple_devices_migration_blocker;
> -static Error *giommu_migration_blocker;
>   
>   static unsigned int vfio_migratable_device_num(void)
>   {
> @@ -420,55 +419,9 @@ void vfio_unblock_multiple_devices_migration(void)
>       multiple_devices_migration_blocker = NULL;
>   }
>   
> -static bool vfio_viommu_preset(void)
> +bool vfio_viommu_preset(VFIODevice *vbasedev)
>   {
> -    VFIOAddressSpace *space;
> -
> -    QLIST_FOREACH(space, &vfio_address_spaces, list) {
> -        if (space->as != &address_space_memory) {
> -            return true;
> -        }
> -    }
> -
> -    return false;
> -}
> -
> -int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
> -{
> -    int ret;
> -
> -    if (giommu_migration_blocker ||
> -        !vfio_viommu_preset()) {
> -        return 0;
> -    }
> -
> -    if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
> -        error_setg(errp,
> -                   "Migration is currently not supported with vIOMMU enabled");
> -        return -EINVAL;
> -    }
> -
> -    error_setg(&giommu_migration_blocker,
> -               "Migration is currently not supported with vIOMMU enabled");
> -    ret = migrate_add_blocker(giommu_migration_blocker, errp);
> -    if (ret < 0) {
> -        error_free(giommu_migration_blocker);
> -        giommu_migration_blocker = NULL;
> -    }
> -
> -    return ret;
> -}
> -
> -void vfio_migration_finalize(void)
> -{
> -    if (!giommu_migration_blocker ||
> -        vfio_viommu_preset()) {
> -        return;
> -    }
> -
> -    migrate_del_blocker(giommu_migration_blocker);
> -    error_free(giommu_migration_blocker);
> -    giommu_migration_blocker = NULL;
> +    return vbasedev->group->container->space->as != &address_space_memory;
>   }
>   
>   static void vfio_set_migration_error(int err)
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 1db7d52ab2c1..e6e5e85f7580 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -878,9 +878,10 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>           return ret;
>       }
>   
> -    ret = vfio_block_giommu_migration(vbasedev, errp);
> -    if (ret) {
> -        return ret;
> +    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);
>       }
>   
>       trace_vfio_migration_realize(vbasedev->name);
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 31c4ab250fbe..c2cf7454ece6 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3255,7 +3255,6 @@ static void vfio_instance_finalize(Object *obj)
>        */
>       vfio_put_device(vdev);
>       vfio_put_group(group);
> -    vfio_migration_finalize();
>   }
>   
>   static void vfio_exitfn(PCIDevice *pdev)
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 93429b9abba0..45167c8a8a54 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -227,7 +227,7 @@ extern VFIOGroupList vfio_group_list;
>   bool vfio_mig_active(void);
>   int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
>   void vfio_unblock_multiple_devices_migration(void);
> -int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp);
> +bool vfio_viommu_preset(VFIODevice *vbasedev);
>   int64_t vfio_mig_bytes_transferred(void);
>   void vfio_reset_bytes_transferred(void);
>   
> @@ -254,6 +254,5 @@ int vfio_spapr_remove_window(VFIOContainer *container,
>   
>   int vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
>   void vfio_migration_exit(VFIODevice *vbasedev);
> -void vfio_migration_finalize(void);
>   
>   #endif /* HW_VFIO_VFIO_COMMON_H */

Re: [PATCH v6 7/7] vfio/migration: Return bool type for vfio_migration_realize()

[Cédric Le Goater]

On 7/3/23 09:15, Zhenzhong Duan wrote:
> Make vfio_migration_realize() adhere to the convention of other realize()
> callbacks(like qdev_realize) by returning bool instead of int.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/migration.c           | 15 ++++++++++-----
>   hw/vfio/pci.c                 |  3 +--
>   include/hw/vfio/vfio-common.h |  2 +-
>   3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index e3954570c853..2674f4bc472d 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -846,7 +846,12 @@ void vfio_reset_bytes_transferred(void)
>       bytes_transferred = 0;
>   }
>   
> -int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
> +/*
> + * Return true when either migration initialized or blocker registered.
> + * Currently only return false when adding blocker fails which will
> + * de-register vfio device.
> + */
> +bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>   {
>       Error *err = NULL;
>       int ret;
> @@ -854,7 +859,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>       if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
>           error_setg(&err, "%s: Migration is disabled for VFIO device",
>                      vbasedev->name);
> -        return vfio_block_migration(vbasedev, err, errp);
> +        return !vfio_block_migration(vbasedev, err, errp);
>       }
>   
>       ret = vfio_migration_init(vbasedev);
> @@ -869,7 +874,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>                          vbasedev->name, ret, strerror(-ret));
>           }
>   
> -        return vfio_block_migration(vbasedev, err, errp);
> +        return !vfio_block_migration(vbasedev, err, errp);
>       }
>   
>       if (!vbasedev->dirty_pages_supported) {
> @@ -896,7 +901,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>       }
>   
>       trace_vfio_migration_realize(vbasedev->name);
> -    return 0;
> +    return true;
>   
>   add_blocker:
>       ret = vfio_block_migration(vbasedev, err, errp);
> @@ -904,7 +909,7 @@ out_deinit:
>       if (ret) {
>           vfio_migration_deinit(vbasedev);
>       }
> -    return ret;
> +    return !ret;
>   }
>   
>   void vfio_migration_exit(VFIODevice *vbasedev)
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index eefd4ec330d9..68dd99283620 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3207,8 +3207,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>       }
>   
>       if (!pdev->failover_pair_id) {
> -        ret = vfio_migration_realize(vbasedev, errp);
> -        if (ret) {
> +        if (!vfio_migration_realize(vbasedev, errp)) {
>               goto out_deregister;
>           }
>       }
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 45167c8a8a54..da43d273524e 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -252,7 +252,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
>   int vfio_spapr_remove_window(VFIOContainer *container,
>                                hwaddr offset_within_address_space);
>   
> -int vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
> +bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
>   void vfio_migration_exit(VFIODevice *vbasedev);
>   
>   #endif /* HW_VFIO_VFIO_COMMON_H */

Re: [PATCH v6 6/7] vfio/migration: Remove print of "Migration disabled"

[Cédric Le Goater]

On 7/3/23 09:15, Zhenzhong Duan wrote:
> Property enable_migration supports [on/off/auto].
> In ON mode, error pointer is passed to errp and logged.
> In OFF mode, we doesn't need to log "Migration disabled" as it's intentional.
> In AUTO mode, we should only ever see errors or warnings if the device
> supports migration and an error or incompatibility occurs while further
> probing or configuring it. Lack of support for migration shoundn't
> generate an error or warning.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Reviewed-by: Joao Martins <joao.m.martins@oracle.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/pci.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 9154dd929d07..eefd4ec330d9 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3209,7 +3209,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>       if (!pdev->failover_pair_id) {
>           ret = vfio_migration_realize(vbasedev, errp);
>           if (ret) {
> -            error_report("%s: Migration disabled", vbasedev->name);
>               goto out_deregister;
>           }
>       }

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

[Joao Martins]

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

Re: [PATCH v6 7/7] vfio/migration: Return bool type for vfio_migration_realize()

[Joao Martins]

On 03/07/2023 08:15, Zhenzhong Duan wrote:
> Make vfio_migration_realize() adhere to the convention of other realize()
> callbacks(like qdev_realize) by returning bool instead of int.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Reviewed-by: Joao Martins <joao.m.martins@oracle.com>

> ---
>  hw/vfio/migration.c           | 15 ++++++++++-----
>  hw/vfio/pci.c                 |  3 +--
>  include/hw/vfio/vfio-common.h |  2 +-
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index e3954570c853..2674f4bc472d 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -846,7 +846,12 @@ void vfio_reset_bytes_transferred(void)
>      bytes_transferred = 0;
>  }
>  
> -int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
> +/*
> + * Return true when either migration initialized or blocker registered.
> + * Currently only return false when adding blocker fails which will
> + * de-register vfio device.
> + */
> +bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>  {
>      Error *err = NULL;
>      int ret;
> @@ -854,7 +859,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>      if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
>          error_setg(&err, "%s: Migration is disabled for VFIO device",
>                     vbasedev->name);
> -        return vfio_block_migration(vbasedev, err, errp);
> +        return !vfio_block_migration(vbasedev, err, errp);
>      }
>  
>      ret = vfio_migration_init(vbasedev);
> @@ -869,7 +874,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>                         vbasedev->name, ret, strerror(-ret));
>          }
>  
> -        return vfio_block_migration(vbasedev, err, errp);
> +        return !vfio_block_migration(vbasedev, err, errp);
>      }
>  
>      if (!vbasedev->dirty_pages_supported) {
> @@ -896,7 +901,7 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>      }
>  
>      trace_vfio_migration_realize(vbasedev->name);
> -    return 0;
> +    return true;
>  
>  add_blocker:
>      ret = vfio_block_migration(vbasedev, err, errp);
> @@ -904,7 +909,7 @@ out_deinit:
>      if (ret) {
>          vfio_migration_deinit(vbasedev);
>      }
> -    return ret;
> +    return !ret;
>  }
>  
>  void vfio_migration_exit(VFIODevice *vbasedev)
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index eefd4ec330d9..68dd99283620 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3207,8 +3207,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>      }
>  
>      if (!pdev->failover_pair_id) {
> -        ret = vfio_migration_realize(vbasedev, errp);
> -        if (ret) {
> +        if (!vfio_migration_realize(vbasedev, errp)) {
>              goto out_deregister;
>          }
>      }
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 45167c8a8a54..da43d273524e 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -252,7 +252,7 @@ int vfio_spapr_create_window(VFIOContainer *container,
>  int vfio_spapr_remove_window(VFIOContainer *container,
>                               hwaddr offset_within_address_space);
>  
> -int vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
> +bool vfio_migration_realize(VFIODevice *vbasedev, Error **errp);
>  void vfio_migration_exit(VFIODevice *vbasedev);
>  
>  #endif /* HW_VFIO_VFIO_COMMON_H */

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

[Avihai Horon]

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
>

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

[Cédric Le Goater]

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

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

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

[Duan, Zhenzhong]

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

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

[Duan, Zhenzhong]

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