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?
> + */
> + 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)
> {
(...)
