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

[Kirti Wankhede]

On 6/23/2020 4:20 AM, Alex Williamson wrote:
On Sun, 21 Jun 2020 01:51:14 +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           | 87 +++++++++++++++++++++++++++++++++++++++++++
  hw/vfio/trace-events          |  2 +
  include/hw/vfio/vfio-common.h |  4 ++
  3 files changed, 93 insertions(+)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 48ac385d80a7..fcecc0bb0874 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -10,6 +10,7 @@
  #include "qemu/osdep.h"
  #include <linux/vfio.h>
  
+#include "sysemu/runstate.h"
  #include "hw/vfio/vfio-common.h"
  #include "cpu.h"
  #include "migration/migration.h"
@@ -74,6 +75,85 @@ err:
      return ret;
  }
  
+static int vfio_migration_set_state(VFIODevice *vbasedev, uint32_t mask,
+                                    uint32_t value)
+{
+    VFIOMigration *migration = vbasedev->migration;
+    VFIORegion *region = &migration->region;
+    uint32_t device_state;
+    int ret;
+
+    ret = pread(vbasedev->fd, &device_state, sizeof(device_state),
+                region->fd_offset + offsetof(struct vfio_device_migration_info,
+                                              device_state));
+    if (ret < 0) {
+        error_report("%s: Failed to read device state %d %s",
+                     vbasedev->name, ret, strerror(errno));
+        return ret;
+    }
+
+    device_state = (device_state & mask) | value;
+
+    if (!VFIO_DEVICE_STATE_VALID(device_state)) {
+        return -EINVAL;
+    }
+
+    ret = pwrite(vbasedev->fd, &device_state, sizeof(device_state),
+                 region->fd_offset + offsetof(struct 
vfio_device_migration_info,
+                                              device_state));
+    if (ret < 0) {
+        error_report("%s: Failed to set device state %d %s",
+                     vbasedev->name, ret, strerror(errno));
+
+        ret = pread(vbasedev->fd, &device_state, sizeof(device_state),
+                region->fd_offset + offsetof(struct vfio_device_migration_info,
+                device_state));
+        if (ret < 0) {
+            error_report("%s: On failure, failed to read device state %d %s",
+                    vbasedev->name, ret, strerror(errno));
+            return ret;
+        }
+
+        if (VFIO_DEVICE_STATE_IS_ERROR(device_state)) {
+            error_report("%s: Device is in error state 0x%x",
+                         vbasedev->name, device_state);
+            return -EFAULT;
+        }
+    }
+
+    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;
+            }
+        } else {
+            mask = ~VFIO_DEVICE_STATE_RUNNING;
+        }
+
+        ret = vfio_migration_set_state(vbasedev, mask, value);
+        if (ret) {
+            error_report("%s: Failed to set device state 0x%x",
+                         vbasedev->name, value & mask);


Is there nothing more we should do here?  It seems like in either the
case of an outbound migration where we can't stop the device or an
inbound migration where we can't start the device, we'd want this to
trigger an abort of the migration.  Should there at least be a TODO
comment if the reason is that QEMU migration doesn't yet support failure
here?  Thanks,


Checked other modules in QEMU, at some places error message is reported 
as above while at some places abort() is called (for example 
kvmclock_vm_state_change() in hw/i386/kvm/clock.c). Abort will abort 
QEMU process, that is VM crash. Should we abort here on error case? 
Anyways VM will not recover properly on migration if there is such error.

Thanks,
Kirti