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

Tue Jun 13 17:15:11 UTC 2023

[Sorry for breaking threading, replying to my own message id with reply
 content from Yi since the Cc list got broken]

On Tue, 13 Jun 2023 15:28:06 +0000
"Liu, Yi L" <yi.l.liu at intel.com> wrote:

> > From: Alex Williamson <alex.williamson at redhat.com>
> > Sent: Tuesday, June 13, 2023 11:13 PM
> > 
> > On Tue, 13 Jun 2023 15:01:35 +0000
> > "Liu, Yi L" <yi.l.liu at intel.com> wrote:
> >   
> > > > From: Alex Williamson <alex.williamson at 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 at intel.com> wrote:
> > > >  
> > > > > > From: Alex Williamson <alex.williamson at 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 at intel.com> wrote:
> > > > > >  
> > > > > > > > From: Alex Williamson <alex.williamson at redhat.com>
> > > > > > > > Sent: Tuesday, June 13, 2023 6:42 AM
> > > > > > > >
> > > > > > > > On Fri,  2 Jun 2023 05:16:50 -0700
> > > > > > > > Yi Liu <yi.l.liu at 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,  
> 
> Hmmm. So your suggestion is to fail the vfio_alloc_device() if the input
> device is not IOMMU backed. right? But at this point, we don't know if
> the caller is trying to allocate vfio_device for a physical device or an
> emulated device. For emulated devices, cdev entry can always be created.
> Is it? I think the iommu_group test should be done for only physical devices
> in the register time.
> 
> Can I have an iommu_backed flag to store the iommu_group test result
> and check it when trying to create/remove cdev entry?

Ok, let me rephrase, the probe function needs to fail for a physical
(VFIO_IOMMU) device when VFIO_NO_IOIMMU is not configured and
vfio_noiommu is not enabled, there should never be a vfio group or cdev
device file created and the vfio_device should never be fully registered.
I overreacted a bit that we should never have a vfio_device at all, we
clearly need one leading up to determining if we can proceed.

If we renamed your function above to vfio_device_has_iommu_group(),
couldn't we just wrap device_add like below instead to not have cdev
setup for a noiommu device, generate an error for a physical device w/o
IOMMU backing, and otherwise setup the cdev device?

static inline int vfio_device_add(struct vfio_device *device, enum vfio_group_type type)
{
#if IS_ENABLED(CONFIG_VFIO_GROUP)
	if (device->group->type == VFIO_NO_IOMMU)
		return device_add(&device->device);
#else
	if (type == VFIO_IOMMU && !vfio_device_has_iommu_group(device))
		return -EINVAL;
#endif
	vfio_init_device_cdev(device);
	return cdev_device_add(&device->cdev, &device->device);
}

static inline void vfio_device_del(struct vfio_device *device)
{
#if IS_ENABLED(CONFIG_VFIO_GROUP)
	if (device->group->type == VFIO_NO_IOMMU)
		return device_del(&device->device);
#endif
	cdev_device_del(&device->cdev, &device->device);
}

I think this is the only extent to which noiommu needs to be a factor
here, skip cdev setup for a noiommu device.  Thanks,

Alex