Re: [PATCH v3 3/4] vfio: Add VFIO_DEVICE_PASID_[AT|DE]TACH_IOMMUFD_PT

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

On 2024/10/14 23:49, Alex Williamson wrote:
> On Sat, 12 Oct 2024 21:49:05 +0800
> Yi Liu <yi.l.liu@intel.com> wrote:
> 
>> On 2024/10/1 20:11, Jason Gunthorpe wrote:
>>> On Mon, Sep 30, 2024 at 07:55:08AM +0000, Tian, Kevin wrote:
>>>    
>>>>> +struct vfio_device_pasid_attach_iommufd_pt {
>>>>> +	__u32	argsz;
>>>>> +	__u32	flags;
>>>>> +	__u32	pasid;
>>>>> +	__u32	pt_id;
>>>>> +};
>>>>> +
>>>>> +#define VFIO_DEVICE_PASID_ATTACH_IOMMUFD_PT	_IO(VFIO_TYPE,
>>>>> VFIO_BASE + 21)
>>>>
>>>> Not sure whether this was discussed before. Does it make sense
>>>> to reuse the existing VFIO_DEVICE_ATTACH_IOMMUFD_PT
>>>> by introducing a new pasid field and a new flag bit?
>>>
>>> Maybe? I don't have a strong feeling either way.
>>>
>>> There is somewhat less code if you reuse the ioctl at least
>>
>> I had a rough memory that I was suggested to add a separate ioctl for
>> PASID. Let's see Alex's opinion.
> 
> I don't recall any previous arguments for separate ioctls, but it seems
> to make a lot of sense to me to extend the existing ioctls with a flag
> to indicate pasid cscope and id.  Thanks,

thanks for the confirmation. How about the below?

diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
index bb1817bd4ff3..c78533bce3c6 100644
--- a/drivers/vfio/device_cdev.c
+++ b/drivers/vfio/device_cdev.c
@@ -162,21 +162,34 @@ 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;
+	unsigned long minsz = offsetofend(
+			struct vfio_device_attach_iommufd_pt, pt_id);
  	struct vfio_device_attach_iommufd_pt attach;
-	unsigned long minsz;
+	struct vfio_device *device = df->device;
+	u32 user_size;
  	int ret;

-	minsz = offsetofend(struct vfio_device_attach_iommufd_pt, pt_id);
+	ret = get_user(user_size, (u32 __user *)arg);
+	if (ret)
+		return ret;

-	if (copy_from_user(&attach, arg, minsz))
-		return -EFAULT;
+	ret = copy_struct_from_user(&attach, sizeof(attach), arg, user_size);
+	if (ret)
+		return ret;

-	if (attach.argsz < minsz || attach.flags)
+	if (attach.argsz < minsz || attach.flags & (~VFIO_DEVICE_ATTACH_PASID))
  		return -EINVAL;

+	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)
+		ret = device->ops->pasid_attach_ioas(device, attach.pasid,
+						     &attach.pt_id);
+	else
+		ret = device->ops->attach_ioas(device, &attach.pt_id);
  	if (ret)
  		goto out_unlock;

@@ -198,20 +211,33 @@ 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;
+	unsigned long minsz =
+		offsetofend(struct vfio_device_detach_iommufd_pt, flags);
  	struct vfio_device_detach_iommufd_pt detach;
-	unsigned long minsz;
+	struct vfio_device *device = df->device;
+	u32 user_size;
+	int ret;

-	minsz = offsetofend(struct vfio_device_detach_iommufd_pt, flags);
+	ret = get_user(user_size, (u32 __user *)arg);
+	if (ret)
+		return ret;

-	if (copy_from_user(&detach, arg, minsz))
-		return -EFAULT;
+	ret = copy_struct_from_user(&detach, sizeof(detach), arg, user_size);
+	if (ret)
+		return ret;

-	if (detach.argsz < minsz || detach.flags)
+	if (detach.argsz < minsz || detach.flags & (~VFIO_DEVICE_DETACH_PASID))
  		return -EINVAL;

+	if ((detach.flags & VFIO_DEVICE_DETACH_PASID) &&
+	    !device->ops->pasid_detach_ioas)
+		return -EOPNOTSUPP;
+
  	mutex_lock(&device->dev_set->lock);
