| Message ID | 20230117134942.101112-10-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add vfio_device cdev for iommufd support | expand |
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Tuesday, January 17, 2023 9:50 PM > 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); having both ioas_id and pt_id in one function is a bit confusing. Does it read better with below? if (device->group->iommufd) ret = vfio_device_open(df, NULL, &ioas_id); else ret = vfio_device_open(df, NULL, NULL); > +/* @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); > +} what benefit does this one-line wrapper give actually? especially pt_id==NULL is checked in the callback instead of in this wrapper.
On Tue, 17 Jan 2023 05:49:38 -0800 Yi Liu <yi.l.liu@intel.com> 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@intel.com> > --- > 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
> From: Tian, Kevin <kevin.tian@intel.com> > Sent: Thursday, January 19, 2023 5:45 PM > > > From: Liu, Yi L <yi.l.liu@intel.com> > > Sent: Tuesday, January 17, 2023 9:50 PM > > 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); > > having both ioas_id and pt_id in one function is a bit confusing. > > Does it read better with below? > > if (device->group->iommufd) > ret = vfio_device_open(df, NULL, &ioas_id); > else > ret = vfio_device_open(df, NULL, NULL); Yes.
> From: Alex Williamson <alex.williamson@redhat.com> > Sent: Friday, January 20, 2023 7:05 AM > On Tue, 17 Jan 2023 05:49:38 -0800 > Yi Liu <yi.l.liu@intel.com> 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@intel.com> > > --- > > 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
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); +} + +int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx, + u32 *dev_id, u32 *pt_id) { - u32 ioas_id; u32 device_id; int ret; @@ -29,17 +37,14 @@ int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) if (ret) return ret; - ret = iommufd_vfio_compat_ioas_id(ictx, &ioas_id); - if (ret) - goto err_unbind; - ret = vdev->ops->attach_ioas(vdev, &ioas_id); - if (ret) - goto err_unbind; + if (pt_id) { + ret = vfio_iommufd_attach(vdev, pt_id); + if (ret) + goto err_unbind; + } - /* - * The legacy path has no way to return the device id or the selected - * pt_id - */ + if (dev_id) + *dev_id = device_id; return 0; err_unbind: @@ -74,14 +79,18 @@ int vfio_iommufd_physical_bind(struct vfio_device *vdev, } EXPORT_SYMBOL_GPL(vfio_iommufd_physical_bind); +static void __vfio_iommufd_detach(struct vfio_device *vdev) +{ + iommufd_device_detach(vdev->iommufd_device); + vdev->iommufd_attached = false; +} + void vfio_iommufd_physical_unbind(struct vfio_device *vdev) { lockdep_assert_held(&vdev->dev_set->lock); - if (vdev->iommufd_attached) { - iommufd_device_detach(vdev->iommufd_device); - vdev->iommufd_attached = false; - } + if (vdev->iommufd_attached) + __vfio_iommufd_detach(vdev); iommufd_device_unbind(vdev->iommufd_device); vdev->iommufd_device = NULL; } @@ -91,6 +100,20 @@ int vfio_iommufd_physical_attach_ioas(struct vfio_device *vdev, u32 *pt_id) { int rc; + lockdep_assert_held(&vdev->dev_set->lock); + + if (!vdev->iommufd_device) + return -EINVAL; + + if (!pt_id) { + if (vdev->iommufd_attached) + __vfio_iommufd_detach(vdev); + return 0; + } + + if (vdev->iommufd_attached) + return -EBUSY; + rc = iommufd_device_attach(vdev->iommufd_device, pt_id); if (rc) return rc; @@ -129,14 +152,18 @@ int vfio_iommufd_emulated_bind(struct vfio_device *vdev, } EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_bind); +static void __vfio_iommufd_access_destroy(struct vfio_device *vdev) +{ + iommufd_access_destroy(vdev->iommufd_access); + vdev->iommufd_access = NULL; +} + void vfio_iommufd_emulated_unbind(struct vfio_device *vdev) { lockdep_assert_held(&vdev->dev_set->lock); - if (vdev->iommufd_access) { - iommufd_access_destroy(vdev->iommufd_access); - vdev->iommufd_access = NULL; - } + if (vdev->iommufd_access) + __vfio_iommufd_access_destroy(vdev); iommufd_ctx_put(vdev->iommufd_ictx); vdev->iommufd_ictx = NULL; } @@ -148,6 +175,18 @@ int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id) lockdep_assert_held(&vdev->dev_set->lock); + if (!vdev->iommufd_ictx) + return -EINVAL; + + if (!pt_id) { + if (vdev->iommufd_access) + __vfio_iommufd_access_destroy(vdev); + return 0; + } + + if (vdev->iommufd_access) + return -EBUSY; + user = iommufd_access_create(vdev->iommufd_ictx, *pt_id, &vfio_user_ops, vdev); if (IS_ERR(user)) diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h index c69a9902ea84..fe0fcfa78710 100644 --- a/drivers/vfio/vfio.h +++ b/drivers/vfio/vfio.h @@ -25,7 +25,8 @@ struct vfio_device_file { void vfio_device_put_registration(struct vfio_device *device); bool vfio_device_try_get_registration(struct vfio_device *device); -int vfio_device_open(struct vfio_device_file *df); +int vfio_device_open(struct vfio_device_file *df, + u32 *dev_id, u32 *pt_id); void vfio_device_close(struct vfio_device_file *device); struct vfio_device_file * @@ -230,11 +231,14 @@ static inline void vfio_container_cleanup(void) #endif #if IS_ENABLED(CONFIG_IOMMUFD) -int vfio_iommufd_bind(struct vfio_device *device, struct iommufd_ctx *ictx); +int vfio_iommufd_bind(struct vfio_device *device, struct iommufd_ctx *ictx, + u32 *dev_id, u32 *pt_id); void vfio_iommufd_unbind(struct vfio_device *device); +int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id); #else static inline int vfio_iommufd_bind(struct vfio_device *device, - struct iommufd_ctx *ictx) + struct iommufd_ctx *ictx, + u32 *dev_id, u32 *pt_id) { return -EOPNOTSUPP; } @@ -242,6 +246,11 @@ static inline int vfio_iommufd_bind(struct vfio_device *device, static inline void vfio_iommufd_unbind(struct vfio_device *device) { } + +static inline int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id) +{ + return -EOPNOTSUPP; +} #endif #if IS_ENABLED(CONFIG_VFIO_VIRQFD) diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index d442ebaa4b21..90174a9015c4 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -358,7 +358,8 @@ vfio_allocate_device_file(struct vfio_device *device) return df; } -static int vfio_device_first_open(struct vfio_device_file *df) +static int vfio_device_first_open(struct vfio_device_file *df, + u32 *dev_id, u32 *pt_id) { struct vfio_device *device = df->device; struct iommufd_ctx *iommufd = df->iommufd; @@ -371,7 +372,7 @@ static int vfio_device_first_open(struct vfio_device_file *df) return -ENODEV; if (iommufd) - ret = vfio_iommufd_bind(device, iommufd); + ret = vfio_iommufd_bind(device, iommufd, dev_id, pt_id); else ret = vfio_device_group_use_iommu(device); if (ret) @@ -413,7 +414,8 @@ static void vfio_device_last_close(struct vfio_device_file *df) module_put(device->dev->driver->owner); } -int vfio_device_open(struct vfio_device_file *df) +int vfio_device_open(struct vfio_device_file *df, + u32 *dev_id, u32 *pt_id) { struct vfio_device *device = df->device; @@ -423,7 +425,7 @@ int vfio_device_open(struct vfio_device_file *df) if (device->open_count == 1) { int ret; - ret = vfio_device_first_open(df); + ret = vfio_device_first_open(df, dev_id, pt_id); if (ret) { device->open_count--; return ret;
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@intel.com> --- 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(-)