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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Cornelia Huck (cohuck@redhat.com) wrote:
> On Wed, 23 Sep 2020 04:54:07 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
> > VM state change handler gets called on change in VM's state. This is used to set
> > VFIO device state to _RUNNING.
> > 
> > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> > Reviewed-by: Neo Jia <cjia@nvidia.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  hw/vfio/migration.c           | 136 ++++++++++++++++++++++++++++++++++++++++++
> >  hw/vfio/trace-events          |   3 +-
> >  include/hw/vfio/vfio-common.h |   4 ++
> >  3 files changed, 142 insertions(+), 1 deletion(-)
> > 
> 
> (...)
> 
> > +static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask,
> > +                                    uint32_t value)
> 
> I think I've mentioned that before, but this function could really
> benefit from a comment what mask and value mean. 
> 
> > +{
> > +    VFIOMigration *migration = vbasedev->migration;
> > +    VFIORegion *region = &migration->region;
> > +    off_t dev_state_off = region->fd_offset +
> > +                      offsetof(struct vfio_device_migration_info, device_state);
> > +    uint32_t device_state;
> > +    int ret;
> > +
> > +    ret = vfio_mig_read(vbasedev, &device_state, sizeof(device_state),
> > +                        dev_state_off);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    device_state = (device_state & mask) | value;
> > +
> > +    if (!VFIO_DEVICE_STATE_VALID(device_state)) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    ret = vfio_mig_write(vbasedev, &device_state, sizeof(device_state),
> > +                         dev_state_off);
> > +    if (ret < 0) {
> > +        ret = vfio_mig_read(vbasedev, &device_state, sizeof(device_state),
> > +                          dev_state_off);
> > +        if (ret < 0) {
> > +            return ret;
> > +        }
> > +
> > +        if (VFIO_DEVICE_STATE_IS_ERROR(device_state)) {
> > +            hw_error("%s: Device is in error state 0x%x",
> > +                     vbasedev->name, device_state);
> > +            return -EFAULT;
> 
> Is -EFAULT a good return value here? Maybe -EIO?
> 
> > +        }
> > +    }
> > +
> > +    vbasedev->device_state = device_state;
> > +    trace_vfio_migration_set_state(vbasedev->name, device_state);
> > +    return 0;
> > +}
> > +
> > +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
> - if _RESUMING was not set, clear everything previously set
> This would really benefit from a comment (or am I the only one
> struggling here?)
> 
> > +            }
> > +        } else {
> > +            mask = ~VFIO_DEVICE_STATE_RUNNING;
> > +        }
> > +
> > +        ret = vfio_migration_set_state(vbasedev, mask, value);
> > +        if (ret) {
> > +            /*
> > +             * vm_state_notify() doesn't support reporting failure. If such
> > +             * error reporting support added in furure, migration should be
> > +             * aborted.
> 
> 
> "We should abort the migration in this case, but vm_state_notify()
> currently does not support reporting failures."
> 
> ?
> 
> Can/should we mark the failing device in some way?

I think you can call qemu_file_set_error on the migration stream to
force an error.

Dave

> > +             */
> > +            error_report("%s: Failed to set device state 0x%x",
> > +                         vbasedev->name, value & mask);
> > +        }
> > +        vbasedev->vm_running = running;
> > +        trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
> > +                                  value & mask);
> > +    }
> > +}
> > +
> >  static int vfio_migration_init(VFIODevice *vbasedev,
> >                                 struct vfio_region_info *info)
> >  {
> 
> (...)
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK