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

Kirti Wankhede
2020-01-08T20:41:28Z

On 1/9/2020 12:01 AM, Alex Williamson wrote:
> On Wed, 8 Jan 2020 15:59:55 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Tue, 7 Jan 2020 11:56:02 -0700
>> Alex Williamson <alex.williamson@redhat.com> wrote:
>>
>>> On Tue, 7 Jan 2020 23:23:17 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>
>>>> There are 3 invalid states:
>>>> * 101b => Invalid state
>>>> * 110b => Invalid state
>>>> * 111b => Invalid state
>>>>
>>>> why only 110b should be used to report error from vendor driver to
>>>> report error? Aren't we adding more confusions in the interface?
>>>
>>> I think the only chance of confusion is poor documentation. If we
>>> define all of the above as invalid and then say any invalid state
>>> indicates an error condition, then the burden is on the user to
>>> enumerate all the invalid states. That's not a good idea. Instead we
>>> could say 101b (_RESUMING|_RUNNING) is reserved, it's not currently
>>> used but it might be useful some day. Therefore there are no valid
>>> transitions into or out of this state. A vendor driver should fail a
>>> write(2) attempting to enter this state.
>>>
>>> That leaves 11Xb, where we consider _RESUMING and _SAVING as mutually
>>> exclusive, so neither are likely to ever be valid states. Logically,
>>> if the device is in a failed state such that it needs to be reset to be
>>> recovered, I would hope the device is not running, so !_RUNNING (110b)
>>> seems appropriate. I'm not sure we need that level of detail yet
>>> though, so I was actually just assuming both 11Xb states would indicate
>>> an error state and the undefined _RUNNING bit might differentiate
>>> something in the future.
>>>
>>> Therefore, I think we'd have:
>>>
>>> * 101b => Reserved
>>> * 11Xb => Error
>>>
>>> Where the device can only self transition into the Error state on a
>>> failed device_state transition and the only exit from the Error state
>>> is via the reset ioctl. The Reserved state is unreachable. The vendor
>>> driver must error on device_state writes to enter or exit the Error
>>> state and must error on writes to enter Reserved states. Is that still
>>> confusing?
>>
>> I think one thing we could do is start to tie the meaning more to the
>> actual state (bit combination) and less to the individual bits. I.e.
>>
>> - bit 0 indicates 'running',
>> - bit 1 indicates 'saving',
>> - bit 2 indicates 'resuming',
>> - bits 3-31 are reserved. [Aside: reserved-and-ignored or
>> reserved-and-must-be-zero?]
>
> This version specified them as:
>
> Bits 3 - 31 are reserved for future use. User should perform
> read-modify-write operation on this field.
>
> The intention is that the user should not make any assumptions about
> the state of the reserved bits, but should preserve them when changing
> known bits. Therefore I think it's ignored but preserved. If we
> specify them as zero, then I think we lose any chance to define them
> later.
>
>> [Note that I don't specify what happens when a bit is set or unset.]
>>
>> States are then defined as:
>> 000b => stopped state (not saving or resuming)
>> 001b => running state (not saving or resuming)
>> 010b => stop-and-copy state
>> 011b => pre-copy state
>> 100b => resuming state
>>
>> [Transitions between these states defined, as before.]
>>
>> 101b => reserved [for post-copy; no transitions defined]
>> 111b => reserved [state does not make sense; no transitions defined]
>> 110b => error state [state does not make sense per se, but it does not
>> indicate running; transitions into this state *are* possible]
>>
>> To a 'reserved' state, we can later assign a different meaning (we
>> could even re-use 111b for a different error state, if needed); while
>> the error state must always stay the error state.
>>
>> We should probably use some kind of feature indication to signify
>> whether a 'reserved' state actually has a meaning. Also, maybe we also
>> should designate the states > 111b as 'reserved'.
>>
>> Does that make sense?
>
> It seems you have an opinion to restrict this particular error state to
> 110b rather than 11Xb, reserving 111b for some future error condition.
> That's fine and I think we agree that using the state with _RUNNING set
> to zero is more logical as we expect the device to be non-operational
> in this state.
>
> I'm also thinking more of these as states, but at the same time we're
> not doing away with the bit definitions. I think the states are much
> easier to decode and use if we think about the function of each bit,
> which leads to the logical incongruity that the 11Xb states are
> impossible and therefore must be error states.
>

I agree on bit definition is better.

Ok. Should there be a defined value for error, which can be used by
vendor driver for error state?

#define VFIO_DEVICE_STATE_ERROR \
(VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RESUMING)

Thanks,
Kirti

> I took a look at drawing a state transitions diagram, but I think we're
> fully interconnected for the 6 states we're defining. The user can
> invoke transition to any of the 5 states Connie lists above from any of
> those states and the 6th error state is only reached via failed
> transition and only exited via device reset, returning the user to the
> running state. There are a couple transitions of questionable value,
> particularly 01Xb -> 100b (_SAVING -> _RESUMING), but I can't convince
> myself that it's worthwhile to force the user to pass through another
> state in order to restrict those. Are there any cases I'm missing
> where the vendor driver has good reason not to support arbitrary
> transitions between the above 5 states? Thanks,
>
> Alex
>