Re: [PATCH v10 Kernel 1/5] vfio: KABI for migration interface for device state

[Kirti Wankhede <kwankhede@nvidia.com>]

On 12/19/2019 10:57 PM, Alex Williamson wrote:

<Snip>


>>>>>> + * 2. When VFIO application save state or suspend application, VFIO device
>>>>>> + *    state transition is from _RUNNING to stop-and-copy state and then to
>>>>>> + *    _STOP.
>>>>>> + *    On state transition from _RUNNING to stop-and-copy, driver must
>>>>>> + *    stop device, save device state and send it to application through
>>>>>> + *    migration region.
>>>>>> + *    On _RUNNING to stop-and-copy state transition failure, application should
>>>>>> + *    set VFIO device state to _RUNNING.
>>>>>
>>>>> A state transition failure means that the user's write to device_state
>>>>> failed, so is it the user's responsibility to set the next state?
>>>>
>>>> Right.
>>>
>>> If a transition failure occurs, ie. errno from write(2), what value is
>>> reported by a read(2) of device_state in the interim between the failure
>>> and a next state written by the user?
>>
>> Since state transition has failed, driver should return previous state.
>>
>>> If this is a valid state,
>>> wouldn't it be reasonable for the user to assume the device is already
>>> operating in that state?
> 
> This ^^^
> 
>>   If it's an invalid state, do we need to
>>> define the use cases for those invalid states?  If the user needs to
>>> set the state back to _RUNNING, that suggests the device might be
>>> stopped, which has implications beyond the migration state.
>>>    
>>
>> Not necessarily stopped. For example, during live migration:
>>
>> *              _RESUMING  _RUNNING    Pre-copy    Stop-and-copy   _STOP
>> *                (100b)     (001b)     (011b)        (010b)       (000b)
>> *
>> * 3. Save state during live migration
>> *                             |----------->|------------>|---------->|
>>
>> on any state transition failure, user should set _RUNNING state.
>> pre-copy (011b) -> stop-and-copy(010b)  =====> _SAVING flag is cleared
>> and device returned back to _RUNNING.
>> Stop-and-copy(010b) -> STOP (000b) ====> device is already stopped.
> 
> IMO, the user may modify the state, but the vendor driver should report
> the current state of the device via device_state, which the user may
> read after a transition error occurs.  The spec lacks a provision for
> indicating the device is in a non-functional state.
>

Are you proposing to add a bit in device state to report error?

#define VFIO_DEVICE_STATE_ERROR  (1 << 3)

which can be set by vendor driver and when its set, other bits set/clear 
doesn't matter.


>>>>>    Why
>>>>> is it necessarily _RUNNING vs _STOP?
>>>>>      
>>>>
>>>> While changing From pre-copy to stop-and-copy transition, device is
>>>> still running, only saving of device state started. Now if transition to
>>>> stop-and-copy fails, from user point of view application or VM is still
>>>> running, device state should be set to _RUNNING so that whatever the
>>>> application/VM is running should continue at source.
>>>
>>> Seems it's the users discretion whether to consider this continuable or
>>> fatal, the vfio interface specification should support a given usage
>>> model, not prescribe it.
>>>    
>>
>> Updating comment.
>>
>>>>>> + * 3. In VFIO application live migration, state transition is from _RUNNING
>>>>>> + *    to pre-copy to stop-and-copy to _STOP.
>>>>>> + *    On state transition from _RUNNING to pre-copy, driver should start
>>>>>> + *    gathering device state while application is still running and send device
>>>>>> + *    state data to application through migration region.
>>>>>> + *    On state transition from pre-copy to stop-and-copy, driver must stop
>>>>>> + *    device, save device state and send it to application through migration
>>>>>> + *    region.
>>>>>> + *    On any failure during any of these state transition, VFIO device state
>>>>>> + *    should be set to _RUNNING.
>>>>>
>>>>> Same comment as above regarding next state on failure.
>>>>>       
>>>>
>>>> If application or VM migration fails, it should continue to run at
>>>> source. In case of VM, guest user isn't aware of migration, and from his
>>>> point VM should be running.
>>>
>>> vfio is not prescribing the migration semantics to userspace, it's
>>> presenting an interface that support the user semantics.  Therefore,
>>> while it's useful to understand the expected usage model, I think we
>>> also need a mechanism that the user can always determine the
>>> device_state after a fault
>>
>> If state transition fails, device is in previous state and driver should
>> return previous state
> 
> Then why is it stated that the user needs to set the _RUNNING state?
> It's the user's choice.  But I do think we're lacking a state to
> indicate an internal fault.
>


