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

[Cédric Le Goater]

Hello Avihai

On 1/10/23 15:08, Avihai Horon wrote:
> 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?
The patchset removes v1 later on, I think we are fine. I was reading it
sequentially and it felt like a weak spot.

All errors are translated in EOPNOTSUPP. That's always true for pre-v6.0
kernels, returning -ENOTTY, and v6.0+ kernels will do the same unless a
mlx5vf device is passthru. I still wonder if we should report some errors
for the ! -ENOTTY case. So the code below could be a good addition.

Thanks,

C.

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