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?
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;
> > > };
> > >
> > > /**
>
