On Tue, 17 Jan 2023 05:49:38 -0800 Yi Liu <yi.l.liu@xxxxxxxxx> wrote: > This prepares to add ioctls for device cdev fd. This infrastructure includes: > - add vfio_iommufd_attach() to support iommufd pgtable attach after > bind_iommufd. A NULL pt_id indicates detach. > - let vfio_iommufd_bind() to accept pt_id, e.g. the comapt_ioas_id in the > legacy group path, and also return back dev_id if caller requires it. > > Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx> > --- > drivers/vfio/group.c | 12 +++++- > drivers/vfio/iommufd.c | 79 ++++++++++++++++++++++++++++++---------- > drivers/vfio/vfio.h | 15 ++++++-- > drivers/vfio/vfio_main.c | 10 +++-- > 4 files changed, 88 insertions(+), 28 deletions(-) > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > index 7200304663e5..9484bb1c54a9 100644 > --- a/drivers/vfio/group.c > +++ b/drivers/vfio/group.c > @@ -157,6 +157,8 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group, > static int vfio_device_group_open(struct vfio_device_file *df) > { > struct vfio_device *device = df->device; > + u32 ioas_id; > + u32 *pt_id = NULL; > int ret; > > mutex_lock(&device->group->group_lock); > @@ -165,6 +167,14 @@ static int vfio_device_group_open(struct vfio_device_file *df) > goto err_unlock_group; > } > > + if (device->group->iommufd) { > + ret = iommufd_vfio_compat_ioas_id(device->group->iommufd, > + &ioas_id); > + if (ret) > + goto err_unlock_group; > + pt_id = &ioas_id; > + } > + > mutex_lock(&device->dev_set->lock); > /* > * Here we pass the KVM pointer with the group under the lock. If the > @@ -174,7 +184,7 @@ static int vfio_device_group_open(struct vfio_device_file *df) > df->kvm = device->group->kvm; > df->iommufd = device->group->iommufd; > > - ret = vfio_device_open(df); > + ret = vfio_device_open(df, NULL, pt_id); > if (ret) > goto err_unlock_device; > mutex_unlock(&device->dev_set->lock); > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c > index 4f82a6fa7c6c..412644fdbf16 100644 > --- a/drivers/vfio/iommufd.c > +++ b/drivers/vfio/iommufd.c > @@ -10,9 +10,17 @@ > MODULE_IMPORT_NS(IOMMUFD); > MODULE_IMPORT_NS(IOMMUFD_VFIO); > > -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) > +/* @pt_id == NULL implies detach */ > +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id) > +{ > + lockdep_assert_held(&vdev->dev_set->lock); > + > + return vdev->ops->attach_ioas(vdev, pt_id); > +} I find this patch pretty confusing, I think it's rooted in all these multiplexed interfaces, which extend all the way out to userspace with a magic, reserved page table ID to detach a device from an IOAS. It seems like it would be simpler to make a 'detach' API, a detach_ioas callback on the vfio_device_ops, and certainly not an vfio_iommufd_attach() function that does a detach provided the correct args while also introducing a __vfio_iommufd_detach() function. This series is also missing an update to Documentation/driver-api/vfio.rst, which is already behind relative to the iommufd interfaces. Thanks, Alex