[Intel-gfx] [PATCH v12 21/24] vfio: Determine noiommu device in __vfio_register_dev()

[Liu, Yi L]

> From: Alex Williamson <alex.williamson at redhat.com>
> Sent: Tuesday, June 13, 2023 10:19 PM
> 
> On Tue, 13 Jun 2023 05:53:42 +0000
> "Liu, Yi L" <yi.l.liu at intel.com> wrote:
> 
> > > From: Alex Williamson <alex.williamson at redhat.com>
> > > Sent: Tuesday, June 13, 2023 6:42 AM
> > >
> > > On Fri,  2 Jun 2023 05:16:50 -0700
> > > Yi Liu <yi.l.liu at intel.com> wrote:
> > >
> > > > This moves the noiommu device determination and noiommu taint out of
> > > > vfio_group_find_or_alloc(). noiommu device is determined in
> > > > __vfio_register_dev() and result is stored in flag vfio_device->noiommu,
> > > > the noiommu taint is added in the end of __vfio_register_dev().
> > > >
> > > > This is also a preparation for compiling out vfio_group infrastructure
> > > > as it makes the noiommu detection and taint common between the cdev path
> > > > and group path though cdev path does not support noiommu.
> > >
> > > Does this really still make sense?  The motivation for the change is
> > > really not clear without cdev support for noiommu.  Thanks,
> >
> > I think it still makes sense. When CONFIG_VFIO_GROUP==n, the kernel
> > only supports cdev interface. If there is noiommu device, vfio should
> > fail the registration. So, the noiommu determination is still needed. But
> > I'd admit the taint might still be in the group code.
> 
> How is there going to be a noiommu device when VFIO_GROUP is unset?

How about booting a kernel with iommu disabled, then all the devices
are not protected by iommu. I suppose they are noiommu devices. If
user wants to bound them to vfio, the kernel should have VFIO_GROUP.
Otherwise, needs to fail.

Regards,
Yi Liu

> Thanks,
> 
> Alex
> 
> 
> > > > Suggested-by: Alex Williamson <alex.williamson at redhat.com>
> > > > Signed-off-by: Yi Liu <yi.l.liu at intel.com>
> > > > ---
> > > >  drivers/vfio/group.c     | 15 ---------------
> > > >  drivers/vfio/vfio_main.c | 31 ++++++++++++++++++++++++++++++-
> > > >  include/linux/vfio.h     |  1 +
> > > >  3 files changed, 31 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > > > index 653b62f93474..64cdd0ea8825 100644
> > > > --- a/drivers/vfio/group.c
> > > > +++ b/drivers/vfio/group.c
> > > > @@ -668,21 +668,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);
> > > >
> > > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > > > index 6d8f9b0f3637..00a699b9f76b 100644
> > > > --- a/drivers/vfio/vfio_main.c
> > > > +++ b/drivers/vfio/vfio_main.c
> > > > @@ -265,6 +265,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)
> > > >  {
> > > > @@ -277,6 +289,13 @@ static int __vfio_register_dev(struct vfio_device *device,
> > > >  		     !device->ops->detach_ioas)))
> > > >  		return -EINVAL;
> > > >
> > > > +	/* Only physical devices can be noiommu device */
> > > > +	if (type == VFIO_IOMMU) {
> > > > +		ret = vfio_device_set_noiommu(device);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > >  	/*
> > > >  	 * If the driver doesn't specify a set then the device is added to a
> > > >  	 * singleton set just for itself.
> > > > @@ -288,7 +307,8 @@ static int __vfio_register_dev(struct vfio_device *device,
> > > >  	if (ret)
> > > >  		return ret;
> > > >
> > > > -	ret = vfio_device_set_group(device, type);
> > > > +	ret = vfio_device_set_group(device,
> > > > +				    device->noiommu ? VFIO_NO_IOMMU : type);
> > > >  	if (ret)
> > > >  		return ret;
> > > >
> > > > @@ -301,6 +321,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;
> > > >  };
> > > >
> > > >  /**
> >