Re: [Qemu-devel] [PATCH V3 3/4] vfio: Add struct vfio_vmstate_info to introduce put/get callback funtion for vfio device status save/restore

[Zhang, Yulei]

> -----Original Message-----
> From: Dr. David Alan Gilbert [mailto:address@hidden
> Sent: Friday, March 9, 2018 7:59 PM
> To: Zhang, Yulei <address@hidden>
> Cc: address@hidden; Tian, Kevin <address@hidden>;
> address@hidden; address@hidden;
> address@hidden
> Subject: Re: [Qemu-devel] [PATCH V3 3/4] vfio: Add struct vfio_vmstate_info
> to introduce put/get callback funtion for vfio device status save/restore
> 
> * Yulei Zhang (address@hidden) wrote:
> > Introduce vfio_device_put/vfio_device_get funtion for vfio device
> > state save/restore usage.
> >
> > For VFIO pci device status migrate, on the source side with funtion
> > vfio_device_put to save the following states 1. pci configuration
> > space addr0~addr5 2. pci configuration space msi_addr msi_data 3. pci
> > device status fetch from device driver
> >
> > And on the target side with funtion vfio_device_get to restore the
> > same states 1. re-setup the pci bar configuration 2. re-setup the pci
> > device msi configuration 3. restore the pci device status
> >
> > Signed-off-by: Yulei Zhang <address@hidden>
> > ---
> >  hw/vfio/pci.c              | 137
> +++++++++++++++++++++++++++++++++++++++++++++
> >  linux-headers/linux/vfio.h |   3 +
> >  2 files changed, 140 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 3e2289c..c1676cf
> > 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2982,6 +2982,123 @@ static void
> vfio_vm_change_state_handler(void *pv, int running, RunState state)
> >      vbasedev->device_state = dev_state;  }
> >
> > +static int vfio_device_put(QEMUFile *f, void *pv, size_t size,
> > +                           VMStateField *field, QJSON *vmdesc) {
> > +    VFIOPCIDevice *vdev = pv;
> > +    PCIDevice *pdev = &vdev->pdev;
> > +    int sz = vdev->device_state.size - VFIO_DEVICE_STATE_OFFSET;
> > +    uint8_t *buf = NULL;
> > +    uint32_t msi_cfg, msi_lo, msi_hi, msi_data, bar_cfg, i;
> > +    bool msi_64bit;
> > +
> > +    for (i = 0; i < PCI_ROM_SLOT; i++) {
> > +        bar_cfg = pci_default_read_config(pdev, PCI_BASE_ADDRESS_0 + i *
> 4, 4);
> > +        qemu_put_be32(f, bar_cfg);
> > +    }
> > +
> > +    msi_cfg = pci_default_read_config(pdev, pdev->msi_cap +
> PCI_MSI_FLAGS, 2);
> > +    msi_64bit = !!(msi_cfg & PCI_MSI_FLAGS_64BIT);
> > +
> > +    msi_lo = pci_default_read_config(pdev,
> > +                                     pdev->msi_cap + PCI_MSI_ADDRESS_LO, 
> > 4);
> > +    qemu_put_be32(f, msi_lo);
> > +
> > +    if (msi_64bit) {
> > +        msi_hi = pci_default_read_config(pdev,
> > +                                         pdev->msi_cap + 
> > PCI_MSI_ADDRESS_HI,
> > +                                         4);
> > +        qemu_put_be32(f, msi_hi);
> > +    }
> > +
> > +    msi_data = pci_default_read_config(pdev,
> > +               pdev->msi_cap + (msi_64bit ? PCI_MSI_DATA_64 :
> PCI_MSI_DATA_32),
> > +               2);
> > +    qemu_put_be32(f, msi_data);
> > +
> > +    buf = g_malloc(sz);
> > +    if (buf == NULL) {
> > +        error_report("vfio: Failed to allocate memory for migrate");
> > +        goto exit;
> 
> Yet exit does a 'return 0' - so we've not failed the migration and we've
> generated a broken migration stream.
> (same in the other exit cases)
> 
> > +    }
> > +
> > +    if (pread(vdev->vbasedev.fd, buf, sz,
> > +              vdev->device_state.offset + VFIO_DEVICE_STATE_OFFSET) != sz) 
> > {
> > +        error_report("vfio: Failed to read Device State Region");
> > +        goto exit;
> > +    }
> > +
> > +    qemu_put_buffer(f, buf, sz);
> > +
> > +exit:
> > +    g_free(buf);
> > +
> > +    return 0;
> > +}
> 
> We really try and avoid qemu_put_* and qemu_get* as much as possible
> these days.
> Please try building a datastructure (possible in a pre_save) and then
> construction a VMState with all the fields in.
> VMSTATE_WITH_TMP might help you in this case.
> 
> Doing it that way is much more robust against later changes.
> 
> Also, I wonder if it'sa good idea to include 'sz' (or at least
> state.size) in the migration stream; if it was different on teh destination
> then things would go badly wrong. I don't know about mdev
> to know if that can happen.   You'd need to validate it when it's
> loaded.
> 
> Dave
> 
Hi Dave, this size is not included in the migration stream, it is reported by
device state region when vifo pci initialize, it's value will be varied 
according to 
different mdev devices. 

