On Wed, 8 Jan 2020 15:44:28 -0700 Alex Williamson <alex.williamson@xxxxxxxxxx> wrote: > On Thu, 9 Jan 2020 02:11:11 +0530 > Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote: > > > On 1/9/2020 12:01 AM, Alex Williamson wrote: > > > On Wed, 8 Jan 2020 15:59:55 +0100 > > > Cornelia Huck <cohuck@xxxxxxxxxx> 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.
