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

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 9 Jan 2020 02:11:11 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> 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)

Seems like a good idea for consistency.  Thanks,

Alex