> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Friday, January 20, 2023 7:05 AM
> 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.
Sure. Will change it.
>
> This series is also missing an update to
> Documentation/driver-api/vfio.rst, which is already behind relative to
> the iommufd interfaces. Thanks,
Yeah, the vfio.rst is already a bit out-of-date. Will try to update it as well.
Regards,
Yi Liu
