Re: [PATCH v16 QEMU 07/16] vfio: Add migration state change notifier

[Kirti Wankhede]

On 4/1/2020 4:57 PM, Dr. David Alan Gilbert wrote:
> * Kirti Wankhede (address@hidden) wrote:
> > Added migration state change notifier to get notification on migration state
> > change. These states are translated to VFIO device state and conveyed to vendor
> > driver.
> >
> > Signed-off-by: Kirti Wankhede <address@hidden>
> > Reviewed-by: Neo Jia <address@hidden>
> > ---
> >   hw/vfio/migration.c           | 29 +++++++++++++++++++++++++++++
> >   hw/vfio/trace-events          |  1 +
> >   include/hw/vfio/vfio-common.h |  1 +
> >   3 files changed, 31 insertions(+)
> >
> > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > index af9443c275fb..22ded9d28cf3 100644
> > --- a/hw/vfio/migration.c
> > +++ b/hw/vfio/migration.c
> > @@ -154,6 +154,27 @@ static void vfio_vmstate_change(void *opaque, int running, 
> > RunState state)
> >       }
> >   }
> >   
> > +static void vfio_migration_state_notifier(Notifier *notifier, void *data)
> > +{
> > +    MigrationState *s = data;
> > +    VFIODevice *vbasedev = container_of(notifier, VFIODevice, migration_state);
> > +    int ret;
> > +
> > +    trace_vfio_migration_state_notifier(vbasedev->name, s->state);
>
> You might want to use MigrationStatus_str(s->status) to make that
> readable.

Yes.

> > +    switch (s->state) {
> > +    case MIGRATION_STATUS_CANCELLING:
> > +    case MIGRATION_STATUS_CANCELLED:
> > +    case MIGRATION_STATUS_FAILED:
> > +        ret = vfio_migration_set_state(vbasedev,
> > +                      ~(VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RESUMING),
> > +                      VFIO_DEVICE_STATE_RUNNING);
> > +        if (ret) {
> > +            error_report("%s: Failed to set state RUNNING", vbasedev->name);
> > +        }
>
> In the migration code we check to see if the VM was running prior to the
> start of the migration before we start the CPUs going again (see
> migration_iteration_finish):
>      case MIGRATION_STATUS_FAILED:
>      case MIGRATION_STATUS_CANCELLED:
>      case MIGRATION_STATUS_CANCELLING:
>          if (s->vm_was_running) {
>              vm_start();
>          } else {
>              if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
>                  runstate_set(RUN_STATE_POSTMIGRATE);
>              }
>
> so if the guest was paused before a migration we don't falsely restart
> it.  Maybe you need something similar?

Guest paused means vCPUs are paused, but that doesn't pause device. Init 
state of VFIO device is also RUNNING and device will not get any 
instructions until vCPUs are running. So I think putting device in 
RUNNING is still fine.

Thanks,
Kirti

> Dave
>
> > +    }
> > +}
> > +
> >   static int vfio_migration_init(VFIODevice *vbasedev,
> >                                  struct vfio_region_info *info)
> >   {
> > @@ -173,6 +194,9 @@ static int vfio_migration_init(VFIODevice *vbasedev,
> >       vbasedev->vm_state = qemu_add_vm_change_state_handler(vfio_vmstate_change,
> >                                                             vbasedev);
> >   
> > +    vbasedev->migration_state.notify = vfio_migration_state_notifier;
> > +    add_migration_state_change_notifier(&vbasedev->migration_state);
> > +
> >       return 0;
> >   }
> >   
> > @@ -211,6 +235,11 @@ add_blocker:
> >   
> >   void vfio_migration_finalize(VFIODevice *vbasedev)
> >   {
> > +
> > +    if (vbasedev->migration_state.notify) {
> > +        remove_migration_state_change_notifier(&vbasedev->migration_state);
> > +    }
> > +
> >       if (vbasedev->vm_state) {
> >           qemu_del_vm_change_state_handler(vbasedev->vm_state);
> >       }
> > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> > index 3d15bacd031a..69503228f20e 100644
> > --- a/hw/vfio/trace-events
> > +++ b/hw/vfio/trace-events
> > @@ -148,3 +148,4 @@ vfio_display_edid_write_error(void) ""
> >   vfio_migration_probe(char *name, uint32_t index) " (%s) Region %d"
> >   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"
> > +vfio_migration_state_notifier(char *name, int state) " (%s) state %d"
> > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> > index 3d18eb146b33..28f55f66d019 100644
> > --- a/include/hw/vfio/vfio-common.h
> > +++ b/include/hw/vfio/vfio-common.h
> > @@ -123,6 +123,7 @@ typedef struct VFIODevice {
> >       VMChangeStateEntry *vm_state;
> >       uint32_t device_state;
> >       int vm_running;
> > +    Notifier migration_state;
> >   } VFIODevice;
> >   
> >   struct VFIODeviceOps {
> > --
> > 2.7.0
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK