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

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

[Zhenzhong Duan]

Hello,

PATCH4,5,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,6 fix them.

See patch description for details.

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

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: Return bool type for some vfio migration related
    functions
  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"

 hw/vfio/common.c              | 59 ++++-------------------------------
 hw/vfio/migration.c           | 59 +++++++++++++++++++++++------------
 hw/vfio/pci.c                 |  8 +++--
 include/hw/vfio/vfio-common.h |  7 ++---
 4 files changed, 53 insertions(+), 80 deletions(-)

-- 
2.34.1

[PATCH v5 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>
---
 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

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

[Joao Martins]

On 30/06/2023 08:36, Zhenzhong Duan wrote:
> 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>

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

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

[Cédric Le Goater]

On 6/30/23 09:36, Zhenzhong Duan wrote:
> 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: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


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

[PATCH v5 4/7] vfio/migration: Return bool type for some vfio migration related functions

[Zhenzhong Duan]

Include:
vfio_block_multiple_devices_migration(),
vfio_block_giommu_migration(),
vfio_block_migration(),
vfio_migration_realize().

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/common.c              | 16 ++++++++--------
 hw/vfio/migration.c           | 19 ++++++++++++-------
 include/hw/vfio/vfio-common.h |  6 +++---
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 77e2ee0e5c6e..00fef5aa08be 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -381,19 +381,19 @@ static unsigned int vfio_migratable_device_num(void)
     return device_num;
 }
 
-int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
+bool vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
 {
     int ret;
 
     if (multiple_devices_migration_blocker ||
         vfio_migratable_device_num() <= 1) {
-        return 0;
+        return true;
     }
 
     if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
         error_setg(errp, "Migration is currently not supported with multiple "
                          "VFIO devices");
-        return -EINVAL;
+        return false;
     }
 
     error_setg(&multiple_devices_migration_blocker,
@@ -405,7 +405,7 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
         multiple_devices_migration_blocker = NULL;
     }
 
-    return ret;
+    return !ret;
 }
 
 void vfio_unblock_multiple_devices_migration(void)
@@ -433,19 +433,19 @@ static bool vfio_viommu_preset(void)
     return false;
 }
 
-int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
+bool vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
 {
     int ret;
 
     if (giommu_migration_blocker ||
         !vfio_viommu_preset()) {
-        return 0;
+        return true;
     }
 
     if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
         error_setg(errp,
                    "Migration is currently not supported with vIOMMU enabled");
-        return -EINVAL;
+        return false;
     }
 
     error_setg(&giommu_migration_blocker,
@@ -456,7 +456,7 @@ int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
         giommu_migration_blocker = NULL;
     }
 
-    return ret;
+    return !ret;
 }
 
 void vfio_migration_finalize(void)
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 1db7d52ab2c1..09fa4714a085 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -802,13 +802,13 @@ static int vfio_migration_init(VFIODevice *vbasedev)
     return 0;
 }
 
-static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
+static bool vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
 {
     int ret;
 
     if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
         error_propagate(errp, err);
-        return -EINVAL;
+        return false;
     }
 
     vbasedev->migration_blocker = error_copy(err);
@@ -820,7 +820,7 @@ static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
         vbasedev->migration_blocker = NULL;
     }
 
-    return ret;
+    return !ret;
 }
 
 /* ---------------------------------------------------------------------- */
@@ -835,7 +835,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;
@@ -874,17 +879,17 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
     }
 
     ret = vfio_block_multiple_devices_migration(vbasedev, errp);
-    if (ret) {
+    if (!ret) {
         return ret;
     }
 
     ret = vfio_block_giommu_migration(vbasedev, errp);
-    if (ret) {
+    if (!ret) {
         return ret;
     }
 
     trace_vfio_migration_realize(vbasedev->name);
-    return 0;
+    return true;
 }
 
 void vfio_migration_exit(VFIODevice *vbasedev)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 93429b9abba0..b3014ece2edf 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -225,9 +225,9 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
 extern VFIOGroupList vfio_group_list;
 
 bool vfio_mig_active(void);
-int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
+bool 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_block_giommu_migration(VFIODevice *vbasedev, Error **errp);
 int64_t vfio_mig_bytes_transferred(void);
 void vfio_reset_bytes_transferred(void);
 
@@ -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);
 void vfio_migration_finalize(void);
 
-- 
2.34.1

Re: [PATCH v5 4/7] vfio/migration: Return bool type for some vfio migration related functions

[Joao Martins]

On 30/06/2023 08:36, Zhenzhong Duan wrote:
> Include:
> vfio_block_multiple_devices_migration(),
> vfio_block_giommu_migration(),
> vfio_block_migration(),
> vfio_migration_realize().
> 

Maybe some brief commit message would be useful as to why we are renaming it
("Migration disabled" no longer applies as that is being removed):

Make vfio_migration_realize() adhere to the convention of other realize()
callbacks by returning bool instead of int. In doing so, change also the the
functions used in vfio_migration_realize() return path to have bool too.

