Re: [PATCH 3/3] vfio/migration: Make VFIO migration non-experimental

["Cédric Le Goater" <clg@redhat.com>]

Hello Avihai,

On 6/26/23 10:23, Avihai Horon wrote:
> The major parts of VFIO migration are supported today in QEMU. This
> includes basic VFIO migration, device dirty page tracking and precopy
> support.
> 
> Thus, at this point in time, it seems appropriate to make VFIO migration
> non-experimental: remove the x prefix from enable_migration property,
> change it to ON_OFF_AUTO and let the default value be AUTO.
> 
> In addition, make the following adjustments:
> 1. Require device dirty tracking support when enable_migration is AUTO
>     (i.e., not explicitly enabled). This is because device dirty tracking
>     is currently the only method to do dirty page tracking, which is
>     essential for migrating in a reasonable downtime. 

hmm, I don't think QEMU should decide to disable a feature for all
devices supposedly because it could be slow for some. That's too
restrictive. What about devices with have small states ? for which
the downtime would be reasonable even without device dirty tracking
support.


> Setting
>     enable_migration to ON will not require device dirty tracking.
> 2. Make migration blocker messages more elaborate.
> 3. Remove error prints in vfio_migration_query_flags().
> 4. Remove a redundant assignment in vfio_migration_realize().
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> ---
>   include/hw/vfio/vfio-common.h |  2 +-
>   hw/vfio/migration.c           | 29 ++++++++++++++++-------------
>   hw/vfio/pci.c                 |  4 ++--
>   3 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b4c28f318f..387eabde60 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -139,7 +139,7 @@ typedef struct VFIODevice {
>       bool needs_reset;
>       bool no_mmap;
>       bool ram_block_discard_allowed;
> -    bool enable_migration;
> +    OnOffAuto enable_migration;
>       VFIODeviceOps *ops;
>       unsigned int num_irqs;
>       unsigned int num_regions;
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 79eb81dfd7..d8e0848635 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -731,14 +731,6 @@ static int vfio_migration_query_flags(VFIODevice *vbasedev, uint64_t *mig_flags)
>       feature->argsz = sizeof(buf);
>       feature->flags = VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_MIGRATION;
>       if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
> -        if (errno == ENOTTY) {
> -            error_report("%s: VFIO migration is not supported in kernel",
> -                         vbasedev->name);
> -        } else {
> -            error_report("%s: Failed to query VFIO migration support, err: %s",
> -                         vbasedev->name, strerror(errno));
> -        }
> -
>           return -errno;
>       }
>   
> @@ -831,14 +823,28 @@ void vfio_reset_bytes_transferred(void)
>   
>   int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>   {
> -    int ret = -ENOTSUP;
> +    int ret;
>   
> -    if (!vbasedev->enable_migration) {
> +    if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
> +        error_setg(&vbasedev->migration_blocker,
> +                   "%s: Migration is disabled for VFIO device", vbasedev->name);
>           goto add_blocker;
>       }
>   
>       ret = vfio_migration_init(vbasedev);
>       if (ret) {

It would be good to keep the message for 'errno == ENOTTY' as it was in
vfio_migration_query_flags(). When migration fails, it is an important
information to know that it is because the VFIO PCI host device driver
doesn't support the feature. The root cause could be deep below in FW or
how the VF was set up.

> +        error_setg(&vbasedev->migration_blocker,
> +                   "%s: Migration couldn't be initialized for VFIO device, "
> +                   "err: %d (%s)",
> +                   vbasedev->name, ret, strerror(-ret));
> +        goto add_blocker;
> +    }
> +
> +    if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO &&
> +        !vbasedev->dirty_pages_supported) {

I don't agree with this test.

> +        error_setg(&vbasedev->migration_blocker,
> +                   "%s: VFIO device doesn't support device dirty tracking",
> +                   vbasedev->name);
>           goto add_blocker;
>       }
I agree that with ON_OFF_AUTO_AUTO, errors at realize time should be recorded
in a migration blocker. What about the ON_OFF_AUTO_ON case ? If migration was
explicitly requested for the device and the conditions on the host are not met,
I think realize should fail and the machine abort.

Thanks,

C.



> @@ -856,9 +862,6 @@ int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>       return 0;
>   
>   add_blocker:
> -    error_setg(&vbasedev->migration_blocker,
> -               "VFIO device doesn't support migration");
> -
>       ret = migrate_add_blocker(vbasedev->migration_blocker, errp);
>       if (ret < 0) {
>           error_free(vbasedev->migration_blocker);
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 73874a94de..48584e3b01 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3347,8 +3347,8 @@ static Property vfio_pci_dev_properties[] = {
>                       VFIO_FEATURE_ENABLE_REQ_BIT, true),
>       DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
>                       VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
> -    DEFINE_PROP_BOOL("x-enable-migration", VFIOPCIDevice,
> -                     vbasedev.enable_migration, false),
> +    DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
> +                            vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
>       DEFINE_PROP_BOOL("x-no-mmap", VFIOPCIDevice, vbasedev.no_mmap, false),
>       DEFINE_PROP_BOOL("x-balloon-allowed", VFIOPCIDevice,
>                        vbasedev.ram_block_discard_allowed, false),