> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Tuesday, June 13, 2023 10:19 PM
>
> On Tue, 13 Jun 2023 05:53:42 +0000
> "Liu, Yi L" <yi.l.liu@xxxxxxxxx> wrote:
>
> > > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > > Sent: Tuesday, June 13, 2023 6:42 AM
> > >
> > > On Fri, 2 Jun 2023 05:16:50 -0700
> > > Yi Liu <yi.l.liu@xxxxxxxxx> 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@xxxxxxxxxx>
> > > > Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> > > > ---
> > > > 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;
> > > > };
> > > >
> > > > /**
> >
