iommufd: Add vIOMMU infrastructure (Part-4 HW QUEUE)

The access object has been used externally by VFIO mdev devices, allowing them to pin/unpin physical pages (via needs_pin_pages). Meanwhile, a racy unmap can occur in this case, so these devices usually implement an unmap handler, invoked by iommufd_access_notify_unmap().

The new HW queue object will need the same pin/unpin feature, although it (unlike the mdev case) wants to reject any unmap attempt, during its life cycle.

To reuse the access object for the new HW queue, allow its access->ops to be NULL. Error out a NULL access->ops in iommufd_access_notify_unmap() to reject an unmap operation and propagatethe errno upwards.

Then, update iommufd_access_change_ioas() and iommufd_access_pin_pages(), to allow this new use case.

Suggested-by Jason Gunthorpe<[email protected]>

Signed-off-by Nicolin Chen<[email protected]>

drivers/iommu/iommufd/device.c int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, · line 1332

Quoted 7 lines · Nicolin Chen

drivers/iommu/iommufd/device.c +1 -1

@@ -1321,7 +1328,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 
 	/* Driver's ops don't support pin_pages */
 	if (IS_ENABLED(CONFIG_IOMMUFD_TEST) &&
-	    WARN_ON(access->iova_alignment != PAGE_SIZE || !access->ops->unmap))
+	    WARN_ON(access->iova_alignment != PAGE_SIZE))
 		return -EINVAL;

I don't want to loose this check, continuing blocking mdevs is still important. Only the internal access should be able to use this mechanism.

Quoted 12 lines · Jason Gunthorpe

drivers/iommu/iommufd/device.c int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, · line 1332

Quoted 7 lines · Nicolin Chen

drivers/iommu/iommufd/device.c +1 -1

@@ -1321,7 +1328,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 
 	/* Driver's ops don't support pin_pages */
 	if (IS_ENABLED(CONFIG_IOMMUFD_TEST) &&
-	    WARN_ON(access->iova_alignment != PAGE_SIZE || !access->ops->unmap))
+	    WARN_ON(access->iova_alignment != PAGE_SIZE))
 		return -EINVAL;

I don't want to loose this check, continuing blocking mdevs is still important. Only the internal access should be able to use this mechanism.

