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@xxxxxxxxxx>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
Reviewed-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
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
