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

From: Jason Gunthorpe <jgg@nvidia.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: "Liu, Yi L" <yi.l.liu@intel.com>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"joro@8bytes.org" <joro@8bytes.org>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"cohuck@redhat.com" <cohuck@redhat.com>,
	"eric.auger@redhat.com" <eric.auger@redhat.com>,
	"nicolinc@nvidia.com" <nicolinc@nvidia.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"mjrosato@linux.ibm.com" <mjrosato@linux.ibm.com>,
	"chao.p.peng@linux.intel.com" <chao.p.peng@linux.intel.com>,
	"yi.y.sun@linux.intel.com" <yi.y.sun@linux.intel.com>,
	"peterx@redhat.com" <peterx@redhat.com>,
	"jasowang@redhat.com" <jasowang@redhat.com>,
	"shameerali.kolothum.thodi@huawei.com" 
	<shameerali.kolothum.thodi@huawei.com>,
	"lulu@redhat.com" <lulu@redhat.com>,
	"suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
	"intel-gvt-dev@lists.freedesktop.org" 
	<intel-gvt-dev@lists.freedesktop.org>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
	"Hao, Xudong" <xudong.hao@intel.com>,
	"Zhao, Yan Y" <yan.y.zhao@intel.com>,
	"Xu, Terrence" <terrence.xu@intel.com>
Subject: Re: [PATCH v6 21/24] vfio: Add VFIO_DEVICE_BIND_IOMMUFD
Date: Tue, 21 Mar 2023 09:00:44 -0300	[thread overview]
Message-ID: <ZBmcbMuE4lyDIpT6@nvidia.com> (raw)
In-Reply-To: <BN9PR11MB527605E6609222431DFB8AA58C819@BN9PR11MB5276.namprd11.prod.outlook.com>

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.

If you want to copy what the group code does then noiommu needs to be
statically determined at physical vfio device allocation time.

Jason