qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [RFC PATCH V4 2/4] vfio: Add vm status change callback


From: Alex Williamson
Subject: Re: [Qemu-devel] [RFC PATCH V4 2/4] vfio: Add vm status change callback to stop/restart the mdev device
Date: Mon, 16 Apr 2018 14:23:28 -0600

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.

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




reply via email to

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