diff mbox series

[05/14] vfio: refactor noiommu group creation

Message ID 20210826133424.3362-6-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev() | expand

Commit Message

Christoph Hellwig Aug. 26, 2021, 1:34 p.m. UTC 
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>
---
 drivers/vfio/vfio.c | 101 ++++++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 50 deletions(-)

Comments

Alex Williamson Aug. 26, 2021, 10:47 p.m. UTC | #1 
On Thu, 26 Aug 2021 15:34:15 +0200
Christoph Hellwig <hch@lst.de> wrote:
> +#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 (group) {
> +			add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +			dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n");
> +		}

Perhaps what Kevin was pointing out here in the previous version,
vfio_noiommu_group_alloc() returns a pointer, so this should test
!IS_ERR(group).  Thanks,

Alex
Tian, Kevin Aug. 27, 2021, 2:08 a.m. UTC | #2 
> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Friday, August 27, 2021 6:47 AM
> 
> On Thu, 26 Aug 2021 15:34:15 +0200
> Christoph Hellwig <hch@lst.de> wrote:
> > +#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 (group) {
> > +			add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> > +			dev_warn(dev, "Adding kernel taint for vfio-
> noiommu group on device\n");
> > +		}
> 
> Perhaps what Kevin was pointing out here in the previous version,
> vfio_noiommu_group_alloc() returns a pointer, so this should test
> !IS_ERR(group).  Thanks,
> 

yes, that is what I meant.
diff mbox series

Patch

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 852fe22125520d..109d3ef57665b0 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 (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);
 
@@ -844,10 +849,6 @@  struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
 	return group;
 
 out_put:
-#ifdef CONFIG_VFIO_NOIOMMU
-	if (iommu_group_get_iommudata(iommu_group) == &noiommu)
-		iommu_group_remove_device(dev);
-#endif
 	iommu_group_put(iommu_group);
 	return group;
 }