Re: [PATCH v5 10/14] vfio/migration: Implement VFIO migration protocol v2

[Avihai Horon]

On 09/01/2023 20:36, Jason Gunthorpe wrote:
> On Mon, Jan 09, 2023 at 06:27:21PM +0100, Cédric Le Goater wrote:
> > also, in vfio_migration_query_flags() :
> >
> >    +static int vfio_migration_query_flags(VFIODevice *vbasedev, uint64_t 
> > *mig_flags)
> >    +{
> >    +    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
> >    +                                  sizeof(struct 
> > vfio_device_feature_migration),
> >    +                              sizeof(uint64_t))] = {};
> >    +    struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
> >    +    struct vfio_device_feature_migration *mig =
> >    +        (struct vfio_device_feature_migration *)feature->data;
> >    +
> >    +    feature->argsz = sizeof(buf);
> >    +    feature->flags = VFIO_DEVICE_FEATURE_GET | 
> > VFIO_DEVICE_FEATURE_MIGRATION;
> >    +    if (ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature)) {
> >    +        return -EOPNOTSUPP;
> >    +    }
> >    +
> >    +    *mig_flags = mig->flags;
> >    +
> >    +    return 0;
> >    +}
> >
> >
> > The code is using any possible error returned by the VFIO_DEVICE_FEATURE
> > ioctl to distinguish protocol v1 from v2.
> I'm comfortable with that from a kernel perspective.
>
> There is no such thing as this API failing in the kernel but userspace
> should continue on, no matter what the error code. So always failing
> here is correct.
>
> About the only thing you might want to do is convert anything other
> than ENOTTY into a hard qemu failure similar to failing to open
> /dev/vfio or something - it means something has gone really
> wrong.. But that is pretty obscure stuff

Hi Cedric,

With Jason's input, is it ok by you to leave the code as is?

if not, would this be fine?

+static int vfio_migration_query_flags(VFIODevice *vbasedev, uint64_t 
*mig_flags)
+{
+    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
+                                  sizeof(struct 
vfio_device_feature_migration),
+                              sizeof(uint64_t))] = {};
+    struct vfio_device_feature *feature = (struct vfio_device_feature 
*)buf;
+    struct vfio_device_feature_migration *mig =
+        (struct vfio_device_feature_migration *)feature->data;
+
+    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;
+    }
+
+    *mig_flags = mig->flags;
+
+    return 0;
+}
+

and then in vfio_migration_init() prior v1 removal:

+    ret = vfio_migration_query_flags(vbasedev, &mig_flags);
+    if (!ret) {
+        /* Migration v2 */
+    } else if (ret == -ENOTTY) {
+        /* Migration v1 */
+    } else {
+        return ret;
+    }

and after v1 removal vfio_migration_init() will be:

    ret = vfio_migration_query_flags(vbasedev, &mig_flags);
    if (ret) {
        return ret;

    }

Thanks.