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

[Kirti Wankhede]

On 7/23/2019 5:43 PM, Cornelia Huck wrote:
> On Tue, 16 Jul 2019 14:56:32 -0600
> Alex Williamson <address@hidden> wrote:
> 
>> On Tue, 9 Jul 2019 15:19:08 +0530
>> Kirti Wankhede <address@hidden> 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?
>