RE: [PATCH v11 21/23] vfio: Determine noiommu device in __vfio_register_dev()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Alex,

I've two new patches to address the comment in this patch. If
it makes sense then I'll put them in cdev v12.

> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Sent: Tuesday, May 23, 2023 10:13 AM
>
> > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Sent: Tuesday, May 23, 2023 7:04 AM
> >
> > On Sat, 13 May 2023 06:28:25 -0700
> > Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
> >
> > > This is to make the cdev path and group path consistent for the noiommu
> > > devices registration. If vfio_noiommu is disabled, such registration
> > > should fail. However, this check is vfio_device_set_group() which is part
> > > of the vfio_group code. If the vfio_group code is compiled out, noiommu
> > > devices would be registered even vfio_noiommu is disabled.
> > >
> > > This adds vfio_device_set_noiommu() which can fail and calls it in the
> > > device registration. For now, it never fails as long as
> > > vfio_device_set_group() is successful. But when the vfio_group code is
> > > compiled out, vfio_device_set_noiommu() would fail the noiommu devices
> > > when vfio_noiommu is disabled.
> >
> > I'm lost.  After the next patch we end up with the following when
> > CONFIG_VFIO_GROUP is set:
> >
> > static inline int vfio_device_set_noiommu(struct vfio_device *device)
> > {
> >         device->noiommu = IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> >                           device->group->type == VFIO_NO_IOMMU;
> >         return 0;
> > }
> >
> > I think this is relying on the fact that vfio_device_set_group() which
> > is called immediately prior to this function would have performed the
> > testing for noiommu and failed prior to this function being called and
> > therefore there is no error return here.
> 
> Yes.

Remove the IS_ENABLED(CONFIG_VFIO_NOIOMMU) check:

>From 3e93d33dc426350389a89130557a212cf370fee6 Mon Sep 17 00:00:00 2001
From: Yi Liu <yi.l.liu@xxxxxxxxx>
Date: Tue, 23 May 2023 20:48:08 -0700
Subject: [PATCH 19/23] vfio: Only check group->type for noiommu test