Yulei
> > +
> > +static int vfio_device_get(QEMUFile *f, void *pv,
> > +                           size_t size, VMStateField *field) {
> > +    VFIOPCIDevice *vdev = pv;
> > +    PCIDevice *pdev = &vdev->pdev;
> > +    int sz = vdev->device_state.size - VFIO_DEVICE_STATE_OFFSET;
> > +    uint8_t *buf = NULL;
> > +    uint32_t ctl, msi_lo, msi_hi, msi_data, bar_cfg, i;
> > +    bool msi_64bit;
> > +
> > +    /* retore pci bar configuration */
> > +    ctl = pci_default_read_config(pdev, PCI_COMMAND, 2);
> > +    vfio_pci_write_config(pdev, PCI_COMMAND,
> > +                          ctl & (!(PCI_COMMAND_IO | PCI_COMMAND_MEMORY)),
> 2);
> > +    for (i = 0; i < PCI_ROM_SLOT; i++) {
> > +        bar_cfg = qemu_get_be32(f);
> > +        vfio_pci_write_config(pdev, PCI_BASE_ADDRESS_0 + i * 4, bar_cfg, 
> > 4);
> > +    }
> > +    vfio_pci_write_config(pdev, PCI_COMMAND,
> > +                          ctl | PCI_COMMAND_IO | PCI_COMMAND_MEMORY,
> > + 2);
> > +
> > +    /* restore msi configuration */
> > +    ctl = pci_default_read_config(pdev, pdev->msi_cap + PCI_MSI_FLAGS,
> 2);
> > +    msi_64bit = !!(ctl & PCI_MSI_FLAGS_64BIT);
> > +
> > +    vfio_pci_write_config(&vdev->pdev,
> > +                          pdev->msi_cap + PCI_MSI_FLAGS,
> > +                          ctl & (!PCI_MSI_FLAGS_ENABLE), 2);
> > +
> > +    msi_lo = qemu_get_be32(f);
> > +    vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_ADDRESS_LO,
> > + msi_lo, 4);
> > +
> > +    if (msi_64bit) {
> > +        msi_hi = qemu_get_be32(f);
> > +        vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_ADDRESS_HI,
> > +                              msi_hi, 4);
> > +    }
> > +    msi_data = qemu_get_be32(f);
> > +    vfio_pci_write_config(pdev,
> > +              pdev->msi_cap + (msi_64bit ? PCI_MSI_DATA_64 :
> PCI_MSI_DATA_32),
> > +              msi_data, 2);
> > +
> > +    vfio_pci_write_config(&vdev->pdev, pdev->msi_cap + PCI_MSI_FLAGS,
> > +                          ctl | PCI_MSI_FLAGS_ENABLE, 2);
> > +
> > +    buf = g_malloc(sz);
> > +    if (buf == NULL) {
> > +        error_report("vfio: Failed to allocate memory for migrate");
> > +        return -1;
> > +    }
> > +
> > +    qemu_get_buffer(f, buf, sz);
> > +    if (pwrite(vdev->vbasedev.fd, buf, sz,
> > +               vdev->device_state.offset + VFIO_DEVICE_STATE_OFFSET) != 
> > sz) {
> > +        error_report("vfio: Failed to write Device State Region");
> > +        return -1;
> > +    }
> > +
> > +    g_free(buf);
> > +
> > +    return 0;
> > +}
> > +
> >  static void vfio_instance_init(Object *obj)  {
> >      PCIDevice *pci_dev = PCI_DEVICE(obj); @@ -3026,9 +3143,29 @@
> > static Property vfio_pci_dev_properties[] = {
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > +static const VMStateInfo vfio_vmstate_info = {
> > +    .name = "vfio-state",
> > +    .get = vfio_device_get,
> > +    .put = vfio_device_put,
> > +};
> > +
> >  static VMStateDescription vfio_pci_vmstate = {
> >      .name = "vfio-pci",
> >      .unmigratable = 1,
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        {
> > +            .name         = "vfio dev",
> > +            .version_id   = 0,
> > +            .field_exists = NULL,
> > +            .size         = 0,
> > +            .info         = &vfio_vmstate_info,
> > +            .flags        = VMS_SINGLE,
> > +            .offset       = 0,
> > +         },
> > +        VMSTATE_END_OF_LIST()
> > +    },
> >  };
> >
> >  static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
> > diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
> > index 4ddeebc..4451a8f 100644
> > --- a/linux-headers/linux/vfio.h
> > +++ b/linux-headers/linux/vfio.h
> > @@ -303,6 +303,9 @@ 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
> >
> > --
> > 2.7.4
> >
> >
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK