[PATCH v1 0/5] vfio: Make emulated devices prepared for vfio device cdev

[PATCH v1 0/5] vfio: Make emulated devices prepared for vfio device cdev

[Yi Liu]

The .bind_iommufd op of vfio emulated devices are either empty or does
nothing. This is different with the vfio physical devices, to add vfio
device cdev, need to make them act the same.

This series first makes the .bind_iommufd op of vfio emulated devices
to create iommufd_access, this introduces a new iommufd API. Then let
the driver that does not provide .bind_iommufd op to use the vfio emulated
iommufd op set.

Thanks,
	Yi Liu

Nicolin Chen (1):
  iommufd: Create access in vfio_iommufd_emulated_bind()

Yi Liu (4):
  vfio-iommufd: No need to record iommufd_ctx in vfio_device
  vfio-iommufd: Make vfio_iommufd_emulated_bind() return iommufd_access
    ID
  Samples/mdev: Uses the vfio emulated iommufd ops set in the mdev
    sample drivers
  vfio: Check the presence for iommufd callbacks in
    __vfio_register_dev()

 drivers/iommu/iommufd/device.c          | 107 ++++++++++++++++++------
 drivers/iommu/iommufd/iommufd_private.h |   1 +
 drivers/iommu/iommufd/selftest.c        |   8 +-
 drivers/vfio/iommufd.c                  |  35 ++++----
 drivers/vfio/vfio_main.c                |   5 +-
 include/linux/iommufd.h                 |   5 +-
 include/linux/vfio.h                    |   1 -
 samples/vfio-mdev/mbochs.c              |   3 +
 samples/vfio-mdev/mdpy.c                |   3 +
 samples/vfio-mdev/mtty.c                |   3 +
 10 files changed, 119 insertions(+), 52 deletions(-)

-- 
2.34.1

[PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Yi Liu]

From: Nicolin Chen <nicolinc@nvidia.com>

There are needs to created iommufd_access prior to have an IOAS and set
IOAS later. Like the vfio device cdev needs to have an iommufd object
to represent the bond (iommufd_access) and IOAS replacement.

This moves iommufd_access_create() call into vfio_iommufd_emulated_bind(),
making it symmetric with the __vfio_iommufd_access_destroy() call in
vfio_iommufd_emulated_unbind(). This means an access is created/destroyed
by the bind()/unbind(), and the vfio_iommufd_emulated_attach_ioas() only
updates the access->ioas pointer.

Since there's no longer an ioas_id input for iommufd_access_create(), add
a new helper iommufd_access_set_ioas() to set access->ioas. We can later
add a "replace" feature simply to the new iommufd_access_set_ioas() too.

Leaving the access->ioas in vfio_iommufd_emulated_attach_ioas(), however,
can introduce some potential of a race condition during pin_/unpin_pages()
call where access->ioas->iopt is getting referenced. So, add an ioas_lock
to protect it.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/device.c          | 103 ++++++++++++++++++------
 drivers/iommu/iommufd/iommufd_private.h |   1 +
 drivers/iommu/iommufd/selftest.c        |   5 +-
 drivers/vfio/iommufd.c                  |  23 +++---
 include/linux/iommufd.h                 |   3 +-
 5 files changed, 100 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index a0c66f47a65a..71c4d38994b3 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -412,9 +412,12 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
 	struct iommufd_access *access =
 		container_of(obj, struct iommufd_access, obj);
 
-	iopt_remove_access(&access->ioas->iopt, access);
+	if (access->ioas) {
+		iopt_remove_access(&access->ioas->iopt, access);
+		refcount_dec(&access->ioas->obj.users);
+	}
 	iommufd_ctx_put(access->ictx);
-	refcount_dec(&access->ioas->obj.users);
+	mutex_destroy(&access->ioas_lock);
 }
 
 /**
@@ -431,12 +434,10 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
  * The provided ops are required to use iommufd_access_pin_pages().
  */
 struct iommufd_access *
-iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id,
+iommufd_access_create(struct iommufd_ctx *ictx,
 		      const struct iommufd_access_ops *ops, void *data)
 {
 	struct iommufd_access *access;
-	struct iommufd_object *obj;
-	int rc;
 
 	/*
 	 * There is no uAPI for the access object, but to keep things symmetric
@@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id,
 	access->data = data;
 	access->ops = ops;
 
-	obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
-	if (IS_ERR(obj)) {
-		rc = PTR_ERR(obj);
-		goto out_abort;
-	}
-	access->ioas = container_of(obj, struct iommufd_ioas, obj);
-	iommufd_ref_to_users(obj);
-
 	if (ops->needs_pin_pages)
 		access->iova_alignment = PAGE_SIZE;
 	else
 		access->iova_alignment = 1;
-	rc = iopt_add_access(&access->ioas->iopt, access);
-	if (rc)
-		goto out_put_ioas;
 
 	/* The calling driver is a user until iommufd_access_destroy() */
 	refcount_inc(&access->obj.users);
+	mutex_init(&access->ioas_lock);
 	access->ictx = ictx;
 	iommufd_ctx_get(ictx);
 	iommufd_object_finalize(ictx, &access->obj);
 	return access;
-out_put_ioas:
-	refcount_dec(&access->ioas->obj.users);
-out_abort:
-	iommufd_object_abort(ictx, &access->obj);
-	return ERR_PTR(rc);
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD);
 
@@ -494,6 +480,50 @@ void iommufd_access_destroy(struct iommufd_access *access)
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
 
+int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id)
+{
+	struct iommufd_ioas *new_ioas = NULL, *cur_ioas;
+	struct iommufd_ctx *ictx = access->ictx;
+	struct iommufd_object *obj;
+	int rc = 0;
+
+	if (ioas_id) {
+		obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
+		if (IS_ERR(obj))
+			return PTR_ERR(obj);
+		new_ioas = container_of(obj, struct iommufd_ioas, obj);
+	}
+
+	mutex_lock(&access->ioas_lock);
+	cur_ioas = access->ioas;
+	if (cur_ioas == new_ioas)
+		goto out_unlock;
+
+	if (new_ioas) {
+		rc = iopt_add_access(&new_ioas->iopt, access);
+		if (rc)
+			goto out_unlock;
+		iommufd_ref_to_users(obj);
+	}
+
+	if (cur_ioas) {
+		iopt_remove_access(&cur_ioas->iopt, access);
+		refcount_dec(&cur_ioas->obj.users);
+	}
+
+	access->ioas = new_ioas;
+	mutex_unlock(&access->ioas_lock);
+
+	return 0;
+
+out_unlock:
+	mutex_unlock(&access->ioas_lock);
+	if (new_ioas)
+		iommufd_put_object(obj);
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_access_set_ioas, IOMMUFD);
+
 /**
  * iommufd_access_notify_unmap - Notify users of an iopt to stop using it
  * @iopt: iopt to work on
@@ -544,8 +574,8 @@ void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
 void iommufd_access_unpin_pages(struct iommufd_access *access,
 				unsigned long iova, unsigned long length)
 {
-	struct io_pagetable *iopt = &access->ioas->iopt;
 	struct iopt_area_contig_iter iter;
+	struct io_pagetable *iopt;
 	unsigned long last_iova;
 	struct iopt_area *area;
 
@@ -553,6 +583,13 @@ void iommufd_access_unpin_pages(struct iommufd_access *access,
 	    WARN_ON(check_add_overflow(iova, length - 1, &last_iova)))
 		return;
 
+	mutex_lock(&access->ioas_lock);
+	if (!access->ioas) {
+		mutex_unlock(&access->ioas_lock);
+		return;
+	}
+	iopt = &access->ioas->iopt;
+
 	down_read(&iopt->iova_rwsem);
 	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova)
 		iopt_area_remove_access(
@@ -562,6 +599,7 @@ void iommufd_access_unpin_pages(struct iommufd_access *access,
 				min(last_iova, iopt_area_last_iova(area))));
 	up_read(&iopt->iova_rwsem);
 	WARN_ON(!iopt_area_contig_done(&iter));
+	mutex_unlock(&access->ioas_lock);
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_unpin_pages, IOMMUFD);
 
@@ -607,8 +645,8 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 			     unsigned long length, struct page **out_pages,
 			     unsigned int flags)
 {
-	struct io_pagetable *iopt = &access->ioas->iopt;
 	struct iopt_area_contig_iter iter;
+	struct io_pagetable *iopt;
 	unsigned long last_iova;
 	struct iopt_area *area;
 	int rc;
@@ -623,6 +661,13 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 	if (check_add_overflow(iova, length - 1, &last_iova))
 		return -EOVERFLOW;
 
+	mutex_lock(&access->ioas_lock);
+	if (!access->ioas) {
+		mutex_unlock(&access->ioas_lock);
+		return -ENOENT;
+	}
+	iopt = &access->ioas->iopt;
+
 	down_read(&iopt->iova_rwsem);
 	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
 		unsigned long last = min(last_iova, iopt_area_last_iova(area));
@@ -653,6 +698,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 	}
 
 	up_read(&iopt->iova_rwsem);
+	mutex_unlock(&access->ioas_lock);
 	return 0;
 
 err_remove:
@@ -667,6 +713,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 						  iopt_area_last_iova(area))));
 	}
 	up_read(&iopt->iova_rwsem);
+	mutex_unlock(&access->ioas_lock);
 	return rc;
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_pin_pages, IOMMUFD);
@@ -686,8 +733,8 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_pin_pages, IOMMUFD);
 int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
 		      void *data, size_t length, unsigned int flags)
 {
-	struct io_pagetable *iopt = &access->ioas->iopt;
 	struct iopt_area_contig_iter iter;
+	struct io_pagetable *iopt;
 	struct iopt_area *area;
 	unsigned long last_iova;
 	int rc;
@@ -697,6 +744,13 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
 	if (check_add_overflow(iova, length - 1, &last_iova))
 		return -EOVERFLOW;
 
+	mutex_lock(&access->ioas_lock);
+	if (!access->ioas) {
+		mutex_unlock(&access->ioas_lock);
+		return -ENOENT;
+	}
+	iopt = &access->ioas->iopt;
+
 	down_read(&iopt->iova_rwsem);
 	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
 		unsigned long last = min(last_iova, iopt_area_last_iova(area));
@@ -723,6 +777,7 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
 		rc = -ENOENT;
 err_out:
 	up_read(&iopt->iova_rwsem);
+	mutex_unlock(&access->ioas_lock);
 	return rc;
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_rw, IOMMUFD);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 9d7f71510ca1..820251b83ae4 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -263,6 +263,7 @@ struct iommufd_access {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;
 	struct iommufd_ioas *ioas;
+	struct mutex ioas_lock;
 	const struct iommufd_access_ops *ops;
 	void *data;
 	unsigned long iova_alignment;
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index cfb5fe9a5e0e..db4011bdc8a9 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -571,7 +571,7 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd,
 	}
 
 	access = iommufd_access_create(
-		ucmd->ictx, ioas_id,
+		ucmd->ictx,
 		(flags & MOCK_FLAGS_ACCESS_CREATE_NEEDS_PIN_PAGES) ?
 			&selftest_access_ops_pin :
 			&selftest_access_ops,
@@ -580,6 +580,9 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd,
 		rc = PTR_ERR(access);
 		goto out_put_fdno;
 	}
+	rc = iommufd_access_set_ioas(access, ioas_id);
+	if (rc)
+		goto out_destroy;
 	cmd->create_access.out_access_fd = fdno;
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 	if (rc)
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index db4efbd56042..1f87294c1931 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -138,10 +138,18 @@ static const struct iommufd_access_ops vfio_user_ops = {
 int vfio_iommufd_emulated_bind(struct vfio_device *vdev,
 			       struct iommufd_ctx *ictx, u32 *out_device_id)
 {
+	struct iommufd_access *user;
+
 	lockdep_assert_held(&vdev->dev_set->lock);
 
-	vdev->iommufd_ictx = ictx;
 	iommufd_ctx_get(ictx);
+	user = iommufd_access_create(ictx, &vfio_user_ops, vdev);
+	if (IS_ERR(user)) {
+		iommufd_ctx_put(ictx);
+		return PTR_ERR(user);
+	}
+	vdev->iommufd_access = user;
+	vdev->iommufd_ictx = ictx;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_bind);
@@ -161,15 +169,12 @@ EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_unbind);
 
 int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id)
 {
-	struct iommufd_access *user;
-
 	lockdep_assert_held(&vdev->dev_set->lock);
 
-	user = iommufd_access_create(vdev->iommufd_ictx, *pt_id, &vfio_user_ops,
-				     vdev);
-	if (IS_ERR(user))
-		return PTR_ERR(user);
-	vdev->iommufd_access = user;
-	return 0;
+	if (!vdev->iommufd_ictx)
+		return -EINVAL;
+	if (!vdev->iommufd_access)
+		return -ENOENT;
+	return iommufd_access_set_ioas(vdev->iommufd_access, *pt_id);
 }
 EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_attach_ioas);
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index c0b5b3ac34f1..247b11609c79 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -40,9 +40,10 @@ enum {
 };
 
 struct iommufd_access *
-iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id,
+iommufd_access_create(struct iommufd_ctx *ictx,
 		      const struct iommufd_access_ops *ops, void *data);
 void iommufd_access_destroy(struct iommufd_access *access);
+int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id);
 
 void iommufd_ctx_get(struct iommufd_ctx *ictx);
 
-- 
2.34.1

[PATCH v1 2/5] vfio-iommufd: No need to record iommufd_ctx in vfio_device

[Yi Liu]

iommufd_ctx is stored in vfio_device for emulated devices per bind_iommufd.
However, as iommufd_access is created in bind, no more need to stored it
since iommufd_access implicitly stores it.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/vfio/iommufd.c | 10 +---------
 include/linux/vfio.h   |  1 -
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index 1f87294c1931..b55f94271cc7 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -142,14 +142,10 @@ int vfio_iommufd_emulated_bind(struct vfio_device *vdev,
 
 	lockdep_assert_held(&vdev->dev_set->lock);
 
-	iommufd_ctx_get(ictx);
 	user = iommufd_access_create(ictx, &vfio_user_ops, vdev);
-	if (IS_ERR(user)) {
-		iommufd_ctx_put(ictx);
+	if (IS_ERR(user))
 		return PTR_ERR(user);
-	}
 	vdev->iommufd_access = user;
-	vdev->iommufd_ictx = ictx;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_bind);
@@ -162,8 +158,6 @@ void vfio_iommufd_emulated_unbind(struct vfio_device *vdev)
 		iommufd_access_destroy(vdev->iommufd_access);
 		vdev->iommufd_access = NULL;
 	}
-	iommufd_ctx_put(vdev->iommufd_ictx);
-	vdev->iommufd_ictx = NULL;
 }
 EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_unbind);
 
@@ -171,8 +165,6 @@ int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id)
 {
 	lockdep_assert_held(&vdev->dev_set->lock);
 
-	if (!vdev->iommufd_ictx)
-		return -EINVAL;
 	if (!vdev->iommufd_access)
 		return -ENOENT;
 	return iommufd_access_set_ioas(vdev->iommufd_access, *pt_id);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 93134b023968..3188d8a374bd 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -60,7 +60,6 @@ struct vfio_device {
 	void (*put_kvm)(struct kvm *kvm);
 #if IS_ENABLED(CONFIG_IOMMUFD)
 	struct iommufd_device *iommufd_device;
-	struct iommufd_ctx *iommufd_ictx;
 	bool iommufd_attached;
 #endif
 };
-- 
2.34.1

[PATCH v1 3/5] vfio-iommufd: Make vfio_iommufd_emulated_bind() return iommufd_access ID

[Yi Liu]

vfio device cdev needs to return iommufd_access ID to userspace if
bind_iommufd succeeds.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/device.c   | 4 +++-
 drivers/iommu/iommufd/selftest.c | 3 ++-
 drivers/vfio/iommufd.c           | 2 +-
 include/linux/iommufd.h          | 2 +-
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 71c4d38994b3..9087cd8ed3ea 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -426,6 +426,7 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
  * @ioas_id: ID for a IOMMUFD_OBJ_IOAS
  * @ops: Driver's ops to associate with the access
  * @data: Opaque data to pass into ops functions
+ * @id: Output ID number to return to userspace for this access
  *
  * An iommufd_access allows a driver to read/write to the IOAS without using
  * DMA. The underlying CPU memory can be accessed using the
@@ -435,7 +436,7 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
  */
 struct iommufd_access *
 iommufd_access_create(struct iommufd_ctx *ictx,
-		      const struct iommufd_access_ops *ops, void *data)
+		      const struct iommufd_access_ops *ops, void *data, u32 *id)
 {
 	struct iommufd_access *access;
 
@@ -461,6 +462,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
 	access->ictx = ictx;
 	iommufd_ctx_get(ictx);
 	iommufd_object_finalize(ictx, &access->obj);
+	*id = access->obj.id;
 	return access;
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD);
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index db4011bdc8a9..b796fd13a417 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -554,6 +554,7 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd,
 	struct iommu_test_cmd *cmd = ucmd->cmd;
 	struct selftest_access *staccess;
 	struct iommufd_access *access;