But see a remark below -- I think if you move the rename patch to be the last
one, you will have less to rename (?) Unless we really want to rename everything
that vfio_migration_realize() ends up calling to return bool. In which case my
comment then no longer applies. I generally favour not loosing error information
until last minute, but OTOH Error is provided so errors will be propagated in
some way.

> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/common.c              | 16 ++++++++--------
>  hw/vfio/migration.c           | 19 ++++++++++++-------
>  include/hw/vfio/vfio-common.h |  6 +++---
>  3 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 77e2ee0e5c6e..00fef5aa08be 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -381,19 +381,19 @@ static unsigned int vfio_migratable_device_num(void)
>      return device_num;
>  }
>  
> -int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
> +bool vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
>  {
>      int ret;
>  
>      if (multiple_devices_migration_blocker ||
>          vfio_migratable_device_num() <= 1) {
> -        return 0;
> +        return true;
>      }
>  
>      if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
>          error_setg(errp, "Migration is currently not supported with multiple "
>                           "VFIO devices");
> -        return -EINVAL;
> +        return false;
>      }
>  
>      error_setg(&multiple_devices_migration_blocker,
> @@ -405,7 +405,7 @@ int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp)
>          multiple_devices_migration_blocker = NULL;
>      }
>  
> -    return ret;
> +    return !ret;
>  }
>  
>  void vfio_unblock_multiple_devices_migration(void)
> @@ -433,19 +433,19 @@ static bool vfio_viommu_preset(void)
>      return false;
>  }
>  
> -int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
> +bool vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
>  {
>      int ret;
>  
>      if (giommu_migration_blocker ||
>          !vfio_viommu_preset()) {
> -        return 0;
> +        return true;
>      }
>  
>      if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
>          error_setg(errp,
>                     "Migration is currently not supported with vIOMMU enabled");
> -        return -EINVAL;
> +        return false;
>      }
>  
>      error_setg(&giommu_migration_blocker,
> @@ -456,7 +456,7 @@ int vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
>          giommu_migration_blocker = NULL;
>      }
>  
> -    return ret;
> +    return !ret;
>  }
>  
>  void vfio_migration_finalize(void)
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 1db7d52ab2c1..09fa4714a085 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -802,13 +802,13 @@ static int vfio_migration_init(VFIODevice *vbasedev)
>      return 0;
>  }
>  
> -static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
> +static bool vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
>  {
>      int ret;
>  
>      if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
>          error_propagate(errp, err);
> -        return -EINVAL;
> +        return false;
>      }
>  
>      vbasedev->migration_blocker = error_copy(err);
> @@ -820,7 +820,7 @@ static int vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
>          vbasedev->migration_blocker = NULL;
>      }
>  
> -    return ret;
> +    return !ret;
>  }
>  
>  /* ---------------------------------------------------------------------- */
> @@ -835,7 +835,12 @@ void vfio_reset_bytes_transferred(void)
>      bytes_transferred = 0;
>  }
> 

With your later patches where you remove vIOMMU migration blocker and have a
common return path in vfio_migration_realize() I suspect you will need to only
rename vfio_migration_realize() and you negate all the values accordingly in
that function, and thus hopefully reducing (...)

> -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;
> @@ -874,17 +879,17 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>      }
>  
>      ret = vfio_block_multiple_devices_migration(vbasedev, errp);
> -    if (ret) {
> +    if (!ret) {
>          return ret;
>      }
>  
>      ret = vfio_block_giommu_migration(vbasedev, errp);
> -    if (ret) {
> +    if (!ret) {
>          return ret;
>      }
>  
>      trace_vfio_migration_realize(vbasedev->name);
> -    return 0;
> +    return true;
>  }
>  

(...) to just this one last chunk?

>  void vfio_migration_exit(VFIODevice *vbasedev)
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 93429b9abba0..b3014ece2edf 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -225,9 +225,9 @@ typedef QLIST_HEAD(VFIOGroupList, VFIOGroup) VFIOGroupList;
>  extern VFIOGroupList vfio_group_list;
>  
>  bool vfio_mig_active(void);
> -int vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
> +bool 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_block_giommu_migration(VFIODevice *vbasedev, Error **errp);
>  int64_t vfio_mig_bytes_transferred(void);
>  void vfio_reset_bytes_transferred(void);
>  
> @@ -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);
>  void vfio_migration_finalize(void);
>  

RE: [PATCH v5 4/7] vfio/migration: Return bool type for some vfio migration related functions

