Re: [Qemu-devel] [RFC PATCH V4 2/4] vfio: Add vm status change callback to stop/restart the mdev device

[Zhang, Yulei]

> -----Original Message-----
> From: Alex Williamson [mailto:address@hidden
> Sent: Tuesday, April 17, 2018 4:23 AM
> To: Kirti Wankhede <address@hidden>
> Cc: Zhang, Yulei <address@hidden>; address@hidden; Tian,
> Kevin <address@hidden>; address@hidden;
> address@hidden; Wang, Zhi A <address@hidden>;
> address@hidden; address@hidden
> Subject: Re: [RFC PATCH V4 2/4] vfio: Add vm status change callback to
> stop/restart the mdev device
> 
> On Mon, 16 Apr 2018 20:14:27 +0530
> Kirti Wankhede <address@hidden> wrote:
> 
> > On 4/10/2018 11:32 AM, Yulei Zhang wrote:
> > > VM status change handler is added to change the vfio pci device
> > > status during the migration, write the demanded device status
> > > to the DEVICE STATUS subregion to stop the device on the source side
> > > before fetch its status and start the deivce on the target side
> > > after restore its status.
> > >
> > > Signed-off-by: Yulei Zhang <address@hidden>
> > > ---
> > >  hw/vfio/pci.c                 | 20 ++++++++++++++++++++
> > >  include/hw/vfio/vfio-common.h |  1 +
> > >  linux-headers/linux/vfio.h    |  6 ++++++
> > >  roms/seabios                  |  2 +-
> > >  4 files changed, 28 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index f98a9dd..13d8c73 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -38,6 +38,7 @@
> > >
> > >  static void vfio_disable_interrupts(VFIOPCIDevice *vdev);
> > >  static void vfio_mmap_set_enabled(VFIOPCIDevice *vdev, bool enabled);
> > > +static void vfio_vm_change_state_handler(void *pv, int running,
> RunState state);
> > >
> > >  /*
> > >   * Disabling BAR mmaping can be slow, but toggling it around INTx can
> > > @@ -2896,6 +2897,7 @@ static void vfio_realize(PCIDevice *pdev, Error
> **errp)
> > >      vfio_register_err_notifier(vdev);
> > >      vfio_register_req_notifier(vdev);
> > >      vfio_setup_resetfn_quirk(vdev);
> > > +
> qemu_add_vm_change_state_handler(vfio_vm_change_state_handler,
> vdev);
> > >
> > >      return;
> > >
> > > @@ -2982,6 +2984,24 @@ post_reset:
> > >      vfio_pci_post_reset(vdev);
> > >  }
> > >
> > > +static void vfio_vm_change_state_handler(void *pv, int running,
> RunState state)
> > > +{
> > > +    VFIOPCIDevice *vdev = pv;
> > > +    VFIODevice *vbasedev = &vdev->vbasedev;
> > > +    uint8_t dev_state;
> > > +    uint8_t sz = 1;
> > > +
> > > +    dev_state = running ? VFIO_DEVICE_START : VFIO_DEVICE_STOP;
> > > +
> > > +    if (pwrite(vdev->vbasedev.fd, &dev_state,
> > > +               sz, vdev->device_state.offset) != sz) {
> > > +        error_report("vfio: Failed to %s device", running ? "start" : 
> > > "stop");
> > > +        return;
> > > +    }
> > > +
> > > +    vbasedev->device_state = dev_state;
> > > +}
> > > +
> >
> > Is it expected to trap device_state region by vendor driver?
> > Can this information be communicated to vendor driver through an ioctl?
> 
> Either the mdev vendor driver or vfio bus driver (ie. vfio-pci) would
> be providing REGION_INFO for this region, so the vendor driver is
> already in full control here using existing ioctls.  I don't see that
> we need new ioctls, we just need to fully define the API of the
> proposed regions here.
> 
If the device state region is mmaped, we may not be able to use
region device state offset to convey the running state. It may need
a new ioctl to set the device state.

> > Here only device state is conveyed to vendor driver but knowing
> > 'RunState' in vendor driver is very useful and vendor driver can take
> > necessary action accordingly like RUN_STATE_PAUSED indicating that VM
> is
> > in paused state, similarly RUN_STATE_SUSPENDED,
> > RUN_STATE_FINISH_MIGRATE, RUN_STATE_IO_ERROR. If these states are
> > handled properly, all the cases can be supported with same interface
> > like VM suspend-resume, VM pause-restore.
> 
> I agree, but let's remember that we're talking about device state, not
> VM state.  vfio is a userspace device interface, not specifically a
> virtual machine interface, so states should be in terms of the device.
> The API of this region needs to be clearly defined and using only 1
> byte at the start of the region is not very forward looking.  Thanks,
> 
> Alex
> 
> > >  static void vfio_instance_init(Object *obj)
> > >  {
> > >      PCIDevice *pci_dev = PCI_DEVICE(obj);
> > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
> common.h
> > > index f3a2ac9..9c14a8f 100644
> > > --- a/include/hw/vfio/vfio-common.h
> > > +++ b/include/hw/vfio/vfio-common.h
> > > @@ -125,6 +125,7 @@ typedef struct VFIODevice {
> > >      unsigned int num_irqs;
> > >      unsigned int num_regions;
> > >      unsigned int flags;
> > > +    bool device_state;
> > >  } VFIODevice;
> > >
> > >  struct VFIODeviceOps {
> > > diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
> > > index e3380ad..8f02f2f 100644
> > > --- a/linux-headers/linux/vfio.h
> > > +++ b/linux-headers/linux/vfio.h
> > > @@ -304,6 +304,12 @@ struct vfio_region_info_cap_type {
> > >  /* Mdev sub-type for device state save and restore */
> > >  #define VFIO_REGION_SUBTYPE_DEVICE_STATE (4)
> > >
> > > +/* Offset in region to save device state */
> > > +#define VFIO_DEVICE_STATE_OFFSET 1
> > > +
> > > +#define VFIO_DEVICE_START        0
> > > +#define VFIO_DEVICE_STOP 1
> > > +
> > >  /**
> > >   * VFIO_DEVICE_GET_IRQ_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 9,
> > >   *                                   struct vfio_irq_info)
> > > diff --git a/roms/seabios b/roms/seabios
> > > index 63451fc..5f4c7b1 160000
> > > --- a/roms/seabios
> > > +++ b/roms/seabios
> > > @@ -1 +1 @@
> > > -Subproject commit 63451fca13c75870e1703eb3e20584d91179aebc
> > > +Subproject commit 5f4c7b13cdf9c450eb55645f4362ea58fa61b79b
> > >