* Cornelia Huck (coh...@redhat.com) wrote: > On Wed, 23 Sep 2020 04:54:07 +0530 > Kirti Wankhede <kwankh...@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 <kwankh...@nvidia.com> > > Reviewed-by: Neo Jia <c...@nvidia.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilb...@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 / dgilb...@redhat.com / Manchester, UK