If device state it at pre-copy state (011b).
Transition, i.e., write to device state as stop-and-copy state (010b) 
failed, then by previous state I meant device should return pre-copy 
state(011b), i.e. previous state which was successfully set, or as you 
said current state which was successfully set.


>>> and allowable state transitions independent
>>> of the expected usage model.
>>
>> Do you mean to define array of ['from','to'], same as runstate
>> transition array in QEMU?
>>    static const RunStateTransition runstate_transitions_def[]
> 
> I'm thinking that independent of expected QEMU usage models, are there
> any invalid transitions or is every state reachable from every other
> state.  I'm afraid this design is so focused on a specific usage model
> that vendor drivers are going to fall over if the user invokes a
> transition outside of those listed above.  If there are invalid
> transitions, those should be listed so they can be handled
> consistently.  If there are no invalid transitions, it should be noted
> in the spec to encourage vendor drivers to expect this.
> 

I think vendor driver can decide which state transitions it can support, 
rather than defining/prescribing that all.
Suppose, if vendor driver doesn't want to support save-restore 
functionality, then vendor driver can return error -EINVAL for write() 
operation on device_state for transition from _RUNNING to 
stop-and-copy(010b) state.

>>> For example, I think a user should always
>>> be allowed to transition a device to stopped regardless of the expected
>>> migration flow.  An error might have occurred elsewhere and we want to
>>> stop everything for debugging.  I think it's also allowable to switch
>>> directly from running to stop-and-copy, for example to save and resume
>>> a VM offline.
>>>      
>>>>> Also, it seems like it's the vendor driver's discretion to actually
>>>>> provide data during the pre-copy phase.  As we've defined it, the
>>>>> vendor driver needs to participate in the migration region regardless,
>>>>> they might just always report no pending_bytes until we enter
>>>>> stop-and-copy.
>>>>>       
>>>>
>>>> Yes. And if pending_bytes are reported as 0 in pre-copy by vendor driver
>>>> then QEMU doesn't reiterate for that device.
>>>
>>> Maybe we can state that as the expected mechanism to avoid a vendor
>>> driver trying to invent alternative means, ex. failing transition to
>>> pre-copy, requesting new flags, etc.
>>>    
>>
>> Isn't Sequence to be followed below sufficient to state that?
> 
> I think we understand it because we've been discussing it so long, but
> without that background it could be subtle.
>   
>>>>>> + * 4. To start resuming phase, VFIO device state should be transitioned from
>>>>>> + *    _RUNNING to _RESUMING state.
>>>>>> + *    In _RESUMING state, driver should use received device state data through
>>>>>> + *    migration region to resume device.
>>>>>> + *    On failure during this state transition, application should set _RUNNING
>>>>>> + *    state.
>>>>>
>>>>> Same comment regarding setting next state after failure.
>>>>
>>>> If device couldn't be transitioned to _RESUMING, then it should be set
>>>> to default state, that is _RUNNING.
>>>>   
>>>>>       
>>>>>> + * 5. On providing saved device data to driver, appliation should change state
>>>>>> + *    from _RESUMING to _RUNNING.
>>>>>> + *    On failure to transition to _RUNNING state, VFIO application should reset
>>>>>> + *    the device and set _RUNNING state so that device doesn't remain in unknown
>>>>>> + *    or bad state. On reset, driver must reset device and device should be
>>>>>> + *    available in default usable state.
>>>>>
>>>>> Didn't we discuss that the reset ioctl should return the device to the
>>>>> initial state, including the transition to _RUNNING?
>>>>
>>>> Yes, that's default usable state, rewording it to initial state.
>>>>   
>>>>>    Also, as above,
>>>>> it's the user write that triggers the failure, this register is listed
>>>>> as read-write, so what value does the vendor driver report for the
>>>>> state when read after a transition failure?  Is it reported as _RESUMING
>>>>> as it was prior to the attempted transition, or may the invalid states
>>>>> be used by the vendor driver to indicate the device is broken?
>>>>>       
>>>>
>>>> If transition as failed, device should report its previous state and
>>>> reset device should bring back to usable _RUNNING state.
>>>
>>> If device_state reports previous state then user should reasonably
>>> infer that the device is already in that sate without a need for them
>>> to set it, IMO.
>>
>> But if there is any error in read()/write() then user should device
>> which next state device should be put in, which would be different that
>> previous state.
> 
> That's a different answer than telling the user the next state should
> be _RUNNING.
> 
>>>>>> + *
>>>>>> + * pending bytes: (read only)
>>>>>> + *      Number of pending bytes yet to be migrated from vendor driver
>>>>>> + *
>>>>>> + * data_offset: (read only)
>>>>>> + *      User application should read data_offset in migration region from where
>>>>>> + *      user application should read device data during _SAVING state or write
>>>>>> + *      device data during _RESUMING state. See below for detail of sequence to
>>>>>> + *      be followed.
>>>>>> + *
>>>>>> + * data_size: (read/write)
>>>>>> + *      User application should read data_size to get size of data copied in
>>>>>> + *      bytes in migration region during _SAVING state and write size of data
>>>>>> + *      copied in bytes in migration region during _RESUMING state.
>>>>>> + *
>>>>>> + * Migration region looks like:
>>>>>> + *  ------------------------------------------------------------------
>>>>>> + * |vfio_device_migration_info|    data section                      |
>>>>>> + * |                          |     ///////////////////////////////  |
>>>>>> + * ------------------------------------------------------------------
>>>>>> + *   ^                              ^
>>>>>> + *  offset 0-trapped part        data_offset
>>>>>> + *
>>>>>> + * Structure vfio_device_migration_info is always followed by data section in
>>>>>> + * the region, so data_offset will always be non-0. Offset from where data is
>>>>>> + * copied is decided by kernel driver, data section can be trapped or mapped
>>>>>> + * or partitioned, depending on how kernel driver defines data section.
>>>>>> + * Data section partition can be defined as mapped by sparse mmap capability.
>>>>>> + * If mmapped, then data_offset should be page aligned, where as initial section
>>>>>> + * which contain vfio_device_migration_info structure might not end at offset
>>>>>> + * which is page aligned. The user is not required to access via mmap regardless
>>>>>> + * of the region mmap capabilities.
>>>>>> + * Vendor driver should decide whether to partition data section and how to
>>>>>> + * partition the data section. Vendor driver should return data_offset
>>>>>> + * accordingly.
>>>>>> + *
>>>>>> + * Sequence to be followed for _SAVING|_RUNNING device state or pre-copy phase
>>>>>> + * and for _SAVING device state or stop-and-copy phase:
>>>>>> + * a. read pending_bytes, indicates start of new iteration to get device data.
>>>>>> + *    If there was previous iteration, then this read operation indicates
>>>>>> + *    previous iteration is done. If pending_bytes > 0, go through below steps.
>>>>>> + * b. read data_offset, indicates kernel driver to make data available through
>>>>>> + *    data section. Kernel driver should return this read operation only after
>>>>>> + *    data is available from (region + data_offset) to (region + data_offset +
>>>>>> + *    data_size).
>>>>>> + * c. read data_size, amount of data in bytes available through migration
>>>>>> + *    region.
>>>>>> + * d. read data of data_size bytes from (region + data_offset) from migration
>>>>>> + *    region.
>>>>>> + * e. process data.
>>>>>> + * f. Loop through a to e.
>>>>>
>>>>> It seems we always need to end an iteration by reading pending_bytes to
>>>>> signal to the vendor driver to release resources, so should the end of
>>>>> the loop be:
>>>>>
>>>>> e. Read pending_bytes
>>>>> f. Goto b. or optionally restart next iteration at a.
>>>>>
>>>>> I think this is defined such that reading data_offset commits resources
>>>>> and reading pending_bytes frees them, allowing userspace to restart at
>>>>> reading pending_bytes with no side-effects.  Therefore reading
>>>>> pending_bytes repeatedly is supported.  Is the same true for
>>>>> data_offset and data_size?  It seems reasonable that the vendor driver
>>>>> can simply return offset and size for the current buffer if the user
>>>>> reads these more than once.
>>>>>      
>>>>
>>>> Right.
>>>
>>> Can we add that to the spec?
>>>    
>>
>> ok.
>>
>>>>> How is a protocol or device error signaled?  For example, we can have a
>>>>> user error where they read data_size before data_offset.  Should the
>>>>> vendor driver generate a fault reading data_size in this case.  We can
>>>>> also have internal errors in the vendor driver, should the vendor
>>>>> driver use a special errno or update device_state autonomously to
>>>>> indicate such an error?
>>>>
>>>> If there is any error during the sequence, vendor driver can return
>>>> error code for next read/write operation, that will terminate the loop
>>>> and migration would fail.
>>>
>>> Please add to spec.
>>>    
>>
>> Ok
>>
>>>>> I believe it's also part of the intended protocol that the user can
>>>>> transition from _SAVING|_RUNNING to _SAVING at any point, regardless of
>>>>> pending_bytes.  This should be noted.
>>>>>       
>>>>
>>>> Ok. Updating comment.
>>>>   
>>>>>> + *
>>>>>> + * Sequence to be followed while _RESUMING device state:
>>>>>> + * While data for this device is available, repeat below steps:
>>>>>> + * a. read data_offset from where user application should write data.
>>>>>> + * b. write data of data_size to migration region from data_offset.
>>>>>
>>>>> Whose's data_size, the _SAVING end or the _RESUMING end?  I think this
>>>>> is intended to be the transaction size from the _SAVING source,
>>>>
>>>> Not necessarily. data_size could be MIN(transaction size of source,
>>>> migration data section). If migration data section is smaller than data
>>>> packet size at source, then it has to be broken and iteratively sent.
>>>
>>> So you're saying that a transaction from the source is divisible by the
>>> user under certain conditions.  What other conditions exist?
>>
>> I don't think there are any other conditions than above.
>>
>>>   Can the
>>> user decide arbitrary sizes less than the MIN() stated above?  This
>>> needs to be specified.
>>>   
>>
>> No, User can't decide arbitrary sizes.
> 
> TBH, I'd expect a vendor driver that offers a different migration
> region size, such that it becomes the user's responsibility to split
> transactions should just claim it's not compatible with the source, as
> determined by the previously defined compatibility protocol.  If we
> really need this requirement, it needs to be justified and the exact
> conditions under which the user performs this needs to be specified.
> 

