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

["Cédric Le Goater" <clg@kaod.org>]

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