diff mbox series

[v11,10/23] vfio-iommufd: Move noiommu compat probe out of vfio_iommufd_bind()

Message ID 20230513132827.39066-11-yi.l.liu@intel.com (mailing list archive)
State New, archived
Headers show
Series Add vfio_device cdev for iommufd support | expand

Commit Message

Yi Liu May 13, 2023, 1:28 p.m. UTC 
into vfio_device_group_open(). This is more consistent with what will
be done in vfio device cdev path.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Tested-by: Terrence Xu <terrence.xu@intel.com>
Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Tested-by: Yanting Jiang <yanting.jiang@intel.com>
Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/vfio/group.c   |  6 ++++++
 drivers/vfio/iommufd.c | 32 +++++++++++++++++++-------------
 drivers/vfio/vfio.h    |  9 +++++++++
 3 files changed, 34 insertions(+), 13 deletions(-)

Comments

Alex Williamson May 22, 2023, 8:24 p.m. UTC | #1 
On Sat, 13 May 2023 06:28:14 -0700
Yi Liu <yi.l.liu@intel.com> wrote:

> into vfio_device_group_open(). This is more consistent with what will
> be done in vfio device cdev path.

Same comment regarding flowing commit subject into body on this series.

> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Tested-by: Terrence Xu <terrence.xu@intel.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Yanting Jiang <yanting.jiang@intel.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/vfio/group.c   |  6 ++++++
>  drivers/vfio/iommufd.c | 32 +++++++++++++++++++-------------
>  drivers/vfio/vfio.h    |  9 +++++++++
>  3 files changed, 34 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index a17584e8be15..cfd0b9254bbc 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -192,6 +192,12 @@ static int vfio_device_group_open(struct vfio_device_file *df)
>  		vfio_device_group_get_kvm_safe(device);
>  
>  	df->iommufd = device->group->iommufd;
> +	if (df->iommufd && vfio_device_is_noiommu(device) && device->open_count == 0) {
> +		ret = vfio_iommufd_compat_probe_noiommu(device,
> +							df->iommufd);
> +		if (ret)
> +			goto out_put_kvm;
> +	}
>  
>  	ret = vfio_device_open(df);
>  	if (ret) {
> diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> index a18e920be164..7a654a1437f0 100644
> --- a/drivers/vfio/iommufd.c
> +++ b/drivers/vfio/iommufd.c
> @@ -46,6 +46,24 @@ static void vfio_iommufd_noiommu_unbind(struct vfio_device *vdev)
>  	}
>  }
>  
> +int vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
> +				      struct iommufd_ctx *ictx)
> +{
> +	u32 ioas_id;
> +
> +	if (!capable(CAP_SYS_RAWIO))
> +		return -EPERM;
> +
> +	/*
> +	 * Require no compat ioas to be assigned to proceed.  The basic
> +	 * statement is that the user cannot have done something that
> +	 * implies they expected translation to exist
> +	 */
> +	if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
> +		return -EPERM;
> +	return 0;
> +}

I think the purpose of this function is to keep the iommufd namespace
out of the group, but we're muddying it as a general grab bag of
noiommu validation.  What if the caller retained the RAWIO test and
comment, and this function simply became a wrapper around the iommufd
function, ex:

bool vfio_iommufd_device_has_compat_ioas(struct vfio_device *device,
					 struct iommufd_ctx *ictx)
{
	u32 ioas_id;

	return !iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id));
}

Thanks,
Alex