-	device->ops->detach_ioas(device);
+	if (detach.flags & VFIO_DEVICE_DETACH_PASID)
+		device->ops->pasid_detach_ioas(device, detach.pasid);
+	else
+		device->ops->detach_ioas(device);
  	mutex_unlock(&device->dev_set->lock);

  	return 0;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 2b68e6cdf190..40b414e642f5 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -931,29 +931,34 @@ struct vfio_device_bind_iommufd {
   * VFIO_DEVICE_ATTACH_IOMMUFD_PT - _IOW(VFIO_TYPE, VFIO_BASE + 19,
   *					struct vfio_device_attach_iommufd_pt)
   * @argsz:	User filled size of this data.
- * @flags:	Must be 0.
+ * @flags:	Flags for attach.
   * @pt_id:	Input the target id which can represent an ioas or a hwpt
   *		allocated via iommufd subsystem.
   *		Output the input ioas id or the attached hwpt id which could
   *		be the specified hwpt itself or a hwpt automatically created
   *		for the specified ioas by kernel during the attachment.
+ * @pasid:	The pasid to be attached, only meaningful when
+ *		VFIO_DEVICE_ATTACH_PASID is set in @flags
   *
   * Associate the device with an address space within the bound iommufd.
   * Undo by VFIO_DEVICE_DETACH_IOMMUFD_PT or device fd close.  This is only
   * allowed on cdev fds.
   *
- * If a vfio device is currently attached to a valid hw_pagetable, without 
doing
- * a VFIO_DEVICE_DETACH_IOMMUFD_PT, a second VFIO_DEVICE_ATTACH_IOMMUFD_PT 
ioctl
- * passing in another hw_pagetable (hwpt) id is allowed. This action, also 
known
- * as a hw_pagetable replacement, will replace the device's currently attached
- * hw_pagetable with a new hw_pagetable corresponding to the given pt_id.
+ * If a vfio device or a pasid of this device is currently attached to a valid
+ * hw_pagetable (hwpt), without doing a VFIO_DEVICE_DETACH_IOMMUFD_PT, a 
second
+ * VFIO_DEVICE_ATTACH_IOMMUFD_PT ioctl passing in another hwpt id is allowed.
+ * This action, also known as a hw_pagetable replacement, will replace the
+ * currently attached hwpt of the device or the pasid of this device with 
a new
+ * hwpt corresponding to the given pt_id.
   *
   * Return: 0 on success, -errno on failure.
   */
  struct vfio_device_attach_iommufd_pt {
  	__u32	argsz;
  	__u32	flags;
+#define VFIO_DEVICE_ATTACH_PASID	(1 << 0)
  	__u32	pt_id;
+	__u32	pasid;
  };

  #define VFIO_DEVICE_ATTACH_IOMMUFD_PT		_IO(VFIO_TYPE, VFIO_BASE + 19)
@@ -962,17 +967,21 @@ struct vfio_device_attach_iommufd_pt {
   * VFIO_DEVICE_DETACH_IOMMUFD_PT - _IOW(VFIO_TYPE, VFIO_BASE + 20,
   *					struct vfio_device_detach_iommufd_pt)
   * @argsz:	User filled size of this data.
- * @flags:	Must be 0.
+ * @flags:	Flags for detach.
+ * @pasid:	The pasid to be detached, only meaningful when
+ *		VFIO_DEVICE_DETACH_PASID is set in @flags
   *
- * Remove the association of the device and its current associated address
- * space.  After it, the device should be in a blocking DMA state.  This 
is only
- * allowed on cdev fds.
+ * Remove the association of the device or a pasid of the device and its 
current
+ * associated address space.  After it, the device or the pasid should be in a
+ * blocking DMA state.  This is only allowed on cdev fds.
   *
   * Return: 0 on success, -errno on failure.
   */
  struct vfio_device_detach_iommufd_pt {
  	__u32	argsz;
  	__u32	flags;
+#define VFIO_DEVICE_DETACH_PASID	(1 << 0)
+	__u32	pasid;
  };

  #define VFIO_DEVICE_DETACH_IOMMUFD_PT		_IO(VFIO_TYPE, VFIO_BASE + 20)
-- 
2.34.1


-- 
Regards,
Yi Liu