Let User decide whether it wants to support different migration region 
sizes at source and destination or not instead of putting hard requirement.

>>>>> but it
>>>>> could easily be misinterpreted as reading data_size on the _RESUMING
>>>>> end.
>>>>>       
>>>>>> + * c. write data_size which indicates vendor driver that data is written in
>>>>>> + *    staging buffer. Vendor driver should read this data from migration
>>>>>> + *    region and resume device's state.
>>>>>
>>>>> I think we also need to define the error protocol.  The user could
>>>>> mis-order transactions or there could be an internal error in the
>>>>> vendor driver or device.  Are all read(2)/write(2) operations
>>>>> susceptible to defined errnos to signal this?
>>>>
>>>> Yes.
>>>
>>> And those defined errnos are specified...
>>>    
>>
>> Those could be standard errors like -EINVAL, ENOMEM....
> 
> I thought we might specify specific errors to consistently indicate
> non-continuable faults among vendor drivers.  Is anything other than
> -EAGAIN considered non-fatal to the operation?  For example, could
> EEXIST indicate duplicate data that the user has already written but
> not be considered a fatal error?  Would EFAULT perhaps indicate a
> continuable ordering error?  If any fault indicates the save/resume has
> failed, shouldn't the user be able to see the device is in such a state
> by reading device_state (except we have no state defined to indicate
> that)?
>   

