[Intel-gfx] [PATCH v6 21/24] vfio: Add VFIO_DEVICE_BIND_IOMMUFD

[Liu, Yi L]

> From: Jason Gunthorpe <jgg at nvidia.com>
> Sent: Tuesday, March 21, 2023 10:41 PM
> 
> On Tue, Mar 21, 2023 at 02:37:58PM +0000, Liu, Yi L wrote:
> > > From: Jason Gunthorpe <jgg at nvidia.com>
> > > Sent: Tuesday, March 21, 2023 8:01 PM
> > >
> > > On Tue, Mar 21, 2023 at 01:30:34AM +0000, Tian, Kevin wrote:
> > > > > From: Jason Gunthorpe <jgg at nvidia.com>
> > > > > Sent: Tuesday, March 21, 2023 1:17 AM
> > > > >
> > > > > On Mon, Mar 20, 2023 at 10:31:53PM +0800, Yi Liu wrote:
> > > > > > On 2023/3/20 22:09, Jason Gunthorpe wrote:
> > > > > > > On Wed, Mar 15, 2023 at 04:40:19AM +0000, Liu, Yi L wrote:
> > > > > > >
> > > > > > > > # if IS_ENABLED(CONFIG_VFIO_GROUP)
> > > > > > > > static inline bool vfio_device_is_noiommu(struct vfio_device
> > > *vdev)
> > > > > > > > {
> > > > > > > >          return IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> > > > > > > >                 vdev->group->type == VFIO_NO_IOMMU;
> > > > > > > > }
> > > > > > > > #else
> > > > > > > > static inline bool vfio_device_is_noiommu(struct vfio_device
> > > *vdev)
> > > > > > > > {
> > > > > > > >          struct iommu_group *iommu_group;
> > > > > > > >
> > > > > > > >          if (!IS_ENABLED(CONFIG_VFIO_NOIOMMU)
> > > || !vfio_noiommu)
> > > > > > > >                  return -EINVAL;
> > > > > > > >
> > > > > > > >          iommu_group = iommu_group_get(vdev->dev);
> > > > > > > >          if (iommu_group)
> > > > > > > >                  iommu_group_put(iommu_group);
> > > > > > > >
> > > > > > > >          return !iommu_group;
> > > > > > >
> > > > > > > If we don't have VFIO_GROUP then no-iommu is signaled by a
> NULL
> > > > > > > iommu_ctx pointer in the vdev, don't mess with groups
> > > > > >
> > > > > > yes, NULL iommufd_ctx pointer would be set in vdev and passed
> to
> > > the
> > > > > > vfio_device_open(). But here, we want to use this helper to check
> if
> > > > > > user can use noiommu mode. This is before calling
> vfio_device_open().
> > > > > > e.g. if the device is protected by iommu, then user cannot use
> > > noiommu
> > > > > > mode on it.
> > > > >
> > > > > Why not allow it?
> > > > >
> > > > > If the admin has enabled this mode we may as well let it be used.
> > > > >
> > > > > You explicitly ask for no-iommu mode by passing -1 for the iommufd
> > > > > parameter. If the module parameter says it is allowed then that is all
> > > > > you need.
> > > > >
> > > >
> > > > IMHO we should disallow noiommu on a device which already has
> > > > a iommu group. This is how noiommu works with vfio group. I don't
> > > > think it's a good idea to further relax it in cdev.
> > >
> > > This isn't the same thing, this will trigger for mdevs and stuff that
> > > should not be noiommu as well.
> >
> > But the group path does disallow noiommu usage if the device has
> > a real iommu_group (the one created by VFIO code is not real). Would
> > it be better to keep it consistent from this angle?
> >
> > > If you want to copy what the group code does then noiommu needs to
> be
> > > statically determined at physical vfio device allocation time.
> >
> > There is another reason which may not that strong. For devices protected
> > by iommu, user needs to program IOVA mappings in order to do DMA.
> Such
> > device has a real iommu_group.
> 
> Oh that is a good reason for sure
> 
> But still, this check should be done at device creation time just like
> in group mode, not during each attach call.

Seems like requiring a noiommu_capable flag in vfio_device. We
set this flag only when the vfio_group->type==VFIO_NO_IOMMU
or no real iommu_group (for the case in which vfio group code is
compiled out). Perhaps the below check should be added as well.
Then in the time of bind, just check the noiommu_capable flag
and capable(CAP_SYS_RAWIO).

if (!IS_ENABLED(CONFIG_VFIO_NOIOMMU) || !vfio_noiommu)

Regards,
Yi Liu