Re: [PATCH v6 5/7] vfio: Enable cdev noiommu mode under iommufd

[Yi Liu <yi.l.liu@intel.com>]

On 5/29/26 02:52, Jacob Pan wrote:

>>>> static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
>>>> {
>>>>        return IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
>>>>               vdev->group->type == VFIO_NO_IOMMU;
>>>> }
>>>>   
>>>>>     		return 0;
>>>>>     
>>>>>     	return vdev->ops->bind_iommufd(vdev, ictx, &df->devid);
>>>>> @@ -58,7 +58,7 @@ void vfio_df_iommufd_unbind(struct
>>>>> vfio_device_file *df)
>>>>>     	lockdep_assert_held(&vdev->dev_set->lock);
>>>>>     
>>>>> -	if (vfio_device_is_noiommu(vdev))
>>>>> +	if (df->group && vfio_device_is_noiommu(vdev))
>>>>>     		return;
>>>>>     
>>>>>     	if (vdev->ops->unbind_iommufd)
>>>>> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
>>>>> index e4b72e79b7e3..6f0a2dfc8a00 100644
>>>>> --- a/drivers/vfio/vfio.h
>>>>> +++ b/drivers/vfio/vfio.h
>>>>> @@ -358,19 +358,13 @@ void vfio_init_device_cdev(struct
>>>>> vfio_device *device);
>>>>>     static inline int vfio_device_add(struct vfio_device *device)
>>>>>     {
>>>>> -	/* cdev does not support noiommu device */
>>>>> -	if (vfio_device_is_noiommu(device))
>>>>> -		return device_add(&device->device);
>>>>>     	vfio_init_device_cdev(device);
>>>>>     	return cdev_device_add(&device->cdev, &device->device);
>>>>>     }
>>>>>     
>>>>>     static inline void vfio_device_del(struct vfio_device *device)
>>>>>     {
>>>>> -	if (vfio_device_is_noiommu(device))
>>>>> -		device_del(&device->device);
>>>>> -	else
>>>>> -		cdev_device_del(&device->cdev, &device->device);
>>>>> +	cdev_device_del(&device->cdev, &device->device);
>>>>>     }
>>>>>     
>>>>>     int vfio_device_fops_cdev_open(struct inode *inode, struct file
>>>>> *filep); @@ -420,6 +414,18 @@ static inline void
>>>>> vfio_cdev_cleanup(void) }
>>>>>     #endif /* CONFIG_VFIO_DEVICE_CDEV */
>>>>>     
>>>>> +#if IS_ENABLED(CONFIG_VFIO_NOIOMMU)
>>>>> +static inline bool vfio_device_is_cdev_noiommu(struct vfio_device
>>>>> *vdev) +{
>>>>> +	return vdev->noiommu;
>>>>> +}
>>>>> +#else
>>>>> +static inline bool vfio_device_is_cdev_noiommu(struct vfio_device
>>>>> *vdev) +{
>>>>> +	return false;
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>>     #if IS_ENABLED(CONFIG_VFIO_VIRQFD)
>>>>>     int __init vfio_virqfd_init(void);
>>>>>     void vfio_virqfd_exit(void);
>>>>> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
>>>>> index 6222376ab6ab..84381c500623 100644
>>>>> --- a/drivers/vfio/vfio_main.c
>>>>> +++ b/drivers/vfio/vfio_main.c
>>>>> @@ -321,6 +321,20 @@ static int vfio_init_device(struct
>>>>> vfio_device *device, struct device *dev, return ret;
>>>>>     }
>>>>>     
>>>>> +static int vfio_device_set_noiommu_and_name(struct vfio_device
>>>>> *device) +{
>>>>> +	if (IS_ENABLED(CONFIG_VFIO_DEVICE_CDEV) && vfio_noiommu
>>>>> && !device->dev->iommu) {
>>>>> +		device->noiommu = true;
>>>>> +		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
>>>>> +		dev_warn(device->dev,
>>>>> +			 "Adding kernel taint for vfio-noiommu
>>>>> cdev on device\n");
>>>>> +	}
>>>>> +
>>>>> +	/* Just to be safe, expose to user explicitly noiommu
>>>>> cdev node */
>>>>> +	return dev_set_name(&device->device, "%svfio%d",
>>>>> +		     device->noiommu ? "noiommu-" : "",
>>>>> device->index); +}
>>>>> +
>>>>>     static int __vfio_register_dev(struct vfio_device *device,
>>>>>     			       enum vfio_group_type type)
>>>>>     {
>>>>> @@ -340,20 +354,21 @@ static int __vfio_register_dev(struct
>>>>> vfio_device *device, if (!device->dev_set)
>>>>>     		vfio_assign_device_set(device, device);
>>>>>     
>>>>> -	ret = dev_set_name(&device->device, "vfio%d",
>>>>> device->index);
>>>>> +	ret = vfio_device_set_group(device, type);
>>>>>     	if (ret)
>>>>>     		return ret;
>>>>>     
>>>>> -	ret = vfio_device_set_group(device, type);
>>>>> +	ret = vfio_device_set_noiommu_and_name(device);
>>>>
>>>> the order of dev_set_name and vfio_device_set_group() are swapped,
>>>> any special reason?
>>> The ordering was intentional in an earlier version where the cdev
>>> noiommu check depended on device->group. With the current check
>>> using !device->dev->iommu, the ordering is no longer strictly
>>> required for that test.
>>>    
>>> I kept vfio_device_set_group() first because the rest of
>>> registration already treats group setup as the first VFIO state to
>>> unwind, and this lets the existing err_out path handle failures
>>> after group assignment, including dev_set_name(). I can restore the
>>> old order if you prefer, since it is not functionally required
>>> anymore.
>>
>> I think it's better to keep the original order if no functional
>> requirement anymore.
> will do,
> 
>>
>>>>>     	if (ret)
>>>>> -		return ret;
>>>>> +		goto err_out;
>>>>>     
>>>>>     	/*
>>>>>     	 * VFIO always sets IOMMU_CACHE because we offer no way
>>>>> for userspace to
>>>>>     	 * restore cache coherency. It has to be checked here
>>>>> because it is only
>>>>>     	 * valid for cases where we are using iommu groups.
>>>>>     	 */
>>>>> -	if (type == VFIO_IOMMU && !vfio_device_is_noiommu(device)
>>>>> &&
>>>>> +	if (type == VFIO_IOMMU &&
>>>>> !(vfio_device_is_noiommu(device) ||
>>>>> +
>>>>> vfio_device_is_cdev_noiommu(device)) &&
>>>>
>>>> now, the group path and cdev path have their own is_noiommu helper,
>>>> can the two helpers be consolidated?
>>>>   
>>> They could be consolidated mechanically, but I feel they are
>>> checking different things it is more clear to keep them separate?
>>
>> IMHO. They are actually checking if the device is noiommu. I found the
>> current usage of vfio_device_is_noiommu(). #1 is totally specific for
>> group path. #2, #3 and #4 are for the common path to identify the
>> noiommu of group. It also implies the info of the open path (group
>> path?). #7 and #8 is going to be dropped. And 9 is totally for
>> checking noiommu attribute. So I'm wondering if using !dev->iommu is
>> a good choice. Should be able to cover both group and cdev path. Let
>> me know if this is not workable.
> I'm not sure I follow -- do you mean using !dev->iommu as the common
> no-IOMMU check?
>   
> I don't think that should be the helper condition directly. !dev->iommu
> is the low-level device state, but VFIO no-IOMMU is a VFIO mode. For
> example, VFIO_EMULATED_IOMMU devices may also not have dev->iommu, but
> they should not be treated as VFIO_NO_IOMMU.

you are right. :)

> What I can do is consolidate the helpers around the VFIO state instead:
> legacy group no-IOMMU is represented by group->type, while the cdev
> path uses vdev->noiommu
> 
> static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
> {
> #if IS_ENABLED(CONFIG_VFIO_GROUP)
> 	if (vdev->group && vdev->group->type == VFIO_NO_IOMMU)
> 		return true;
> #endif
> 
> 	return IS_ENABLED(CONFIG_IOMMUFD_NOIOMMU) && vdev->noiommu;
> }

looks good. :)

Regards,
Yi Liu