On Fri, 3 Feb 2023 15:21:09 -0700 Alex Williamson <alex.william...@redhat.com> wrote:
> On Wed, 1 Feb 2023 21:55:39 -0800 > John Johnson <john.g.john...@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.john...@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-avih...@nvidia.com/ [2]https://lore.kernel.org/all/20230126184948.10478-1-avih...@nvidia.com/