Re: [PATCH v26 05/17] vfio: Add VM state change handler to know state of VM

[Kirti Wankhede]

On 10/22/2020 1:21 PM, Cornelia Huck wrote:
> On Wed, 21 Oct 2020 11:03:23 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
> > On 10/20/2020 4:21 PM, Cornelia Huck wrote:
> > > On Sun, 18 Oct 2020 01:54:56 +0530
> > > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> > >    
> > > > On 9/29/2020 4:33 PM, Dr. David Alan Gilbert wrote:
> > > > > * Cornelia Huck (cohuck@redhat.com) wrote:
> > > > > > On Wed, 23 Sep 2020 04:54:07 +0530
> > > > > > Kirti Wankhede <kwankhede@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;
> > > > > > > +        }