Re: [PATCH v2 03/23] vfio-user: add container IO ops vector

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 3 Feb 2023 15:21:09 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Wed,  1 Feb 2023 21:55:39 -0800
> John Johnson <john.g.johnson@oracle.com> wrote:
> 
> > Used for communication with VFIO driver
> > (prep work for vfio-user, which will communicate over a socket)
> > 
> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> > ---
> >  include/hw/vfio/vfio-common.h |  24 ++++++++
> >  hw/vfio/common.c              | 128 ++++++++++++++++++++++++++++--------------
> >  2 files changed, 110 insertions(+), 42 deletions(-)
> > 
> > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> > index e573f5a..953bc0f 100644
> > --- a/include/hw/vfio/vfio-common.h
> > +++ b/include/hw/vfio/vfio-common.h
> > @@ -75,6 +75,7 @@ typedef struct VFIOAddressSpace {
> >  } VFIOAddressSpace;
> >  
> >  struct VFIOGroup;
> > +typedef struct VFIOContainerIO VFIOContainerIO;
> >  
> >  typedef struct VFIOContainer {
> >      VFIOAddressSpace *space;
> > @@ -83,6 +84,7 @@ typedef struct VFIOContainer {
> >      MemoryListener prereg_listener;
> >      unsigned iommu_type;
> >      Error *error;
> > +    VFIOContainerIO *io;
> >      bool initialized;
> >      bool dirty_pages_supported;
> >      uint64_t dirty_pgsizes;
> > @@ -154,6 +156,28 @@ struct VFIODeviceOps {
> >      int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
> >  };
> >  
> > +#ifdef CONFIG_LINUX
> > +
> > +/*
> > + * The next 2 ops vectors are how Devices and Containers
> > + * communicate with the server.  The default option is
> > + * through ioctl() to the kernel VFIO driver, but vfio-user
> > + * can use a socket to a remote process.
> > + */
> > +
> > +struct VFIOContainerIO {
> > +    int (*dma_map)(VFIOContainer *container,
> > +                   struct vfio_iommu_type1_dma_map *map);
> > +    int (*dma_unmap)(VFIOContainer *container,
> > +                     struct vfio_iommu_type1_dma_unmap *unmap,
> > +                     struct vfio_bitmap *bitmap);
> > +    int (*dirty_bitmap)(VFIOContainer *container,
> > +                        struct vfio_iommu_type1_dirty_bitmap *bitmap,
> > +                        struct vfio_iommu_type1_dirty_bitmap_get *range);
> > +};
> > +
> > +#endif /* CONFIG_LINUX */
> > +
> >  typedef struct VFIOGroup {
> >      int fd;
> >      int groupid;
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index ace9562..9310a7f 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -58,6 +58,8 @@ static QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces =
> >  static int vfio_kvm_device_fd = -1;
> >  #endif
> >  
> > +static VFIOContainerIO vfio_cont_io_ioctl;
> > +
> >  /*
> >   * Common VFIO interrupt disable
> >   */
> > @@ -432,12 +434,12 @@ static int vfio_dma_unmap_bitmap(VFIOContainer *container,
> >          goto unmap_exit;
> >      }
> >  
> > -    ret = ioctl(container->fd, VFIO_IOMMU_UNMAP_DMA, unmap);
> > +    ret = container->io->dma_unmap(container, unmap, bitmap);  
> 
> The bitmap arg doesn't make a lot of sense here, its use doesn't come
> around until vfio_user_dma_unmap() is added, but even then, it's
> readily available at &unmap->data with validity determined by
> unmap->flags.  The eventual sanity test in vfio_user_dma_unmap() really
> only seems to prove the arg is unnecessary.  Thanks,


The same comment really applies to .dirty_bitmap() with respect to the
range arg, but I think it's also important to note that both of these
are all but deprecated interfaces for the kernel.  The migration
series[1] is pre-enabling these kernel interfaces to be removed by
adding support to QEMU to dirty all pages when support isn't present,
then we replace it with what we expect to be a longer term solution
with device dirty tracking support[2].  All the more reason not to make
these part of the internal API shared by kernel and user versions.
Thanks,

Alex

[1]https://lore.kernel.org/all/20230116141135.12021-1-avihaih@nvidia.com/
[2]https://lore.kernel.org/all/20230126184948.10478-1-avihaih@nvidia.com/