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.
>
>
> >>
> >>
> >>>>> + }
> >>>>> + } else {
> >>>>> + mask = ~VFIO_DEVICE_STATE_RUNNING;
> >>>>> + }
