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.
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