> +
>  int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
>  {
>  	u32 ioas_id;
> @@ -54,20 +72,8 @@ int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
>  
>  	lockdep_assert_held(&vdev->dev_set->lock);
>  
> -	if (vfio_device_is_noiommu(vdev)) {
> -		if (!capable(CAP_SYS_RAWIO))
> -			return -EPERM;
> -
> -		/*
> -		 * Require no compat ioas to be assigned to proceed. The basic
> -		 * statement is that the user cannot have done something that
> -		 * implies they expected translation to exist
> -		 */
> -		if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
> -			return -EPERM;
> -
> +	if (vfio_device_is_noiommu(vdev))
>  		return vfio_iommufd_noiommu_bind(vdev, ictx, &device_id);
> -	}
>  
>  	ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id);
>  	if (ret)
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index 785afc40ece8..8884b557fb26 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -234,9 +234,18 @@ static inline void vfio_container_cleanup(void)
>  #endif
>  
>  #if IS_ENABLED(CONFIG_IOMMUFD)
> +int vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
> +				      struct iommufd_ctx *ictx);
>  int vfio_iommufd_bind(struct vfio_device *device, struct iommufd_ctx *ictx);
>  void vfio_iommufd_unbind(struct vfio_device *device);
>  #else
> +static inline int
> +vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
> +				  struct iommufd_ctx *ictx)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>  static inline int vfio_iommufd_bind(struct vfio_device *device,
>  				    struct iommufd_ctx *ictx)
>  {
Yi Liu May 23, 2023, 12:45 a.m. UTC | #2 
> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Tuesday, May 23, 2023 4:25 AM
> 
> On Sat, 13 May 2023 06:28:14 -0700
> Yi Liu <yi.l.liu@intel.com> wrote:
> 
> > into vfio_device_group_open(). This is more consistent with what will
> > be done in vfio device cdev path.
> 
> Same comment regarding flowing commit subject into body on this series.

Yes, I've modified the commit message in my local branch.:-)

> >
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > Tested-by: Terrence Xu <terrence.xu@intel.com>
> > Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> > Tested-by: Yanting Jiang <yanting.jiang@intel.com>
> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >  drivers/vfio/group.c   |  6 ++++++
> >  drivers/vfio/iommufd.c | 32 +++++++++++++++++++-------------
> >  drivers/vfio/vfio.h    |  9 +++++++++
> >  3 files changed, 34 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > index a17584e8be15..cfd0b9254bbc 100644
> > --- a/drivers/vfio/group.c
> > +++ b/drivers/vfio/group.c
> > @@ -192,6 +192,12 @@ static int vfio_device_group_open(struct vfio_device_file *df)
> >  		vfio_device_group_get_kvm_safe(device);
> >
> >  	df->iommufd = device->group->iommufd;
> > +	if (df->iommufd && vfio_device_is_noiommu(device) && device->open_count
> == 0) {
> > +		ret = vfio_iommufd_compat_probe_noiommu(device,
> > +							df->iommufd);
> > +		if (ret)
> > +			goto out_put_kvm;
> > +	}
> >
> >  	ret = vfio_device_open(df);
> >  	if (ret) {
> > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > index a18e920be164..7a654a1437f0 100644
> > --- a/drivers/vfio/iommufd.c
> > +++ b/drivers/vfio/iommufd.c
> > @@ -46,6 +46,24 @@ static void vfio_iommufd_noiommu_unbind(struct vfio_device
> *vdev)
> >  	}
> >  }
> >
> > +int vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
> > +				      struct iommufd_ctx *ictx)
> > +{
> > +	u32 ioas_id;
> > +
> > +	if (!capable(CAP_SYS_RAWIO))
> > +		return -EPERM;
> > +
> > +	/*
> > +	 * Require no compat ioas to be assigned to proceed.  The basic
> > +	 * statement is that the user cannot have done something that
> > +	 * implies they expected translation to exist
> > +	 */
> > +	if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
> > +		return -EPERM;
> > +	return 0;
> > +}
> 
> I think the purpose of this function is to keep the iommufd namespace
> out of the group,

Yes.

> but we're muddying it as a general grab bag of
> noiommu validation.  What if the caller retained the RAWIO test and
> comment, and this function simply became a wrapper around the iommufd
> function, ex:
> 
> bool vfio_iommufd_device_has_compat_ioas(struct vfio_device *device,
> 					 struct iommufd_ctx *ictx)
> {
> 	u32 ioas_id;
> 
> 	return !iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id));
> }

Sure. This looks better.

Regards,
Yi Liu
 
