|
From: | Kirti Wankhede |
Subject: | Re: [PATCH v26 05/17] vfio: Add VM state change handler to know state of VM |
Date: | Sun, 18 Oct 2020 01:54:56 +0530 |
User-agent: | Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 |
On 9/29/2020 4:33 PM, Dr. David Alan Gilbert wrote:
* 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.
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. */
+{ + 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.
+ } + } 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); 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) {(...)
[Prev in Thread] | Current Thread | [Next in Thread] |