+	u32 id;
 	int fdno;
 	int rc;
 
@@ -575,7 +576,7 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd,
 		(flags & MOCK_FLAGS_ACCESS_CREATE_NEEDS_PIN_PAGES) ?
 			&selftest_access_ops_pin :
 			&selftest_access_ops,
-		staccess);
+		staccess, &id);
 	if (IS_ERR(access)) {
 		rc = PTR_ERR(access);
 		goto out_put_fdno;
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index b55f94271cc7..7a4458a10656 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -142,7 +142,7 @@ int vfio_iommufd_emulated_bind(struct vfio_device *vdev,
 
 	lockdep_assert_held(&vdev->dev_set->lock);
 
-	user = iommufd_access_create(ictx, &vfio_user_ops, vdev);
+	user = iommufd_access_create(ictx, &vfio_user_ops, vdev, out_device_id);
 	if (IS_ERR(user))
 		return PTR_ERR(user);
 	vdev->iommufd_access = user;
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 247b11609c79..365f11e8e615 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -41,7 +41,7 @@ enum {
 
 struct iommufd_access *
 iommufd_access_create(struct iommufd_ctx *ictx,
-		      const struct iommufd_access_ops *ops, void *data);
+		      const struct iommufd_access_ops *ops, void *data, u32 *id);
 void iommufd_access_destroy(struct iommufd_access *access);
 int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id);
 
-- 
2.34.1

[PATCH v1 4/5] Samples/mdev: Uses the vfio emulated iommufd ops set in the mdev sample drivers

[Yi Liu]

This harmonizes the no-DMA devices (the vfio-mdev sample drivers) with
the emulated devices (gvt-g, vfio-ap etc.). It makes it easier to add
BIND_IOMMUFD user interface which requires to return an iommufd ID to
represent the device/iommufd bond.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/vfio/iommufd.c     | 14 ++++++--------
 samples/vfio-mdev/mbochs.c |  3 +++
 samples/vfio-mdev/mdpy.c   |  3 +++
 samples/vfio-mdev/mtty.c   |  3 +++
 4 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index 7a4458a10656..768d353cb6fa 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -32,12 +32,8 @@ int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
 		return 0;
 	}
 
-	/*
-	 * If the driver doesn't provide this op then it means the device does
-	 * not do DMA at all. So nothing to do.
-	 */
-	if (!vdev->ops->bind_iommufd)
-		return 0;
+	if (WARN_ON(!vdev->ops->bind_iommufd))
+		return -ENODEV;
 
 	ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id);
 	if (ret)
