Re: [PATCH v9 QEMU 15/15] vfio: Make vfio-pci device migration capable.

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 12 Nov 2019 22:35:24 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> If device is not failover primary device call vfio_migration_probe()
> and vfio_migration_finalize() functions for vfio-pci device to enable
> migration for vfio PCI device which support migration.
> Removed vfio_pci_vmstate structure.
> Removed migration blocker from VFIO PCI device specific structure and use
> migration blocker from generic structure of  VFIO device.
>
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Neo Jia <cjia@nvidia.com>
> ---
>  hw/vfio/pci.c | 30 +++++++++++-------------------
>  hw/vfio/pci.h |  1 -
>  2 files changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 2c22cca0c3be..3d2ebc7abfdc 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2909,21 +2909,11 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          return;
>      }
>  
> -    if (!pdev->failover_pair_id) {
> -        error_setg(&vdev->migration_blocker,
> -                "VFIO device doesn't support migration");
> -        ret = migrate_add_blocker(vdev->migration_blocker, &err);
> -        if (err) {
> -            error_propagate(errp, err);
> -            error_free(vdev->migration_blocker);
> -            return;
> -        }
> -    }
> -
>      vdev->vbasedev.name = g_path_get_basename(vdev->vbasedev.sysfsdev);
>      vdev->vbasedev.ops = &vfio_pci_ops;
>      vdev->vbasedev.type = VFIO_DEVICE_TYPE_PCI;
>      vdev->vbasedev.dev = DEVICE(vdev);
> +    vdev->vbasedev.device_state = 0;

But it's not.

>  
>      tmp = g_strdup_printf("%s/iommu_group", vdev->vbasedev.sysfsdev);
>      len = readlink(tmp, group_path, sizeof(group_path));
> @@ -3184,6 +3174,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          }
>      }
>  
> +    if (!pdev->failover_pair_id) {
> +        ret = vfio_migration_probe(&vdev->vbasedev, errp);

Hmm, I suppose this prevents us from breaking failover previously, but
does it make more sense to enable it earlier in the series, even before
it's feature complete so that we can iteratively debug?

> +        if (ret) {
> +                error_report("%s: Failed to setup for migration",
> +                             vdev->vbasedev.name);
> +        }
> +    }
> +
>      vfio_register_err_notifier(vdev);
>      vfio_register_req_notifier(vdev);
>      vfio_setup_resetfn_quirk(vdev);
> @@ -3196,10 +3194,6 @@ out_teardown:
>      vfio_bars_exit(vdev);
>  error:
>      error_prepend(errp, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> -    if (vdev->migration_blocker) {
> -        migrate_del_blocker(vdev->migration_blocker);
> -        error_free(vdev->migration_blocker);
> -    }
>  }
>  
>  static void vfio_instance_finalize(Object *obj)
> @@ -3207,14 +3201,11 @@ static void vfio_instance_finalize(Object *obj)
>      VFIOPCIDevice *vdev = PCI_VFIO(obj);
>      VFIOGroup *group = vdev->vbasedev.group;
>  
> +    vdev->vbasedev.device_state = 0;

Nor is this accurate or meaningful unless we do actually stop the
device.

>      vfio_display_finalize(vdev);
>      vfio_bars_finalize(vdev);
>      g_free(vdev->emulated_config_bits);
>      g_free(vdev->rom);
> -    if (vdev->migration_blocker) {
> -        migrate_del_blocker(vdev->migration_blocker);
> -        error_free(vdev->migration_blocker);
> -    }
>      /*
>       * XXX Leaking igd_opregion is not an oversight, we can't remove the
>       * fw_cfg entry therefore leaking this allocation seems like the safest
> @@ -3239,6 +3230,7 @@ static void vfio_exitfn(PCIDevice *pdev)
>      }
>      vfio_teardown_msi(vdev);
>      vfio_bars_exit(vdev);
> +    vfio_migration_finalize(&vdev->vbasedev);
>  }
>  
>  static void vfio_pci_reset(DeviceState *dev)
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index b329d50338b5..834a90d64686 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -168,7 +168,6 @@ typedef struct VFIOPCIDevice {
>      bool no_vfio_ioeventfd;
>      bool enable_ramfb;
>      VFIODisplay *dpy;
> -    Error *migration_blocker;
>  } VFIOPCIDevice;
>  
>  uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);