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

[Nicolin Chen <nicolinc@nvidia.com>]

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?

> +	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))
?

> @@ -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