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

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 8 Jan 2020 15:44:28 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

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

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

Nod. What about extending the description to:

"Bits 3-31 are reserved for future use. In order to preserve them, a
read-modify-write operation on this field should be used when modifying
the specified bits."

?

> > >     
> > >> [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.

Good.

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

Yes, that's fine.

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

Agreed, I like that as well.