On Mon, 19 Oct 2020 11:51:36 -0600 Alex Williamson <alex.william...@redhat.com> wrote:
> On Sun, 18 Oct 2020 23:13:39 +0530 > Kirti Wankhede <kwankh...@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. Agreed. Also, it is not obvious from the naming that 'device_state' is related to migration, and it is easy to assume that this field is useful even in the non-migration case. Moving it would solve that problem.