On 7/23/2019 5:43 PM, Cornelia Huck wrote:
> 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'?
>
Yes.
>>> + * 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?
>
Yes.
>>> + * - 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 this will be valid state in postcopy case, though I'm not very sure.
>>
>> 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.
>
Updating comment in next version.
Thanks,
Kirti
> (...)
>
>>> +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?
>
