Re: [Qemu-devel] [PATCH v7 01/13] vfio: KABI for migration interface

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 16 Jul 2019 14:56:32 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Tue, 9 Jul 2019 15:19:08 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:

I'm still a bit unsure about the device_state bit handling as well.

> > + * device_state: (read/write)
> > + *      To indicate vendor driver the state VFIO device should be transitioned
> > + *      to. If device state transition fails, write on this field return error.

Does 'device state transition fails' include 'the device state written
was invalid'?

> > + *      It consists of 3 bits:
> > + *      - If bit 0 set, indicates _RUNNING state. When its reset, that indicates
> > + *        _STOPPED state. When device is changed to _STOPPED, driver should stop
> > + *        device before write() returns.

So _STOPPED is always !_RUNNING, regardless of which other bits are set?

> > + *      - If bit 1 set, indicates _SAVING state.
> > + *      - If bit 2 set, indicates _RESUMING state.
> > + *      _SAVING and _RESUMING set at the same time is invalid state.  

What about _RUNNING | _RESUMING -- does that make sense?

> 
> I think in the previous version there was a question of how we handle
> yet-to-be-defined bits.  For instance, if we defined a
> SUBTYPE_MIGRATIONv2 with the intention of making it backwards
> compatible with this version, do we declare the undefined bits as
> preserved so that the user should do a read-modify-write operation?

Or can we state that undefined bits are ignored, and may or may not
preserved, so that we can skip the read-modify-write requirement? v1
and v2 can hopefully be distinguished in a different way.

(...)

> > +struct vfio_device_migration_info {
> > +        __u32 device_state;         /* VFIO device state */
> > +#define VFIO_DEVICE_STATE_RUNNING   (1 << 0)
> > +#define VFIO_DEVICE_STATE_SAVING    (1 << 1)
> > +#define VFIO_DEVICE_STATE_RESUMING  (1 << 2)
> > +#define VFIO_DEVICE_STATE_MASK      (VFIO_DEVICE_STATE_RUNNING | \
> > +                                     VFIO_DEVICE_STATE_SAVING | \
> > +                                     VFIO_DEVICE_STATE_RESUMING)  
> 
> Yes, we have the mask in here now, but no mention above how the user
> should handle undefined bits.  Thanks,
> 
> Alex
> 
> > +#define VFIO_DEVICE_STATE_INVALID   (VFIO_DEVICE_STATE_SAVING | \
> > +                                     VFIO_DEVICE_STATE_RESUMING)

As mentioned above, does _RESUMING | _RUNNING make sense?