Re: [Qemu-devel] [RFC 4/5] vfio: use vfio_device_put/vfio_device_get for device status save/restore

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Yulei Zhang (yulei.zhang@intel.com) wrote:
> 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 <yulei.zhang@intel.com>
> ---
>  hw/vfio/pci.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 104 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 605a473..833cd90 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2961,18 +2961,121 @@ static void vfio_vm_change_state_handler(void *pv, int running, RunState state)
>      vfio_status->flags = running ? VFIO_DEVICE_PCI_START :
>  				  VFIO_DEVICE_PCI_STOP;
>  
> -    ioctl(vdev->vbasedev.fd, VFIO_DEVICE_PCI_STATUS_SET, vfio_status);
> +    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_PCI_STATUS_SET, vfio_status)) {
> +        error_report("vfio: Failed to %s device\n", running ? "start" : "stop");
> +    }
>      g_free(vfio_status);
>  }
>  
>  static int vfio_device_put(QEMUFile *f, void *pv, size_t size, VMStateField *field,
>                              QJSON *vmdesc)
>  {
> +    VFIOPCIDevice *vdev = pv;
> +    PCIDevice *pdev = &vdev->pdev;
> +    VFIORegion *region = &vdev->device_state.region;
> +    int sz = region->size;
> +    uint8_t *buf = NULL;
> +    uint32_t msi_cfg, msi_lo, msi_hi, msi_data, bar_cfg, i;
> +    bool msi_64bit;


Please use vmstate rather than qemu_get/qemu_put - we're trying to
get rid of all the qemu_put/qemu_get throughout the devices.
Probably using a pre_save/post_load is the right way to then tie the
data you've loaded to call the code that uploads it to the device.

> +    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);

Isn't all this stuff standard PCI config data that's already migrated?

> +    buf = g_malloc(sz);
> +    if (buf == NULL) {
> +        error_report("vfio: Failed to allocate memory for migrate\n");
> +        goto exit;
> +    }

g_malloc asserts rather than returns NULL if it fails to allocate.

> +    if (pread(vdev->vbasedev.fd, buf, sz, region->fd_offset) != sz) {
> +        error_report("vfio: Failed to read Device State Region\n");

Note error_report's shouldn't have \n's

> +        goto exit;
> +    }
> +
> +    qemu_put_buffer(f, buf, sz);

OK, so this is an opaque blob coming from the device.  Hmm.
Is it versioned?  Does it really need to be a closed blob?

Dave

> +exit:
> +    if (buf)
> +        g_free(buf);
> +
>      return 0;
>  }
>  
>  static int vfio_device_get(QEMUFile *f, void *pv, size_t size, VMStateField *field)
>  {
> +    VFIOPCIDevice *vdev = pv;
> +    PCIDevice *pdev = &vdev->pdev;
> +    VFIORegion *region = &vdev->device_state.region;
> +    int sz = region->size;
> +    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\n");
> +        return -1;
> +    }
> +
> +    qemu_get_buffer(f, buf, sz);
> +    if (pwrite(vdev->vbasedev.fd, buf, sz, region->fd_offset) != sz) {
> +        error_report("vfio: Failed to write Device State Region\n");
> +        return -1;
> +    }
> +
> +    if (buf)
> +	g_free(buf);
>      return 0;
>  }
>  
> -- 
> 2.7.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK