On Sun, 18 Oct 2020 01:54:56 +0530 Kirti Wankhede <kwankh...@nvidia.com> wrote:
> On 9/29/2020 4:33 PM, Dr. David Alan Gilbert wrote: > > * 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. > >> > > Adding a comment as: > > /* > * Write device_state field to inform the vendor driver about the > device state > * to be transitioned to. > * vbasedev: VFIO device > * mask : bits set in the mask are preserved in device_state > * value: bits set in the value are set in device_state > * Remaining bits in device_state are cleared. > */ Maybe: "Change the device_state register for device @vbasedev. Bits set in @mask are preserved, bits set in @value are set, and bits not set in either @mask or @value are cleared in device_state. If the register cannot be accessed, the resulting state would be invalid, or the device enters an error state, an error is returned." ? > > > >>> +{ > >>> + 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? > >> > > Ok. Changing to -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 > > Upto here, you're correct. > > >> - if _RESUMING was not set, clear everything previously set > >> This would really benefit from a comment (or am I the only one > >> struggling here?) > >> > > Here mask should be ~0. Correcting it. Hm, now I'm confused. With value == _RUNNING, ~_RUNNING and ~0 as mask should be equivalent, shouldn't they? > > > >>> + } > >>> + } 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." > >> > >> ? > >> > > Ok. Updating comment as suggested here. > > >> 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. > > > > It should be as below, right? > qemu_file_set_error(migrate_get_current()->to_dst_file, ret); Does this indicate in any way which device was causing problems? (I'm not sure how visible the error_report would be?) > > > Thanks, > Kirti > > > 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) > >>> { > >> > >> (...) >