Re: [PATCH v6 21/24] vfio: Add VFIO_DEVICE_BIND_IOMMUFD

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Mar 21, 2023 at 02:37:58PM +0000, Liu, Yi L wrote:
> > From: Jason Gunthorpe <jgg@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@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.

Jason