On Tue, 15 Oct 2024 19:07:52 +0800
Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
> On 2024/10/14 23:49, Alex Williamson wrote:
> > On Sat, 12 Oct 2024 21:49:05 +0800
> > Yi Liu <yi.l.liu@xxxxxxxxx> 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;
I think this could break current users. For better or worse, we don't
currently have any requirements for the remainder of the user buffer,
whereas copy_struct_from_user() returns an error for non-zero trailing
bytes. I think we need to monotonically increase the structure size,
but maybe something more like below, using flags. The expectation
would be that if we add another flag that extends the structure, we'd
test that flag after PASID and clobber xend to a new value further into
the new structure. We'd also add that flag to the flags mask, but we'd
share the copy code.
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);
if (xend) {
if (attach.argsz < xend)
return -EINVAL;
if (copy_from_user((void *)&attach + minsz,
(void __user *)arg + minsz, xend - minsz))
return -EFAULT;
}
Maybe there are still more elegant options available.
We also generally try to label flags with FLAGS in the name, but it
does get rather unwieldy, so I'm open to suggestions. Thanks,
Alex
>
> - 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)
