> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Sent: Monday, December 19, 2022 4:47 PM
>
> @@ -415,7 +416,7 @@ static int vfio_device_first_open(struct
> vfio_device_file *df,
> if (!try_module_get(device->dev->driver->owner))
> return -ENODEV;
>
> - if (iommufd)
> + if (iommufd && !IS_ERR(iommufd))
> ret = vfio_iommufd_bind(device, iommufd, dev_id, pt_id);
> else
> ret = vfio_device_group_use_iommu(device);
can you elaborate how noiommu actually works in the cdev path?
I'm a bit lost here.
> @@ -592,6 +600,8 @@ static int vfio_device_fops_release(struct inode
> *inode, struct file *filep)
> */
> if (!df->single_open)
> vfio_device_group_close(df);
> + else
> + vfio_device_close(df);
> kfree(df);
> vfio_device_put_registration(device);
belong to last patch?
> + mutex_lock(&device->dev_set->lock);
> + /* Paired with smp_store_release() in vfio_device_open/close() */
> + access = smp_load_acquire(&df->access_granted);
> + if (access) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
Not sure it's required. The lock is already held then just checking
df->iommufd should be sufficient.
> + mutex_lock(&device->dev_set->lock);
> + pt_id = attach.pt_id;
> + ret = vfio_iommufd_attach(device,
> + pt_id != IOMMUFD_INVALID_ID ? &pt_id :
> NULL);
> + if (ret)
> + goto out_unlock;
> +
> + if (pt_id != IOMMUFD_INVALID_ID) {
it's clearer to use an 'attach' local variable
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index 98ebba80cfa1..87680274c01b 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -9,6 +9,8 @@
>
> #define IOMMUFD_TYPE (';')
>
> +#define IOMMUFD_INVALID_ID 0 /* valid ID starts from 1 */
Can you point out where valid IDs are guaranteed to start
from 1?
According to _iommufd_object_alloc() it uses xa_limit_32b as
limit which includes '0' as the lowest ID
> +/*
> + * VFIO_DEVICE_BIND_IOMMUFD - _IOR(VFIO_TYPE, VFIO_BASE + 19,
> + * struct vfio_device_bind_iommufd)
> + *
> + * Bind a vfio_device to the specified iommufd and an ioas or a hardware
> + * page table.
this is stale. BIND now is only about bind. No ioas.
> + * VFIO_DEVICE_ATTACH_IOMMUFD_PT - _IOW(VFIO_TYPE, VFIO_BASE +
> 20,
> + * struct
> vfio_device_attach_iommufd_pt)
> + *
> + * Attach a vfio device to an iommufd address space specified by IOAS
> + * id or hardware page table id.
> + *
> + * Available only after a device has been bound to iommufd via
> + * VFIO_DEVICE_BIND_IOMMUFD
> + *
> + * Undo by passing pt_id == IOMMUFD_INVALID_ID
> + *
> + * @argsz: user filled size of this data.
> + * @flags: must be 0.
> + * @pt_id: Input the target id, can be an ioas or a hwpt allocated
> + * via iommufd subsystem, and output the attached pt_id. It
> + * be the ioas, hwpt itself or an hwpt created by kernel
> + * during the attachment.
Input the target id which can represent an ioas or a hwpt allocated
via iommufd subsystem. Output 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.
