On 2023/1/9 15:47, Tian, Kevin wrote:
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?
not really. In last patch, it only adds the cdev, but no ioctls.
Only when the BIND_IOMMUFD is added, should the df->single_open
possible be true. So I added it in this patch instead of last one.
+ 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.
you are right. check df->iommufd is enough.
+ 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
not quit get. We already have 'attach' in above lines.:-)
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
xa_init_flags(&ictx->objects, XA_FLAGS_ALLOC1 | XA_FLAGS_ACCOUNT);
yes, but the xarray init uses XA_FLAGS_ALLOC1, and it means to allocate
ID from 1.
/* ALLOC is for a normal 0-based alloc. ALLOC1 is for an 1-based alloc */
+/*
+ * 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.
Oops, yes.
+ * 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.
got it.
--
Regards,
Yi Liu
