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

[Dr. David Alan Gilbert]

* Kirti Wankhede (kwankhede@nvidia.com) wrote:
> 
> 
> On 6/23/2020 4:20 AM, Alex Williamson wrote:
> > On Sun, 21 Jun 2020 01:51:14 +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           | 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.

I prefer to avoid aborts on migration if possible; unless the VM is
realyl dead.
Failing the migration is preferable.

Dave

> Thanks,
> Kirti
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK