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

[Alex Williamson]

On Sun, 18 Oct 2020 23:13:39 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> <snip>
> 
> >>>> +vfio_migration_set_state(char *name, uint32_t state) " (%s) state %d"
> >>>> +vfio_vmstate_change(char *name, int running, const char *reason, 
> >>>> uint32_t dev_state) " (%s) running %d reason %s device state %d"
> >>>> diff --git a/include/hw/vfio/vfio-common.h 
> >>>> b/include/hw/vfio/vfio-common.h
> >>>> index 8275c4c68f45..25e3b1a3b90a 100644
> >>>> --- a/include/hw/vfio/vfio-common.h
> >>>> +++ b/include/hw/vfio/vfio-common.h
> >>>> @@ -29,6 +29,7 @@
> >>>>    #ifdef CONFIG_LINUX
> >>>>    #include <linux/vfio.h>
> >>>>    #endif
> >>>> +#include "sysemu/sysemu.h"
> >>>>    
> >>>>    #define VFIO_MSG_PREFIX "vfio %s: "
> >>>>    
> >>>> @@ -119,6 +120,9 @@ typedef struct VFIODevice {
> >>>>        unsigned int flags;
> >>>>        VFIOMigration *migration;
> >>>>        Error *migration_blocker;
> >>>> +    VMChangeStateEntry *vm_state;
> >>>> +    uint32_t device_state;
> >>>> +    int vm_running;  
> >>>
> >>> Could these be placed in VFIOMigration?  Thanks,
> >>>     
> >>
> >> I think device_state should be part of VFIODevice since its about device
> >> rather than only related to migration, others can be moved to 
> >> VFIOMigration.  
> > 
> > But these are only valid when migration is supported and thus when
> > VFIOMigration exists.  Thanks,
> >   
> 
> Even though it is used when migration is supported, its device's attribute.

device_state is a local copy of the migration region register, so it
serves no purpose when a migration region is not present.  In fact the
initial value would indicate the device is stopped, which is incorrect.
vm_running is never initialized and cannot be set other than through a
migration region update of device_state, so at least two of these
values show incorrect state when migration is not supported by the
device.  vm_state is unused when migration isn't present, so if nothing
else the pointer here is wasteful.  It's not clear to me what
justification is being presented here as a "device's attribute",
supporting migration as indicated by a non-NULL migration pointer is
also a device attribute and these are attributes further defining the
state of that support.

BTW, device_state is used in patch 03/ but only defined here in 05/, so
the series would fail to compile on bisect.  Thanks,

Alex