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

[Cornelia Huck <cohuck@redhat.com>]

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.

> 
> 
> >>
> >>  
> >>>>> +            }
> >>>>> +        } else {
> >>>>> +            mask = ~VFIO_DEVICE_STATE_RUNNING;
> >>>>> +        }