group->type can be VFIO_NO_IOMMU only when vfio_noiommu option is true.
And vfio_noiommu option can only be true if CONFIG_VFIO_NOIOMMU is enabled.
So checking group->type is enough when testing noiommu.

Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
 drivers/vfio/group.c | 3 +--
 drivers/vfio/vfio.h  | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index c930406cc261..3b56959fcdbb 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -133,8 +133,7 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group,
 
 	iommufd = iommufd_ctx_from_file(f.file);
 	if (!IS_ERR(iommufd)) {
-		if (IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
-		    group->type == VFIO_NO_IOMMU)
+		if (group->type == VFIO_NO_IOMMU)
 			ret = iommufd_vfio_compat_set_no_iommu(iommufd);
 		else
 			ret = iommufd_vfio_compat_ioas_create(iommufd);
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 0f66d0934e91..104c2ee93833 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -108,8 +108,7 @@ void vfio_group_cleanup(void);
 
 static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
 {
-	return IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
-	       vdev->group->type == VFIO_NO_IOMMU;
+	return vdev->group->type == VFIO_NO_IOMMU;
 }
 
 #if IS_ENABLED(CONFIG_VFIO_CONTAINER)
-- 
2.34.1

> >
> > Note also here that I think CONFIG_VFIO_NOIOMMU was only being tested
> > here previously so that a smart enough compiler would optimize out the
> > entire function, we can never set a VFIO_NO_IOMMU type when
> > !CONFIG_VFIO_NOIOMMU.
> 
> You are right. VFIO_NO_IOMMU type is only set when vfio_noiommu==true.
> 
> > That's no longer the case if the function is
> > refactored like this.
> >
> > When !CONFIG_VFIO_GROUP:
> >
> > static inline int vfio_device_set_noiommu(struct vfio_device *device)
> > {
> >         struct iommu_group *iommu_group;
> >
> >         iommu_group = iommu_group_get(device->dev);
> >         if (!iommu_group) {
> >                 if (!IS_ENABLED(CONFIG_VFIO_NOIOMMU) || !vfio_noiommu)
> >                         return -EINVAL;
> >                 device->noiommu = true;
> >         } else {
> >                 iommu_group_put(iommu_group);
> >                 device->noiommu = false;
> >         }
> >
> >         return 0;
> > }
> >
> > Here again, the NOIOMMU config option is irrelevant, vfio_noiommu can
> > only be true if the config option is enabled.  Therefore if there's no
> > IOMMU group and the module option to enable noiommu is not set, return
> > an error.
> 
> Yes. I think I missed the part that the vfio_noiommu option can only be
> true when CONFIG_VFIO_NOIOMMU is enabled. That's why the check
> is "IS_ENABLED(CONFIG_VFIO_NOIOMMU) && device->group->type ==
> VFIO_NO_IOMMU;".
> This appears that the two conditions are orthogonal.
> 
> >
> > It's really quite ugly that in one mode we rely on this function to
> > generate the error and in the other mode it happens prior to getting
> > here.
> >
> > The above could be simplified to something like:
> >
> > 	iommu_group = iommu_group_get(device->dev);
> > 	if (!iommu_group && !vfio_iommu)
> > 		return -EINVAL;
> >
> > 	device->noiommu = !iommu_group;
> > 	iommu_group_put(iommu_group); /* Accepts NULL */
> > 	return 0;
> >
> > Which would actually work regardless of CONFIG_VFIO_GROUP, where maybe
> > this could then be moved before vfio_device_set_group() and become the
> > de facto exit point for invalid noiommu configurations and maybe we
> > could remove the test from the group code (with a comment to note that
> > it's been tested prior)?  Thanks,
> 
> Yes, this looks much clearer. I think this new logic must be called before
> vfio_device_set_group(). Otherwise,  iommu_group_get () may return
> the faked groups. Then it would need to work differently per CONFIG_VFIO_GROUP
> configuration.

Below patch adopts your suggestion on noiommu determination.
It also moves the noiommu taint in vfio_device_set_noiommu().

>From 757a168acca7e94beec4448fba4600155569d823 Mon Sep 17 00:00:00 2001
From: Yi Liu <yi.l.liu@xxxxxxxxx>
Date: Tue, 9 May 2023 01:55:28 -0700
Subject: [PATCH 20/23] vfio: Determine noiommu device in __vfio_register_dev()

This moves the noiommu device determination and noiommu taint out of
vfio_group_find_or_alloc().

This is also helpful for compiling out vfio_group infrastructure when
vfio device cdev is added as noiommu determination is common between
the cdev path and group path.

Suggested-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
 drivers/vfio/group.c     | 18 +++---------------
 drivers/vfio/vfio_main.c | 25 +++++++++++++++++++++++++
 include/linux/vfio.h     |  1 +
 3 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index 3b56959fcdbb..9ee6e70531d3 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -670,21 +670,6 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
 	struct vfio_group *group;
 
 	iommu_group = iommu_group_get(dev);
-	if (!iommu_group && vfio_noiommu) {
-		/*
-		 * With noiommu enabled, create an IOMMU group for devices that
-		 * don't already have one, implying no IOMMU hardware/driver
-		 * exists.  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, VFIO_NO_IOMMU);
-		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;
-	}
-
 	if (!iommu_group)
 		return ERR_PTR(-EINVAL);
 
@@ -720,6 +705,9 @@ int vfio_device_set_group(struct vfio_device *device,
 {
 	struct vfio_group *group;
 
+	if (device->noiommu)
+		type = VFIO_NO_IOMMU;
+
 	if (type == VFIO_IOMMU)
 		group = vfio_group_find_or_alloc(device->dev);
 	else
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 20f463088e71..da46e2e74642 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -266,6 +266,18 @@ static int vfio_init_device(struct vfio_device *device, struct device *dev,
 	return ret;
 }
 
+static int vfio_device_set_noiommu(struct vfio_device *device)
+{
+	struct iommu_group *iommu_group = iommu_group_get(device->dev);
+
+	if (!iommu_group && !vfio_noiommu)
+		return -EINVAL;
+
+	device->noiommu = !iommu_group;
+	iommu_group_put(iommu_group); /* Accepts NULL */
+	return 0;
+}
+
 static int __vfio_register_dev(struct vfio_device *device,
 			       enum vfio_group_type type)
 {
@@ -285,6 +297,10 @@ static int __vfio_register_dev(struct vfio_device *device,
 	if (!device->dev_set)
 		vfio_assign_device_set(device, device);
 
+	ret = vfio_device_set_noiommu(device);
+	if (ret)
+		return ret;
+
 	ret = vfio_device_set_group(device, type);
 	if (ret)
 		return ret;
@@ -303,6 +319,15 @@ static int __vfio_register_dev(struct vfio_device *device,
 
 	vfio_device_group_register(device);
 
+	if (device->noiommu) {
+		/*
+		 * noiommu deivces have no IOMMU hardware/driver.  Taint the
+		 * kernel because we're about to give a DMA capable device to
+		 * a user without IOMMU protection.
+		 */
+		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+		dev_warn(device->dev, "Adding kernel taint for vfio-noiommu on device\n");
+	}
 	return 0;
 err_out:
 	vfio_device_remove_group(device);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index e80a8ac86e46..183e620009e7 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -67,6 +67,7 @@ struct vfio_device {
 	bool iommufd_attached;
 #endif
 	bool cdev_opened:1;
+	bool noiommu:1;
 };
 
 /**
-- 
2.34.1





[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux