Re: [Qemu-devel] [RFC 1/5] vfio: introduce a new VFIO region for migration support

[Zhang, Yulei]

> -----Original Message-----
> From: Alex Williamson [mailto:address@hidden
> Sent: Tuesday, June 27, 2017 4:19 AM
> To: Zhang, Yulei <address@hidden>
> Cc: address@hidden; Tian, Kevin <address@hidden>;
> address@hidden; address@hidden; Zheng, Xiao
> <address@hidden>; Wang, Zhi A <address@hidden>
> Subject: Re: [Qemu-devel] [RFC 1/5] vfio: introduce a new VFIO region for
> migration support
> 
> On Tue,  4 Apr 2017 18:26:57 +0800
> Yulei Zhang <address@hidden> wrote:
> 
> > New VFIO region VFIO_PCI_DEVICE_STATE_REGION_INDEX is added to
> fetch
> > and restore the pci device status during the live migration.
> >
> > Signed-off-by: Yulei Zhang <address@hidden>
> > ---
> >  hw/vfio/pci.c              | 17 +++++++++++++++++
> >  hw/vfio/pci.h              |  1 +
> >  linux-headers/linux/vfio.h |  5 ++++-
> >  3 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 03a3d01..bf2e0ff 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2360,6 +2360,23 @@ static void vfio_populate_device(VFIOPCIDevice
> *vdev, Error **errp)
> >          QLIST_INIT(&vdev->bars[i].quirks);
> >      }
> >
> > +    /* device state region setup */
> > +    if (vbasedev->flags & VFIO_DEVICE_FLAGS_MIGRATABLE) {
> > +        char *name = g_strdup_printf("%s BAR %d", vbasedev->name,
> VFIO_PCI_DEVICE_STATE_REGION_INDEX);
> > +
> > +        ret = vfio_region_setup(OBJECT(vdev), vbasedev,
> > +                                &vdev->device_state.region,
> VFIO_PCI_DEVICE_STATE_REGION_INDEX, name);
> > +        g_free(name);
> > +
> > +        if (ret) {
> > +            error_setg_errno(errp, -ret, "failed to get region %d info",
> > +                             VFIO_PCI_DEVICE_STATE_REGION_INDEX);
> > +            return;
> > +        }
> > +
> > +        QLIST_INIT(&vdev->device_state.quirks);
> > +    }
> > +
> >      ret = vfio_get_region_info(vbasedev,
> >                                 VFIO_PCI_CONFIG_REGION_INDEX, &reg_info);
> >      if (ret) {
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index a8366bb..bd98618 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -115,6 +115,7 @@ typedef struct VFIOPCIDevice {
> >      int interrupt; /* Current interrupt type */
> >      VFIOBAR bars[PCI_NUM_REGIONS - 1]; /* No ROM */
> >      VFIOVGA *vga; /* 0xa0000, 0x3b0, 0x3c0 */
> > +    VFIOBAR device_state;
> 
> But it's not a BAR...
> 
> >      void *igd_opregion;
> >      PCIHostDeviceAddress host;
> >      EventNotifier err_notifier;
> > diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
> > index 531cb2e..c87d05c 100644
> > --- a/linux-headers/linux/vfio.h
> > +++ b/linux-headers/linux/vfio.h
> > @@ -198,6 +198,8 @@ struct vfio_device_info {
> >  #define VFIO_DEVICE_FLAGS_PCI      (1 << 1)        /* vfio-pci device */
> >  #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)        /* vfio-platform
> device */
> >  #define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)   /* vfio-amba device */
> > +#define VFIO_DEVICE_FLAGS_CCW   (1 << 4)        /* vfio-ccw device */
> > +#define VFIO_DEVICE_FLAGS_MIGRATABLE (1 << 5)      /* Device supports
> migrate */
> >     __u32   num_regions;    /* Max region index + 1 */
> >     __u32   num_irqs;       /* Max IRQ index + 1 */
> >  };
> > @@ -433,7 +435,8 @@ enum {
> >      * between described ranges are unimplemented.
> >      */
> >     VFIO_PCI_VGA_REGION_INDEX,
> > -   VFIO_PCI_NUM_REGIONS = 9 /* Fixed user ABI, region indexes >=9
> use */
> > +   VFIO_PCI_DEVICE_STATE_REGION_INDEX,
> > +   VFIO_PCI_NUM_REGIONS = 10 /* Fixed user ABI, region indexes >=9
> use */
> >                              /* device specific cap to define content. */
> >  };
> >
> 
> Nak, please read the comment on the line you changed.  We're not adding
> any more static region indexes, anything new should be added with
> device specific capabilities.  Look at how we do opregions and various
> other IGD related regions.  There's also no reason to add a flag, the
> existence of the device specific region should indicate that it
> supports migration.  Also, we call these device specific, but I would
> still encourage a shared format, userspace doesn't want to support a
> different mechanism for each device.  Thanks,
> 
> Alex

Thanks, Alex. I will revise it accordingly.