[Duan, Zhenzhong]

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Sent: Friday, June 30, 2023 6:41 PM
>Subject: Re: [PATCH v5 4/7] vfio/migration: Return bool type for some vfio
>migration related functions
>
>On 30/06/2023 08:36, Zhenzhong Duan wrote:
>> Include:
>> vfio_block_multiple_devices_migration(),
>> vfio_block_giommu_migration(),
>> vfio_block_migration(),
>> vfio_migration_realize().
>>
>
>Maybe some brief commit message would be useful as to why we are
>renaming it
>("Migration disabled" no longer applies as that is being removed):
>
>Make vfio_migration_realize() adhere to the convention of other realize()
>callbacks by returning bool instead of int. In doing so, change also the the
>functions used in vfio_migration_realize() return path to have bool too.
>
>But see a remark below -- I think if you move the rename patch to be the last
>one, you will have less to rename (?) Unless we really want to rename
>everything
>that vfio_migration_realize() ends up calling to return bool. In which case my
>comment then no longer applies. I generally favour not loosing error
>information
>until last minute, but OTOH Error is provided so errors will be propagated in
>some way.
Make sense, I'll only rename vfio_migration_realize() and for other
internal functions, keep their error info in return value as you suggested.
This way we still meet Cedric's expectation of vfio_migration_realize()
following the qdev_realize() prototype.

Thanks
Zhenzhong

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

[Zhenzhong Duan]

vIOMMU migration blocker can be consolidated to per device default
blocker, then some vIOMMU global blocker related functions and data
could be removed.

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>
---
 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 00fef5aa08be..a8439447b990 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;
-}
-
-bool vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
-{
-    int ret;
-
-    if (giommu_migration_blocker ||
-        !vfio_viommu_preset()) {
-        return true;
-    }
-
-    if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
-        error_setg(errp,
-                   "Migration is currently not supported with vIOMMU enabled");
-        return false;
-    }
-
-    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 09fa4714a085..80509958f0d3 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -883,9 +883,10 @@ bool 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 b3014ece2edf..3c18572322fc 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);
 bool vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
 void vfio_unblock_multiple_devices_migration(void);
-bool 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,
 
 bool 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

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

[Joao Martins]

On 30/06/2023 08:36, Zhenzhong Duan wrote:
> vIOMMU migration blocker can be consolidated to per device default
> blocker, then some vIOMMU global blocker related functions and data
> could be removed.
> 
Perhaps expand to something like below on the why it justifies the consolidation:

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.
> 
Cool callout.

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

With above adjustment:

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 00fef5aa08be..a8439447b990 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;
> -}
> -
> -bool vfio_block_giommu_migration(VFIODevice *vbasedev, Error **errp)
> -{
> -    int ret;
> -
> -    if (giommu_migration_blocker ||
> -        !vfio_viommu_preset()) {
> -        return true;
> -    }
> -
> -    if (vbasedev->enable_migration == ON_OFF_AUTO_ON) {
> -        error_setg(errp,
> -                   "Migration is currently not supported with vIOMMU enabled");
> -        return false;
> -    }
> -
> -    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 09fa4714a085..80509958f0d3 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -883,9 +883,10 @@ bool 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 b3014ece2edf..3c18572322fc 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);
>  bool vfio_block_multiple_devices_migration(VFIODevice *vbasedev, Error **errp);
>  void vfio_unblock_multiple_devices_migration(void);
> -bool 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,
>  
>  bool 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 v5 5/7] vfio/migration: Change vIOMMU blocker from global to per device

[Duan, Zhenzhong]

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Sent: Friday, June 30, 2023 7:17 PM
>Subject: Re: [PATCH v5 5/7] vfio/migration: Change vIOMMU blocker from
>global to per device
>
>On 30/06/2023 08:36, Zhenzhong Duan wrote:
>> vIOMMU migration blocker can be consolidated to per device default
>> blocker, then some vIOMMU global blocker related functions and data
>> could be removed.
>>
>Perhaps expand to something like below on the why it justifies the
>consolidation:
>
>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.
Great, will use this words.

Thanks
Zhenzhong

[PATCH v5 6/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 | 37 +++++++++++++++++++++++++------------
 hw/vfio/pci.c       |  3 ++-
 2 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 80509958f0d3..2110c4dc21e2 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -802,6 +802,19 @@ static int vfio_migration_init(VFIODevice *vbasedev)
     return 0;
 }
 
+static void vfio_migration_deinit(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();
+    }
+}
+
 static bool vfio_block_migration(VFIODevice *vbasedev, Error *err, Error **errp)
 {
     int ret;
@@ -871,7 +884,7 @@ bool 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",
@@ -880,30 +893,30 @@ bool 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 true;
+
+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) {
         migrate_del_blocker(vbasedev->migration_blocker);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c2cf7454ece6..9cf70c1965d3 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3208,8 +3208,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
 
     if (!pdev->failover_pair_id) {
         ret = vfio_migration_realize(vbasedev, errp);
-        if (ret) {
+        if (!ret) {
             error_report("%s: Migration disabled", vbasedev->name);
+            goto out_deregister;
         }
     }
 
-- 
2.34.1

[PATCH v5 7/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>
---
 hw/vfio/pci.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 9cf70c1965d3..1c969fccc543 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

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

[Joao Martins]

On 30/06/2023 08:36, 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>

> ---
>  hw/vfio/pci.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 9cf70c1965d3..1c969fccc543 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;
>          }
>      }