On Tue, 16 Jul 2019 14:56:32 -0600 Alex Williamson <alex.william...@redhat.com> wrote:
> On Tue, 9 Jul 2019 15:19:08 +0530 > Kirti Wankhede <kwankh...@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?