On 10/22/2020 1:21 PM, Cornelia Huck wrote:
On Wed, 21 Oct 2020 11:03:23 +0530
Kirti Wankhede <kwankh...@nvidia.com> wrote:
On 10/20/2020 4:21 PM, Cornelia Huck wrote:
On Sun, 18 Oct 2020 01:54:56 +0530
Kirti Wankhede <kwankh...@nvidia.com> wrote:
On 9/29/2020 4:33 PM, Dr. David Alan Gilbert wrote:
* Cornelia Huck (coh...@redhat.com) wrote:
On Wed, 23 Sep 2020 04:54:07 +0530
Kirti Wankhede <kwankh...@nvidia.com> wrote:
+static void vfio_vmstate_change(void *opaque, int running, RunState state)
+{
+ VFIODevice *vbasedev = opaque;
+
+ if ((vbasedev->vm_running != running)) {
+ int ret;
+ uint32_t value = 0, mask = 0;
+
+ if (running) {
+ value = VFIO_DEVICE_STATE_RUNNING;
+ if (vbasedev->device_state & VFIO_DEVICE_STATE_RESUMING) {
+ mask = ~VFIO_DEVICE_STATE_RESUMING;
I've been staring at this for some time and I think that the desired
result is
- set _RUNNING
- if _RESUMING was set, clear it, but leave the other bits intact
Upto here, you're correct.
- if _RESUMING was not set, clear everything previously set
This would really benefit from a comment (or am I the only one
struggling here?)
Here mask should be ~0. Correcting it.
Hm, now I'm confused. With value == _RUNNING, ~_RUNNING and ~0 as mask
should be equivalent, shouldn't they?
I too got confused after reading your comment.
Lets walk through the device states and transitions can happen here:
if running
- device state could be either _SAVING or _RESUMING or _STOP. Both
_SAVING and _RESUMING can't be set at a time, that is the error state.
_STOP means 0.
- Transition from _SAVING to _RUNNING can happen if there is migration
failure, in that case we have to clear _SAVING
- Transition from _RESUMING to _RUNNING can happen on resuming and we
have to clear _RESUMING.
- In both the above cases, we have to set _RUNNING and clear rest 2 bits.
Then:
mask = ~VFIO_DEVICE_STATE_MASK;
value = VFIO_DEVICE_STATE_RUNNING;
ok
if !running
- device state could be either _RUNNING or _SAVING|_RUNNING. Here we
have to reset running bit.
Then:
mask = ~VFIO_DEVICE_STATE_RUNNING;
value = 0;
ok
I'll add comment in the code above.
That will help.
I'm a bit worried though that all that reasoning which flags are set or
cleared when is quite complex, and it's easy to make mistakes.
Can we model this as a FSM, where an event (running state changes)
transitions the device state from one state to another? I (personally)
find FSMs easier to comprehend, but I'm not sure whether that change
would be too invasive. If others can parse the state changes with that
mask/value interface, I won't object to it.
I agree FSM will be easy and for long term may be easy to maintain. But
at this moment it will be intrusive change. For now we can go ahead with
this code and later we can change to FSM model, if all agrees on it.
Thanks,
Kirti
+ }
+ } else {
+ mask = ~VFIO_DEVICE_STATE_RUNNING;
+ }