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

From: Cornelia Huck <cohuck@redhat.com>
To: Kirti Wankhede <kwankhede@nvidia.com>
Cc: cjia@nvidia.com, aik@ozlabs.ru, Zhengxiao.zx@Alibaba-inc.com,
	shuangtai.tst@alibaba-inc.com, qemu-devel@nongnu.org,
	peterx@redhat.com, eauger@redhat.com, yi.l.liu@intel.com,
	quintela@redhat.com, ziye.yang@intel.com, armbru@redhat.com,
	mlevitsk@redhat.com, pasic@linux.ibm.com, felipe@nutanix.com,
	zhi.a.wang@intel.com, kevin.tian@intel.com, yan.y.zhao@intel.com,
	dgilbert@redhat.com, alex.williamson@redhat.com,
	changpeng.liu@intel.com, eskultet@redhat.com, Ken.Xue@amd.com,
	jonathan.davies@nutanix.com, pbonzini@redhat.com
Subject: Re: [PATCH v26 05/17] vfio: Add VM state change handler to know state of VM
Date: Thu, 24 Sep 2020 17:02:19 +0200	[thread overview]
Message-ID: <20200924170220.0a9836fe.cohuck@redhat.com> (raw)
In-Reply-To: <1600817059-26721-6-git-send-email-kwankhede@nvidia.com>

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?

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

(...)