Re: [PATCH v5 3/5] vfio: VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT support pasid

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

On 2024/11/12 08:02, Alex Williamson wrote:
> On Fri,  8 Nov 2024 04:17:40 -0800
> Yi Liu <yi.l.liu@intel.com> wrote:
> 
>> This extends the VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT ioctls to attach/detach
>> a given pasid of a vfio device to/from an IOAS/HWPT.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/vfio/device_cdev.c | 69 +++++++++++++++++++++++++++++++++-----
>>   include/uapi/linux/vfio.h  | 29 ++++++++++------
>>   2 files changed, 80 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
>> index bb1817bd4ff3..4519f482e212 100644
>> --- a/drivers/vfio/device_cdev.c
>> +++ b/drivers/vfio/device_cdev.c
>> @@ -162,9 +162,9 @@ void vfio_df_unbind_iommufd(struct vfio_device_file *df)
>>   int vfio_df_ioctl_attach_pt(struct vfio_device_file *df,
>>   			    struct vfio_device_attach_iommufd_pt __user *arg)
>>   {
>> -	struct vfio_device *device = df->device;
>>   	struct vfio_device_attach_iommufd_pt attach;
>> -	unsigned long minsz;
>> +	struct vfio_device *device = df->device;
>> +	unsigned long minsz, xend = 0;
>>   	int ret;
>>   
>>   	minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
>> @@ -172,11 +172,38 @@ int vfio_df_ioctl_attach_pt(struct vfio_device_file *df,
>>   	if (copy_from_user(&attach, arg, minsz))
>>   		return -EFAULT;
>>   
>> -	if (attach.argsz < minsz || attach.flags)
>> +	if (attach.argsz < minsz)
>>   		return -EINVAL;
>>   
>> +	if (attach.flags & (~VFIO_DEVICE_ATTACH_PASID))
>> +		return -EINVAL;
>> +
>> +	if (attach.flags & VFIO_DEVICE_ATTACH_PASID)
>> +		xend = offsetofend(struct vfio_device_attach_iommufd_pt, pasid);
>> +
>> +	/*
>> +	 * xend may be equal to minsz if a flag is defined for reusing a
>> +	 * reserved field or a special usage of an existing field.
>> +	 */
>> +	if (xend > minsz) {
>> +		if (attach.argsz < xend)
>> +			return -EINVAL;
>> +
>> +		if (copy_from_user((void *)&attach + minsz,
>> +				   (void __user *)arg + minsz, xend - minsz))
>> +			return -EFAULT;
>> +	}
>> +
>> +	if ((attach.flags & VFIO_DEVICE_ATTACH_PASID) &&
>> +	    !device->ops->pasid_attach_ioas)
>> +		return -EOPNOTSUPP;
>> +
>>   	mutex_lock(&device->dev_set->lock);
>> -	ret = device->ops->attach_ioas(device, &attach.pt_id);
>> +	if (attach.flags & VFIO_DEVICE_ATTACH_PASID)
> 
> I'd just do the ops test here:
> 							{
> 		if (!device->ops->pasid_attach_ios)
> 			ret = -EOPNOTSUPP;
> 		else...
> 
>> +		ret = device->ops->pasid_attach_ioas(device, attach.pasid,
>> +						     &attach.pt_id);

got it.

> 	} else {
> 
> (Obviously if we weren't about to generalize the prior chunk of code,
> we'd test ops before the 2nd copy_from_user)  Thanks,

yes. that's the trade-off for the generalization. :)

-- 
Regards,
Yi Liu