@@ -119,7 +115,8 @@ EXPORT_SYMBOL_GPL(vfio_iommufd_physical_attach_ioas);
 /*
  * The emulated standard ops mean that vfio_device is going to use the
  * "mdev path" and will call vfio_pin_pages()/vfio_dma_rw(). Drivers using this
- * ops set should call vfio_register_emulated_iommu_dev().
+ * ops set should call vfio_register_emulated_iommu_dev(). Drivers that do
+ * not call vfio_pin_pages()/vfio_dma_rw() no need to provide dma_unmap.
  */
 
 static void vfio_emulated_unmap(void *data, unsigned long iova,
@@ -127,7 +124,8 @@ static void vfio_emulated_unmap(void *data, unsigned long iova,
 {
 	struct vfio_device *vdev = data;
 
-	vdev->ops->dma_unmap(vdev, iova, length);
+	if (vdev->ops->dma_unmap)
+		vdev->ops->dma_unmap(vdev, iova, length);
 }
 
 static const struct iommufd_access_ops vfio_user_ops = {
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index e54eb752e1ba..19391dda5fba 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -1374,6 +1374,9 @@ static const struct vfio_device_ops mbochs_dev_ops = {
 	.write = mbochs_write,
 	.ioctl = mbochs_ioctl,
 	.mmap = mbochs_mmap,
+	.bind_iommufd	= vfio_iommufd_emulated_bind,
+	.unbind_iommufd	= vfio_iommufd_emulated_unbind,
+	.attach_ioas	= vfio_iommufd_emulated_attach_ioas,
 };
 
 static struct mdev_driver mbochs_driver = {
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index e8400fdab71d..5f48aef36995 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -663,6 +663,9 @@ static const struct vfio_device_ops mdpy_dev_ops = {
 	.write = mdpy_write,
 	.ioctl = mdpy_ioctl,
 	.mmap = mdpy_mmap,
+	.bind_iommufd	= vfio_iommufd_emulated_bind,
+	.unbind_iommufd	= vfio_iommufd_emulated_unbind,
+	.attach_ioas	= vfio_iommufd_emulated_attach_ioas,
 };
 
 static struct mdev_driver mdpy_driver = {
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index e887de672c52..35460901b9f7 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -1269,6 +1269,9 @@ static const struct vfio_device_ops mtty_dev_ops = {
 	.read = mtty_read,
 	.write = mtty_write,
 	.ioctl = mtty_ioctl,
+	.bind_iommufd	= vfio_iommufd_emulated_bind,
+	.unbind_iommufd	= vfio_iommufd_emulated_unbind,
+	.attach_ioas	= vfio_iommufd_emulated_attach_ioas,
 };
 
 static struct mdev_driver mtty_driver = {
-- 
2.34.1

[PATCH v1 5/5] vfio: Check the presence for iommufd callbacks in __vfio_register_dev()

[Yi Liu]

After making the no-DMA drivers (samples/vfio-mdev) providing iommufd
callbacks, __vfio_register_dev() should check the presence of the iommufd
callbacks if CONFIG_IOMMUFD is enabled.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/vfio/vfio_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 43bd6b76e2b6..89497c933490 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -255,8 +255,9 @@ static int __vfio_register_dev(struct vfio_device *device,
 {
 	int ret;
 
-	if (WARN_ON(device->ops->bind_iommufd &&
-		    (!device->ops->unbind_iommufd ||
+	if (WARN_ON(IS_ENABLED(CONFIG_IOMMUFD) &&
+		    (!device->ops->bind_iommufd ||
+		     !device->ops->unbind_iommufd ||
 		     !device->ops->attach_ioas)))
 		return -EINVAL;
 
-- 
2.34.1

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, March 8, 2023 9:14 PM
>
> @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> u32 ioas_id,
>  	access->data = data;
>  	access->ops = ops;
> 
> -	obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> -	if (IS_ERR(obj)) {
> -		rc = PTR_ERR(obj);
> -		goto out_abort;
> -	}
> -	access->ioas = container_of(obj, struct iommufd_ioas, obj);
> -	iommufd_ref_to_users(obj);
> -
>  	if (ops->needs_pin_pages)
>  		access->iova_alignment = PAGE_SIZE;
>  	else
>  		access->iova_alignment = 1;
> -	rc = iopt_add_access(&access->ioas->iopt, access);
> -	if (rc)
> -		goto out_put_ioas;
> 
>  	/* The calling driver is a user until iommufd_access_destroy() */
>  	refcount_inc(&access->obj.users);
> +	mutex_init(&access->ioas_lock);
>  	access->ictx = ictx;
>  	iommufd_ctx_get(ictx);

this refcnt get should be moved to the start given next patch
removes the reference in the caller side.

RE: [PATCH v1 3/5] vfio-iommufd: Make vfio_iommufd_emulated_bind() return iommufd_access ID

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, March 8, 2023 9:14 PM
> 
> vfio device cdev needs to return iommufd_access ID to userspace if
> bind_iommufd succeeds.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v1 4/5] Samples/mdev: Uses the vfio emulated iommufd ops set in the mdev sample drivers

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, March 8, 2023 9:14 PM
> 
> @@ -32,12 +32,8 @@ int vfio_iommufd_bind(struct vfio_device *vdev,
> struct iommufd_ctx *ictx)
>  		return 0;
>  	}
> 
> -	/*
> -	 * If the driver doesn't provide this op then it means the device does
> -	 * not do DMA at all. So nothing to do.
> -	 */
> -	if (!vdev->ops->bind_iommufd)
> -		return 0;
> +	if (WARN_ON(!vdev->ops->bind_iommufd))
> +		return -ENODEV;
> 

this is not required given next patch adds the WARN_ON() at device
registration.

let's just remove the check (together in next patch).

RE: [PATCH v1 5/5] vfio: Check the presence for iommufd callbacks in __vfio_register_dev()

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, March 8, 2023 9:14 PM
> 
> After making the no-DMA drivers (samples/vfio-mdev) providing iommufd
> callbacks, __vfio_register_dev() should check the presence of the iommufd
> callbacks if CONFIG_IOMMUFD is enabled.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/vfio/vfio_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 43bd6b76e2b6..89497c933490 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -255,8 +255,9 @@ static int __vfio_register_dev(struct vfio_device
> *device,
>  {
>  	int ret;
> 
> -	if (WARN_ON(device->ops->bind_iommufd &&
> -		    (!device->ops->unbind_iommufd ||
> +	if (WARN_ON(IS_ENABLED(CONFIG_IOMMUFD) &&
> +		    (!device->ops->bind_iommufd ||
> +		     !device->ops->unbind_iommufd ||
>  		     !device->ops->attach_ioas)))
>  		return -EINVAL;
> 

I don't think IS_ENABLED() check is necessary. those ops are
defined in the driver side w/o a conditional CONFIG_IOMMUFD.

We should warn out lacking of those ops to the driver developer
as early as possible, instead of postponing it until someone
starts to build kernel with CONFIG_IOMMUFD.

Re: [PATCH v1 5/5] vfio: Check the presence for iommufd callbacks in __vfio_register_dev()

[Jason Gunthorpe]

On Fri, Mar 10, 2023 at 02:15:32AM +0000, Tian, Kevin wrote:
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Wednesday, March 8, 2023 9:14 PM
> > 
> > After making the no-DMA drivers (samples/vfio-mdev) providing iommufd
> > callbacks, __vfio_register_dev() should check the presence of the iommufd
> > callbacks if CONFIG_IOMMUFD is enabled.
> > 
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >  drivers/vfio/vfio_main.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > index 43bd6b76e2b6..89497c933490 100644
> > --- a/drivers/vfio/vfio_main.c
> > +++ b/drivers/vfio/vfio_main.c
> > @@ -255,8 +255,9 @@ static int __vfio_register_dev(struct vfio_device
> > *device,
> >  {
> >  	int ret;
> > 
> > -	if (WARN_ON(device->ops->bind_iommufd &&
> > -		    (!device->ops->unbind_iommufd ||
> > +	if (WARN_ON(IS_ENABLED(CONFIG_IOMMUFD) &&
> > +		    (!device->ops->bind_iommufd ||
> > +		     !device->ops->unbind_iommufd ||
> >  		     !device->ops->attach_ioas)))
> >  		return -EINVAL;
> > 
> 
> I don't think IS_ENABLED() check is necessary. those ops are
> defined in the driver side w/o a conditional CONFIG_IOMMUFD.
> 
> We should warn out lacking of those ops to the driver developer
> as early as possible, instead of postponing it until someone
> starts to build kernel with CONFIG_IOMMUFD.

The ops are NULL if !CONFIG_IOMMUFD. The previous code was OK because
it checked for non-null bind before demanding the others be non-null.

Jason

RE: [PATCH v1 5/5] vfio: Check the presence for iommufd callbacks in __vfio_register_dev()

[Liu, Yi L]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, March 10, 2023 10:04 PM
> 
> On Fri, Mar 10, 2023 at 02:15:32AM +0000, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Wednesday, March 8, 2023 9:14 PM
> > >
> > > After making the no-DMA drivers (samples/vfio-mdev) providing
> iommufd
> > > callbacks, __vfio_register_dev() should check the presence of the
> iommufd
> > > callbacks if CONFIG_IOMMUFD is enabled.
> > >
> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > ---
> > >  drivers/vfio/vfio_main.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > > index 43bd6b76e2b6..89497c933490 100644
> > > --- a/drivers/vfio/vfio_main.c
> > > +++ b/drivers/vfio/vfio_main.c
> > > @@ -255,8 +255,9 @@ static int __vfio_register_dev(struct vfio_device
> > > *device,
> > >  {
> > >  	int ret;
> > >
> > > -	if (WARN_ON(device->ops->bind_iommufd &&
> > > -		    (!device->ops->unbind_iommufd ||
> > > +	if (WARN_ON(IS_ENABLED(CONFIG_IOMMUFD) &&
> > > +		    (!device->ops->bind_iommufd ||
> > > +		     !device->ops->unbind_iommufd ||
> > >  		     !device->ops->attach_ioas)))
> > >  		return -EINVAL;
> > >
> >
> > I don't think IS_ENABLED() check is necessary. those ops are
> > defined in the driver side w/o a conditional CONFIG_IOMMUFD.
> >
> > We should warn out lacking of those ops to the driver developer
> > as early as possible, instead of postponing it until someone
> > starts to build kernel with CONFIG_IOMMUFD.
> 
> The ops are NULL if !CONFIG_IOMMUFD. The previous code was OK
> because
> it checked for non-null bind before demanding the others be non-null.

Now we want the no-dma drivers to reuse the emulated iommu op set,
so if CONFIG_IOMMUFD==y, bind_iommufd/unbind_iommufd/attach_ioas
should be non-null for all the drivers registered to vfio. Is it?

Regards,
Yi Liu

Re: [PATCH v1 5/5] vfio: Check the presence for iommufd callbacks in __vfio_register_dev()

[Jason Gunthorpe]

On Fri, Mar 10, 2023 at 02:12:21PM +0000, Liu, Yi L wrote:

> > The ops are NULL if !CONFIG_IOMMUFD. The previous code was OK
> > because
> > it checked for non-null bind before demanding the others be non-null.
> 
> Now we want the no-dma drivers to reuse the emulated iommu op set,
> so if CONFIG_IOMMUFD==y, bind_iommufd/unbind_iommufd/attach_ioas
> should be non-null for all the drivers registered to vfio. Is it?

Yes

Jason

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Jason Gunthorpe]

On Wed, Mar 08, 2023 at 05:13:36AM -0800, Yi Liu wrote:

> +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id)
> +{
> +	struct iommufd_ioas *new_ioas = NULL, *cur_ioas;
> +	struct iommufd_ctx *ictx = access->ictx;
> +	struct iommufd_object *obj;
> +	int rc = 0;
> +
> +	if (ioas_id) {
> +		obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> +		if (IS_ERR(obj))
> +			return PTR_ERR(obj);
> +		new_ioas = container_of(obj, struct iommufd_ioas, obj);
> +	}
> +
> +	mutex_lock(&access->ioas_lock);
> +	cur_ioas = access->ioas;
> +	if (cur_ioas == new_ioas)
> +		goto out_unlock;
> +
> +	if (new_ioas) {
> +		rc = iopt_add_access(&new_ioas->iopt, access);
> +		if (rc)
> +			goto out_unlock;
> +		iommufd_ref_to_users(obj);
> +	}
> +
> +	if (cur_ioas) {
> +		iopt_remove_access(&cur_ioas->iopt, access);
> +		refcount_dec(&cur_ioas->obj.users);
> +	}

This should match the physical side with an add/remove/replace
API. Especially since remove is implicit in destroy this series only
needs the add API

And the locking shouldn't come in another patch that brings the
replace/remove since with just split add we don't need it.

That will make this patch alot smaller

Jason

Re: [PATCH v1 2/5] vfio-iommufd: No need to record iommufd_ctx in vfio_device

[Jason Gunthorpe]

On Wed, Mar 08, 2023 at 05:13:37AM -0800, Yi Liu wrote:
> iommufd_ctx is stored in vfio_device for emulated devices per bind_iommufd.
> However, as iommufd_access is created in bind, no more need to stored it
> since iommufd_access implicitly stores it.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/vfio/iommufd.c | 10 +---------
>  include/linux/vfio.h   |  1 -
>  2 files changed, 1 insertion(+), 10 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v1 3/5] vfio-iommufd: Make vfio_iommufd_emulated_bind() return iommufd_access ID

[Jason Gunthorpe]

On Wed, Mar 08, 2023 at 05:13:38AM -0800, Yi Liu wrote:
> vfio device cdev needs to return iommufd_access ID to userspace if
> bind_iommufd succeeds.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommufd/device.c   | 4 +++-
>  drivers/iommu/iommufd/selftest.c | 3 ++-
>  drivers/vfio/iommufd.c           | 2 +-
>  include/linux/iommufd.h          | 2 +-
>  4 files changed, 7 insertions(+), 4 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v1 4/5] Samples/mdev: Uses the vfio emulated iommufd ops set in the mdev sample drivers

[Jason Gunthorpe]

On Wed, Mar 08, 2023 at 05:13:39AM -0800, Yi Liu wrote:
> This harmonizes the no-DMA devices (the vfio-mdev sample drivers) with
> the emulated devices (gvt-g, vfio-ap etc.). It makes it easier to add
> BIND_IOMMUFD user interface which requires to return an iommufd ID to
> represent the device/iommufd bond.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/vfio/iommufd.c     | 14 ++++++--------
>  samples/vfio-mdev/mbochs.c |  3 +++
>  samples/vfio-mdev/mdpy.c   |  3 +++
>  samples/vfio-mdev/mtty.c   |  3 +++
>  4 files changed, 15 insertions(+), 8 deletions(-)

Subject should be 'vfio/mdev: ..'

> @@ -119,7 +115,8 @@ EXPORT_SYMBOL_GPL(vfio_iommufd_physical_attach_ioas);
>  /*
>   * The emulated standard ops mean that vfio_device is going to use the
>   * "mdev path" and will call vfio_pin_pages()/vfio_dma_rw(). Drivers using this
> - * ops set should call vfio_register_emulated_iommu_dev().
> + * ops set should call vfio_register_emulated_iommu_dev(). Drivers that do
> + * not call vfio_pin_pages()/vfio_dma_rw() no need to provide dma_unmap.
>   */

'have no need'

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v1 5/5] vfio: Check the presence for iommufd callbacks in __vfio_register_dev()

[Jason Gunthorpe]

On Wed, Mar 08, 2023 at 05:13:40AM -0800, Yi Liu wrote:
> After making the no-DMA drivers (samples/vfio-mdev) providing iommufd
> callbacks, __vfio_register_dev() should check the presence of the iommufd
> callbacks if CONFIG_IOMMUFD is enabled.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/vfio/vfio_main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Alex, when you take this can it please go on a branch that will also
have the cdev series?

Thanks,
Jason

RE: [PATCH v1 5/5] vfio: Check the presence for iommufd callbacks in __vfio_register_dev()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, March 10, 2023 10:04 PM
> 
> On Fri, Mar 10, 2023 at 02:15:32AM +0000, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Wednesday, March 8, 2023 9:14 PM
> > >
> > > After making the no-DMA drivers (samples/vfio-mdev) providing
> iommufd
> > > callbacks, __vfio_register_dev() should check the presence of the
> iommufd
> > > callbacks if CONFIG_IOMMUFD is enabled.
> > >
> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > ---
> > >  drivers/vfio/vfio_main.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > > index 43bd6b76e2b6..89497c933490 100644
> > > --- a/drivers/vfio/vfio_main.c
> > > +++ b/drivers/vfio/vfio_main.c
> > > @@ -255,8 +255,9 @@ static int __vfio_register_dev(struct vfio_device
> > > *device,
> > >  {
> > >  	int ret;
> > >
> > > -	if (WARN_ON(device->ops->bind_iommufd &&
> > > -		    (!device->ops->unbind_iommufd ||
> > > +	if (WARN_ON(IS_ENABLED(CONFIG_IOMMUFD) &&
> > > +		    (!device->ops->bind_iommufd ||
> > > +		     !device->ops->unbind_iommufd ||
> > >  		     !device->ops->attach_ioas)))
> > >  		return -EINVAL;
> > >
> >
> > I don't think IS_ENABLED() check is necessary. those ops are
> > defined in the driver side w/o a conditional CONFIG_IOMMUFD.
> >
> > We should warn out lacking of those ops to the driver developer
> > as early as possible, instead of postponing it until someone
> > starts to build kernel with CONFIG_IOMMUFD.
> 
> The ops are NULL if !CONFIG_IOMMUFD. The previous code was OK because
> it checked for non-null bind before demanding the others be non-null.
> 

ah I didn't note they are NULL defined.

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

On Fri, Mar 10, 2023 at 01:36:22PM -0400, Jason Gunthorpe wrote:
> On Wed, Mar 08, 2023 at 05:13:36AM -0800, Yi Liu wrote:
> 
> > +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id)
> > +{
> > +	struct iommufd_ioas *new_ioas = NULL, *cur_ioas;
> > +	struct iommufd_ctx *ictx = access->ictx;
> > +	struct iommufd_object *obj;
> > +	int rc = 0;
> > +
> > +	if (ioas_id) {
> > +		obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > +		if (IS_ERR(obj))
> > +			return PTR_ERR(obj);
> > +		new_ioas = container_of(obj, struct iommufd_ioas, obj);
> > +	}
> > +
> > +	mutex_lock(&access->ioas_lock);
> > +	cur_ioas = access->ioas;
> > +	if (cur_ioas == new_ioas)
> > +		goto out_unlock;
> > +
> > +	if (new_ioas) {
> > +		rc = iopt_add_access(&new_ioas->iopt, access);
> > +		if (rc)
> > +			goto out_unlock;
> > +		iommufd_ref_to_users(obj);
> > +	}
> > +
> > +	if (cur_ioas) {
> > +		iopt_remove_access(&cur_ioas->iopt, access);
> > +		refcount_dec(&cur_ioas->obj.users);
> > +	}
> 
> This should match the physical side with an add/remove/replace
> API. Especially since remove is implicit in destroy this series only
> needs the add API

I assume that the API would be iommufd_access_attach,
iommufd_access_detach, and iommufd_access_replace(). And there
might be an iommufd_access_change_pt(access, pt, bool replace)?

> And the locking shouldn't come in another patch that brings the
> replace/remove since with just split add we don't need it.

Hmm. The iommufd_access_detach would be needed in the following
cdev series, while the iommufd_access_replace would be need in
my replace series. So, that would make the API be divided into
three series.

Perhaps we can have iommufd_access_attach/detach in this series
along with a vfio_iommufd_emulated_detach_ioas, and the locking
will come with another patch in replace series?

Thanks
Nic

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Wednesday, March 8, 2023 9:14 PM
> >
> > @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> > u32 ioas_id,
> >       access->data = data;
> >       access->ops = ops;
> >
> > -     obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > -     if (IS_ERR(obj)) {
> > -             rc = PTR_ERR(obj);
> > -             goto out_abort;
> > -     }
> > -     access->ioas = container_of(obj, struct iommufd_ioas, obj);
> > -     iommufd_ref_to_users(obj);
> > -
> >       if (ops->needs_pin_pages)
> >               access->iova_alignment = PAGE_SIZE;
> >       else
> >               access->iova_alignment = 1;
> > -     rc = iopt_add_access(&access->ioas->iopt, access);
> > -     if (rc)
> > -             goto out_put_ioas;
> >
> >       /* The calling driver is a user until iommufd_access_destroy() */
> >       refcount_inc(&access->obj.users);
> > +     mutex_init(&access->ioas_lock);
> >       access->ictx = ictx;
> >       iommufd_ctx_get(ictx);
> 
> this refcnt get should be moved to the start given next patch
> removes the reference in the caller side.

I don't feel quite convincing to justify for moving it in this
patch. Perhaps we should do that in the following patch, where
it needs this? Or another individual patch moving this alone?

Thanks
Nic

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

Hi Jason/Kevin,

On Tue, Mar 14, 2023 at 01:20:52AM -0700, Nicolin Chen wrote:
> On Fri, Mar 10, 2023 at 01:36:22PM -0400, Jason Gunthorpe wrote:
> > On Wed, Mar 08, 2023 at 05:13:36AM -0800, Yi Liu wrote:
> > 
> > > +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id)
> > > +{
> > > +	struct iommufd_ioas *new_ioas = NULL, *cur_ioas;
> > > +	struct iommufd_ctx *ictx = access->ictx;
> > > +	struct iommufd_object *obj;
> > > +	int rc = 0;
> > > +
> > > +	if (ioas_id) {
> > > +		obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > +		if (IS_ERR(obj))
> > > +			return PTR_ERR(obj);
> > > +		new_ioas = container_of(obj, struct iommufd_ioas, obj);
> > > +	}
> > > +
> > > +	mutex_lock(&access->ioas_lock);
> > > +	cur_ioas = access->ioas;
> > > +	if (cur_ioas == new_ioas)
> > > +		goto out_unlock;
> > > +
> > > +	if (new_ioas) {
> > > +		rc = iopt_add_access(&new_ioas->iopt, access);
> > > +		if (rc)
> > > +			goto out_unlock;
> > > +		iommufd_ref_to_users(obj);
> > > +	}
> > > +
> > > +	if (cur_ioas) {
> > > +		iopt_remove_access(&cur_ioas->iopt, access);
> > > +		refcount_dec(&cur_ioas->obj.users);
> > > +	}
> > 
> > This should match the physical side with an add/remove/replace
> > API. Especially since remove is implicit in destroy this series only
> > needs the add API
> 
> I assume that the API would be iommufd_access_attach,
> iommufd_access_detach, and iommufd_access_replace(). And there
> might be an iommufd_access_change_pt(access, pt, bool replace)?
> 
> > And the locking shouldn't come in another patch that brings the
> > replace/remove since with just split add we don't need it.
> 
> Hmm. The iommufd_access_detach would be needed in the following
> cdev series, while the iommufd_access_replace would be need in
> my replace series. So, that would make the API be divided into
> three series.
> 
> Perhaps we can have iommufd_access_attach/detach in this series
> along with a vfio_iommufd_emulated_detach_ioas, and the locking
> will come with another patch in replace series?

I recall that we previously concluded that the unbind() is safe
to go without doing access->ops->unmap, because close_device()
would be called prior to the unbind().

But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev
series, we'd need the access->ops->unmap call, and the locking
and "ioas_unpin" too in the detach and attach APIs, right?

I tried a bit of some update, across this series, cdev series,
and the replace series. Though we might be able to simplify a
bit of this patch/series, yet it doesn't seem to simplify the
changes overall, particularly in the following cdev series for
having an unmap() call and the locking.

Then the replace API would mostly overlap with the attach API,
by only having an additional detach(), which means actually we
only need an iommufd_access_attach API to cover both cases...

Can you please take a look at the final access APIs that I've
attached at the end of the email for things mentioned above?
Hopefully we can confirm and put them correctly into the next
version of the three series.

Thanks
Nic

-----------------------------------------------------------------------
static void __iommufd_access_detach(struct iommufd_access *access)
{
	struct iommufd_ioas *cur_ioas = access->ioas;

	lockdep_assert_held(&access->ioas_lock);
	/*
	 * Set ioas to NULL to block any further iommufd_access_pin_pages().
	 * iommufd_access_unpin_pages() can continue using access->ioas_unpin.
	 */
	access->ioas = NULL;

	if (access->ops->unmap) {
		mutex_unlock(&access->ioas_lock);
		access->ops->unmap(access->data, 0, ULONG_MAX);
		mutex_lock(&access->ioas_lock);
	}
	iopt_remove_access(&cur_ioas->iopt, access);
	refcount_dec(&cur_ioas->obj.users);
}

static int iommufd_access_change_pt(struct iommufd_access *access, u32 ioas_id)
{
	struct iommufd_ioas *new_ioas, *cur_ioas;
	struct iommufd_object *obj;
	int rc = 0;

	obj = iommufd_get_object(access->ictx, ioas_id, IOMMUFD_OBJ_IOAS);
	if (IS_ERR(obj))
		return PTR_ERR(obj);
	new_ioas = container_of(obj, struct iommufd_ioas, obj);

	mutex_lock(&access->ioas_lock);
	cur_ioas = access->ioas;
	if (cur_ioas == new_ioas)
		goto out_unlock;

	rc = iopt_add_access(&new_ioas->iopt, access);
	if (rc)
		goto out_unlock;
	iommufd_ref_to_users(obj);

	if (cur_ioas)
		__iommufd_access_detach(access);
	access->ioas_unpin = new_ioas;
	access->ioas = new_ioas;
	mutex_unlock(&access->ioas_lock);
	return 0;

out_unlock:
	mutex_unlock(&access->ioas_lock);
	iommufd_put_object(obj);
	return rc;
}

int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
{
	return iommufd_access_change_pt(access, ioas_id);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, IOMMUFD);

/* Identical to iommufd_access_attach now... */
int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id)
{
	return iommufd_access_change_pt(access, ioas_id);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_replace, IOMMUFD);

void iommufd_access_detach(struct iommufd_access *access)
{
	mutex_lock(&access->ioas_lock);
	if (WARN_ON(!access->ioas))
		goto out;
	__iommufd_access_detach(access);
out:
	access->ioas_unpin = NULL;
	mutex_unlock(&access->ioas_lock);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, March 15, 2023 9:01 AM
> 
> Hi Jason/Kevin,
> 
> >
> > Perhaps we can have iommufd_access_attach/detach in this series
> > along with a vfio_iommufd_emulated_detach_ioas, and the locking
> > will come with another patch in replace series?
> 
> I recall that we previously concluded that the unbind() is safe
> to go without doing access->ops->unmap, because close_device()
> would be called prior to the unbind().
> 
> But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev
> series, we'd need the access->ops->unmap call, and the locking
> and "ioas_unpin" too in the detach and attach APIs, right?

yes. We need locking since detach can happen any time with 
cdev while driver is conducting pinning.

> 
> I tried a bit of some update, across this series, cdev series,
> and the replace series. Though we might be able to simplify a
> bit of this patch/series, yet it doesn't seem to simplify the
> changes overall, particularly in the following cdev series for
> having an unmap() call and the locking.
> 
> Then the replace API would mostly overlap with the attach API,
> by only having an additional detach(), which means actually we
> only need an iommufd_access_attach API to cover both cases...

there is a subtle difference. to match the physical path:

for attach we expect the existing access->ioas is either NULL or
same as the new ioas.

for replace access->ioas must exist.

they need different condition checks.

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Wednesday, March 15, 2023 2:51 AM
> 
> On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Wednesday, March 8, 2023 9:14 PM
> > >
> > > @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx
> *ictx,
> > > u32 ioas_id,
> > >       access->data = data;
> > >       access->ops = ops;
> > >
> > > -     obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > -     if (IS_ERR(obj)) {
> > > -             rc = PTR_ERR(obj);
> > > -             goto out_abort;
> > > -     }
> > > -     access->ioas = container_of(obj, struct iommufd_ioas, obj);
> > > -     iommufd_ref_to_users(obj);
> > > -
> > >       if (ops->needs_pin_pages)
> > >               access->iova_alignment = PAGE_SIZE;
> > >       else
> > >               access->iova_alignment = 1;
> > > -     rc = iopt_add_access(&access->ioas->iopt, access);
> > > -     if (rc)
> > > -             goto out_put_ioas;
> > >
> > >       /* The calling driver is a user until iommufd_access_destroy() */
> > >       refcount_inc(&access->obj.users);
> > > +     mutex_init(&access->ioas_lock);
> > >       access->ictx = ictx;
> > >       iommufd_ctx_get(ictx);
> >
> > this refcnt get should be moved to the start given next patch
> > removes the reference in the caller side.
> 
> I don't feel quite convincing to justify for moving it in this
> patch. Perhaps we should do that in the following patch, where
> it needs this? Or another individual patch moving this alone?
> 

Next patch. I just tried to point out the required change caused
by next patch. 😊

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

On Wed, Mar 15, 2023 at 06:16:37AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen
> > Sent: Wednesday, March 15, 2023 2:51 AM
> >
> > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > > Sent: Wednesday, March 8, 2023 9:14 PM
> > > >
> > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx
> > *ictx,
> > > > u32 ioas_id,
> > > >       access->data = data;
> > > >       access->ops = ops;
> > > >
> > > > -     obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > > -     if (IS_ERR(obj)) {
> > > > -             rc = PTR_ERR(obj);
> > > > -             goto out_abort;
> > > > -     }
> > > > -     access->ioas = container_of(obj, struct iommufd_ioas, obj);
> > > > -     iommufd_ref_to_users(obj);
> > > > -
> > > >       if (ops->needs_pin_pages)
> > > >               access->iova_alignment = PAGE_SIZE;
> > > >       else
> > > >               access->iova_alignment = 1;
> > > > -     rc = iopt_add_access(&access->ioas->iopt, access);
> > > > -     if (rc)
> > > > -             goto out_put_ioas;
> > > >
> > > >       /* The calling driver is a user until iommufd_access_destroy() */
> > > >       refcount_inc(&access->obj.users);
> > > > +     mutex_init(&access->ioas_lock);
> > > >       access->ictx = ictx;
> > > >       iommufd_ctx_get(ictx);
> > >
> > > this refcnt get should be moved to the start given next patch
> > > removes the reference in the caller side.
> >
> > I don't feel quite convincing to justify for moving it in this
> > patch. Perhaps we should do that in the following patch, where
> > it needs this? Or another individual patch moving this alone?
> >
> 
> Next patch. I just tried to point out the required change caused
> by next patch. 😊

OK. I made a small individual patch. Posting here so Yi can just
squash it into the next patch:

From dbfe7457d35ea9a4da9c8e6daa838b101bc8f621 Mon Sep 17 00:00:00 2001
From: Nicolin Chen <nicolinc@nvidia.com>
Date: Tue, 14 Mar 2023 12:51:07 -0700
Subject: [PATCH] iommufd/device: Do iommufd_ctx_get() at the top of
 iommufd_access_create()

The following patch will remove the iommufd_ctx_get() call prior to the
iommufd_access_create() call in vfio_iommufd_emulated_bind(), expecting
iommufd_access_create() call covers the iommufd_ctx_get(). However, the
iommufd_access_create() only does iommufd_ctx_get() at the end. Thus,
move the iommufd_ctx_get() call to the top of iommufd_access_create().

Suggested-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/device.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 0132803449be..dc1015b02a53 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -649,13 +649,16 @@ iommufd_access_create(struct iommufd_ctx *ictx,
 {
 	struct iommufd_access *access;
 
+	iommufd_ctx_get(ictx);
 	/*
 	 * There is no uAPI for the access object, but to keep things symmetric
 	 * use the object infrastructure anyhow.
 	 */
 	access = iommufd_object_alloc(ictx, access, IOMMUFD_OBJ_ACCESS);
-	if (IS_ERR(access))
+	if (IS_ERR(access)) {
+		iommufd_ctx_put(ictx);
 		return access;
+	}
 
 	access->data = data;
 	access->ops = ops;
@@ -668,7 +671,6 @@ iommufd_access_create(struct iommufd_ctx *ictx,
 	/* The calling driver is a user until iommufd_access_destroy() */
 	refcount_inc(&access->obj.users);
 	access->ictx = ictx;
-	iommufd_ctx_get(ictx);
 	iommufd_object_finalize(ictx, &access->obj);
 	return access;
 }
-- 
2.39.2

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

On Wed, Mar 15, 2023 at 06:15:23AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Wednesday, March 15, 2023 9:01 AM
> >
> > Hi Jason/Kevin,
> >
> > >
> > > Perhaps we can have iommufd_access_attach/detach in this series
> > > along with a vfio_iommufd_emulated_detach_ioas, and the locking
> > > will come with another patch in replace series?
> >
> > I recall that we previously concluded that the unbind() is safe
> > to go without doing access->ops->unmap, because close_device()
> > would be called prior to the unbind().
> >
> > But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev
> > series, we'd need the access->ops->unmap call, and the locking
> > and "ioas_unpin" too in the detach and attach APIs, right?
> 
> yes. We need locking since detach can happen any time with
> cdev while driver is conducting pinning.

So, this preparatory series will add a pair of simple attach()
and detach() APIs. Then the cdev series will add the locking
and the ioas_unpin stuff as a rework of the detach() API.

> > I tried a bit of some update, across this series, cdev series,
> > and the replace series. Though we might be able to simplify a
> > bit of this patch/series, yet it doesn't seem to simplify the
> > changes overall, particularly in the following cdev series for
> > having an unmap() call and the locking.
> >
> > Then the replace API would mostly overlap with the attach API,
> > by only having an additional detach(), which means actually we
> > only need an iommufd_access_attach API to cover both cases...
> 
> there is a subtle difference. to match the physical path:
> 
> for attach we expect the existing access->ioas is either NULL or
> same as the new ioas.
> 
> for replace access->ioas must exist.
> 
> they need different condition checks.

I think they can be something mingled... the sample code that
I sent previously could take care of those conditions. But, I
am also thinking a bit that maybe attach() does not need the
locking? I can do a separate replace() function in this case.

Thanks
Nic

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Wednesday, March 15, 2023 2:33 PM
> 
> On Wed, Mar 15, 2023 at 06:15:23AM +0000, Tian, Kevin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Wednesday, March 15, 2023 9:01 AM
> > >
> > > Hi Jason/Kevin,
> > >
> > > >
> > > > Perhaps we can have iommufd_access_attach/detach in this series
> > > > along with a vfio_iommufd_emulated_detach_ioas, and the locking
> > > > will come with another patch in replace series?
> > >
> > > I recall that we previously concluded that the unbind() is safe
> > > to go without doing access->ops->unmap, because close_device()
> > > would be called prior to the unbind().
> > >
> > > But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev
> > > series, we'd need the access->ops->unmap call, and the locking
> > > and "ioas_unpin" too in the detach and attach APIs, right?
> >
> > yes. We need locking since detach can happen any time with
> > cdev while driver is conducting pinning.
> 
> So, this preparatory series will add a pair of simple attach()
> and detach() APIs. Then the cdev series will add the locking
> and the ioas_unpin stuff as a rework of the detach() API.
> 
> > > I tried a bit of some update, across this series, cdev series,
> > > and the replace series. Though we might be able to simplify a
> > > bit of this patch/series, yet it doesn't seem to simplify the
> > > changes overall, particularly in the following cdev series for
> > > having an unmap() call and the locking.
> > >
> > > Then the replace API would mostly overlap with the attach API,
> > > by only having an additional detach(), which means actually we
> > > only need an iommufd_access_attach API to cover both cases...
> >
> > there is a subtle difference. to match the physical path:
> >
> > for attach we expect the existing access->ioas is either NULL or
> > same as the new ioas.
> >
> > for replace access->ioas must exist.
> >
> > they need different condition checks.
> 
> I think they can be something mingled... the sample code that
> I sent previously could take care of those conditions. But, I
> am also thinking a bit that maybe attach() does not need the
> locking? I can do a separate replace() function in this case.
> 

w/o locking then you need smp_store_release() and its pair.

anyway it's not in perf critical path. Keeping lock for attach
is simpler and safe.

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Wednesday, March 15, 2023 2:22 PM
> 
> On Wed, Mar 15, 2023 at 06:16:37AM +0000, Tian, Kevin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > > From: Nicolin Chen
> > > Sent: Wednesday, March 15, 2023 2:51 AM
> > >
> > > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote:
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > > > Sent: Wednesday, March 8, 2023 9:14 PM
> > > > >
> > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct
> iommufd_ctx
> > > *ictx,
> > > > > u32 ioas_id,
> > > > >       access->data = data;
> > > > >       access->ops = ops;
> > > > >
> > > > > -     obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > > > -     if (IS_ERR(obj)) {
> > > > > -             rc = PTR_ERR(obj);
> > > > > -             goto out_abort;
> > > > > -     }
> > > > > -     access->ioas = container_of(obj, struct iommufd_ioas, obj);
> > > > > -     iommufd_ref_to_users(obj);
> > > > > -
> > > > >       if (ops->needs_pin_pages)
> > > > >               access->iova_alignment = PAGE_SIZE;
> > > > >       else
> > > > >               access->iova_alignment = 1;
> > > > > -     rc = iopt_add_access(&access->ioas->iopt, access);
> > > > > -     if (rc)
> > > > > -             goto out_put_ioas;
> > > > >
> > > > >       /* The calling driver is a user until iommufd_access_destroy() */
> > > > >       refcount_inc(&access->obj.users);
> > > > > +     mutex_init(&access->ioas_lock);
> > > > >       access->ictx = ictx;
> > > > >       iommufd_ctx_get(ictx);
> > > >
> > > > this refcnt get should be moved to the start given next patch
> > > > removes the reference in the caller side.
> > >
> > > I don't feel quite convincing to justify for moving it in this
> > > patch. Perhaps we should do that in the following patch, where
> > > it needs this? Or another individual patch moving this alone?
> > >
> >
> > Next patch. I just tried to point out the required change caused
> > by next patch. 😊
> 
> OK. I made a small individual patch. Posting here so Yi can just
> squash it into the next patch:
> 
> From dbfe7457d35ea9a4da9c8e6daa838b101bc8f621 Mon Sep 17 00:00:00
> 2001
> From: Nicolin Chen <nicolinc@nvidia.com>
> Date: Tue, 14 Mar 2023 12:51:07 -0700
> Subject: [PATCH] iommufd/device: Do iommufd_ctx_get() at the top of
>  iommufd_access_create()
> 
> The following patch will remove the iommufd_ctx_get() call prior to the
> iommufd_access_create() call in vfio_iommufd_emulated_bind(), expecting
> iommufd_access_create() call covers the iommufd_ctx_get(). However, the
> iommufd_access_create() only does iommufd_ctx_get() at the end. Thus,
> move the iommufd_ctx_get() call to the top of iommufd_access_create().
> 
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommufd/device.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c
> b/drivers/iommu/iommufd/device.c
> index 0132803449be..dc1015b02a53 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -649,13 +649,16 @@ iommufd_access_create(struct iommufd_ctx *ictx,
>  {
>  	struct iommufd_access *access;
> 
> +	iommufd_ctx_get(ictx);
>  	/*
>  	 * There is no uAPI for the access object, but to keep things
> symmetric
>  	 * use the object infrastructure anyhow.
>  	 */
>  	access = iommufd_object_alloc(ictx, access, IOMMUFD_OBJ_ACCESS);
> -	if (IS_ERR(access))
> +	if (IS_ERR(access)) {
> +		iommufd_ctx_put(ictx);
>  		return access;
> +	}
> 
>  	access->data = data;
>  	access->ops = ops;
> @@ -668,7 +671,6 @@ iommufd_access_create(struct iommufd_ctx *ictx,
>  	/* The calling driver is a user until iommufd_access_destroy() */
>  	refcount_inc(&access->obj.users);
>  	access->ictx = ictx;
> -	iommufd_ctx_get(ictx);
>  	iommufd_object_finalize(ictx, &access->obj);
>  	return access;
>  }

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Liu, Yi L]

> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Wednesday, March 15, 2023 2:52 PM
> 
> > From: Nicolin Chen
> > Sent: Wednesday, March 15, 2023 2:22 PM
> >
> > On Wed, Mar 15, 2023 at 06:16:37AM +0000, Tian, Kevin wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > > From: Nicolin Chen
> > > > Sent: Wednesday, March 15, 2023 2:51 AM
> > > >
> > > > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote:
> > > > > External email: Use caution opening links or attachments
> > > > >
> > > > >
> > > > > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > > > > Sent: Wednesday, March 8, 2023 9:14 PM
> > > > > >
> > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct
> > iommufd_ctx
> > > > *ictx,
> > > > > > u32 ioas_id,
> > > > > >       access->data = data;
> > > > > >       access->ops = ops;
> > > > > >
> > > > > > -     obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > > > > -     if (IS_ERR(obj)) {
> > > > > > -             rc = PTR_ERR(obj);
> > > > > > -             goto out_abort;
> > > > > > -     }
> > > > > > -     access->ioas = container_of(obj, struct iommufd_ioas, obj);
> > > > > > -     iommufd_ref_to_users(obj);
> > > > > > -
> > > > > >       if (ops->needs_pin_pages)
> > > > > >               access->iova_alignment = PAGE_SIZE;
> > > > > >       else
> > > > > >               access->iova_alignment = 1;
> > > > > > -     rc = iopt_add_access(&access->ioas->iopt, access);
> > > > > > -     if (rc)
> > > > > > -             goto out_put_ioas;
> > > > > >
> > > > > >       /* The calling driver is a user until iommufd_access_destroy() */
> > > > > >       refcount_inc(&access->obj.users);
> > > > > > +     mutex_init(&access->ioas_lock);
> > > > > >       access->ictx = ictx;
> > > > > >       iommufd_ctx_get(ictx);
> > > > >
> > > > > this refcnt get should be moved to the start given next patch
> > > > > removes the reference in the caller side.

This change is ok but seems not necessary.

Yes, vfio_iommufd_emulated_bind() will not have reference on the
ictx after the next patch. However, it gets reference only because it
wants to store it in vfio_device. Now, it does not store it. So no get.
I think the caller of vfio_iommufd_emulated_bind() should ensure
the ictx is valid. Also check the physical device bind. So maybe not
necessary to get ictx before calling iommufd_access_create(). This is
the same with the vfio_iommufd_physical_bind() which calls
iommufd_device_bind() without ictx get, and iommufd_device_bind()
also gets ictx in the end.
 
If it's really necessary, maybe let the vfio_iommufd_physical_bind()
and vfio_iommufd_emulated_bind() get/put ictx around calling
iommufd_access_create()/iommufd_device_bind().

> > > >
> > > > I don't feel quite convincing to justify for moving it in this
> > > > patch. Perhaps we should do that in the following patch, where
> > > > it needs this? Or another individual patch moving this alone?
> > > >
> > >
> > > Next patch. I just tried to point out the required change caused
> > > by next patch. 😊
>
> > OK. I made a small individual patch. Posting here so Yi can just
> > squash it into the next patch:
> >
> > From dbfe7457d35ea9a4da9c8e6daa838b101bc8f621 Mon Sep 17 00:00:00
> > 2001
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Date: Tue, 14 Mar 2023 12:51:07 -0700
> > Subject: [PATCH] iommufd/device: Do iommufd_ctx_get() at the top of
> >  iommufd_access_create()
> >
> > The following patch will remove the iommufd_ctx_get() call prior to the
> > iommufd_access_create() call in vfio_iommufd_emulated_bind(),
> expecting
> > iommufd_access_create() call covers the iommufd_ctx_get(). However,
> the
> > iommufd_access_create() only does iommufd_ctx_get() at the end. Thus,
> > move the iommufd_ctx_get() call to the top of iommufd_access_create().
> >
> > Suggested-by: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > ---
> >  drivers/iommu/iommufd/device.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/iommufd/device.c
> > b/drivers/iommu/iommufd/device.c
> > index 0132803449be..dc1015b02a53 100644
> > --- a/drivers/iommu/iommufd/device.c
> > +++ b/drivers/iommu/iommufd/device.c
> > @@ -649,13 +649,16 @@ iommufd_access_create(struct iommufd_ctx
> *ictx,
> >  {
> >  	struct iommufd_access *access;
> >
> > +	iommufd_ctx_get(ictx);
> >  	/*
> >  	 * There is no uAPI for the access object, but to keep things
> > symmetric
> >  	 * use the object infrastructure anyhow.
> >  	 */
> >  	access = iommufd_object_alloc(ictx, access,
> IOMMUFD_OBJ_ACCESS);
> > -	if (IS_ERR(access))
> > +	if (IS_ERR(access)) {
> > +		iommufd_ctx_put(ictx);
> >  		return access;
> > +	}
> >
> >  	access->data = data;
> >  	access->ops = ops;
> > @@ -668,7 +671,6 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> >  	/* The calling driver is a user until iommufd_access_destroy() */
> >  	refcount_inc(&access->obj.users);
> >  	access->ictx = ictx;
> > -	iommufd_ctx_get(ictx);
> >  	iommufd_object_finalize(ictx, &access->obj);
> >  	return access;
> >  }
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

Hi,

On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote:

> > So, this preparatory series will add a pair of simple attach()
> > and detach() APIs. Then the cdev series will add the locking
> > and the ioas_unpin stuff as a rework of the detach() API.

> > I think they can be something mingled... the sample code that
> > I sent previously could take care of those conditions. But, I
> > am also thinking a bit that maybe attach() does not need the
> > locking? I can do a separate replace() function in this case.
> >
> 
> w/o locking then you need smp_store_release() and its pair.
> 
> anyway it's not in perf critical path. Keeping lock for attach
> is simpler and safe.

OK. Basically I followed what Jason suggested by having three
APIs and combined Kevin's inputs about the difference between
the attach/replace(). I also updated the replace changes, and
rebased all nesting (infrastructure, VT-d and SMMU):
https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023

The major three changes for those APIs:
[1] This adds iommufd_access_attach() in this series:
    "iommufd: Create access in vfio_iommufd_emulated_bind()"
    https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcca5ceaeb40e22b5
[2] This adds iommufd_access_detach() in the cdev series:
    "iommufd/device: Add iommufd_access_detach() API"
    https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a097e2c9d9e26af4
[3] This adds iommufd_access_replace() in the replace series:
    "iommufd: Add iommufd_access_replace() API"
    https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9bc2bf319b7654a8

Please check if they look okay, so that Yi can integrate them
accordingly to the emulated/cdev series.

[*] This is the patch that I posted in the other mail addressing
    Kevin's comments on iommufd_ctx_get():
    "iommufd/device: Do iommufd_ctx_get() at the top of iommufd_access_create()"
    https://github.com/nicolinc/iommufd/commit/077b09bb83329dc046753f4ef672f5bf6386755c
    (I just saw Yi's reply concerning its necessity. Feel free
     to drop in that case.)

Thanks
Nicolin

P.S. Attaching the list of changes with their locations:
3791dedf98e8 cover-letter: Add IO page table replacement support
c8ebf51c3c9b vfio: Support IO page table replacement
c5710f23e8f6 iommufd/selftest: Add IOMMU_TEST_OP_ACCESS_REPLACE_IOAS coverage
[3] 36507fa9f0f4 iommufd: Add iommufd_access_replace() API
0263855d1e8b vfio: Do not allow !ops->dma_unmap in vfio_pin/unpin_pages()
e39ed55e77a0 cover-letter: Add vfio_device cdev for iommufd support
26fd7fccaef3 docs: vfio: Add vfio device cdev description
f10f3e3162bb vfio: Compile group optionally
9588ae4c4049 vfio: Add VFIO_DEVICE_AT[DE]TACH_IOMMUFD_PT
3e57108eac64 vfio: Add VFIO_DEVICE_BIND_IOMMUFD
b925716dd226 vfio: Add cdev for vfio_device
db309463ab92 vfio-iommufd: Add detach_ioas support for emulated VFIO devices
[2] 4110522146ca iommufd/device: Add iommufd_access_detach() API
abca7e1e063a vfio-iommufd: Add detach_ioas support for physical VFIO devices
9d368f7247c7 vfio: Record devid in vfio_device_file
683af0a471e1 vfio-iommufd: Split the compat_ioas attach out from vfio_iommufd_bind()
32a2e7de1d53 vfio-iommufd: Split the no-iommu support out from vfio_iommufd_bind()
8a1c042379f5 vfio: Make vfio_device_first_open() to accept NULL iommufd for noiommu
fc6e0ed2aa44 vfio: Make vfio_device_open() single open for device cdev path
3f6821d507a4 vfio: Add cdev_device_open_cnt to vfio_group
896cde40a016 vfio: Block device access via device fd until device is opened
f422c4216a19 vfio: Pass struct vfio_device_file * to vfio_device_open/close()
b187f9980fed kvm/vfio: Accept vfio device file from userspace
721e2e60ff54 kvm/vfio: Rename kvm_vfio_group to prepare for accepting vfio device fd
8993c4c75c20 vfio: Accept vfio device file in the KVM facing kAPI
a92c45ae0ce6 vfio: Remove vfio_file_is_group()
fb586f783934 vfio: Refine vfio file kAPIs for KVM
50694af6f3c0 vfio: Allocate per device file structure
df21c0737eef cover-letter: Make vfio-pci hot reset prepared for vfio device cdev
5c25c874d7e0 vfio/pci: Accept device fd in VFIO_DEVICE_PCI_HOT_RESET ioctl
7c30ce8b54db vfio: Accpet device file from vfio PCI hot reset path
e3209342db44 vfio: Refine vfio file kAPIs for vfio PCI hot reset
8354fd79944e vfio/pci: Rename the helpers and data in hot reset path to accept device fd
54387efb858c vfio/pci: Allow passing zero-length fd array in VFIO_DEVICE_PCI_HOT_RESET
cd93ffb62c51 vfio/pci: Only need to check opened devices in the dev_set for hot reset
2a6fd7231cbf vfio/pci: Update comment around group_fd get in vfio_pci_ioctl_pci_hot_reset()
480abea5961e cover-letter: vfio: Make emulated devices prepared for vfio device cdev
46b6d1ae1754 vfio: Check the presence for iommufd callbacks in __vfio_register_dev()
6064b9f81817 Samples/mdev: Uses the vfio emulated iommufd ops set in the mdev sample drivers
c20852af7291 vfio-iommufd: Make vfio_iommufd_emulated_bind() return iommufd_access ID
3405865b0b3f vfio-iommufd: No need to record iommufd_ctx in vfio_device
[*] 077b09bb8332 iommufd/device: Do iommufd_ctx_get() at the top of iommufd_access_create()
[1] 34fba7509429 iommufd: Create access in vfio_iommufd_emulated_bind()
a5d8ac47554f docs: kvm: vfio: Require call KVM_DEV_VFIO_GROUP_ADD before VFIO_GROUP_GET_DEVICE_FD

RE: [PATCH v1 5/5] vfio: Check the presence for iommufd callbacks in __vfio_register_dev()

[Liu, Yi L]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, March 11, 2023 1:40 AM
> 
> On Wed, Mar 08, 2023 at 05:13:40AM -0800, Yi Liu wrote:
> > After making the no-DMA drivers (samples/vfio-mdev) providing
> iommufd
> > callbacks, __vfio_register_dev() should check the presence of the
> iommufd
> > callbacks if CONFIG_IOMMUFD is enabled.
> >
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > ---
> >  drivers/vfio/vfio_main.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> 
> Alex, when you take this can it please go on a branch that will also
> have the cdev series?

Hi Alex,

I've got a cdev v7 candidate in below branch. It has three sub-series per
the suggestion in prior cdev review.

 - cover-letter: Add vfio_device cdev for iommufd support
 - cover-letter: Introduce new methods for verifying ownership in vfio PCI hot reset
 - cover-letter: vfio: Make emulated devices prepared for vfio device cdev

https://github.com/yiliu1765/iommufd/tree/wip/vfio_device_cdev_v7_candidate

However, there is one open on Nicolin's patch in below link. So I need
to wait for it before sending v7.

https://lore.kernel.org/kvm/ZBGJzefTm4p%2FReIu@Asurada-Nvidia/

Regards,
Yi Liu

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Liu, Yi L]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, March 15, 2023 5:03 PM
> 
> Hi,
> 
> On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote:
> 
> > > So, this preparatory series will add a pair of simple attach()
> > > and detach() APIs. Then the cdev series will add the locking
> > > and the ioas_unpin stuff as a rework of the detach() API.
> 
> > > I think they can be something mingled... the sample code that
> > > I sent previously could take care of those conditions. But, I
> > > am also thinking a bit that maybe attach() does not need the
> > > locking? I can do a separate replace() function in this case.
> > >
> >
> > w/o locking then you need smp_store_release() and its pair.
> >
> > anyway it's not in perf critical path. Keeping lock for attach
> > is simpler and safe.
> 
> OK. Basically I followed what Jason suggested by having three
> APIs and combined Kevin's inputs about the difference between
> the attach/replace(). I also updated the replace changes, and
> rebased all nesting (infrastructure, VT-d and SMMU):
> https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023
> 
> The major three changes for those APIs:
> [1] This adds iommufd_access_attach() in this series:
>     "iommufd: Create access in vfio_iommufd_emulated_bind()"
> 
> https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dc
> ca5ceaeb40e22b5
> [2] This adds iommufd_access_detach() in the cdev series:
>     "iommufd/device: Add iommufd_access_detach() API"
> 
> https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a
> 097e2c9d9e26af4
> [3] This adds iommufd_access_replace() in the replace series:
>     "iommufd: Add iommufd_access_replace() API"
> 
> https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9
> bc2bf319b7654a8
> 
> Please check if they look okay, so that Yi can integrate them
> accordingly to the emulated/cdev series.

Thanks. I'll start to integrate after ack from Kevin or Jason. btw.
Below is my latest code (rebased on top of rc-2). 😊

https://github.com/yiliu1765/iommufd/tree/wip/vfio_device_cdev_v7_candidate

> 
> [*] This is the patch that I posted in the other mail addressing
>     Kevin's comments on iommufd_ctx_get():
>     "iommufd/device: Do iommufd_ctx_get() at the top of
> iommufd_access_create()"
> 
> https://github.com/nicolinc/iommufd/commit/077b09bb83329dc046753f4ef
> 672f5bf6386755c
>     (I just saw Yi's reply concerning its necessity. Feel free
>      to drop in that case.)

Yeah, let's see Kevin's feedback.

> 
> Thanks
> Nicolin
> 
> P.S. Attaching the list of changes with their locations:
> 3791dedf98e8 cover-letter: Add IO page table replacement support
> c8ebf51c3c9b vfio: Support IO page table replacement
> c5710f23e8f6 iommufd/selftest: Add
> IOMMU_TEST_OP_ACCESS_REPLACE_IOAS coverage
> [3] 36507fa9f0f4 iommufd: Add iommufd_access_replace() API
> 0263855d1e8b vfio: Do not allow !ops->dma_unmap in
> vfio_pin/unpin_pages()
> e39ed55e77a0 cover-letter: Add vfio_device cdev for iommufd support
> 26fd7fccaef3 docs: vfio: Add vfio device cdev description
> f10f3e3162bb vfio: Compile group optionally
> 9588ae4c4049 vfio: Add VFIO_DEVICE_AT[DE]TACH_IOMMUFD_PT
> 3e57108eac64 vfio: Add VFIO_DEVICE_BIND_IOMMUFD
> b925716dd226 vfio: Add cdev for vfio_device
> db309463ab92 vfio-iommufd: Add detach_ioas support for emulated VFIO
> devices
> [2] 4110522146ca iommufd/device: Add iommufd_access_detach() API
> abca7e1e063a vfio-iommufd: Add detach_ioas support for physical VFIO
> devices
> 9d368f7247c7 vfio: Record devid in vfio_device_file
> 683af0a471e1 vfio-iommufd: Split the compat_ioas attach out from
> vfio_iommufd_bind()
> 32a2e7de1d53 vfio-iommufd: Split the no-iommu support out from
> vfio_iommufd_bind()
> 8a1c042379f5 vfio: Make vfio_device_first_open() to accept NULL iommufd
> for noiommu
> fc6e0ed2aa44 vfio: Make vfio_device_open() single open for device cdev
> path
> 3f6821d507a4 vfio: Add cdev_device_open_cnt to vfio_group
> 896cde40a016 vfio: Block device access via device fd until device is opened
> f422c4216a19 vfio: Pass struct vfio_device_file * to vfio_device_open/close()
> b187f9980fed kvm/vfio: Accept vfio device file from userspace
> 721e2e60ff54 kvm/vfio: Rename kvm_vfio_group to prepare for accepting
> vfio device fd
> 8993c4c75c20 vfio: Accept vfio device file in the KVM facing kAPI
> a92c45ae0ce6 vfio: Remove vfio_file_is_group()
> fb586f783934 vfio: Refine vfio file kAPIs for KVM
> 50694af6f3c0 vfio: Allocate per device file structure
> df21c0737eef cover-letter: Make vfio-pci hot reset prepared for vfio device
> cdev
> 5c25c874d7e0 vfio/pci: Accept device fd in VFIO_DEVICE_PCI_HOT_RESET
> ioctl
> 7c30ce8b54db vfio: Accpet device file from vfio PCI hot reset path
> e3209342db44 vfio: Refine vfio file kAPIs for vfio PCI hot reset
> 8354fd79944e vfio/pci: Rename the helpers and data in hot reset path to
> accept device fd
> 54387efb858c vfio/pci: Allow passing zero-length fd array in
> VFIO_DEVICE_PCI_HOT_RESET
> cd93ffb62c51 vfio/pci: Only need to check opened devices in the dev_set
> for hot reset
> 2a6fd7231cbf vfio/pci: Update comment around group_fd get in
> vfio_pci_ioctl_pci_hot_reset()
> 480abea5961e cover-letter: vfio: Make emulated devices prepared for vfio
> device cdev
> 46b6d1ae1754 vfio: Check the presence for iommufd callbacks in
> __vfio_register_dev()
> 6064b9f81817 Samples/mdev: Uses the vfio emulated iommufd ops set in
> the mdev sample drivers
> c20852af7291 vfio-iommufd: Make vfio_iommufd_emulated_bind() return
> iommufd_access ID
> 3405865b0b3f vfio-iommufd: No need to record iommufd_ctx in vfio_device
> [*] 077b09bb8332 iommufd/device: Do iommufd_ctx_get() at the top of
> iommufd_access_create()
> [1] 34fba7509429 iommufd: Create access in vfio_iommufd_emulated_bind()
> a5d8ac47554f docs: kvm: vfio: Require call KVM_DEV_VFIO_GROUP_ADD
> before VFIO_GROUP_GET_DEVICE_FD

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Wednesday, March 15, 2023 4:53 PM
> 
> > From: Tian, Kevin <kevin.tian@intel.com>
> > Sent: Wednesday, March 15, 2023 2:52 PM
> >
> > > From: Nicolin Chen
> > > Sent: Wednesday, March 15, 2023 2:22 PM
> > >
> > > On Wed, Mar 15, 2023 at 06:16:37AM +0000, Tian, Kevin wrote:
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > > From: Nicolin Chen
> > > > > Sent: Wednesday, March 15, 2023 2:51 AM
> > > > >
> > > > > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote:
> > > > > > External email: Use caution opening links or attachments
> > > > > >
> > > > > >
> > > > > > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > > > > > Sent: Wednesday, March 8, 2023 9:14 PM
> > > > > > >
> > > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct
> > > iommufd_ctx
> > > > > *ictx,
> > > > > > > u32 ioas_id,
> > > > > > >       access->data = data;
> > > > > > >       access->ops = ops;
> > > > > > >
> > > > > > > -     obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > > > > > -     if (IS_ERR(obj)) {
> > > > > > > -             rc = PTR_ERR(obj);
> > > > > > > -             goto out_abort;
> > > > > > > -     }
> > > > > > > -     access->ioas = container_of(obj, struct iommufd_ioas, obj);
> > > > > > > -     iommufd_ref_to_users(obj);
> > > > > > > -
> > > > > > >       if (ops->needs_pin_pages)
> > > > > > >               access->iova_alignment = PAGE_SIZE;
> > > > > > >       else
> > > > > > >               access->iova_alignment = 1;
> > > > > > > -     rc = iopt_add_access(&access->ioas->iopt, access);
> > > > > > > -     if (rc)
> > > > > > > -             goto out_put_ioas;
> > > > > > >
> > > > > > >       /* The calling driver is a user until iommufd_access_destroy()
> */
> > > > > > >       refcount_inc(&access->obj.users);
> > > > > > > +     mutex_init(&access->ioas_lock);
> > > > > > >       access->ictx = ictx;
> > > > > > >       iommufd_ctx_get(ictx);
> > > > > >
> > > > > > this refcnt get should be moved to the start given next patch
> > > > > > removes the reference in the caller side.
> 
> This change is ok but seems not necessary.
> 
> Yes, vfio_iommufd_emulated_bind() will not have reference on the
> ictx after the next patch. However, it gets reference only because it
> wants to store it in vfio_device. Now, it does not store it. So no get.
> I think the caller of vfio_iommufd_emulated_bind() should ensure
> the ictx is valid. Also check the physical device bind. So maybe not
> necessary to get ictx before calling iommufd_access_create(). This is
> the same with the vfio_iommufd_physical_bind() which calls
> iommufd_device_bind() without ictx get, and iommufd_device_bind()
> also gets ictx in the end.
> 

You are right. I overlooked the fact that ictx is already held by the
caller of bind.

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

On Thu, Mar 16, 2023 at 12:17:11AM +0000, Tian, Kevin wrote:

> > > > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct
> > > > iommufd_ctx

> > > > > > > >       refcount_inc(&access->obj.users);
> > > > > > > > +     mutex_init(&access->ioas_lock);
> > > > > > > >       access->ictx = ictx;
> > > > > > > >       iommufd_ctx_get(ictx);
> > > > > > >
> > > > > > > this refcnt get should be moved to the start given next patch
> > > > > > > removes the reference in the caller side.
> >
> > This change is ok but seems not necessary.
> >
> > Yes, vfio_iommufd_emulated_bind() will not have reference on the
> > ictx after the next patch. However, it gets reference only because it
> > wants to store it in vfio_device. Now, it does not store it. So no get.
> > I think the caller of vfio_iommufd_emulated_bind() should ensure
> > the ictx is valid. Also check the physical device bind. So maybe not
> > necessary to get ictx before calling iommufd_access_create(). This is
> > the same with the vfio_iommufd_physical_bind() which calls
> > iommufd_device_bind() without ictx get, and iommufd_device_bind()
> > also gets ictx in the end.
> >
> 
> You are right. I overlooked the fact that ictx is already held by the
> caller of bind.

I am dropping it then :)

Nic

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

On Wed, Mar 15, 2023 at 12:18:01PM +0000, Liu, Yi L wrote:
> > OK. Basically I followed what Jason suggested by having three
> > APIs and combined Kevin's inputs about the difference between
> > the attach/replace(). I also updated the replace changes, and
> > rebased all nesting (infrastructure, VT-d and SMMU):
> > https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023
> >
> > The major three changes for those APIs:
> > [1] This adds iommufd_access_attach() in this series:
> >     "iommufd: Create access in vfio_iommufd_emulated_bind()"
> >
> > https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dc
> > ca5ceaeb40e22b5
> > [2] This adds iommufd_access_detach() in the cdev series:
> >     "iommufd/device: Add iommufd_access_detach() API"
> >
> > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a
> > 097e2c9d9e26af4
> > [3] This adds iommufd_access_replace() in the replace series:
> >     "iommufd: Add iommufd_access_replace() API"
> >
> > https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9
> > bc2bf319b7654a8
> >
> > Please check if they look okay, so that Yi can integrate them
> > accordingly to the emulated/cdev series.
> 
> Thanks. I'll start to integrate after ack from Kevin or Jason. btw.
> Below is my latest code (rebased on top of rc-2). 😊
> 
> https://github.com/yiliu1765/iommufd/tree/wip/vfio_device_cdev_v7_candidate

Jason is travelling per his email in the iommufd group. Perhaps
Kevin can help us here. After that, we can integrate a version
and (if necessary) rework a bit after Jason comes back. Overall
I think they are pretty close to what Jason suggested.

Thanks
Nic

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, March 15, 2023 5:03 PM
> 
> Hi,
> 
> On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote:
> 
> > > So, this preparatory series will add a pair of simple attach()
> > > and detach() APIs. Then the cdev series will add the locking
> > > and the ioas_unpin stuff as a rework of the detach() API.
> 
> > > I think they can be something mingled... the sample code that
> > > I sent previously could take care of those conditions. But, I
> > > am also thinking a bit that maybe attach() does not need the
> > > locking? I can do a separate replace() function in this case.
> > >
> >
> > w/o locking then you need smp_store_release() and its pair.
> >
> > anyway it's not in perf critical path. Keeping lock for attach
> > is simpler and safe.
> 
> OK. Basically I followed what Jason suggested by having three
> APIs and combined Kevin's inputs about the difference between
> the attach/replace(). I also updated the replace changes, and
> rebased all nesting (infrastructure, VT-d and SMMU):
> https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-
> 03142023
> 
> The major three changes for those APIs:
> [1] This adds iommufd_access_attach() in this series:
>     "iommufd: Create access in vfio_iommufd_emulated_bind()"
> 
> https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcc
> a5ceaeb40e22b5

WARN_ON(!vdev->iommufd_access) in vfio_iommufd_emulated_attach_ioas()

> [2] This adds iommufd_access_detach() in the cdev series:
>     "iommufd/device: Add iommufd_access_detach() API"
> 
> https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a
> 097e2c9d9e26af4

also add a check if old_ioas exists it must equal to the new_ioas in attach.

> [3] This adds iommufd_access_replace() in the replace series:
>     "iommufd: Add iommufd_access_replace() API"
> 
> https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9b
> c2bf319b7654a8

lockdep_assert_held(&access->ioas_lock) in iommufd_access_change_pt()

cur_ioas is uninitialized in iommufd_access_change_pt(). Actually we don't
need an extra variable given only one reference to access->ioas.

similarly directly refer to access->ioas in iommufd_access_replace().

> 
> Please check if they look okay, so that Yi can integrate them
> accordingly to the emulated/cdev series.

this split looks reasonable to me. Go ahead.

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote:

> > Please check if they look okay, so that Yi can integrate them
> > accordingly to the emulated/cdev series.
> 
> this split looks reasonable to me. Go ahead.

Thanks for the review! I will address those comments and renew
those commits by the end of the day.

Thanks
Nic

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

Hi Kevin,

I've fixed the other two commits. Here is the one that I am
not sure about:

On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote:

> > [2] This adds iommufd_access_detach() in the cdev series:
> >     "iommufd/device: Add iommufd_access_detach() API"
> >
> > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a
> > 097e2c9d9e26af4
> 
> also add a check if old_ioas exists it must equal to the new_ioas in attach.

This is the commit adding detach(). And there's a check in it:
	if (WARN_ON(!access->ioas))

Do you mean having an "if (access->ioas) return -EBUSY;" line
in the commit adding attach()?

And, how should we check in the detach() if it equals to the
new_ioas in attach? Isn't the WARN_ON(!access->ioas) enough?

Thanks
Nic

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Thursday, March 16, 2023 1:33 PM
> 
> Hi Kevin,
> 
> I've fixed the other two commits. Here is the one that I am
> not sure about:
> 
> On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote:
> 
> > > [2] This adds iommufd_access_detach() in the cdev series:
> > >     "iommufd/device: Add iommufd_access_detach() API"
> > >
> > >
> https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a
> > > 097e2c9d9e26af4
> >
> > also add a check if old_ioas exists it must equal to the new_ioas in attach.
> 
> This is the commit adding detach(). And there's a check in it:
> 	if (WARN_ON(!access->ioas))
> 
> Do you mean having an "if (access->ioas) return -EBUSY;" line
> in the commit adding attach()?

if (access->ioas && access->ioas != new_ioas)
	return -EBUSY;

yes this is for attach. 

> 
> And, how should we check in the detach() if it equals to the
> new_ioas in attach? Isn't the WARN_ON(!access->ioas) enough?
> 

this is not required.

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

On Thu, Mar 16, 2023 at 05:38:41AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Thursday, March 16, 2023 1:33 PM
> >
> > Hi Kevin,
> >
> > I've fixed the other two commits. Here is the one that I am
> > not sure about:
> >
> > On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote:
> >
> > > > [2] This adds iommufd_access_detach() in the cdev series:
> > > >     "iommufd/device: Add iommufd_access_detach() API"
> > > >
> > > >
> > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a
> > > > 097e2c9d9e26af4
> > >
> > > also add a check if old_ioas exists it must equal to the new_ioas in attach.
> >
> > This is the commit adding detach(). And there's a check in it:
> >       if (WARN_ON(!access->ioas))
> >
> > Do you mean having an "if (access->ioas) return -EBUSY;" line
> > in the commit adding attach()?
> 
> if (access->ioas && access->ioas != new_ioas)
>         return -EBUSY;
> 
> yes this is for attach.

OK. For attach(), the access->ioas shouldn't be !NULL, I think.
At the point of adding attach(), the uAPI doesn't support the
replacement use case yet. And later we have a separate API for
that.

So I think it'd be just:
	if (access->ioas)
		return -EBUSY;

The reason why I didn't add it is actually because the caller
vfio_iommufd_emulated_attach_ioas() has a check of "attached"
already. Yet, it doesn't hurt to have one more in the API.

Thanks
Nic

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Tian, Kevin]

> From: Nicolin Chen
> Sent: Thursday, March 16, 2023 1:44 PM
> 
> On Thu, Mar 16, 2023 at 05:38:41AM +0000, Tian, Kevin wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Thursday, March 16, 2023 1:33 PM
> > >
> > > Hi Kevin,
> > >
> > > I've fixed the other two commits. Here is the one that I am
> > > not sure about:
> > >
> > > On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote:
> > >
> > > > > [2] This adds iommufd_access_detach() in the cdev series:
> > > > >     "iommufd/device: Add iommufd_access_detach() API"
> > > > >
> > > > >
> > >
> https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a
> > > > > 097e2c9d9e26af4
> > > >
> > > > also add a check if old_ioas exists it must equal to the new_ioas in
> attach.
> > >
> > > This is the commit adding detach(). And there's a check in it:
> > >       if (WARN_ON(!access->ioas))
> > >
> > > Do you mean having an "if (access->ioas) return -EBUSY;" line
> > > in the commit adding attach()?
> >
> > if (access->ioas && access->ioas != new_ioas)
> >         return -EBUSY;
> >
> > yes this is for attach.
> 
> OK. For attach(), the access->ioas shouldn't be !NULL, I think.
> At the point of adding attach(), the uAPI doesn't support the
> replacement use case yet. And later we have a separate API for
> that.

what about user calling attach twice in cdev?

> 
> So I think it'd be just:
> 	if (access->ioas)
> 		return -EBUSY;
> 
> The reason why I didn't add it is actually because the caller
> vfio_iommufd_emulated_attach_ioas() has a check of "attached"
> already. Yet, it doesn't hurt to have one more in the API.
> 

but here the slight difference is that in physical path we allow
attach twice to the same hwpt. they should be consistent:

	if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt)
		return -EINVAL;

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Nicolin Chen]

On Thu, Mar 16, 2023 at 05:49:20AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen
> > Sent: Thursday, March 16, 2023 1:44 PM
> >
> > On Thu, Mar 16, 2023 at 05:38:41AM +0000, Tian, Kevin wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > Sent: Thursday, March 16, 2023 1:33 PM
> > > >
> > > > Hi Kevin,
> > > >
> > > > I've fixed the other two commits. Here is the one that I am
> > > > not sure about:
> > > >
> > > > On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote:
> > > >
> > > > > > [2] This adds iommufd_access_detach() in the cdev series:
> > > > > >     "iommufd/device: Add iommufd_access_detach() API"
> > > > > >
> > > > > >
> > > >
> > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a
> > > > > > 097e2c9d9e26af4
> > > > >
> > > > > also add a check if old_ioas exists it must equal to the new_ioas in
> > attach.
> > > >
> > > > This is the commit adding detach(). And there's a check in it:
> > > >       if (WARN_ON(!access->ioas))
> > > >
> > > > Do you mean having an "if (access->ioas) return -EBUSY;" line
> > > > in the commit adding attach()?
> > >
> > > if (access->ioas && access->ioas != new_ioas)
> > >         return -EBUSY;
> > >
> > > yes this is for attach.
> >
> > OK. For attach(), the access->ioas shouldn't be !NULL, I think.
> > At the point of adding attach(), the uAPI doesn't support the
> > replacement use case yet. And later we have a separate API for
> > that.
> 
> what about user calling attach twice in cdev?
> 
> >
> > So I think it'd be just:
> >       if (access->ioas)
> >               return -EBUSY;
> >
> > The reason why I didn't add it is actually because the caller
> > vfio_iommufd_emulated_attach_ioas() has a check of "attached"
> > already. Yet, it doesn't hurt to have one more in the API.
> >
> 
> but here the slight difference is that in physical path we allow
> attach twice to the same hwpt. they should be consistent:
> 
>         if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt)
>                 return -EINVAL;

I see. The point is to support duplicated calls:
	ATTACH (pt_id = ioas1)
	ATTACH (pt_id = ioas1)

Then I will add this to keep the consistency:
	if (access->ioas != NULL && access->ioas != new_ioas)
		return -EINVAL;

Thanks
Nic

RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Liu, Yi L]

> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Thursday, March 16, 2023 1:49 PM
> 
> > From: Nicolin Chen
> > Sent: Thursday, March 16, 2023 1:44 PM
> >
> > On Thu, Mar 16, 2023 at 05:38:41AM +0000, Tian, Kevin wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > > Sent: Thursday, March 16, 2023 1:33 PM
> > > >
> > > > Hi Kevin,
> > > >
> > > > I've fixed the other two commits. Here is the one that I am
> > > > not sure about:
> > > >
> > > > On Thu, Mar 16, 2023 at 02:53:50AM +0000, Tian, Kevin wrote:
> > > >
> > > > > > [2] This adds iommufd_access_detach() in the cdev series:
> > > > > >     "iommufd/device: Add iommufd_access_detach() API"
> > > > > >
> > > > > >
> > > >
> >
> https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a
> > > > > > 097e2c9d9e26af4
> > > > >
> > > > > also add a check if old_ioas exists it must equal to the new_ioas in
> > attach.
> > > >
> > > > This is the commit adding detach(). And there's a check in it:
> > > >       if (WARN_ON(!access->ioas))
> > > >
> > > > Do you mean having an "if (access->ioas) return -EBUSY;" line
> > > > in the commit adding attach()?
> > >
> > > if (access->ioas && access->ioas != new_ioas)
> > >         return -EBUSY;
> > >
> > > yes this is for attach.
> >
> > OK. For attach(), the access->ioas shouldn't be !NULL, I think.
> > At the point of adding attach(), the uAPI doesn't support the
> > replacement use case yet. And later we have a separate API for
> > that.
> 
> what about user calling attach twice in cdev?

In the cdev series, VFIO only one attach is allowed so far. If
vfio_device->iommufd_attached is set, would return -EBUSY to user.
But this does not prevent the iommufd side to allow attach twice.
Nic's replace series would relax it if user means to replace existing
attached hwpt/ioas.

Regards,
Yi Liu

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Jason Gunthorpe]

On Wed, Mar 15, 2023 at 02:03:09AM -0700, Nicolin Chen wrote:
> Hi,
> 
> On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote:
> 
> > > So, this preparatory series will add a pair of simple attach()
> > > and detach() APIs. Then the cdev series will add the locking
> > > and the ioas_unpin stuff as a rework of the detach() API.
> 
> > > I think they can be something mingled... the sample code that
> > > I sent previously could take care of those conditions. But, I
> > > am also thinking a bit that maybe attach() does not need the
> > > locking? I can do a separate replace() function in this case.
> > >
> > 
> > w/o locking then you need smp_store_release() and its pair.
> > 
> > anyway it's not in perf critical path. Keeping lock for attach
> > is simpler and safe.
> 
> OK. Basically I followed what Jason suggested by having three
> APIs and combined Kevin's inputs about the difference between
> the attach/replace(). I also updated the replace changes, and
> rebased all nesting (infrastructure, VT-d and SMMU):
> https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023
> 
> The major three changes for those APIs:
> [1] This adds iommufd_access_attach() in this series:
>     "iommufd: Create access in vfio_iommufd_emulated_bind()"
>     https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcca5ceaeb40e22b5
> [2] This adds iommufd_access_detach() in the cdev series:
>     "iommufd/device: Add iommufd_access_detach() API"
>     https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a097e2c9d9e26af4
> [3] This adds iommufd_access_replace() in the replace series:
>     "iommufd: Add iommufd_access_replace() API"
>     https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9bc2bf319b7654a8
> 
> Please check if they look okay, so that Yi can integrate them
> accordingly to the emulated/cdev series.

I don't understand why this is being put in front of the cdev series?

Jason

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Yi Liu]

On 2023/3/20 22:49, Jason Gunthorpe wrote:
> On Wed, Mar 15, 2023 at 02:03:09AM -0700, Nicolin Chen wrote:
>> Hi,
>>
>> On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote:
>>
>>>> So, this preparatory series will add a pair of simple attach()
>>>> and detach() APIs. Then the cdev series will add the locking
>>>> and the ioas_unpin stuff as a rework of the detach() API.
>>
>>>> I think they can be something mingled... the sample code that
>>>> I sent previously could take care of those conditions. But, I
>>>> am also thinking a bit that maybe attach() does not need the
>>>> locking? I can do a separate replace() function in this case.
>>>>
>>>
>>> w/o locking then you need smp_store_release() and its pair.
>>>
>>> anyway it's not in perf critical path. Keeping lock for attach
>>> is simpler and safe.
>>
>> OK. Basically I followed what Jason suggested by having three
>> APIs and combined Kevin's inputs about the difference between
>> the attach/replace(). I also updated the replace changes, and
>> rebased all nesting (infrastructure, VT-d and SMMU):
>> https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023
>>
>> The major three changes for those APIs:
>> [1] This adds iommufd_access_attach() in this series:
>>      "iommufd: Create access in vfio_iommufd_emulated_bind()"
>>      https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcca5ceaeb40e22b5
>> [2] This adds iommufd_access_detach() in the cdev series:
>>      "iommufd/device: Add iommufd_access_detach() API"
>>      https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a097e2c9d9e26af4
>> [3] This adds iommufd_access_replace() in the replace series:
>>      "iommufd: Add iommufd_access_replace() API"
>>      https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9bc2bf319b7654a8
>>
>> Please check if they look okay, so that Yi can integrate them
>> accordingly to the emulated/cdev series.
> 
> I don't understand why this is being put in front of the cdev series?

because we want to make emulated devices have iommufd_access in the
bind, then it can return iommufd_access->obj.id to userspace when
adding cdev.

-- 
Regards,
Yi Liu

Re: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

[Jason Gunthorpe]

On Mon, Mar 20, 2023 at 11:11:51PM +0800, Yi Liu wrote:
> 
> 
> On 2023/3/20 22:49, Jason Gunthorpe wrote:
> > On Wed, Mar 15, 2023 at 02:03:09AM -0700, Nicolin Chen wrote:
> > > Hi,
> > > 
> > > On Wed, Mar 15, 2023 at 06:50:53AM +0000, Tian, Kevin wrote:
> > > 
> > > > > So, this preparatory series will add a pair of simple attach()
> > > > > and detach() APIs. Then the cdev series will add the locking
> > > > > and the ioas_unpin stuff as a rework of the detach() API.
> > > 
> > > > > I think they can be something mingled... the sample code that
> > > > > I sent previously could take care of those conditions. But, I
> > > > > am also thinking a bit that maybe attach() does not need the
> > > > > locking? I can do a separate replace() function in this case.
> > > > > 
> > > > 
> > > > w/o locking then you need smp_store_release() and its pair.
> > > > 
> > > > anyway it's not in perf critical path. Keeping lock for attach
> > > > is simpler and safe.
> > > 
> > > OK. Basically I followed what Jason suggested by having three
> > > APIs and combined Kevin's inputs about the difference between
> > > the attach/replace(). I also updated the replace changes, and
> > > rebased all nesting (infrastructure, VT-d and SMMU):
> > > https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023
> > > 
> > > The major three changes for those APIs:
> > > [1] This adds iommufd_access_attach() in this series:
> > >      "iommufd: Create access in vfio_iommufd_emulated_bind()"
> > >      https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcca5ceaeb40e22b5
> > > [2] This adds iommufd_access_detach() in the cdev series:
> > >      "iommufd/device: Add iommufd_access_detach() API"
> > >      https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a097e2c9d9e26af4
> > > [3] This adds iommufd_access_replace() in the replace series:
> > >      "iommufd: Add iommufd_access_replace() API"
> > >      https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9bc2bf319b7654a8
> > > 
> > > Please check if they look okay, so that Yi can integrate them
> > > accordingly to the emulated/cdev series.
> > 
> > I don't understand why this is being put in front of the cdev series?
> 
> because we want to make emulated devices have iommufd_access in the
> bind, then it can return iommufd_access->obj.id to userspace when
> adding cdev.

Ah OK

Jason