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