> Thanks,
> Alex
> 
> > +
> >  int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
> >  {
> >  	u32 ioas_id;
> > @@ -54,20 +72,8 @@ int vfio_iommufd_bind(struct vfio_device *vdev, struct
> iommufd_ctx *ictx)
> >
> >  	lockdep_assert_held(&vdev->dev_set->lock);
> >
> > -	if (vfio_device_is_noiommu(vdev)) {
> > -		if (!capable(CAP_SYS_RAWIO))
> > -			return -EPERM;
> > -
> > -		/*
> > -		 * Require no compat ioas to be assigned to proceed. The basic
> > -		 * statement is that the user cannot have done something that
> > -		 * implies they expected translation to exist
> > -		 */
> > -		if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
> > -			return -EPERM;
> > -
> > +	if (vfio_device_is_noiommu(vdev))
> >  		return vfio_iommufd_noiommu_bind(vdev, ictx, &device_id);
> > -	}
> >
> >  	ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id);
> >  	if (ret)
> > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> > index 785afc40ece8..8884b557fb26 100644
> > --- a/drivers/vfio/vfio.h
> > +++ b/drivers/vfio/vfio.h
> > @@ -234,9 +234,18 @@ static inline void vfio_container_cleanup(void)
> >  #endif
> >
> >  #if IS_ENABLED(CONFIG_IOMMUFD)
> > +int vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
> > +				      struct iommufd_ctx *ictx);
> >  int vfio_iommufd_bind(struct vfio_device *device, struct iommufd_ctx *ictx);
> >  void vfio_iommufd_unbind(struct vfio_device *device);
> >  #else
> > +static inline int
> > +vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
> > +				  struct iommufd_ctx *ictx)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> >  static inline int vfio_iommufd_bind(struct vfio_device *device,
> >  				    struct iommufd_ctx *ictx)
> >  {
diff mbox series

Patch

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index a17584e8be15..cfd0b9254bbc 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -192,6 +192,12 @@  static int vfio_device_group_open(struct vfio_device_file *df)
 		vfio_device_group_get_kvm_safe(device);
 
 	df->iommufd = device->group->iommufd;
+	if (df->iommufd && vfio_device_is_noiommu(device) && device->open_count == 0) {
+		ret = vfio_iommufd_compat_probe_noiommu(device,
+							df->iommufd);
+		if (ret)
+			goto out_put_kvm;
+	}
 
 	ret = vfio_device_open(df);
 	if (ret) {
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index a18e920be164..7a654a1437f0 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -46,6 +46,24 @@  static void vfio_iommufd_noiommu_unbind(struct vfio_device *vdev)
 	}
 }
 
+int vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
+				      struct iommufd_ctx *ictx)
+{
+	u32 ioas_id;
+
+	if (!capable(CAP_SYS_RAWIO))
+		return -EPERM;
+
+	/*
+	 * Require no compat ioas to be assigned to proceed.  The basic
+	 * statement is that the user cannot have done something that
+	 * implies they expected translation to exist
+	 */
+	if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
+		return -EPERM;
+	return 0;
+}
+
 int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
 {
 	u32 ioas_id;
@@ -54,20 +72,8 @@  int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
 
 	lockdep_assert_held(&vdev->dev_set->lock);
 
-	if (vfio_device_is_noiommu(vdev)) {
-		if (!capable(CAP_SYS_RAWIO))
-			return -EPERM;
-
-		/*
-		 * Require no compat ioas to be assigned to proceed. The basic
-		 * statement is that the user cannot have done something that
-		 * implies they expected translation to exist
-		 */
-		if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
-			return -EPERM;
-
+	if (vfio_device_is_noiommu(vdev))
 		return vfio_iommufd_noiommu_bind(vdev, ictx, &device_id);
-	}
 
 	ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id);
 	if (ret)
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 785afc40ece8..8884b557fb26 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -234,9 +234,18 @@  static inline void vfio_container_cleanup(void)
 #endif
 
 #if IS_ENABLED(CONFIG_IOMMUFD)
+int vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
+				      struct iommufd_ctx *ictx);
 int vfio_iommufd_bind(struct vfio_device *device, struct iommufd_ctx *ictx);
 void vfio_iommufd_unbind(struct vfio_device *device);
 #else
+static inline int
+vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
+				  struct iommufd_ctx *ictx)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int vfio_iommufd_bind(struct vfio_device *device,
 				    struct iommufd_ctx *ictx)
 {