OK. So, it probably should be

	if (IS_ENABLED(CONFIG_IOMMUFD_TEST) &&
	    WARN_ON(access->iova_alignment != PAGE_SIZE ||
		    (access->ictx && !access->ops->unmap))

Thanks
Nicolin

From: Nicolin Chen <[email protected]>
Sent: Saturday, June 14, 2025 3:15 PM

+int iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,

drivers/iommu/iommufd/device.c +1 -0

+				unsigned long length)
 {
 	struct iommufd_ioas *ioas =
 		container_of(iopt, struct iommufd_ioas, iopt);

Quoted 9 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, June 14, 2025 3:15 PM

+int iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,

drivers/iommu/iommufd/device.c +1 -0

+				unsigned long length)
 {
 	struct iommufd_ioas *ioas =
 		container_of(iopt, struct iommufd_ioas, iopt);

drivers/iommu/iommufd/device.c EXPORT_SYMBOL_NS_GPL(iommufd_access_replace, "IOMMUFD"); · line 1221

Quoted 10 lines

drivers/iommu/iommufd/device.c +5 -0

 	struct iommufd_access *access;
 	unsigned long index;
+	int ret = 0;
 
 	xa_lock(&ioas->iopt.access_list);
 	xa_for_each(&ioas->iopt.access_list, index, access) {
+		if (!access->ops || !access->ops->unmap) {
+			ret = -EBUSY;
+			goto unlock;
+		}

then accesses before this one have been notified to unpin the area while accesses afterwards are left unnotified.

in the end the unmap fails but with some side-effect incurred.

I'm not sure whether this intermediate state may lead to any undesired effect later. Just raise it in case you or Jason already thought about it.

drivers/iommu/iommufd/io_pagetable.c static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start, · line 757

Quoted 9 lines

/* Something is not responding to unmap requests.
*/

drivers/iommu/iommufd/io_pagetable.c +4 -2

 			tries++;
-			if (WARN_ON(tries > 100))
-				return -EDEADLOCK;
+			if (WARN_ON(tries > 100)) {
+				rc = -EDEADLOCK;
+				goto out_unmapped;
+			}

this looks an unrelated fix?

Quoted 23 lines · Kevin

Quoted 9 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, June 14, 2025 3:15 PM

+int iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,

drivers/iommu/iommufd/device.c +1 -0

+				unsigned long length)
 {
 	struct iommufd_ioas *ioas =
 		container_of(iopt, struct iommufd_ioas, iopt);

drivers/iommu/iommufd/device.c EXPORT_SYMBOL_NS_GPL(iommufd_access_replace, "IOMMUFD"); · line 1221

Quoted 10 lines

drivers/iommu/iommufd/device.c +5 -0

 	struct iommufd_access *access;
 	unsigned long index;
+	int ret = 0;
 
 	xa_lock(&ioas->iopt.access_list);
 	xa_for_each(&ioas->iopt.access_list, index, access) {
+		if (!access->ops || !access->ops->unmap) {
+			ret = -EBUSY;
+			goto unlock;
+		}

then accesses before this one have been notified to unpin the area while accesses afterwards are left unnotified.

Quoted 4 lines

in the end the unmap fails but with some side-effect incurred.

I'm not sure whether this intermediate state may lead to any undesired effect later. Just raise it in case you or Jason already thought about it.

That's a good point. When an access blocks the unmap, there is no unmap happening so no point in notifying devices for ops->unmap.

And, when the function is re-entered, there could be a duplicated ops->unmap call for those devices that are already notified once?

So, if we play safe, there can be a standalone xa_for_each to dig for !access->ops->unmap. And it could be a bit cleaner to add an iommufd_access_has_internal_use() to be called under those rwsems.

Now, access->ops can be NULL, to support an internal use case for the new HW queue object. Since an access object in this case will be allocated by an inernal iommufd object, the refcount on the ictx should be skipped, so as not to deadlock the release of the ictx as it would otherwise wait for the release of the access first during the release of the internal object that could wait for the release of ictx:

    ictx --releases--> hw_queue --releases--> access
      ^                                         |
      |_________________releases________________v

Add a set of lightweight internal APIs to unlink access and ictx:

    ictx --releases--> hw_queue --releases--> access

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/iommufd_private.h |  8 ++++
 drivers/iommu/iommufd/device.c          | 59 +++++++++++++++++++++----
 2 files changed, 58 insertions(+), 9 deletions(-)

drivers/iommu/iommufd/iommufd_private.h +0 -0

index 4a375a8c9216..468717d5e5bc 100644

drivers/iommu/iommufd/iommufd_private.h +3 -0

+int iommufd_access_attach_internal(struct iommufd_access *access,
+				   struct iommufd_ioas *ioas);
+#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
 
 
 struct iommufd_eventq {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;

drivers/iommu/iommufd/device.c +0 -0

index 9293722b9cff..ad33f1e41a24 100644

Quoted 24 lines · Nicolin Chen

Now, access->ops can be NULL, to support an internal use case for the new HW queue object. Since an access object in this case will be allocated by an inernal iommufd object, the refcount on the ictx should be skipped, so as not to deadlock the release of the ictx as it would otherwise wait for the release of the access first during the release of the internal object that could wait for the release of ictx:

    ictx --releases--> hw_queue --releases--> access
      ^                                         |
      |_________________releases________________v

Add a set of lightweight internal APIs to unlink access and ictx:

    ictx --releases--> hw_queue --releases--> access

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/iommufd_private.h |  8 ++++
 drivers/iommu/iommufd/device.c          | 59 +++++++++++++++++++++----
 2 files changed, 58 insertions(+), 9 deletions(-)

drivers/iommu/iommufd/iommufd_private.h +0 -0

index 4a375a8c9216..468717d5e5bc 100644

drivers/iommu/iommufd/iommufd_private.h void iopt_remove_access(struct io_pagetable *iopt, · line 490

Quoted 7 lines

drivers/iommu/iommufd/iommufd_private.h +4 -0

@@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
 			struct iommufd_access *access, u32 iopt_access_list_id);
 void iommufd_access_destroy_object(struct iommufd_object *obj);
 
+/* iommufd_access for internal use */
+struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
+#define iommufd_access_destroy_internal(ictx, access) \
+	iommufd_object_destroy_user(ictx, &(access)->obj)

Use a static inline please

Quoted 14 lines

drivers/iommu/iommufd/iommufd_private.h +3 -0

+int iommufd_access_attach_internal(struct iommufd_access *access,
+				   struct iommufd_ioas *ioas);
+#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
 
 
 struct iommufd_eventq {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;

drivers/iommu/iommufd/device.c +0 -0

index 9293722b9cff..ad33f1e41a24 100644
@@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
 	if (access->ioas)

drivers/iommu/iommufd/device.c void iommufd_access_destroy_object(struct iommufd_object *obj) · line 1088

Quoted 5 lines

drivers/iommu/iommufd/device.c +2 -1

 		WARN_ON(iommufd_access_change_ioas(access, NULL));
 	mutex_unlock(&access->ioas_lock);
-	iommufd_ctx_put(access->ictx);
+	if (access->ops)
+		iommufd_ctx_put(access->ictx);

I was hoping we could null the ictx to signal internal? That didn't work out?

I would at least add a comment here this is filtering internal that doesn't have ictx. Maybe a little inline 'iommufd_access_is_internal' is appropriate. We'll be sad down the road if we need ops for internal.

Quoted 8 lines

Quoted 8 lines

  struct iommufd_eventq {
  	struct iommufd_object obj;
  	struct iommufd_ctx *ictx;

drivers/iommu/iommufd/device.c +0 -0

index 9293722b9cff..ad33f1e41a24 100644
@@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)

Quoted 8 lines

drivers/iommu/iommufd/device.c void iommufd_access_destroy_object(struct iommufd_object *obj) · line 1088

Quoted 6 lines

drivers/iommu/iommufd/device.c +2 -1

  	if (access->ioas)
  		WARN_ON(iommufd_access_change_ioas(access, NULL));
  	mutex_unlock(&access->ioas_lock);
-	iommufd_ctx_put(access->ictx);
+	if (access->ops)
+		iommufd_ctx_put(access->ictx);

I was hoping we could null the ictx to signal internal? That didn't work out?

access->ictx should be NULL for internal. It should have been:

Quoted 16 lines · Nicolin Chen

Quoted 16 lines

Quoted 8 lines

  struct iommufd_eventq {
  	struct iommufd_object obj;
  	struct iommufd_ctx *ictx;

drivers/iommu/iommufd/device.c +0 -0

index 9293722b9cff..ad33f1e41a24 100644

drivers/iommu/iommufd/device.c void iommufd_access_destroy_object(struct iommufd_object *obj) · line 1088

Quoted 6 lines

drivers/iommu/iommufd/device.c +2 -1

@@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
  	if (access->ioas)
  		WARN_ON(iommufd_access_change_ioas(access, NULL));
  	mutex_unlock(&access->ioas_lock);
-	iommufd_ctx_put(access->ictx);
+	if (access->ops)
+		iommufd_ctx_put(access->ictx);

I was hoping we could null the ictx to signal internal? That didn't work out?

Review

Quoted 3 lines

access->ictx should be NULL for internal. It should have been:

(quoted diff) +2 -0

+	if (access->ictx)
+		iommufd_ctx_put(access->ictx);

access->ictx could be treated as user ownership token. If it's NULL, there is no user ownership, indicating it's owned by the kernel. This is the concept here?

Quoted 49 lines · Jason Gunthorpe

Quoted 24 lines · Nicolin Chen

Now, access->ops can be NULL, to support an internal use case for the new HW queue object. Since an access object in this case will be allocated by an inernal iommufd object, the refcount on the ictx should be skipped, so as not to deadlock the release of the ictx as it would otherwise wait for the release of the access first during the release of the internal object that could wait for the release of ictx:

    ictx --releases--> hw_queue --releases--> access
      ^                                         |
      |_________________releases________________v

Add a set of lightweight internal APIs to unlink access and ictx:

    ictx --releases--> hw_queue --releases--> access

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/iommufd_private.h |  8 ++++
 drivers/iommu/iommufd/device.c          | 59 +++++++++++++++++++++----
 2 files changed, 58 insertions(+), 9 deletions(-)

drivers/iommu/iommufd/iommufd_private.h +0 -0

index 4a375a8c9216..468717d5e5bc 100644

drivers/iommu/iommufd/iommufd_private.h void iopt_remove_access(struct io_pagetable *iopt, · line 490

Quoted 7 lines

drivers/iommu/iommufd/iommufd_private.h +4 -0

@@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
 			struct iommufd_access *access, u32 iopt_access_list_id);
 void iommufd_access_destroy_object(struct iommufd_object *obj);
 
+/* iommufd_access for internal use */
+struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
+#define iommufd_access_destroy_internal(ictx, access) \
+	iommufd_object_destroy_user(ictx, &(access)->obj)

Use a static inline please

Quoted 14 lines

drivers/iommu/iommufd/iommufd_private.h +3 -0

+int iommufd_access_attach_internal(struct iommufd_access *access,
+				   struct iommufd_ioas *ioas);
+#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
 
 
 struct iommufd_eventq {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;

drivers/iommu/iommufd/device.c +0 -0

index 9293722b9cff..ad33f1e41a24 100644
@@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
 	if (access->ioas)

Quoted 8 lines

drivers/iommu/iommufd/device.c void iommufd_access_destroy_object(struct iommufd_object *obj) · line 1088

Quoted 5 lines

drivers/iommu/iommufd/device.c +2 -1

 		WARN_ON(iommufd_access_change_ioas(access, NULL));
 	mutex_unlock(&access->ioas_lock);
-	iommufd_ctx_put(access->ictx);
+	if (access->ops)
+		iommufd_ctx_put(access->ictx);

I was hoping we could null the ictx to signal internal? That didn't work out?

access->ictx should be NULL for internal. It should have been:

Quoted 4 lines

I would at least add a comment here this is filtering internal that doesn't have ictx. Maybe a little inline 'iommufd_access_is_internal' is appropriate. We'll be sad down the road if we need ops for internal.

Yea, an inline will be cleaner. Will add that.

Now, access->ops can be NULL, to support an internal use case for the new HW queue object. Since an access object in this case will be allocated by an inernal iommufd object, the refcount on the ictx should be skipped, so as not to deadlock the release of the ictx as it would otherwise wait for the release of the access first during the release of the internal object that could wait for the release of ictx:

    ictx --releases--> hw_queue --releases--> access
      ^                                         |
      |_________________releases________________v

Add a set of lightweight internal APIs to unlink access and ictx:

    ictx --releases--> hw_queue --releases--> access

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/iommufd_private.h |  8 ++++
 drivers/iommu/iommufd/device.c          | 59 +++++++++++++++++++++----
 2 files changed, 58 insertions(+), 9 deletions(-)

drivers/iommu/iommufd/iommufd_private.h +8 -0

index 4a375a8c9216..468717d5e5bc 100644
@@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
 			struct iommufd_access *access, u32 iopt_access_list_id);
 void iommufd_access_destroy_object(struct iommufd_object *obj);
 
+/* iommufd_access for internal use */
+struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
+#define iommufd_access_destroy_internal(ictx, access) \
+	iommufd_object_destroy_user(ictx, &(access)->obj)
+int iommufd_access_attach_internal(struct iommufd_access *access,
+				   struct iommufd_ioas *ioas);
+#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
+
 struct iommufd_eventq {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;

drivers/iommu/iommufd/device.c +0 -0

index 9293722b9cff..ad33f1e41a24 100644

drivers/iommu/iommufd/device.c +18 -0

+}
+
+static struct iommufd_access *__iommufd_access_create(struct iommufd_ctx *ictx)
+{
+	struct iommufd_access *access;
+
+	/*
+	 * 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))
+		return access;
+
+	/* The calling driver is a user until iommufd_access_destroy() */
+	refcount_inc(&access->obj.users);
+	mutex_init(&access->ioas_lock);
+	return access;

drivers/iommu/iommufd/device.c +20 -8

+
+	iommufd_object_finalize(ictx, &access->obj);
+	return access;
 }
 
 /**

@@ -1106,11 +1138,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
 {
 	struct iommufd_access *access;
 
-	/*
-	 * 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);
+	access = __iommufd_access_create(ictx);
 	if (IS_ERR(access))
 		return access;
 

@@ -1122,13 +1150,10 @@ iommufd_access_create(struct iommufd_ctx *ictx,
 	else
 		access->iova_alignment = 1;
 
-	/* 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);
 	*id = access->obj.id;
-	mutex_init(&access->ioas_lock);
 	return access;
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_create, "IOMMUFD");

@@ -1173,6 +1198,22 @@ int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, "IOMMUFD");
 
+int iommufd_access_attach_internal(struct iommufd_access *access,
+				   struct iommufd_ioas *ioas)
+{
+	int rc;
+
+	mutex_lock(&access->ioas_lock);
+	if (WARN_ON(access->ioas)) {
+		mutex_unlock(&access->ioas_lock);
+		return -EINVAL;
+	}
+
+	rc = iommufd_access_change_ioas(access, ioas);
+	mutex_unlock(&access->ioas_lock);
+	return rc;
+}
+
 int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id)
 {
 	int rc;

Signature · 1 line

2.43.0

NVIDIA VCMDQ driver will have a driver-defined vDEVICE structure and do some HW configurations with that.

To allow IOMMU drivers to define their own vDEVICE structures, move the struct iommufd_vdevice to the public header and provide a pair of viommu ops, similar to get_viommu_size and viommu_init.

Doing this, however, creates a new window between the vDEVICE allocation and its driver-level initialization, during which an abort could happen but it can't invoke a driver destroy function from the struct viommu_ops since the driver structure isn't initialized yet. vIOMMU object doesn't have this problem, since its destroy op is set via the viommu_ops by the driver viommu_init function. Thus, vDEVICE should do something similar:
add a destroy function pointer inside the struct iommufd_vdevice instead of the struct iommufd_viommu_ops.

Note that there is unlikely a use case for a type dependent vDEVICE, so a static vdevice_size is probably enough for the near term instead of a get_vdevice_size function op.

Reviewed-by Jason Gunthorpe<[email protected]>

Reviewed-by Kevin Tian<[email protected]>

Signed-off-by Nicolin Chen<[email protected]>

NVIDIA VCMDQ driver will have a driver-defined vDEVICE structure and do some HW configurations with that.

To allow IOMMU drivers to define their own vDEVICE structures, move the struct iommufd_vdevice to the public header and provide a pair of viommu ops, similar to get_viommu_size and viommu_init.

Doing this, however, creates a new window between the vDEVICE allocation and its driver-level initialization, during which an abort could happen but it can't invoke a driver destroy function from the struct viommu_ops since the driver structure isn't initialized yet. vIOMMU object doesn't have this problem, since its destroy op is set via the viommu_ops by the driver viommu_init function. Thus, vDEVICE should do something similar:
add a destroy function pointer inside the struct iommufd_vdevice instead of the struct iommufd_viommu_ops.

Note that there is unlikely a use case for a type dependent vDEVICE, so a static vdevice_size is probably enough for the near term instead of a get_vdevice_size function op.

Reviewed-by Jason Gunthorpe <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/iommufd_private.h |  7 -------
 include/linux/iommufd.h                 | 26 +++++++++++++++++++++++++
 drivers/iommu/iommufd/viommu.c          | 26 ++++++++++++++++++++++++-
 3 files changed, 51 insertions(+), 8 deletions(-)

drivers/iommu/iommufd/iommufd_private.h +0 -7

index 468717d5e5bc..e6b1eb2ab375 100644
@@ -638,13 +638,6 @@ void iommufd_viommu_destroy(struct iommufd_object *obj);
 int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd);
 void iommufd_vdevice_destroy(struct iommufd_object *obj);
 
-struct iommufd_vdevice {
-	struct iommufd_object obj;
-	struct iommufd_viommu *viommu;
-	struct device *dev;
-	u64 id; /* per-vIOMMU virtual ID */
-};
-
 #ifdef CONFIG_IOMMUFD_TEST
 int iommufd_test(struct iommufd_ucmd *ucmd);
 void iommufd_selftest_destroy(struct iommufd_object *obj);

include/linux/iommufd.h +0 -0

index 2d1bf2f97ee3..f3b5cfdb6d53 100644

include/linux/iommufd.h +21 -0

+
+	/* Clean up all driver-specific parts of an iommufd_vdevice */
+	void (*destroy)(struct iommufd_vdevice *vdev);
+};
+
 /**
  * struct iommufd_viommu_ops - vIOMMU specific operations
  * @destroy: Clean up all driver-specific parts of an iommufd_viommu. The memory

@@ -120,6 +130,14 @@ struct iommufd_viommu {
  *                    array->entry_num to report the number of handled requests.
  *                    The data structure of the array entry must be defined in
  *                    include/uapi/linux/iommufd.h
+ * @vdevice_size: Size of the driver-defined vDEVICE structure per this vIOMMU
+ * @vdevice_init: Initialize the driver-level structure of a vDEVICE object, or
+ *                related HW procedure. @vdev is already initialized by iommufd
+ *                core: vdev->dev and vdev->viommu pointers; vdev->id carries a
+ *                per-vIOMMU virtual ID (refer to struct iommu_vdevice_alloc in
+ *                include/uapi/linux/iommufd.h)
+ *                If driver has a deinit function to revert what vdevice_init op
+ *                does, it should set it to the @vdev->destroy function pointer
  */
 struct iommufd_viommu_ops {
 	void (*destroy)(struct iommufd_viommu *viommu);

@@ -128,6 +146,8 @@ struct iommufd_viommu_ops {
 		const struct iommu_user_data *user_data);
 	int (*cache_invalidate)(struct iommufd_viommu *viommu,
 				struct iommu_user_data_array *array);
+	const size_t vdevice_size;
+	int (*vdevice_init)(struct iommufd_vdevice *vdev);
 };
 
 #if IS_ENABLED(CONFIG_IOMMUFD)

@@ -224,4 +244,10 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 	 BUILD_BUG_ON_ZERO(offsetof(drv_struct, member)) +                     \
 	 BUILD_BUG_ON_ZERO(!__same_type(struct iommufd_viommu,                 \
 					((drv_struct *)NULL)->member)))
+
+#define VDEVICE_STRUCT_SIZE(drv_struct, member)                                \
+	(sizeof(drv_struct) +                                                  \
+	 BUILD_BUG_ON_ZERO(offsetof(drv_struct, member)) +                     \
+	 BUILD_BUG_ON_ZERO(!__same_type(struct iommufd_vdevice,                \
+					((drv_struct *)NULL)->member)))
 #endif

drivers/iommu/iommufd/viommu.c +19 -1

index c5eea9900c54..28ea5d026222 100644
@@ -116,6 +116,8 @@ void iommufd_vdevice_destroy(struct iommufd_object *obj)
 		container_of(obj, struct iommufd_vdevice, obj);
 	struct iommufd_viommu *viommu = vdev->viommu;
 
+	if (vdev->destroy)
+		vdev->destroy(vdev);
 	/* xa_cmpxchg is okay to fail if alloc failed xa_cmpxchg previously */
 	xa_cmpxchg(&viommu->vdevs, vdev->id, vdev, NULL, GFP_KERNEL);
 	refcount_dec(&viommu->obj.users);

@@ -126,6 +128,7 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 {
 	struct iommu_vdevice_alloc *cmd = ucmd->cmd;
 	struct iommufd_vdevice *vdev, *curr;
+	size_t vdev_size = sizeof(*vdev);
 	struct iommufd_viommu *viommu;
 	struct iommufd_device *idev;
 	u64 virt_id = cmd->virt_id;

@@ -150,7 +153,22 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		goto out_put_idev;
 	}
 
-	vdev = iommufd_object_alloc_ucmd(ucmd, vdev, IOMMUFD_OBJ_VDEVICE);
+	if (viommu->ops && viommu->ops->vdevice_size) {
+		/*
+		 * It is a driver bug for:
+		 * - ops->vdevice_size smaller than the core structure size
+		 * - not implementing a pairing ops->vdevice_init op
+		 */
+		if (WARN_ON_ONCE(viommu->ops->vdevice_size < vdev_size ||
+				 !viommu->ops->vdevice_init)) {
+			rc = -EOPNOTSUPP;
+			goto out_put_idev;
+		}
+		vdev_size = viommu->ops->vdevice_size;
+	}
+
+	vdev = (struct iommufd_vdevice *)_iommufd_object_alloc_ucmd(
+		ucmd, vdev_size, IOMMUFD_OBJ_VDEVICE);
 	if (IS_ERR(vdev)) {
 		rc = PTR_ERR(vdev);
 		goto out_put_idev;

@@ -168,6 +186,12 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		goto out_put_idev;
 	}

Signature · 1 line

2.43.0

include/linux/iommufd.h struct iommufd_viommu { · line 111

Quoted 5 lines

include/linux/iommufd.h +5 -0

+struct iommufd_vdevice {
+	struct iommufd_object obj;
+	struct iommufd_viommu *viommu;
+	struct device *dev;
+	u64 id; /* per-vIOMMU virtual ID */

Nit: Why not call this viommu_id?

Quoted 7 lines · Pranjal Shrivastava

include/linux/iommufd.h struct iommufd_viommu { · line 111

Quoted 5 lines

include/linux/iommufd.h +5 -0

+struct iommufd_vdevice {
+	struct iommufd_object obj;
+	struct iommufd_viommu *viommu;
+	struct device *dev;
+	u64 id; /* per-vIOMMU virtual ID */

Nit: Why not call this viommu_id?

And lets have a comment here too, we have so many different ids, what id is this id and what it is used for, by who.

include/linux/iommufd.h struct iommufd_vdevice { · line 120

Quoted 3 lines · Nicolin Chen

include/linux/iommufd.h +3 -0

+struct iommufd_hw_queue {
+	struct iommufd_object obj;
+	struct iommufd_ctx *ictx;

Try to avoid this if we don't need it

Introduce a new IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl for user space to allocate a HW QUEUE object for a vIOMMU specific HW-accelerated queue, e.g.:
- NVIDIA's Virtual Command Queue
- AMD vIOMMU's Command Buffer, Event Log Buffers, and PPR Log Buffers

Since this is introduced with NVIDIA's VCMDQs that access the guest memory in the physical address space, add an iommufd_hw_queue_alloc_phys() helper that will create an access object to the queue memory in the IOAS, to avoid the mappings of the guest memory from being unmapped, during the life cycle of the HW queue object.

Reviewed-by Pranjal Shrivastava<[email protected]>

Reviewed-by Kevin Tian<[email protected]>

Signed-off-by Nicolin Chen<[email protected]>

---
  drivers/iommu/iommufd/iommufd_private.h |   2 +
  include/linux/iommufd.h                 |   1 +
  include/uapi/linux/iommufd.h            |  33 +++++
  drivers/iommu/iommufd/main.c            |   6 +
  drivers/iommu/iommufd/viommu.c          | 184 ++++++++++++++++++++++++
  5 files changed, 226 insertions(+)

drivers/iommu/iommufd/viommu.c +139 -0

index 28ea5d026222..506479ece826 100644
@@ -201,3 +201,187 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
  	iommufd_put_object(ucmd->ictx, &viommu->obj);
  	return rc;
  }
+
+static void iommufd_hw_queue_destroy_access(struct iommufd_ctx *ictx,
+					    struct iommufd_access *access,
+					    u64 base_iova, size_t length)
+{
+	iommufd_access_unpin_pages(access, base_iova, length);
+	iommufd_access_detach_internal(access);
+	iommufd_access_destroy_internal(ictx, access);
+}
+
+void iommufd_hw_queue_destroy(struct iommufd_object *obj)
+{
+	struct iommufd_hw_queue *hw_queue =
+		container_of(obj, struct iommufd_hw_queue, obj);
+	struct iommufd_viommu *viommu = hw_queue->viommu;
+
+	if (hw_queue->destroy)
+		hw_queue->destroy(hw_queue);
+	if (hw_queue->access)
+		iommufd_hw_queue_destroy_access(viommu->ictx, hw_queue->access,
+						hw_queue->base_addr,
+						hw_queue->length);
+	refcount_dec(&viommu->obj.users);
+}
+
+/*
+ * When the HW accesses the guest queue via physical addresses, the underlying
+ * physical pages of the guest queue must be contiguous. Also, for the security
+ * concern that IOMMUFD_CMD_IOAS_UNMAP could potentially remove the mappings of
+ * the guest queue from the nesting parent iopt while the HW is still accessing
+ * the guest queue memory physically, such a HW queue must require an access to
+ * pin the underlying pages and prevent that from happening.
+ */
+static struct iommufd_access *
+iommufd_hw_queue_alloc_phys(struct iommu_hw_queue_alloc *cmd,
+			    struct iommufd_viommu *viommu, phys_addr_t *base_pa)
+{
+	struct iommufd_access *access;
+	struct page **pages;
+	int max_npages, i;
+	u64 offset;
+	int rc;
+
+	offset =
+		cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova);
+	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);
+
+	/*
+	 * FIXME allocation may fail when sizeof(*pages) * max_npages is
+	 * larger than PAGE_SIZE. This might need a new API returning a
+	 * bio_vec or something more efficient.
+	 */
+	pages = kcalloc(max_npages, sizeof(*pages), GFP_KERNEL);
+	if (!pages)
+		return ERR_PTR(-ENOMEM);
+
+	access = iommufd_access_create_internal(viommu->ictx);
+	if (IS_ERR(access)) {
+		rc = PTR_ERR(access);
+		goto out_free;
+	}
+
+	rc = iommufd_access_attach_internal(access, viommu->hwpt->ioas);
+	if (rc)
+		goto out_destroy;
+
+	rc = iommufd_access_pin_pages(access, cmd->nesting_parent_iova,
+				      cmd->length, pages, 0);
+	if (rc)
+		goto out_detach;
+
+	/* Validate if the underlying physical pages are contiguous */
+	for (i = 1; i < max_npages; i++) {
+		if (page_to_pfn(pages[i]) == page_to_pfn(pages[i - 1]) + 1)
+			continue;
+		rc = -EFAULT;
+		goto out_unpin;
+	}
+
+	*base_pa = page_to_pfn(pages[0]) << PAGE_SHIFT;
+	kfree(pages);
+	return access;
+
+out_unpin:
+	iommufd_access_unpin_pages(access, cmd->nesting_parent_iova,
+				   cmd->length);
+out_detach:
+	iommufd_access_detach_internal(access);
+out_destroy:
+	iommufd_access_destroy_internal(viommu->ictx, access);
+out_free:
+	kfree(pages);
+	return ERR_PTR(rc);
+}
+
+int iommufd_hw_queue_alloc_ioctl(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_hw_queue_alloc *cmd = ucmd->cmd;
+	struct iommufd_hw_queue *hw_queue;
+	struct iommufd_viommu *viommu;
+	struct iommufd_access *access;
+	size_t hw_queue_size;
+	phys_addr_t base_pa;
+	u64 last;
+	int rc;
+
+	if (cmd->flags || cmd->type == IOMMU_HW_QUEUE_TYPE_DEFAULT)
+		return -EOPNOTSUPP;
+	if (!cmd->length)
+		return -EINVAL;
+	if (check_add_overflow(cmd->nesting_parent_iova, cmd->length - 1,
+			       &last))
+		return -EOVERFLOW;
+
+	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
+	if (IS_ERR(viommu))
+		return PTR_ERR(viommu);
+
+	if (!viommu->ops || !viommu->ops->get_hw_queue_size ||
+	    !viommu->ops->hw_queue_init_phys) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}
+
+	/*
+	 * FIXME once ops->hw_queue_init is introduced, a WARN_ON_ONCE will be
+	 * required, if hw_queue_init and hw_queue_init_phys both exist, since
+	 * they should be mutually exclusive
+	 */
+
+	hw_queue_size = viommu->ops->get_hw_queue_size(viommu, cmd->type);
+	if (!hw_queue_size) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}
+
+	/*
+	 * It is a driver bug for providing a hw_queue_size smaller than the
+	 * core HW queue structure size

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 326

Quoted 5 lines · Nicolin Chen

drivers/iommu/iommufd/viommu.c +5 -0

+	if (!viommu->ops || !viommu->ops->get_hw_queue_size ||
+	    !viommu->ops->hw_queue_init_phys) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 352

Quoted 4 lines

drivers/iommu/iommufd/viommu.c +4 -0

+	/*
+	 * FIXME once ops->hw_queue_init is introduced, this should check "if
+	 * ops->hw_queue_init_phys". And "access" should be initialized to NULL.
+	 */

I just don't follow here. Up until now, only viommu->ops-> hw_queue_init_phys has been added, which means the current code only supports hardware queues that access guest memory using physical addresses. The access object is not needed for the other type of hardware queue that uses guest IOVA.

So, why not just abort here if ops->hw_queue_init_phys is not supported by the IOMMU driver?

Leave other logics to the patches that introduce ops->hw_queue_init? I guess that would make this patch more readible.

Quoted 6 lines · Baolu Lu

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 326

Quoted 5 lines · Nicolin Chen

drivers/iommu/iommufd/viommu.c +5 -0

+	if (!viommu->ops || !viommu->ops->get_hw_queue_size ||
+	    !viommu->ops->hw_queue_init_phys) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}

Hmm, here it does abort when !viommu->ops->hw_queue_init_phys ..

Quoted 10 lines

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 352

Quoted 4 lines

drivers/iommu/iommufd/viommu.c +4 -0

+	/*
+	 * FIXME once ops->hw_queue_init is introduced, this should check "if
+	 * ops->hw_queue_init_phys". And "access" should be initialized to NULL.
+	 */

I just don't follow here. Up until now, only viommu->ops-> hw_queue_init_phys has been added, which means the current code only supports hardware queues that access guest memory using physical addresses. The access object is not needed for the other type of hardware queue that uses guest IOVA.

Quoted 2 lines

So, why not just abort here if ops->hw_queue_init_phys is not supported by the IOMMU driver?

.. so, it already does.

Quoted 2 lines

Leave other logics to the patches that introduce ops->hw_queue_init? I guess that would make this patch more readible.

The patch is doing in the way to support the hw_queue_init_phys case only. It is just adding some extra FIXMEs as the guideline for the future patch adding hw_queue_init op.

I personally don't feel these are confusing. Maybe you can help point out what specific wording feels odd here? Maybe "FIXME"s should be "TODO"s?

I could also drop all of these guideline if they are considered very unnecessary.

Thanks
Nicolin

Quoted 29 lines · Nicolin Chen

Quoted 6 lines · Baolu Lu

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 326

Quoted 5 lines · Nicolin Chen

drivers/iommu/iommufd/viommu.c +5 -0

+	if (!viommu->ops || !viommu->ops->get_hw_queue_size ||
+	    !viommu->ops->hw_queue_init_phys) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}

Hmm, here it does abort when !viommu->ops->hw_queue_init_phys ..

Quoted 10 lines

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 352

Quoted 4 lines

drivers/iommu/iommufd/viommu.c +4 -0

+	/*
+	 * FIXME once ops->hw_queue_init is introduced, this should check "if
+	 * ops->hw_queue_init_phys". And "access" should be initialized to NULL.
+	 */

I just don't follow here. Up until now, only viommu->ops-> hw_queue_init_phys has been added, which means the current code only supports hardware queues that access guest memory using physical addresses. The access object is not needed for the other type of hardware queue that uses guest IOVA.

Quoted 2 lines

So, why not just abort here if ops->hw_queue_init_phys is not supported by the IOMMU driver?

.. so, it already does.

Quoted 2 lines

Leave other logics to the patches that introduce ops->hw_queue_init? I guess that would make this patch more readible.

The patch is doing in the way to support the hw_queue_init_phys case only. It is just adding some extra FIXMEs as the guideline for the future patch adding hw_queue_init op.

Quoted 3 lines

I personally don't feel these are confusing. Maybe you can help point out what specific wording feels odd here? Maybe "FIXME"s should be "TODO"s?

Oh, I probably misunderstood the logic here. For both hw_queue_init and hw_queue_init_phys, using an access object to pin the pages for hardware access is necessary, right? My understanding was that pinning pages is only required for hw_queue_init_phys.

Quoted 30 lines · Baolu Lu

Quoted 29 lines · Nicolin Chen

Quoted 6 lines · Baolu Lu

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 326

Quoted 5 lines · Nicolin Chen

drivers/iommu/iommufd/viommu.c +5 -0

+	if (!viommu->ops || !viommu->ops->get_hw_queue_size ||
+	    !viommu->ops->hw_queue_init_phys) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}

Hmm, here it does abort when !viommu->ops->hw_queue_init_phys ..

Quoted 10 lines

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 352

Quoted 4 lines

drivers/iommu/iommufd/viommu.c +4 -0

+	/*
+	 * FIXME once ops->hw_queue_init is introduced, this should check "if
+	 * ops->hw_queue_init_phys". And "access" should be initialized to NULL.
+	 */

I just don't follow here. Up until now, only viommu->ops-> hw_queue_init_phys has been added, which means the current code only supports hardware queues that access guest memory using physical addresses. The access object is not needed for the other type of hardware queue that uses guest IOVA.

Quoted 2 lines

So, why not just abort here if ops->hw_queue_init_phys is not supported by the IOMMU driver?

.. so, it already does.

Quoted 2 lines

Leave other logics to the patches that introduce ops->hw_queue_init? I guess that would make this patch more readible.

The patch is doing in the way to support the hw_queue_init_phys case only. It is just adding some extra FIXMEs as the guideline for the future patch adding hw_queue_init op.

Quoted 7 lines

Quoted 3 lines

I personally don't feel these are confusing. Maybe you can help point out what specific wording feels odd here? Maybe "FIXME"s should be "TODO"s?

Oh, I probably misunderstood the logic here. For both hw_queue_init and hw_queue_init_phys, using an access object to pin the pages for hardware access is necessary, right? My understanding was that pinning pages is only required for hw_queue_init_phys.

No. The access is only used by the ops->hw_queue_init_phys case.

The ops->hw_queue_init case will use the cmd->nesting_parent_iova directly without calling iommufd_hw_queue_alloc_phys().

This FIXME means that, when adding ops->hw_queue_init, add this:

(quoted diff) +3 -2

-	struct iommufd_access *access;
+	struct iommufd_access *access = NULL;
...
-	access = iommufd_hw_queue_alloc_phys(cmd, viommu, &base_pa);
+	if (ops->hw_queue_init_phys) {
+		access = iommufd_hw_queue_alloc_phys(cmd, viommu, &base_pa);

Also, the other FIXME guideline states that these two ops should be mutually exclusive. So, add this too:

From: Nicolin Chen <[email protected]>
Sent: Monday, June 16, 2025 2:47 PM

Quoted 2 lines

Leave other logics to the patches that introduce ops->hw_queue_init? I guess that would make this patch more readible.

The patch is doing in the way to support the hw_queue_init_phys case only. It is just adding some extra FIXMEs as the guideline for the future patch adding hw_queue_init op.

I personally don't feel these are confusing. Maybe you can help point out what specific wording feels odd here? Maybe "FIXME"s should be "TODO"s?

I could also drop all of these guideline if they are considered very unnecessary.

Quoted 13 lines

From: Nicolin Chen <[email protected]>
Sent: Monday, June 16, 2025 2:47 PM

Quoted 2 lines

Leave other logics to the patches that introduce ops->hw_queue_init? I guess that would make this patch more readible.

The patch is doing in the way to support the hw_queue_init_phys case only. It is just adding some extra FIXMEs as the guideline for the future patch adding hw_queue_init op.

I personally don't feel these are confusing. Maybe you can help point out what specific wording feels odd here? Maybe "FIXME"s should be "TODO"s?

Quoted 3 lines

I could also drop all of these guideline if they are considered very unnecessary.

Probably you can put some info in the function comment instead of being scattered in the code? anyway when the 2nd method is added in future we'll review and make sure it aligned with the caveat here. 😊

Introduce a new IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl for user space to allocate a HW QUEUE object for a vIOMMU specific HW-accelerated queue, e.g.:
- NVIDIA's Virtual Command Queue
- AMD vIOMMU's Command Buffer, Event Log Buffers, and PPR Log Buffers

Since this is introduced with NVIDIA's VCMDQs that access the guest memory in the physical address space, add an iommufd_hw_queue_alloc_phys() helper that will create an access object to the queue memory in the IOAS, to avoid the mappings of the guest memory from being unmapped, during the life cycle of the HW queue object.

Reviewed-by Pranjal Shrivastava<[email protected]>

Reviewed-by Kevin Tian<[email protected]>

Signed-off-by Nicolin Chen<[email protected]>

From: Nicolin Chen <[email protected]>
Sent: Saturday, June 14, 2025 3:15 PM

drivers/iommu/iommufd/viommu.c +3 -0

+
+	offset =
+		cmd->nesting_parent_iova - PAGE_ALIGN(cmd-

Quoted 1 line

nesting_parent_iova);

drivers/iommu/iommufd/viommu.c +1 -0

+	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, June 14, 2025 3:15 PM

drivers/iommu/iommufd/viommu.c +3 -0

+
+	offset =
+		cmd->nesting_parent_iova - PAGE_ALIGN(cmd-

Quoted 1 line

nesting_parent_iova);

drivers/iommu/iommufd/viommu.c int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 259

Quoted 11 lines

drivers/iommu/iommufd/viommu.c +11 -0

+	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);
+
+	/*
+	 * FIXME allocation may fail when sizeof(*pages) * max_npages is
+	 * larger than PAGE_SIZE. This might need a new API returning a
+	 * bio_vec or something more efficient.
+	 */
+	pages = kcalloc(max_npages, sizeof(*pages), GFP_KERNEL);
+	if (!pages)
+		return ERR_PTR(-ENOMEM);
+

any allocation may fail... can you elaborate more here? How does PAGE_SIZE become a boundary?

NVIDIA Virtual Command Queue is one of the iommufd users exposing vIOMMU features to user space VMs. Its hardware has a strict rule when mapping and unmapping multiple global CMDQVs to/from a VM-owned VINTF, requiring mappings in ascending order and unmappings in descending order.

The tegra241-cmdqv driver can apply the rule for a mapping in the LVCMDQ allocation handler. However, it can't do the same for an unmapping since user space could start random destroy calls breaking the rule, while the destroy op in the driver level can't reject a destroy call as it returns void.

Add iommufd_hw_queue_depend/undepend for-driver helpers, allowing LVCMDQ allocator to refcount_inc() a sibling LVCMDQ object and LVCMDQ destroyer to refcount_dec(), so that iommufd core will help block a random destroy call that breaks the rule.

This is a bit of compromise, because a driver might end up with abusing the API that deadlocks the objects. So restrict the API to a dependency between two driver-allocated objects of the same type, as iommufd would unlikely build any core-level dependency in this case. And encourage to use the macro version that currently supports the HW QUEUE objects only.

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Pranjal Shrivastava <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 include/linux/iommufd.h        | 44 ++++++++++++++++++++++++++++++++++
 drivers/iommu/iommufd/driver.c | 28 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)

For vIOMMU passing through HW resources to user space (VMs), allowing a VM to control the passed through HW directly by accessing hardware registers, add an mmap infrastructure to map the physical MMIO pages to user space.

Maintain a maple tree per ictx as a translation table managing mmappable regions, from an allocated for-user mmap offset to an iommufd_mmap struct, where it stores the real PFN range for an io_remap_pfn_range call.

Keep track of the lifecycle of the mmappable region by taking refcount of its owner, so as to enforce user space to unmap the region first before it can destroy its owner object.

To allow an IOMMU driver to add and delete mmappable regions onto/from the maple tree, add iommufd_viommu_alloc/destroy_mmap helpers.

Reviewed-by Kevin Tian<[email protected]>

Signed-off-by Nicolin Chen<[email protected]>

drivers/iommu/iommufd/iommufd_private.h struct iommufd_ctx { · line 65

Quoted 6 lines · Nicolin Chen

drivers/iommu/iommufd/iommufd_private.h +6 -0

+/* Entry for iommufd_ctx::mt_mmap */
+struct iommufd_mmap {
+	struct iommufd_object *owner;
+
+	/* Allocated start position in mt_mmap tree */
+	unsigned long startp;

pgoff_t, looks like this is already in PAGE_SIZE units.

drivers/iommu/iommufd/iommufd_private.h struct iommufd_ctx { · line 68

Quoted 2 lines

drivers/iommu/iommufd/iommufd_private.h +2 -0

+	/* Physical range for io_remap_pfn_range() */
+	unsigned long mmio_pfn;

physaddr_t and maybe don't use pfn?

drivers/iommu/iommufd/iommufd_private.h struct iommufd_ctx { · line 69

Quoted 1 line

drivers/iommu/iommufd/iommufd_private.h +1 -0

+	unsigned long num_pfns;

size_t

Quoted 16 lines · Jason Gunthorpe

drivers/iommu/iommufd/iommufd_private.h struct iommufd_ctx { · line 65

Quoted 6 lines · Nicolin Chen

drivers/iommu/iommufd/iommufd_private.h +6 -0

+/* Entry for iommufd_ctx::mt_mmap */
+struct iommufd_mmap {
+	struct iommufd_object *owner;
+
+	/* Allocated start position in mt_mmap tree */
+	unsigned long startp;

pgoff_t, looks like this is already in PAGE_SIZE units.

drivers/iommu/iommufd/iommufd_private.h struct iommufd_ctx { · line 68

Quoted 2 lines

drivers/iommu/iommufd/iommufd_private.h +2 -0

+	/* Physical range for io_remap_pfn_range() */
+	unsigned long mmio_pfn;

physaddr_t and maybe don't use pfn?

Quoted 3 lines

drivers/iommu/iommufd/iommufd_private.h struct iommufd_ctx { · line 69

Quoted 1 line

drivers/iommu/iommufd/iommufd_private.h +1 -0

+	unsigned long num_pfns;

size_t

The mtree stores in pfn units. Looks like you prefer doing in PAGE_SIZE units? I can give that a try and update them as you suggested.

Review

Quoted 6 lines · Nicolin Chen

(quoted diff) +6 -0

+/* Entry for iommufd_ctx::mt_mmap */
+struct iommufd_mmap {
+	struct iommufd_object *owner;
+
+	/* Allocated start position in mt_mmap tree */
+	unsigned long startp;

pgoff_t, looks like this is already in PAGE_SIZE units.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	/* Physical range for io_remap_pfn_range() */
+	unsigned long mmio_pfn;

physaddr_t and maybe don't use pfn?

Review

Quoted 1 line

(quoted diff) +1 -0

+	unsigned long num_pfns;

size_t

For vIOMMU passing through HW resources to user space (VMs), allowing a VM to control the passed through HW directly by accessing hardware registers, add an mmap infrastructure to map the physical MMIO pages to user space.

Maintain a maple tree per ictx as a translation table managing mmappable regions, from an allocated for-user mmap offset to an iommufd_mmap struct, where it stores the real PFN range for an io_remap_pfn_range call.

Keep track of the lifecycle of the mmappable region by taking refcount of its owner, so as to enforce user space to unmap the region first before it can destroy its owner object.

To allow an IOMMU driver to add and delete mmappable regions onto/from the maple tree, add iommufd_viommu_alloc/destroy_mmap helpers.

Reviewed-by Kevin Tian <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/iommufd_private.h | 14 ++++++
 include/linux/iommufd.h                 | 42 ++++++++++++++++
 drivers/iommu/iommufd/driver.c          | 51 ++++++++++++++++++++
 drivers/iommu/iommufd/main.c            | 64 +++++++++++++++++++++++++
 4 files changed, 171 insertions(+)

drivers/iommu/iommufd/iommufd_private.h +14 -0

index 1bb1c0764bc2..e8192f79fe42 100644
@@ -7,6 +7,7 @@
 #include <linux/iommu.h>
 #include <linux/iommufd.h>
 #include <linux/iova_bitmap.h>
+#include <linux/maple_tree.h>
 #include <linux/rwsem.h>
 #include <linux/uaccess.h>
 #include <linux/xarray.h>

@@ -44,6 +45,7 @@ struct iommufd_ctx {
 	struct xarray groups;
 	wait_queue_head_t destroy_wait;
 	struct rw_semaphore ioas_creation_lock;
+	struct maple_tree mt_mmap;
 
 	struct mutex sw_msi_lock;
 	struct list_head sw_msi_list;

@@ -55,6 +57,18 @@ struct iommufd_ctx {
 	struct iommufd_ioas *vfio_ioas;
 };
 
+/* Entry for iommufd_ctx::mt_mmap */
+struct iommufd_mmap {
+	struct iommufd_object *owner;
+
+	/* Allocated start position in mt_mmap tree */
+	unsigned long startp;
+
+	/* Physical range for io_remap_pfn_range() */
+	unsigned long mmio_pfn;
+	unsigned long num_pfns;
+};
+
 /*
  * The IOVA to PFN map. The map automatically copies the PFNs into multiple
  * domains and permits sharing of PFNs between io_pagetable instances. This

include/linux/iommufd.h +42 -0

index acf0e8f0c630..0da9bc8f94f3 100644
@@ -251,6 +251,11 @@ int _iommufd_object_depend(struct iommufd_object *obj_dependent,
 			   struct iommufd_object *obj_depended);
 void _iommufd_object_undepend(struct iommufd_object *obj_dependent,
 			      struct iommufd_object *obj_depended);
+int _iommufd_alloc_mmap(struct iommufd_ctx *ictx, struct iommufd_object *owner,
+			phys_addr_t mmio_addr, size_t length,
+			unsigned long *offset);
+void _iommufd_destroy_mmap(struct iommufd_ctx *ictx,
+			   struct iommufd_object *owner, unsigned long offset);
 struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 				       unsigned long vdev_id);
 int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,

@@ -271,6 +276,20 @@ _iommufd_object_undepend(struct iommufd_object *obj_dependent,
 {
 }
 
+static inline int _iommufd_alloc_mmap(struct iommufd_ctx *ictx,
+				      struct iommufd_object *owner,
+				      phys_addr_t mmio_addr, size_t length,
+				      unsigned long *offset)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void _iommufd_destroy_mmap(struct iommufd_ctx *ictx,
+					 struct iommufd_object *owner,
+					 unsigned long offset)
+{
+}
+
 static inline struct device *
 iommufd_viommu_find_dev(struct iommufd_viommu *viommu, unsigned long vdev_id)
 {

@@ -338,4 +357,27 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 		_iommufd_object_undepend(&dependent->member.obj,               \
 					 &depended->member.obj);               \
 	})
+
+/*
+ * Helpers for IOMMU driver to alloc/destroy an mmapable area for a structure.
+ *
+ * To support an mmappable MMIO region, kernel driver must first register it to
+ * iommufd core to allocate an @offset, during a driver-structure initialization
+ * (e.g. viommu_init op). Then, it should report to user space this @offset and
+ * the @length of the MMIO region for mmap syscall.
+ */
+static inline int iommufd_viommu_alloc_mmap(struct iommufd_viommu *viommu,
+					    phys_addr_t mmio_addr,
+					    size_t length,
+					    unsigned long *offset)
+{
+	return _iommufd_alloc_mmap(viommu->ictx, &viommu->obj, mmio_addr,
+				   length, offset);
+}
+
+static inline void iommufd_viommu_destroy_mmap(struct iommufd_viommu *viommu,
+					       unsigned long offset)
+{
+	_iommufd_destroy_mmap(viommu->ictx, &viommu->obj, offset);
+}
 #endif

drivers/iommu/iommufd/driver.c +51 -0

index 70b7917da0cb..8220b61d8c8d 100644
@@ -31,6 +31,57 @@ void _iommufd_object_undepend(struct iommufd_object *obj_dependent,
 }
 EXPORT_SYMBOL_NS_GPL(_iommufd_object_undepend, "IOMMUFD");
 
+/*
+ * Allocate an @offset to return to user space to use for an mmap() syscall
+ *
+ * Driver should use a per-structure helper in include/linux/iommufd.h
+ */
+int _iommufd_alloc_mmap(struct iommufd_ctx *ictx, struct iommufd_object *owner,
+			phys_addr_t mmio_addr, size_t length,
+			unsigned long *offset)
+{
+	struct iommufd_mmap *immap;
+	unsigned long startp;
+	int rc;
+
+	if (!PAGE_ALIGNED(mmio_addr))
+		return -EINVAL;
+	if (!length || !PAGE_ALIGNED(length))
+		return -EINVAL;
+
+	immap = kzalloc(sizeof(*immap), GFP_KERNEL);
+	if (!immap)
+		return -ENOMEM;
+	immap->owner = owner;
+	immap->num_pfns = length >> PAGE_SHIFT;
+	immap->mmio_pfn = mmio_addr >> PAGE_SHIFT;
+
+	rc = mtree_alloc_range(&ictx->mt_mmap, &startp, immap, immap->num_pfns,
+			       0, U32_MAX >> PAGE_SHIFT, GFP_KERNEL);
+	if (rc < 0) {
+		kfree(immap);
+		return rc;
+	}
+
+	immap->startp = startp;
+	/* mmap() syscall will right-shift the offset in vma->vm_pgoff */
+	*offset = startp << PAGE_SHIFT;
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(_iommufd_alloc_mmap, "IOMMUFD");
+
+/* Driver should use a per-structure helper in include/linux/iommufd.h */
+void _iommufd_destroy_mmap(struct iommufd_ctx *ictx,
+			   struct iommufd_object *owner, unsigned long offset)
+{
+	struct iommufd_mmap *immap;
+
+	immap = mtree_erase(&ictx->mt_mmap, offset >> PAGE_SHIFT);
+	WARN_ON_ONCE(!immap || immap->owner != owner);
+	kfree(immap);
+}
+EXPORT_SYMBOL_NS_GPL(_iommufd_destroy_mmap, "IOMMUFD");
+
 /* Caller should xa_lock(&viommu->vdevs) to protect the return value */
 struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 				       unsigned long vdev_id)

drivers/iommu/iommufd/main.c +64 -0

index 4e8dbbfac890..339a269ebbc8 100644
@@ -275,6 +275,7 @@ static int iommufd_fops_open(struct inode *inode, struct file *filp)
 	xa_init_flags(&ictx->objects, XA_FLAGS_ALLOC1 | XA_FLAGS_ACCOUNT);
 	xa_init(&ictx->groups);
 	ictx->file = filp;
+	mt_init_flags(&ictx->mt_mmap, MT_FLAGS_ALLOC_RANGE);
 	init_waitqueue_head(&ictx->destroy_wait);
 	mutex_init(&ictx->sw_msi_lock);
 	INIT_LIST_HEAD(&ictx->sw_msi_list);

@@ -479,11 +480,74 @@ static long iommufd_fops_ioctl(struct file *filp, unsigned int cmd,
 	return ret;
 }
 
+static void iommufd_fops_vma_open(struct vm_area_struct *vma)
+{
+	struct iommufd_mmap *immap = vma->vm_private_data;
+
+	refcount_inc(&immap->owner->users);
+}
+
+static void iommufd_fops_vma_close(struct vm_area_struct *vma)
+{
+	struct iommufd_mmap *immap = vma->vm_private_data;
+
+	refcount_dec(&immap->owner->users);
+}
+
+static const struct vm_operations_struct iommufd_vma_ops = {
+	.open = iommufd_fops_vma_open,
+	.close = iommufd_fops_vma_close,
+};
+
+/* The vm_pgoff must be pre-allocated from mt_mmap, and given to user space */
+static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct iommufd_ctx *ictx = filp->private_data;
+	size_t length = vma->vm_end - vma->vm_start;
+	struct iommufd_mmap *immap;
+	int rc;
+
+	if (!PAGE_ALIGNED(length))
+		return -EINVAL;
+	if (!(vma->vm_flags & VM_SHARED))
+		return -EINVAL;
+	if (vma->vm_flags & VM_EXEC)
+		return -EPERM;
+
+	/* vma->vm_pgoff carries an index to an mtree entry (immap) */
+	immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff);
+	if (!immap)
+		return -ENXIO;
+	/*
+	 * mtree_load() returns the immap for any contained pgoff, only allow
+	 * the immap thing to be mapped
+	 */
+	if (vma->vm_pgoff != immap->startp)
+		return -ENXIO;
+	if (length != immap->num_pfns << PAGE_SHIFT)
+		return -ENXIO;
+
+	vma->vm_pgoff = 0;
+	vma->vm_private_data = immap;
+	vma->vm_ops = &iommufd_vma_ops;
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+
+	rc = io_remap_pfn_range(vma, vma->vm_start, immap->mmio_pfn, length,
+				vma->vm_page_prot);
+	if (rc)
+		return rc;
+
+	/* vm_ops.open won't be called for mmap itself. */
+	refcount_inc(&immap->owner->users);
+	return rc;
+}
+
 static const struct file_operations iommufd_fops = {
 	.owner = THIS_MODULE,
 	.open = iommufd_fops_open,
 	.release = iommufd_fops_release,
 	.unlocked_ioctl = iommufd_fops_ioctl,
+	.mmap = iommufd_fops_mmap,
 };
 
 /**

Signature · 1 line

2.43.0