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

[Cédric Le Goater]

On 1/9/23 16:12, Avihai Horon wrote:
> On 09/01/2023 12:20, Cédric Le Goater wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > Hello Avihai,
> >
> >
> > On 12/29/22 12:03, Avihai Horon wrote:
> > > +static int vfio_save_setup(QEMUFile *f, void *opaque)
> > > +{
> > > +    VFIODevice *vbasedev = opaque;
> > > +    VFIOMigration *migration = vbasedev->migration;
> > > +    uint64_t stop_copy_size;
> > > +
> > > +    qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
> > > +
> > > +    if (vfio_query_stop_copy_size(vbasedev, &stop_copy_size)) {
> > > +        stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
> > > +    }
> > > +    migration->data_buffer_size = MIN(VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE,
> > > +                                      stop_copy_size);
> > > +    migration->data_buffer = g_try_malloc0(migration->data_buffer_size);
> > > +    if (!migration->data_buffer) {
> > > +        error_report("%s: Failed to allocate migration data buffer",
> > > +                     vbasedev->name);
> > > +        return -ENOMEM;
> > > +    }
> > > +
> > > +    trace_vfio_save_setup(vbasedev->name, migration->data_buffer_size);
> > > +
> > > +    qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> > > +
> > > +    return qemu_file_get_error(f);
> > > +}
> > > +
> >
> > This fails to compile with :
> >
> >   gcc version 12.2.1 20221121 (Red Hat 12.2.1-4) (GCC) complains with :
> >
> >
> >   ../include/qemu/osdep.h:315:22: error: ‘stop_copy_size’ may be used 
> > uninitialized [-Werror=maybe-uninitialized]
> >     315 |         _a < _b ? _a : _b;                              \
> >         |                      ^
> >   ../hw/vfio/migration.c:262:14: note: ‘stop_copy_size’ was declared here
> >     262 |     uint64_t stop_copy_size;
> >         |              ^~~~~~~~~~~~~~
> >   cc1: all warnings being treated as errors
> >
> > May be rework the code slightly to avoid the breakage :
> >
> > +++ qemu.git/hw/vfio/migration.c
> > @@ -259,13 +259,11 @@ static int vfio_save_setup(QEMUFile *f,
> >  {
> >      VFIODevice *vbasedev = opaque;
> >      VFIOMigration *migration = vbasedev->migration;
> > -    uint64_t stop_copy_size;
> > +    uint64_t stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
> >
> >      qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
> >
> > -    if (vfio_query_stop_copy_size(vbasedev, &stop_copy_size)) {
> > -        stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
> > -    }
> > +    vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
> >      migration->data_buffer_size = MIN(VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE,
> >                                        stop_copy_size);
> >      migration->data_buffer = g_try_malloc0(migration->data_buffer_size);
> >
> >
> > and report the error in vfio_query_stop_copy_size()
> Thanks, Cedric.
>
> There is another similar case in vfio_save_pending().
> I will fix both of them.


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.

Couldn't we use ENOTTY  ? I think a pre-v6.0 kernel would return this errno.
Some error report would be good to have.

Thanks,

C.