Do we have to define all standard errors returned here would mean meant 
what?

Right from initial versions of migration reviews we always thought that 
device_state should be only set by user, vendor driver could return 
error state was never thought of. Returning error to read()/write() 
operation indicate that device is not able to handle that operation so 
user will decide what action to be taken next.
Now you are proposing to add a state that vendor driver can set, as 
defined in my above comment?

>>>>>    Is it reflected in
>>>>> device_state?
>>>>
>>>> No.
>>>
>>> So a user should do what, just keep trying?
>>>   
>>
>> No, fail migration process. If error is at source or destination then
>> user can decide either resume at source or terminate application.
> 
> This is describing the expected QEMU protocol resolution, the question
> is relative to the vfio API we're defining here.  If any fault in the
> save/resume protocol results in the device being unusable, there should
> be an indication (perhaps through device_state) that the device is in a
> broken state, and the mechanism to put it into a new state should be
> defined.  For instance, if the device is resuming, a fault occurs
> writing state data to the device, and the user transitions to running.
> Is the device incorporating the partial state data into its run state?
> I suspect not, and wouldn't that be more obvious if we defined a
> protocol where the device can be inspected to be in a bogus state via
> reading device_state, at which point we might define performing a
> device reset as the only mechanism to change the device_state after
> that point.
>

Same as above my comment.

