qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Kirti Wankhede
Subject: Re: [PATCH v26 05/17] vfio: Add VM state change handler to know state of VM
Date: Wed, 21 Oct 2020 11:03:23 +0530
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.12.1



On 10/20/2020 4:21 PM, Cornelia Huck wrote:
On Sun, 18 Oct 2020 01:54:56 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

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.
   */

Maybe:

"Change the device_state register for device @vbasedev. Bits set in
@mask are preserved, bits set in @value are set, and bits not set in
either @mask or @value are cleared in device_state. If the register
cannot be accessed, the resulting state would be invalid, or the device
enters an error state, an error is returned." ?


Ok.


+{
+    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.

Hm, now I'm confused. With value == _RUNNING, ~_RUNNING and ~0 as mask
should be equivalent, shouldn't they?


I too got confused after reading your comment.
Lets walk through the device states and transitions can happen here:

if running
- device state could be either _SAVING or _RESUMING or _STOP. Both _SAVING and _RESUMING can't be set at a time, that is the error state. _STOP means 0. - Transition from _SAVING to _RUNNING can happen if there is migration failure, in that case we have to clear _SAVING - Transition from _RESUMING to _RUNNING can happen on resuming and we have to clear _RESUMING.
- In both the above cases, we have to set _RUNNING and clear rest 2 bits.
Then:
mask = ~VFIO_DEVICE_STATE_MASK;
value = VFIO_DEVICE_STATE_RUNNING;

if !running
- device state could be either _RUNNING or _SAVING|_RUNNING. Here we have to reset running bit.
Then:
mask = ~VFIO_DEVICE_STATE_RUNNING;
value = 0;

I'll add comment in the code above.




+            }
+        } 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);

Does this indicate in any way which device was causing problems? (I'm
not sure how visible the error_report would be?)


I think it doesn't indicate which device caused problem but it sets error on migration stream

Thanks,
Kirti



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)
   {

(...)





reply via email to

[Prev in Thread] Current Thread [Next in Thread]