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

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

On 2025/3/20 01:41, Nicolin Chen wrote:
> On Thu, Mar 13, 2025 at 05:47:51AM -0700, Yi Liu 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.
>>
>> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> With some nits below:
> 
>>   drivers/vfio/device_cdev.c | 60 +++++++++++++++++++++++++++++++++-----
>>   include/uapi/linux/vfio.h  | 29 +++++++++++-------
>>   2 files changed, 71 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
>> index bb1817bd4ff3..6d436bee8207 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;
> 
> It seems that the movement of this device line isn't necessary?

hmmm. it's just for reverse Christmas tree. no functional reason.
> 
>> +	if (attach.flags & (~VFIO_DEVICE_ATTACH_PASID))
> 
> Any reason for the parentheses? Why it's outside the ~ operator?
> 
> I assume (if adding more flags) we would end up with this:
> 	if (attach.flags & ~(VFIO_DEVICE_ATTACH_PASID | MORE_FLAGS))

yes. need to drop it.

> 
>> @@ -198,20 +221,41 @@ int vfio_df_ioctl_attach_pt(struct vfio_device_file *df,
>>   int vfio_df_ioctl_detach_pt(struct vfio_device_file *df,
>>   			    struct vfio_device_detach_iommufd_pt __user *arg)
>>   {
>> -	struct vfio_device *device = df->device;
>>   	struct vfio_device_detach_iommufd_pt detach;
>> -	unsigned long minsz;
>> +	struct vfio_device *device = df->device;
> 
> Ditto.
> 
>> +	if (detach.flags & (~VFIO_DEVICE_DETACH_PASID))
>> +		return -EINVAL;
> 
> Ditto.
> 
> Thanks
> Nicolin

-- 
Regards,
Yi Liu