>>>>> What's the recovery protocol?
>>>>>       
>>>>
>>>> On read()/write() failure user should take necessary action.
>>>
>>> Where is that necessary action defined?  Can they just try again?  Do
>>> they transition in and out of _RESUMING to try again?  Do they need to
>>> reset the device?
>>>    
>>
>> User application should decide what action to take on failure, right?
>>    "vfio is not prescribing the migration semantics to userspace, it's
>> presenting an interface that support the user semantics."
> 
> Exactly, we're not defining how QEMU handles a fault in this spec,
> we're defining how a user interacting with the device knows a fault has
> occurred, can inspect the device to determine that the device is in a
> broken state, and the "necessary action" to advance the device forward
> to a new state.
> 
>>>>>> + *
>>>>>> + * For user application, data is opaque. User should write data in the same
>>>>>> + * order as received.
>>>>>
>>>>> Order and transaction size, ie. each data_size chunk is indivisible by
>>>>> the user.
>>>>
>>>> Transaction size can differ, but order should remain same.
>>>
>>> Under what circumstances and to what extent can transaction size
>>> differ?
>>
>> It depends in migration region size.
>>
>>>   Is the MIN() algorithm above the absolute lower bound or just
>>> a suggestion?
>>
>>
>>
>>>   Is the user allowed to concatenate transactions from the
>>> source together on the target if the region is sufficiently large?
>>
>> Yes that can be done, because data is just byte stream for user. Vendor
>> driver receives the byte stream and knows how to decode it.
> 
> But that byte stream is opaque to the user, the vendor driver might
> implement it such that every transaction has a header and splitting the
> transaction might mean that the truncated transaction no longer fits
> the expected size.  If we're lucky, the vendor driver's implementation
> might detect that.  If we're not, the vendor driver might misinterpret
> the next packet.  I think if the user is to consider all data as
> opaque, they must also consider every transaction as indivisible or
> else we're assuming something about the contents of that transaction.
> 

User shouldn't assume about contents of transactions.
I think vendor driver should consider incomming data as data byte stream 
and decoding packets should not be based on migration region size.


Thanks,
Kirti

>>>   It
>>> seems like quite an imposition on the vendor driver to support this
>>> flexibility.
>>>    
>>>>>> + */
>>>>>> +
>>>>>> +struct vfio_device_migration_info {
>>>>>> +	__u32 device_state;         /* VFIO device state */
>>>>>> +#define VFIO_DEVICE_STATE_STOP      (1 << 0)
>>>>>> +#define VFIO_DEVICE_STATE_RUNNING   (1 << 0)
>>>>>
>>>>> Huh?  We should probably just refer to it consistently, ie. _RUNNING
>>>>> and !_RUNNING, otherwise we have the incongruity that setting the _STOP
>>>>> value is actually the opposite of the necessary logic value (_STOP = 1
>>>>> is _RUNNING, _STOP = 0 is !_RUNNING).
>>>>
>>>> Ops, my mistake, forgot to update to
>>>> #define VFIO_DEVICE_STATE_STOP      (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)
>>>>>> +
>>>>>> +#define VFIO_DEVICE_STATE_INVALID_CASE1    (VFIO_DEVICE_STATE_SAVING | \
>>>>>> +					    VFIO_DEVICE_STATE_RESUMING)
>>>>>> +
>>>>>> +#define VFIO_DEVICE_STATE_INVALID_CASE2    (VFIO_DEVICE_STATE_RUNNING | \
>>>>>> +					    VFIO_DEVICE_STATE_RESUMING)
>>>>>
>>>>> Gack, we fixed these in the last iteration!
>>>>>       
>>>>
>>>> That solution doesn't scale when new flags will be added. I still prefer
>>>> to define as above.
>>>
>>> I see, the argument was buried in a reply to Yan, sorry if I missed it:
>>>    
>>>>>> These seem difficult to use, maybe we just need a
>>>>>> VFIO_DEVICE_STATE_VALID macro?
>>>>>>
>>>>>> #define VFIO_DEVICE_STATE_VALID(state) \
>>>>>>      (state & VFIO_DEVICE_STATE_RESUMING ? \
>>>>>>      (state & VFIO_DEVICE_STATE_MASK) == VFIO_DEVICE_STATE_RESUMING : 1)
>>>>>>      
>>>>
>>>> This will not be work when use of other bits gets added in future.
>>>> That's the reason I preferred to add individual invalid states which
>>>> user should check.
>>>
>>> I would argue that what doesn't scale is having numerous CASE1, CASE2,
>>> CASEn conditions elsewhere in the kernel rather than have a unified,
>>> single macro that defines a valid state.  How do you worry this will be
>>> a problem when new flags are added, can't we just update the macro?
>>
>> Adding macro you suggested above. Lets figure out how to solve problem
>> with new flags when new flags gets added.
> 
> Agree, I think it's sufficient to assume we'll update the macro at that
> time.  Thanks,
> 
> Alex
>