RE: [PATCH 05/14] vfio: refactor noiommu group creation

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Christoph Hellwig <hch@lst.de>,
	Alex Williamson <alex.williamson@redhat.com>
Cc: Diana Craciun <diana.craciun@oss.nxp.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Kirti Wankhede <kwankhede@nvidia.com>,
	Eric Auger <eric.auger@redhat.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	"Xu, Terrence" <terrence.xu@intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Jason Gunthorpe" <jgg@nvidia.com>
Subject: RE: [PATCH 05/14] vfio: refactor noiommu group creation
Date: Tue, 14 Sep 2021 02:07:34 +0000	[thread overview]
Message-ID: <BN9PR11MB5433842C4B5A4053B09EB0968CDA9@BN9PR11MB5433.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20210913071606.2966-6-hch@lst.de>

> From: Christoph Hellwig <hch@lst.de>
> Sent: Monday, September 13, 2021 3:16 PM
> 
> Split the actual noiommu group creation from vfio_iommu_group_get into a
> new helper, and open code the rest of vfio_iommu_group_get in its only
> caller.  This creates an entirely separate and clear code path for the
> noiommu group creation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/vfio/vfio.c | 104 ++++++++++++++++++++++----------------------
>  1 file changed, 52 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 2b2679c7126fdf..b1ed156adccd04 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -169,50 +169,6 @@ static void vfio_release_device_set(struct
> vfio_device *device)
>  	xa_unlock(&vfio_device_set_xa);
>  }
> 
> -static struct iommu_group *vfio_iommu_group_get(struct device *dev)
> -{
> -	struct iommu_group *group;
> -	int __maybe_unused ret;
> -
> -	group = iommu_group_get(dev);
> -
> -#ifdef CONFIG_VFIO_NOIOMMU
> -	/*
> -	 * With noiommu enabled, an IOMMU group will be created for a
> device
> -	 * that doesn't already have one and doesn't have an iommu_ops on
> their
> -	 * bus.  We set iommudata simply to be able to identify these groups
> -	 * as special use and for reclamation later.
> -	 */
> -	if (group || !noiommu || iommu_present(dev->bus))
> -		return group;
> -
> -	group = iommu_group_alloc();
> -	if (IS_ERR(group))
> -		return NULL;
> -
> -	iommu_group_set_name(group, "vfio-noiommu");
> -	iommu_group_set_iommudata(group, &noiommu, NULL);
> -	ret = iommu_group_add_device(group, dev);
> -	if (ret) {
> -		iommu_group_put(group);
> -		return NULL;
> -	}
> -
> -	/*
> -	 * Where to taint?  At this point we've added an IOMMU group for a
> -	 * device that is not backed by iommu_ops, therefore any iommu_
> -	 * callback using iommu_ops can legitimately Oops.  So, while we
> may
> -	 * be about to give a DMA capable device to a user without IOMMU
> -	 * protection, which is clearly taint-worthy, let's go ahead and do
> -	 * it here.
> -	 */
> -	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> -	dev_warn(dev, "Adding kernel taint for vfio-noiommu group on
> device\n");
> -#endif
> -
> -	return group;
> -}
> -
>  #ifdef CONFIG_VFIO_NOIOMMU
>  static void *vfio_noiommu_open(unsigned long arg)
>  {
> @@ -823,12 +779,61 @@ void vfio_uninit_group_dev(struct vfio_device
> *device)
>  }
>  EXPORT_SYMBOL_GPL(vfio_uninit_group_dev);
> 
> -struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
> +#ifdef CONFIG_VFIO_NOIOMMU
> +static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev)
>  {
>  	struct iommu_group *iommu_group;
>  	struct vfio_group *group;
> +	int ret;
> +
> +	iommu_group = iommu_group_alloc();
> +	if (IS_ERR(iommu_group))
> +		return ERR_CAST(iommu_group);
> 
> -	iommu_group = vfio_iommu_group_get(dev);
> +	iommu_group_set_name(iommu_group, "vfio-noiommu");
> +	iommu_group_set_iommudata(iommu_group, &noiommu, NULL);
> +	ret = iommu_group_add_device(iommu_group, dev);
> +	if (ret)
> +		goto out_put_group;
> +
> +	group = vfio_create_group(iommu_group);
> +	if (IS_ERR(group)) {
> +		ret = PTR_ERR(group);
> +		goto out_remove_device;
> +	}
> +
> +	return group;
> +
> +out_remove_device:
> +	iommu_group_remove_device(dev);
> +out_put_group:
> +	iommu_group_put(iommu_group);
> +	return ERR_PTR(ret);
> +}
> +#endif
> +
> +static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
> +{
> +	struct iommu_group *iommu_group;
> +	struct vfio_group *group;
> +
> +	iommu_group = iommu_group_get(dev);
> +#ifdef CONFIG_VFIO_NOIOMMU
> +	if (!iommu_group && noiommu && !iommu_present(dev->bus)) {
> +		/*
> +		 * With noiommu enabled, create an IOMMU group for
> devices that
> +		 * don't already have one and don't have an iommu_ops on
> their
> +		 * bus.  Taint the kernel because we're about to give a DMA
> +		 * capable device to a user without IOMMU protection.
> +		 */
> +		group = vfio_noiommu_group_alloc(dev);
> +		if (!IS_ERR(group)) {
> +			add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +			dev_warn(dev, "Adding kernel taint for vfio-
> noiommu group on device\n");
> +		}
> +		return group;
> +	}
> +#endif
>  	if (!iommu_group)
>  		return ERR_PTR(-EINVAL);
> 
> @@ -840,14 +845,9 @@ struct vfio_group *vfio_group_find_or_alloc(struct
> device *dev)
>  	/* a newly created vfio_group keeps the reference. */
>  	group = vfio_create_group(iommu_group);
>  	if (IS_ERR(group))
> -		goto out_remove;
> +		goto out_put;
>  	return group;
> 
> -out_remove:
> -#ifdef CONFIG_VFIO_NOIOMMU
> -	if (iommu_group_get_iommudata(iommu_group) == &noiommu)
> -		iommu_group_remove_device(dev);
> -#endif
>  out_put:
>  	iommu_group_put(iommu_group);
>  	return group;
> --
> 2.30.2