Re: [RFC v3 19/19] vfio-user: migration support

[Alex Williamson]

On Mon,  8 Nov 2021 16:46:47 -0800
John Johnson <john.g.johnson@oracle.com> wrote:

> bug fix: only set qemu file error if there is a file


I don't understand this commit log.  Is this meant to be a revision
log?  In general it would be nice to have more detailed commit logs for
many of these patches describing any nuances.

Note that the migration uAPI is being reevaluated on the kernel side,
so I expect we'll want to hold off on vfio-user beyond minimal stub
support.  Thanks,

Alex

> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  hw/vfio/user-protocol.h | 18 +++++++++++++++++
>  hw/vfio/migration.c     | 34 +++++++++++++++----------------
>  hw/vfio/pci.c           |  7 +++++++
>  hw/vfio/user.c          | 54 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 96 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/user-protocol.h b/hw/vfio/user-protocol.h
> index c5d9473..bad067a 100644
> --- a/hw/vfio/user-protocol.h
> +++ b/hw/vfio/user-protocol.h
> @@ -182,6 +182,10 @@ typedef struct {
>      char data[];
>  } VFIOUserDMARW;
>  
> +/*
> + * VFIO_USER_DIRTY_PAGES
> + */
> +
>  /*imported from struct vfio_bitmap */
>  typedef struct {
>      uint64_t pgsize;
> @@ -189,4 +193,18 @@ typedef struct {
>      char data[];
>  } VFIOUserBitmap;
>  
> +/* imported from struct vfio_iommu_type1_dirty_bitmap_get */
> +typedef struct {
> +    uint64_t iova;
> +    uint64_t size;
> +    VFIOUserBitmap bitmap;
> +} VFIOUserBitmapRange;
> +
> +/* imported from struct vfio_iommu_type1_dirty_bitmap */
> +typedef struct {
> +    VFIOUserHdr hdr;
> +    uint32_t argsz;
> +    uint32_t flags;
> +} VFIOUserDirtyPages;
> +
>  #endif /* VFIO_USER_PROTOCOL_H */
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 82f654a..3d379cb 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -27,6 +27,7 @@
>  #include "pci.h"
>  #include "trace.h"
>  #include "hw/hw.h"
> +#include "user.h"
>  
>  /*
>   * Flags to be used as unique delimiters for VFIO devices in the migration
> @@ -49,11 +50,13 @@ static int64_t bytes_transferred;
>  static inline int vfio_mig_access(VFIODevice *vbasedev, void *val, int count,
>                                    off_t off, bool iswrite)
>  {
> +    VFIORegion *region = &vbasedev->migration->region;
>      int ret;
>  
> -    ret = iswrite ? pwrite(vbasedev->fd, val, count, off) :
> -                    pread(vbasedev->fd, val, count, off);
> -    if (ret < count) {
> +    ret = iswrite ?
> +        VDEV_REGION_WRITE(vbasedev, region->nr, off, count, val, false) :
> +        VDEV_REGION_READ(vbasedev, region->nr, off, count, val);
> +     if (ret < count) {
>          error_report("vfio_mig_%s %d byte %s: failed at offset 0x%"
>                       HWADDR_PRIx", err: %s", iswrite ? "write" : "read", 
> count,
>                       vbasedev->name, off, strerror(errno));
> @@ -111,9 +114,7 @@ static int vfio_migration_set_state(VFIODevice *vbasedev, 
> uint32_t mask,
>                                      uint32_t value)
>  {
>      VFIOMigration *migration = vbasedev->migration;
> -    VFIORegion *region = &migration->region;
> -    off_t dev_state_off = region->fd_offset +
> -                          VFIO_MIG_STRUCT_OFFSET(device_state);
> +    off_t dev_state_off = VFIO_MIG_STRUCT_OFFSET(device_state);
>      uint32_t device_state;
>      int ret;
>  
> @@ -201,13 +202,13 @@ static int vfio_save_buffer(QEMUFile *f, VFIODevice 
> *vbasedev, uint64_t *size)
>      int ret;
>  
>      ret = vfio_mig_read(vbasedev, &data_offset, sizeof(data_offset),
> -                      region->fd_offset + 
> VFIO_MIG_STRUCT_OFFSET(data_offset));
> +                        VFIO_MIG_STRUCT_OFFSET(data_offset));
>      if (ret < 0) {
>          return ret;
>      }
>  
>      ret = vfio_mig_read(vbasedev, &data_size, sizeof(data_size),
> -                        region->fd_offset + 
> VFIO_MIG_STRUCT_OFFSET(data_size));
> +                        VFIO_MIG_STRUCT_OFFSET(data_size));
>      if (ret < 0) {
>          return ret;
>      }
> @@ -233,8 +234,7 @@ static int vfio_save_buffer(QEMUFile *f, VFIODevice 
> *vbasedev, uint64_t *size)
>              }
>              buf_allocated = true;
>  
> -            ret = vfio_mig_read(vbasedev, buf, sec_size,
> -                                region->fd_offset + data_offset);
> +            ret = vfio_mig_read(vbasedev, buf, sec_size, data_offset);
>              if (ret < 0) {
>                  g_free(buf);
>                  return ret;
> @@ -269,7 +269,7 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice 
> *vbasedev,
>  
>      do {
>          ret = vfio_mig_read(vbasedev, &data_offset, sizeof(data_offset),
> -                      region->fd_offset + 
> VFIO_MIG_STRUCT_OFFSET(data_offset));
> +                            VFIO_MIG_STRUCT_OFFSET(data_offset));
>          if (ret < 0) {
>              return ret;
>          }
> @@ -309,8 +309,7 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice 
> *vbasedev,
>              qemu_get_buffer(f, buf, sec_size);
>  
>              if (buf_alloc) {
> -                ret = vfio_mig_write(vbasedev, buf, sec_size,
> -                        region->fd_offset + data_offset);
> +                ret = vfio_mig_write(vbasedev, buf, sec_size, data_offset);
>                  g_free(buf);
>  
>                  if (ret < 0) {
> @@ -322,7 +321,7 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice 
> *vbasedev,
>          }
>  
>          ret = vfio_mig_write(vbasedev, &report_size, sizeof(report_size),
> -                        region->fd_offset + 
> VFIO_MIG_STRUCT_OFFSET(data_size));
> +                             VFIO_MIG_STRUCT_OFFSET(data_size));
>          if (ret < 0) {
>              return ret;
>          }
> @@ -334,12 +333,11 @@ static int vfio_load_buffer(QEMUFile *f, VFIODevice 
> *vbasedev,
>  static int vfio_update_pending(VFIODevice *vbasedev)
>  {
>      VFIOMigration *migration = vbasedev->migration;
> -    VFIORegion *region = &migration->region;
>      uint64_t pending_bytes = 0;
>      int ret;
>  
>      ret = vfio_mig_read(vbasedev, &pending_bytes, sizeof(pending_bytes),
> -                    region->fd_offset + 
> VFIO_MIG_STRUCT_OFFSET(pending_bytes));
> +                        VFIO_MIG_STRUCT_OFFSET(pending_bytes));
>      if (ret < 0) {
>          migration->pending_bytes = 0;
>          return ret;
> @@ -744,7 +742,9 @@ static void vfio_vmstate_change(void *opaque, bool 
> running, RunState state)
>           */
>          error_report("%s: Failed to set device state 0x%x", vbasedev->name,
>                       (migration->device_state & mask) | value);
> -        qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
> +        if (value != 0) {
> +            qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
> +        }
>      }
>      vbasedev->migration->vm_running = running;
>      trace_vfio_vmstate_change(vbasedev->name, running, RunState_str(state),
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ccb2563..2e3846e 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3754,6 +3754,13 @@ static void vfio_user_pci_realize(PCIDevice *pdev, 
> Error **errp)
>          }
>      }
>  
> +    if (!pdev->failover_pair_id) {
> +        ret = vfio_migration_probe(&vdev->vbasedev, errp);
> +        if (ret) {
> +            error_report("%s: Migration disabled", vdev->vbasedev.name);
> +        }
> +    }
> +
>      vfio_register_err_notifier(vdev);
>      vfio_register_req_notifier(vdev);
>  
> diff --git a/hw/vfio/user.c b/hw/vfio/user.c
> index 76d0706..460d4e5 100644
> --- a/hw/vfio/user.c
> +++ b/hw/vfio/user.c
> @@ -1398,6 +1398,52 @@ void vfio_user_reset(VFIOProxy *proxy)
>      }
>  }
>  
> +static int vfio_user_dirty_bitmap(VFIOProxy *proxy,
> +                                  struct vfio_iommu_type1_dirty_bitmap *cmd,
> +                                  struct vfio_iommu_type1_dirty_bitmap_get
> +                                  *dbitmap)
> +{
> +    g_autofree struct {
> +        VFIOUserDirtyPages msg;
> +        VFIOUserBitmapRange range;
> +    } *msgp = NULL;
> +    int msize, rsize;
> +
> +    /*
> +     * If just the command is sent, the returned bitmap isn't needed.
> +     * The bitmap structs are different from the ioctl() versions,
> +     * ioctl() returns the bitmap in a local VA
> +     */
> +    if (dbitmap != NULL) {
> +        msize = sizeof(*msgp);
> +        rsize = msize + dbitmap->bitmap.size;
> +        msgp = g_malloc0(rsize);
> +        msgp->range.iova = dbitmap->iova;
> +        msgp->range.size = dbitmap->size;
> +        msgp->range.bitmap.pgsize = dbitmap->bitmap.pgsize;
> +        msgp->range.bitmap.size = dbitmap->bitmap.size;
> +    } else {
> +        msize = rsize = sizeof(VFIOUserDirtyPages);
> +        msgp = g_malloc0(rsize);
> +    }
> +
> +    vfio_user_request_msg(&msgp->msg.hdr, VFIO_USER_DIRTY_PAGES, msize, 0);
> +    msgp->msg.argsz = msize - sizeof(msgp->msg.hdr);
> +    msgp->msg.flags = cmd->flags;
> +
> +    vfio_user_send_wait(proxy, &msgp->msg.hdr, NULL, rsize, false);
> +    if (msgp->msg.hdr.flags & VFIO_USER_ERROR) {
> +        return -msgp->msg.hdr.error_reply;
> +    }
> +
> +    if (dbitmap != NULL) {
> +        memcpy(dbitmap->bitmap.data, &msgp->range.bitmap.data,
> +               dbitmap->bitmap.size);
> +    }
> +
> +    return 0;
> +}
> +
>  
>  /*
>   * Socket-based io_ops
> @@ -1493,6 +1539,13 @@ static int vfio_user_io_dma_unmap(VFIOContainer 
> *container,
>      return vfio_user_dma_unmap(container->proxy, unmap, bitmap, will_commit);
>  }
>  
> +static int vfio_user_io_dirty_bitmap(VFIOContainer *container,
> +                        struct vfio_iommu_type1_dirty_bitmap *bitmap,
> +                        struct vfio_iommu_type1_dirty_bitmap_get *range)
> +{
> +    return vfio_user_dirty_bitmap(container->proxy, bitmap, range);
> +}
> +
>  static void vfio_user_io_wait_commit(VFIOContainer *container)
>  {
>      vfio_user_wait_reqs(container->proxy);
> @@ -1501,5 +1554,6 @@ static void vfio_user_io_wait_commit(VFIOContainer 
> *container)
>  VFIOContIO vfio_cont_io_sock = {
>      .dma_map = vfio_user_io_dma_map,
>      .dma_unmap = vfio_user_io_dma_unmap,
> +    .dirty_bitmap = vfio_user_io_dirty_bitmap,
>      .wait_commit = vfio_user_io_wait_commit,
>  };