Re: [Intel-gfx] [PATCH v12 21/24] vfio: Determine noiommu device in __vfio_register_dev()

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 13 Jun 2023 15:01:35 +0000
"Liu, Yi L" <yi.l.liu@intel.com> wrote:

> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Tuesday, June 13, 2023 10:48 PM
> > 
> > On Tue, 13 Jun 2023 14:33:01 +0000
> > "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> >   
> > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > Sent: Tuesday, June 13, 2023 10:19 PM
> > > >
> > > > On Tue, 13 Jun 2023 05:53:42 +0000
> > > > "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> > > >  
> > > > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > > > Sent: Tuesday, June 13, 2023 6:42 AM
> > > > > >
> > > > > > On Fri,  2 Jun 2023 05:16:50 -0700
> > > > > > Yi Liu <yi.l.liu@intel.com> 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.  
> > 
> > "noiommu" is a vfio designation of a device, it must be created by
> > vfio.    
> 
> Sure.
> 
> > There can certainly be devices which are not IOMMU backed, but
> > without vfio designating them as noiommu devices, which is only done
> > via the legacy and compat paths, there's no such thing as a noiommu
> > device.   
> 
> Yes.
> 
> > Devices without an IOMMU are simply out of scope for cdev,
> > there should never be a vfio cdev entry created for them.  Thanks,  
> 
> Actually, this is what I want to solve. I need to check if a device is
> IOMMU backed or not, and based on this info to prevent creating
> cdev entry for them in the coming cdev support or may need to
> fail registration if VFIO_GROUP is unset.
> 
> If this patch is not good. I can use the vfio_device_is_noiommu()
> written like below when VFIO_GROUP is unset. What about your
> opinion?
> 
> static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
> {
> 	struct iommu_group *iommu_group;
> 
> 	iommu_group = iommu_group_get(vdev->dev);
> 	iommu_group_put(iommu_group); /* Accepts NULL */
> 	return !iommu_group;
> }


No, please do not confuse the issue.  As we agreed above "noiommu"
means a specific thing, it's a device without IOMMU backing that vfio
has artificially included in the environment.  If we don't have
VFIO_NOIOMMU then there's no such thing as a "noiommu" device.

You can certainly use an iommu_group test to decide if a device should
be represented, but there absolutely should never be a vfio_device
created without IOMMU backing and without VFIO_NOIOMMU.  Thanks,

Alex