| Message ID | 20230227111135.61728-11-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add vfio_device cdev for iommufd support | expand |
On Mon, Feb 27, 2023 at 03:11:26AM -0800, Yi Liu wrote: > For the device fd opened from cdev, userspace needs to bind it to an > iommufd and attach it to IOAS managed by iommufd. With such operations, > userspace can set up a secure DMA context and hence access device. > > This changes the existing vfio_iommufd_bind() to accept a pt_id pointer > as an optional input, and also an dev_id pointer to selectively return > the dev_id to prepare for adding bind_iommufd ioctl, which does the bind > first and then attach IOAS. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > drivers/vfio/group.c | 17 ++++++++++++++--- > drivers/vfio/iommufd.c | 21 +++++++++------------ > drivers/vfio/vfio.h | 9 ++++++--- > drivers/vfio/vfio_main.c | 10 ++++++---- > 4 files changed, 35 insertions(+), 22 deletions(-) > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > index d8771d585cb1..e44232551448 100644 > --- a/drivers/vfio/group.c > +++ b/drivers/vfio/group.c > @@ -169,6 +169,7 @@ static void vfio_device_group_get_kvm_safe(struct vfio_device *device) > static int vfio_device_group_open(struct vfio_device_file *df) > { > struct vfio_device *device = df->device; > + u32 ioas_id; > int ret; > > mutex_lock(&device->group->group_lock); > @@ -177,6 +178,13 @@ static int vfio_device_group_open(struct vfio_device_file *df) > goto out_unlock; > } > > + if (device->group->iommufd) { > + ret = iommufd_vfio_compat_ioas_id(device->group->iommufd, > + &ioas_id); > + if (ret) > + goto out_unlock; > + } I don't really like this being moved out of iommufd.c Pass in a NULL pt_id and the do some > -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) > +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 +29,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; io_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx, u32 *dev_id, u32 *pt_id) { u32 tmp_pt_id; if (!pt_id) { pt_id = &tmp_pt_id; ret = iommufd_vfio_compat_ioas_id(ictx, pt_id); if (ret) goto err_unbind; } To handle it And the commit message is sort of out of sync with the patch, more like: vfio: Pass the pt_id as an argument to vfio_iommufd_bind() To support binding the cdev the pt_id must come from userspace instead of being forced to the compat_ioas_id. Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, February 28, 2023 2:30 AM > > On Mon, Feb 27, 2023 at 03:11:26AM -0800, Yi Liu wrote: > > For the device fd opened from cdev, userspace needs to bind it to an > > iommufd and attach it to IOAS managed by iommufd. With such > operations, > > userspace can set up a secure DMA context and hence access device. > > > > This changes the existing vfio_iommufd_bind() to accept a pt_id pointer > > as an optional input, and also an dev_id pointer to selectively return > > the dev_id to prepare for adding bind_iommufd ioctl, which does the bind > > first and then attach IOAS. > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > --- > > drivers/vfio/group.c | 17 ++++++++++++++--- > > drivers/vfio/iommufd.c | 21 +++++++++------------ > > drivers/vfio/vfio.h | 9 ++++++--- > > drivers/vfio/vfio_main.c | 10 ++++++---- > > 4 files changed, 35 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > > index d8771d585cb1..e44232551448 100644 > > --- a/drivers/vfio/group.c > > +++ b/drivers/vfio/group.c > > @@ -169,6 +169,7 @@ static void > vfio_device_group_get_kvm_safe(struct vfio_device *device) > > static int vfio_device_group_open(struct vfio_device_file *df) > > { > > struct vfio_device *device = df->device; > > + u32 ioas_id; > > int ret; > > > > mutex_lock(&device->group->group_lock); > > @@ -177,6 +178,13 @@ static int vfio_device_group_open(struct > vfio_device_file *df) > > goto out_unlock; > > } > > > > + if (device->group->iommufd) { > > + ret = iommufd_vfio_compat_ioas_id(device->group- > >iommufd, > > + &ioas_id); > > + if (ret) > > + goto out_unlock; > > + } > > I don't really like this being moved out of iommufd.c > > Pass in a NULL pt_id and the do some > > > -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx > *ictx) > > +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 +29,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; > > io_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx, > u32 *dev_id, u32 *pt_id) > { > u32 tmp_pt_id; > if (!pt_id) { > pt_id = &tmp_pt_id; > ret = iommufd_vfio_compat_ioas_id(ictx, pt_id); > if (ret) > goto err_unbind; > > } > > To handle it > > And the commit message is sort of out of sync with the patch, more like: > > vfio: Pass the pt_id as an argument to vfio_iommufd_bind() > > To support binding the cdev the pt_id must come from userspace instead > of being forced to the compat_ioas_id. > Got it. not only pt_id, also dev_id.
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Tuesday, February 28, 2023 10:35 AM > > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Tuesday, February 28, 2023 2:30 AM > > > > On Mon, Feb 27, 2023 at 03:11:26AM -0800, Yi Liu wrote: > > > For the device fd opened from cdev, userspace needs to bind it to an > > > iommufd and attach it to IOAS managed by iommufd. With such > > operations, > > > userspace can set up a secure DMA context and hence access device. > > > > > > This changes the existing vfio_iommufd_bind() to accept a pt_id pointer > > > as an optional input, and also an dev_id pointer to selectively return > > > the dev_id to prepare for adding bind_iommufd ioctl, which does the > bind > > > first and then attach IOAS. > > > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > > --- > > > drivers/vfio/group.c | 17 ++++++++++++++--- > > > drivers/vfio/iommufd.c | 21 +++++++++------------ > > > drivers/vfio/vfio.h | 9 ++++++--- > > > drivers/vfio/vfio_main.c | 10 ++++++---- > > > 4 files changed, 35 insertions(+), 22 deletions(-) > > > > > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c > > > index d8771d585cb1..e44232551448 100644 > > > --- a/drivers/vfio/group.c > > > +++ b/drivers/vfio/group.c > > > @@ -169,6 +169,7 @@ static void > > vfio_device_group_get_kvm_safe(struct vfio_device *device) > > > static int vfio_device_group_open(struct vfio_device_file *df) > > > { > > > struct vfio_device *device = df->device; > > > + u32 ioas_id; > > > int ret; > > > > > > mutex_lock(&device->group->group_lock); > > > @@ -177,6 +178,13 @@ static int vfio_device_group_open(struct > > vfio_device_file *df) > > > goto out_unlock; > > > } > > > > > > + if (device->group->iommufd) { > > > + ret = iommufd_vfio_compat_ioas_id(device->group- > > >iommufd, > > > + &ioas_id); > > > + if (ret) > > > + goto out_unlock; > > > + } > > > > I don't really like this being moved out of iommufd.c > > > > Pass in a NULL pt_id and the do some > > > > > -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx > > *ictx) > > > +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 +29,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; > > > > io_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx, > > u32 *dev_id, u32 *pt_id) > > { > > u32 tmp_pt_id; > > if (!pt_id) { > > pt_id = &tmp_pt_id; > > ret = iommufd_vfio_compat_ioas_id(ictx, pt_id); > > if (ret) > > goto err_unbind; > > > > } > > > > To handle it > > > > And the commit message is sort of out of sync with the patch, more like: > > > > vfio: Pass the pt_id as an argument to vfio_iommufd_bind() > > > > To support binding the cdev the pt_id must come from userspace instead > > of being forced to the compat_ioas_id. > > Seems like pt_id is no more needed in the vfio_iommufd_bind() since it can get compat_ioas_id in the function itself. Cdev path never passes a pt_id to vfio_iommufd_bind() as its attach is done by separate ATTACH ioctl. Can we use the dev_id pointer to indicate if it needs to get the compat ioas and attach it? vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx, u32 *dev_id) { ... if (!dev_id) { u32 ioas_id; 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; } ... } Regards, Yi Liu
On Tue, Feb 28, 2023 at 02:35:25AM +0000, Liu, Yi L wrote: > > And the commit message is sort of out of sync with the patch, more like: > > > > vfio: Pass the pt_id as an argument to vfio_iommufd_bind() > > > > To support binding the cdev the pt_id must come from userspace instead > > of being forced to the compat_ioas_id. > > > > Got it. not only pt_id, also dev_id.
On Tue, Feb 28, 2023 at 06:58:38AM +0000, Liu, Yi L wrote: > Seems like pt_id is no more needed in the vfio_iommufd_bind() > since it can get compat_ioas_id in the function itself. Cdev path > never passes a pt_id to vfio_iommufd_bind() as its attach is done > by separate ATTACH ioctl. Can we use the dev_id pointer to indicate > if it needs to get the compat ioas and attach it? In this case you need to split the group code to also use the two step attach and then the attach will take in the null pt_id. Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, February 28, 2023 8:31 PM > > On Tue, Feb 28, 2023 at 06:58:38AM +0000, Liu, Yi L wrote: > > > Seems like pt_id is no more needed in the vfio_iommufd_bind() > > since it can get compat_ioas_id in the function itself. Cdev path > > never passes a pt_id to vfio_iommufd_bind() as its attach is done > > by separate ATTACH ioctl. Can we use the dev_id pointer to indicate > > if it needs to get the compat ioas and attach it? > > In this case you need to split the group code to also use the two step > attach and then the attach will take in the null pt_id. This seems to be the current way in this patch. Right? Group code passes a pt_id pointer to vfio_iommufd_bind(). While the cdev path just passes in a null pt_id pointer. Its attach is done later when user gives pt_id. Regards, Yi Liu
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, February 28, 2023 8:30 PM > > On Tue, Feb 28, 2023 at 02:35:25AM +0000, Liu, Yi L wrote: > > > > And the commit message is sort of out of sync with the patch, more like: > > > > > > vfio: Pass the pt_id as an argument to vfio_iommufd_bind() > > > > > > To support binding the cdev the pt_id must come from userspace > instead > > > of being forced to the compat_ioas_id. > > > > > > > Got it. not only pt_id, also dev_id.
On Tue, Feb 28, 2023 at 12:45:47PM +0000, Liu, Yi L wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Tuesday, February 28, 2023 8:31 PM > > > > On Tue, Feb 28, 2023 at 06:58:38AM +0000, Liu, Yi L wrote: > > > > > Seems like pt_id is no more needed in the vfio_iommufd_bind() > > > since it can get compat_ioas_id in the function itself. Cdev path > > > never passes a pt_id to vfio_iommufd_bind() as its attach is done > > > by separate ATTACH ioctl. Can we use the dev_id pointer to indicate > > > if it needs to get the compat ioas and attach it? > > > > In this case you need to split the group code to also use the two step > > attach and then the attach will take in the null pt_id. > > This seems to be the current way in this patch. Right? Group code passes > a pt_id pointer to vfio_iommufd_bind(). While the cdev path just passes > in a null pt_id pointer. Its attach is done later when user gives pt_id. I mean actually explicitly call attach and remove the implicit attach during bind flow entirely. Jason
On Tue, Feb 28, 2023 at 12:48:23PM +0000, Liu, Yi L wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Tuesday, February 28, 2023 8:30 PM > > > > On Tue, Feb 28, 2023 at 02:35:25AM +0000, Liu, Yi L wrote: > > > > > > And the commit message is sort of out of sync with the patch, more like: > > > > > > > > vfio: Pass the pt_id as an argument to vfio_iommufd_bind() > > > > > > > > To support binding the cdev the pt_id must come from userspace > > instead > > > > of being forced to the compat_ioas_id. > > > > > > > > > > Got it. not only pt_id, also dev_id.
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, February 28, 2023 8:52 PM > > On Tue, Feb 28, 2023 at 12:45:47PM +0000, Liu, Yi L wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Tuesday, February 28, 2023 8:31 PM > > > > > > On Tue, Feb 28, 2023 at 06:58:38AM +0000, Liu, Yi L wrote: > > > > > > > Seems like pt_id is no more needed in the vfio_iommufd_bind() > > > > since it can get compat_ioas_id in the function itself. Cdev path > > > > never passes a pt_id to vfio_iommufd_bind() as its attach is done > > > > by separate ATTACH ioctl. Can we use the dev_id pointer to indicate > > > > if it needs to get the compat ioas and attach it? > > > > > > In this case you need to split the group code to also use the two step > > > attach and then the attach will take in the null pt_id. > > > > This seems to be the current way in this patch. Right? Group code passes > > a pt_id pointer to vfio_iommufd_bind(). While the cdev path just passes > > in a null pt_id pointer. Its attach is done later when user gives pt_id. > > I mean actually explicitly call attach and remove the implicit attach > during bind flow entirely. Okay, so I can wrap the iommufd_vfio_compat_ioas_id() and ops->attach_ioas in a helper for group code to do attach after bind_iommufd. This can avoid to moving the iommufd_vfio_compat_ioas_id() out of iommufd.c as your original remark. Is this ok? Regards, Yi Liu
On Tue, Feb 28, 2023 at 12:56:11PM +0000, Liu, Yi L wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Tuesday, February 28, 2023 8:52 PM > > > > On Tue, Feb 28, 2023 at 12:45:47PM +0000, Liu, Yi L wrote: > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > > Sent: Tuesday, February 28, 2023 8:31 PM > > > > > > > > On Tue, Feb 28, 2023 at 06:58:38AM +0000, Liu, Yi L wrote: > > > > > > > > > Seems like pt_id is no more needed in the vfio_iommufd_bind() > > > > > since it can get compat_ioas_id in the function itself. Cdev path > > > > > never passes a pt_id to vfio_iommufd_bind() as its attach is done > > > > > by separate ATTACH ioctl. Can we use the dev_id pointer to indicate > > > > > if it needs to get the compat ioas and attach it? > > > > > > > > In this case you need to split the group code to also use the two step > > > > attach and then the attach will take in the null pt_id. > > > > > > This seems to be the current way in this patch. Right? Group code passes > > > a pt_id pointer to vfio_iommufd_bind(). While the cdev path just passes > > > in a null pt_id pointer. Its attach is done later when user gives pt_id. > > > > I mean actually explicitly call attach and remove the implicit attach > > during bind flow entirely. > > Okay, so I can wrap the iommufd_vfio_compat_ioas_id() and ops->attach_ioas > in a helper for group code to do attach after bind_iommufd. This can avoid to > moving the iommufd_vfio_compat_ioas_id() out of iommufd.c as your original > remark. > > Is this ok? Yes, some 'attach compat' helper makes sense Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, February 28, 2023 8:53 PM > > On Tue, Feb 28, 2023 at 12:48:23PM +0000, Liu, Yi L wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Tuesday, February 28, 2023 8:30 PM > > > > > > On Tue, Feb 28, 2023 at 02:35:25AM +0000, Liu, Yi L wrote: > > > > > > > > And the commit message is sort of out of sync with the patch, more > like: > > > > > > > > > > vfio: Pass the pt_id as an argument to vfio_iommufd_bind() > > > > > > > > > > To support binding the cdev the pt_id must come from userspace > > > instead > > > > > of being forced to the compat_ioas_id. > > > > > > > > > > > > > Got it. not only pt_id, also dev_id.
diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c index d8771d585cb1..e44232551448 100644 --- a/drivers/vfio/group.c +++ b/drivers/vfio/group.c @@ -169,6 +169,7 @@ static void vfio_device_group_get_kvm_safe(struct vfio_device *device) static int vfio_device_group_open(struct vfio_device_file *df) { struct vfio_device *device = df->device; + u32 ioas_id; int ret; mutex_lock(&device->group->group_lock); @@ -177,6 +178,13 @@ static int vfio_device_group_open(struct vfio_device_file *df) goto out_unlock; } + if (device->group->iommufd) { + ret = iommufd_vfio_compat_ioas_id(device->group->iommufd, + &ioas_id); + if (ret) + goto out_unlock; + } + mutex_lock(&device->dev_set->lock); /* @@ -188,9 +196,12 @@ static int vfio_device_group_open(struct vfio_device_file *df) if (device->open_count == 0) vfio_device_group_get_kvm_safe(device); - df->iommufd = device->group->iommufd; - - ret = vfio_device_open(df); + if (device->group->iommufd) { + df->iommufd = device->group->iommufd; + ret = vfio_device_open(df, NULL, &ioas_id); + } else { + ret = vfio_device_open(df, NULL, NULL); + } if (ret) df->iommufd = NULL; diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c index 4f82a6fa7c6c..beef6ca21107 100644 --- a/drivers/vfio/iommufd.c +++ b/drivers/vfio/iommufd.c @@ -10,9 +10,9 @@ MODULE_IMPORT_NS(IOMMUFD); MODULE_IMPORT_NS(IOMMUFD_VFIO); -int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx) +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 +29,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 = vdev->ops->attach_ioas(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: diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h index 64e862a02dad..04d2bd2e314d 100644 --- a/drivers/vfio/vfio.h +++ b/drivers/vfio/vfio.h @@ -26,7 +26,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 *df); struct vfio_device_file * vfio_allocate_device_file(struct vfio_device *device); @@ -223,7 +224,8 @@ 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); static inline struct iommufd_ctx * vfio_device_iommufd(struct vfio_device *device) @@ -232,7 +234,8 @@ vfio_device_iommufd(struct vfio_device *device) } #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; } diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index d16ac573e290..19ea65a87072 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -411,7 +411,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; @@ -423,7 +424,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) @@ -462,7 +463,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; int ret = 0; @@ -471,7 +473,7 @@ int vfio_device_open(struct vfio_device_file *df) device->open_count++; if (device->open_count == 1) { - ret = vfio_device_first_open(df); + ret = vfio_device_first_open(df, dev_id, pt_id); if (ret) device->open_count--; }