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.d...@intel.com>
With above adjustment:
Reviewed-by: Joao Martins <joao.m.mart...@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 */
