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

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:02 AM

include/linux/iommufd.h +6 -0

+
+#define iommufd_vdevice_alloc(viommu, drv_struct, member)                      \
+	({                                                                     \
+		drv_struct *ret;                                               \
+									       \
+		static_assert(__same_type(struct iommufd_viommu,

*viommu)); \

(quoted diff) +2 -0

+		static_assert(__same_type(struct iommufd_vdevice,              \
+					  ((drv_struct *)NULL)->member));

\

include/linux/iommufd.h +3 -0

+		static_assert(offsetof(drv_struct, member.obj) == 0);          \
+		ret = (drv_struct *)_iommufd_object_alloc(                     \
+			viommu->ictx, sizeof(drv_struct),

IOMMUFD_OBJ_VDEVICE);\

Quoted 17 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:02 AM

include/linux/iommufd.h +6 -0

+
+#define iommufd_vdevice_alloc(viommu, drv_struct, member)                      \
+	({                                                                     \
+		drv_struct *ret;                                               \
+									       \
+		static_assert(__same_type(struct iommufd_viommu,

*viommu)); \

(quoted diff) +2 -0

+		static_assert(__same_type(struct iommufd_vdevice,              \
+					  ((drv_struct *)NULL)->member));

\

include/linux/iommufd.h +3 -0

+		static_assert(offsetof(drv_struct, member.obj) == 0);          \
+		ret = (drv_struct *)_iommufd_object_alloc(                     \
+			viommu->ictx, sizeof(drv_struct),

IOMMUFD_OBJ_VDEVICE);\

include/linux/iommufd.h static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu, · line 285

Quoted 6 lines

include/linux/iommufd.h +6 -0

+		if (!IS_ERR(ret)) {                                            \
+			ret->member.viommu = viommu;                           \
+			ret->member.ictx = viommu->ictx;                       \
+		}                                                              \
+		ret;                                                           \
+	})

viommu->ictx is set in patch5, so logically this is better put after that.

Reviewed-by Kevin Tian <[email protected]>

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:02 AM

The new type of vIOMMU for tegra241-cmdqv allows user space VM to use one
of its virtual command queue HW resources exclusively. This requires user space to mmap the corresponding MMIO page from kernel space for direct HW
control.

To forward the mmap info (offset and length), iommufd should add a driver specific data structure to the IOMMUFD_CMD_VIOMMU_ALLOC ioctl, for driver
to output the info during the allocation back to user space.

Similar to the existing ioctls and their IOMMU handlers, add a user_data to viommu_alloc op to bridge between iommufd and drivers.

Reviewed-by Jason Gunthorpe <[email protected]>

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Pranjal Shrivastava <[email protected]>

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

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:02 AM

+ * @data_len: Length of the type specific data
+ * @__reserved: Must be 0
+ * @data_uptr: User pointer to an array of driver-specific virtual IOMMU data

Quoted 7 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:02 AM

+ * @data_len: Length of the type specific data
+ * @__reserved: Must be 0
+ * @data_uptr: User pointer to an array of driver-specific virtual IOMMU data

I don't think there is 'an array'.

Reviewed-by Kevin Tian <[email protected]>

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:02 AM

Similar to the iommu_copy_struct_from_user helper receiving data from the user space, add an iommu_copy_struct_to_user helper to report output data back to the user space data pointer.

Reviewed-by Jason Gunthorpe <[email protected]>

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Pranjal Shrivastava <[email protected]>

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

When an IOMMU driver calls iommufd_viommu_alloc(), it must pass in an ictx pointer as the underlying _iommufd_object_alloc() helper function requires that to allocate a new object. However, neither the iommufd_viommu_alloc() nor its underlying _iommufd_object_alloc() saves the ictx in the allocated viommu object, although viommu could hold an ictx pointer.

When the IOMMU driver wants to use another iommufd function passing in the allocated viommu, it could have avoided passing in the ictx pointer again, if viommu->ictx is valid.

Save ictx to viommu->ictx in the iommufd_viommu_alloc(), in order to ease a new vIOMMU-based helper that would then get the ictx from viommu->ictx.

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

---
 include/linux/iommufd.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Quoted 10 lines · Nicolin Chen

When an IOMMU driver calls iommufd_viommu_alloc(), it must pass in an ictx pointer as the underlying _iommufd_object_alloc() helper function requires that to allocate a new object. However, neither the iommufd_viommu_alloc() nor its underlying _iommufd_object_alloc() saves the ictx in the allocated viommu object, although viommu could hold an ictx pointer.

When the IOMMU driver wants to use another iommufd function passing in the allocated viommu, it could have avoided passing in the ictx pointer again, if viommu->ictx is valid.

Quoted 7 lines

Save ictx to viommu->ictx in the iommufd_viommu_alloc(), in order to ease a new vIOMMU-based helper that would then get the ictx from viommu->ictx.

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

---
 include/linux/iommufd.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

It is OK, but please think carefully if the ictx is actually needed. The reason most objects don't have an ictx in them is because their contexts are always inside an ioctl so they get the ictx from there.

We don't have a lot of viommu allocations so it isn't such a big deal, but just generally that is how it was built that the ictx comes from the ioctl not the object.

Otherwise, they would be something like:

	// in my_viommu_alloc()
	my_viommu = iommufd_viommu_alloc(ucmd, ...);
	iommufd_viommu_alloc_mmap(ucmd->ictx, my_viommu, ...);
	iommufd_viommu_destroy_mmap(ucmd->ictx, my_viommu, ...);
	// in my_viommu_destory()
	iommufd_viommu_destroy_mmap(my_viommu->ictx, my_viommu ...);

That being said, I ended up doing something more in this patch for the ucmd rework and Kevin's comments:

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

index 98fe5a49c9606..ddd144031af5d 100644

Quoted 7 lines · Nicolin Chen

Otherwise, they would be something like:

	// in my_viommu_alloc()
	my_viommu = iommufd_viommu_alloc(ucmd, ...);
	iommufd_viommu_alloc_mmap(ucmd->ictx, my_viommu, ...);
	iommufd_viommu_destroy_mmap(ucmd->ictx, my_viommu, ...);
	// in my_viommu_destory()
	iommufd_viommu_destroy_mmap(my_viommu->ictx, my_viommu ...);

Yes, this is how the other objects work..

Quoted 11 lines

That being said, I ended up doing something more in this patch for the ucmd rework and Kevin's comments:

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

index 98fe5a49c9606..ddd144031af5d 100644
@@ -136,14 +136,6 @@ int iopt_pin_pages(struct io_pagetable *iopt, unsigned long iova,
 void iopt_unpin_pages(struct io_pagetable *iopt, unsigned long iova,
 		      unsigned long length);
 

drivers/iommu/iommufd/iommufd_private.h int iopt_pin_pages(struct io_pagetable *iopt, unsigned long iova, · line 145

Quoted 7 lines

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

-struct iommufd_ucmd {
-	struct iommufd_ctx *ictx;
-	void __user *ubuffer;
-	u32 user_size;
-	void *cmd;
-	struct iommufd_object *alloced_obj;
-};

You don't need to move this unless you are using inlines. Just use a forward declaration.

Review

Quoted 11 lines · Nicolin Chen

Quoted 2 lines

You don't need to move this unless you are using inlines. Just use a forward declaration.

Since we forward ucmd now, ictx is in the ucmd so we need this structure for:

(quoted diff) +3 -1

-		if (!IS_ERR(ret))                                              \
+		if (!IS_ERR(ret)) {                                            \
 			ret->member.ops = viommu_ops;                          \
+			ret->member.ictx = ucmd->ictx;                         \
+		}                                                              \

De-inline more of that function probably..

Also seem my other remarks about not storing ictx so much..

Quoted 16 lines · Jason Gunthorpe

include/linux/iommufd.h static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu, · line 271

Quoted 11 lines · Nicolin Chen

Quoted 2 lines

You don't need to move this unless you are using inlines. Just use a forward declaration.

Since we forward ucmd now, ictx is in the ucmd so we need this structure for:

include/linux/iommufd.h +3 -1

-		if (!IS_ERR(ret))                                              \
+		if (!IS_ERR(ret)) {                                            \
 			ret->member.ops = viommu_ops;                          \
+			ret->member.ictx = ucmd->ictx;                         \
+		}                                                              \

De-inline more of that function probably..

Quoted 1 line

Also seem my other remarks about not storing ictx so much..

I found that all other ictx pointers in vdev/hw_queue are unused, as the core simply gets an ictx from their viommu pointers. This means that only this viommu allocator here needs such a storing.

With that, how about a change like this v.s. inline:

Quoted 19 lines · Nicolin Chen

Quoted 17 lines · Jason Gunthorpe

include/linux/iommufd.h static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu, · line 271

Quoted 11 lines · Nicolin Chen

Quoted 2 lines

You don't need to move this unless you are using inlines. Just use a forward declaration.

Since we forward ucmd now, ictx is in the ucmd so we need this structure for:

include/linux/iommufd.h +3 -1

-		if (!IS_ERR(ret))                                              \
+		if (!IS_ERR(ret)) {                                            \
 			ret->member.ops = viommu_ops;                          \
+			ret->member.ictx = ucmd->ictx;                         \
+		}                                                              \

De-inline more of that function probably..

Also seem my other remarks about not storing ictx so much..

Quoted 5 lines

I found that all other ictx pointers in vdev/hw_queue are unused, as the core simply gets an ictx from their viommu pointers. This means that only this viommu allocator here needs such a storing.

With that, how about a change like this v.s. inline:

So I think the other suggestion with init vs alloc supersedes this and makes it all pretty clean?

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:02 AM

When an IOMMU driver calls iommufd_viommu_alloc(), it must pass in an ictx
pointer as the underlying _iommufd_object_alloc() helper function requires that to allocate a new object. However, neither the iommufd_viommu_alloc() nor its underlying _iommufd_object_alloc() saves the ictx in the allocated viommu object, although viommu could hold an ictx pointer.

When the IOMMU driver wants to use another iommufd function passing in the
allocated viommu, it could have avoided passing in the ictx pointer again, if viommu->ictx is valid.

Save ictx to viommu->ictx in the iommufd_viommu_alloc(), in order to ease a new vIOMMU-based helper that would then get the ictx from viommu->ictx.

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

An IOMMU driver that allocated a vIOMMU may want to revert the allocation, if it encounters an internal error after the allocation. So, there needs a destroy helper for drivers to use. For instance:

static my_viommu_alloc()

{
	...
	my_viommu = iommufd_viommu_alloc(viomm, struct my_viommu, core);
	...
	ret = init_my_viommu();
	if (ret) {
		/* Need to destroy the my_viommu->core */
		return ERR_PTR(ret);
	}
	return &my_viommu->core;
}

Move iommufd_object_abort() to the driver.c file and the public header, to introduce common iommufd_struct_destroy() helper that will abort all kinds of driver structures, not confined to iommufd_viommu but also the new ones being added in the future.

Reviewed-by Jason Gunthorpe <[email protected]>

Reviewed-by Lu Baolu <[email protected]>

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

---
 drivers/iommu/iommufd/iommufd_private.h |  1 -
 include/linux/iommufd.h                 | 16 ++++++++++++++++
 drivers/iommu/iommufd/driver.c          | 14 ++++++++++++++
 drivers/iommu/iommufd/main.c            | 13 -------------
 4 files changed, 30 insertions(+), 14 deletions(-)

Quoted 25 lines · Nicolin Chen

An IOMMU driver that allocated a vIOMMU may want to revert the allocation, if it encounters an internal error after the allocation. So, there needs a destroy helper for drivers to use. For instance:

static my_viommu_alloc()

{
	...
	my_viommu = iommufd_viommu_alloc(viomm, struct my_viommu, core);
	...
	ret = init_my_viommu();
	if (ret) {
		/* Need to destroy the my_viommu->core */
		return ERR_PTR(ret);
	}
	return &my_viommu->core;
}

Move iommufd_object_abort() to the driver.c file and the public header, to introduce common iommufd_struct_destroy() helper that will abort all kinds of driver structures, not confined to iommufd_viommu but also the new ones being added in the future.

Reviewed-by Jason Gunthorpe <[email protected]>

Reviewed-by Lu Baolu <[email protected]>

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

Quoted 6 lines

---
 drivers/iommu/iommufd/iommufd_private.h |  1 -
 include/linux/iommufd.h                 | 16 ++++++++++++++++
 drivers/iommu/iommufd/driver.c          | 14 ++++++++++++++
 drivers/iommu/iommufd/main.c            | 13 -------------
 4 files changed, 30 insertions(+), 14 deletions(-)

One idea that struck me when I was looking at this was to copy the technique from rdma.

When an object is allocated we keep track of it in the struct iommufd_ucmd.

Then when the command is over the core code either aborts or finalizes the objects in the iommufd_ucmd. This way you don't have to expose abort and related to drivers.

Quoted 3 lines

Then when the command is over the core code either aborts or finalizes the objects in the iommufd_ucmd. This way you don't have to expose abort and related to drivers.

I see! Do you want this to apply to the all objects or just driver allocated ones?

We would need a bigger preparatory series to roll out that to all the allocators, and need to be careful at the existing abort() that intertwines with other steps like an unlock().

Quoted 6 lines · Nicolin Chen

Quoted 3 lines

Then when the command is over the core code either aborts or finalizes the objects in the iommufd_ucmd. This way you don't have to expose abort and related to drivers.

I see! Do you want this to apply to the all objects or just driver allocated ones?

I would do all the ones that can work that way easily

I think it would just be one patch, replace this patch with that one.

Quoted 3 lines

We would need a bigger preparatory series to roll out that to all the allocators, and need to be careful at the existing abort() that intertwines with other steps like an unlock().

Those cases with special locking couldn't use it.

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:02 AM

Add a simple user_data for an input-to-output loopback test.

Reviewed-by Pranjal Shrivastava <[email protected]>

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

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:02 AM

Extend the existing test_cmd/err_viommu_alloc helpers to accept optional user data. And add a TEST_F for a loopback test.

Reviewed-by Pranjal Shrivastava <[email protected]>

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

The new HW QUEUE object will be added for HW to access the guest queue for HW-accelerated virtualization feature. It needs to ensure the guest memory pages are pinned when HW accesses them and they are contiguous in physical address space.

This is very like the existing iommufd_access_pin_pages() that outputs the pinned page list for the caller to test its contiguity.

Move those code from iommufd_access_pin/unpin_pages() and related function for a pair of iopt helpers that can be shared with the HW QUEUE allocator.

Rename check_area_prot() to align with the existing iopt_area helpers, and inline it to the header since iommufd_access_rw() still uses it.

Reviewed-by Pranjal Shrivastava <[email protected]>

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

---
 drivers/iommu/iommufd/io_pagetable.h    |   8 ++
 drivers/iommu/iommufd/iommufd_private.h |   6 ++
 drivers/iommu/iommufd/device.c          | 117 ++----------------------
 drivers/iommu/iommufd/io_pagetable.c    |  95 +++++++++++++++++++
 4 files changed, 117 insertions(+), 109 deletions(-)

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

The new HW QUEUE object will be added for HW to access the guest queue for
HW-accelerated virtualization feature. It needs to ensure the guest memory pages are pinned when HW accesses them and they are contiguous in physical
address space.

This is very like the existing iommufd_access_pin_pages() that outputs the pinned page list for the caller to test its contiguity.

Move those code from iommufd_access_pin/unpin_pages() and related function
for a pair of iopt helpers that can be shared with the HW QUEUE allocator.

Rename check_area_prot() to align with the existing iopt_area helpers, and inline it to the header since iommufd_access_rw() still uses it.

Reviewed-by Pranjal Shrivastava <[email protected]>

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

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

Add IOMMUFD_OBJ_HW_QUEUE with an iommufd_hw_queue structure, representing
a HW-accelerated queue type of IOMMU's physical queue that can be passed through to a user space VM for direct hardware control, such as:
- NVIDIA's Virtual Command Queue
- AMD vIOMMU's Command Buffer, Event Log Buffer, and PPR Log Buffer

Introduce an allocator iommufd_hw_queue_alloc(). And add a pair of viommu
ops for iommufd to forward user space ioctls to IOMMU drivers.

Given that the first user of this HW QUEUE (tegra241-cmdqv) will need to ensure the queue memory to be physically contiguous, add a flag property in iommufd_viommu_ops and IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA to allow
driver to flag it so that the core will validate the physical pages of a given guest queue.

+ * @hw_queue_alloc: Allocate a HW QUEUE object for a HW-accelerated queue given

+ *                  the @type (must be defined in include/uapi/linux/iommufd.h)
+ *                  for the @viommu. @index carries the logical HW QUEUE ID per
+ *                  @viommu in a guest VM, for a multi-queue case; @addr carries
+ *                  the guest physical base address of the queue memory; 

Quoted 14 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

Add IOMMUFD_OBJ_HW_QUEUE with an iommufd_hw_queue structure, representing
a HW-accelerated queue type of IOMMU's physical queue that can be passed through to a user space VM for direct hardware control, such as:
- NVIDIA's Virtual Command Queue
- AMD vIOMMU's Command Buffer, Event Log Buffer, and PPR Log Buffer

Introduce an allocator iommufd_hw_queue_alloc(). And add a pair of viommu
ops for iommufd to forward user space ioctls to IOMMU drivers.

Quoted 6 lines

Given that the first user of this HW QUEUE (tegra241-cmdqv) will need to ensure the queue memory to be physically contiguous, add a flag property in iommufd_viommu_ops and IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA to allow
driver to flag it so that the core will validate the physical pages of a given guest queue.

'READS' is confusing here. What about xxx_CONTIG_PAS?

Quoted 6 lines

+ * @hw_queue_alloc: Allocate a HW QUEUE object for a HW-accelerated queue given

+ *                  the @type (must be defined in include/uapi/linux/iommufd.h)
+ *                  for the @viommu. @index carries the logical HW QUEUE ID per
+ *                  @viommu in a guest VM, for a multi-queue case; @addr carries
+ *                  the guest physical base address of the queue memory; 

s/@addr/@base_addr/

Reviewed-by Kevin Tian <[email protected]>

Quoted 14 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

Add IOMMUFD_OBJ_HW_QUEUE with an iommufd_hw_queue structure, representing
a HW-accelerated queue type of IOMMU's physical queue that can be passed through to a user space VM for direct hardware control, such as:
- NVIDIA's Virtual Command Queue
- AMD vIOMMU's Command Buffer, Event Log Buffer, and PPR Log Buffer

Introduce an allocator iommufd_hw_queue_alloc(). And add a pair of viommu
ops for iommufd to forward user space ioctls to IOMMU drivers.

Quoted 6 lines

Given that the first user of this HW QUEUE (tegra241-cmdqv) will need to ensure the queue memory to be physically contiguous, add a flag property in iommufd_viommu_ops and IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA to allow
driver to flag it so that the core will validate the physical pages of a given guest queue.

'READS' is confusing here. What about xxx_CONTIG_PAS?

From: Nicolin Chen <[email protected]>
Sent: Friday, May 16, 2025 10:30 AM

Quoted 6 lines · Kevin

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

Add IOMMUFD_OBJ_HW_QUEUE with an iommufd_hw_queue structure, representing
a HW-accelerated queue type of IOMMU's physical queue that can be

passed

Quoted 10 lines

Quoted 10 lines

through to a user space VM for direct hardware control, such as:
- NVIDIA's Virtual Command Queue
- AMD vIOMMU's Command Buffer, Event Log Buffer, and PPR Log Buffer

Introduce an allocator iommufd_hw_queue_alloc(). And add a pair of viommu
ops for iommufd to forward user space ioctls to IOMMU drivers.

Given that the first user of this HW QUEUE (tegra241-cmdqv) will need to ensure the queue memory to be physically contiguous, add a flag

property

Quoted 6 lines

Quoted 4 lines

in iommufd_viommu_ops and IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA to allow
driver to flag it so that the core will validate the physical pages of a given guest queue.

'READS' is confusing here. What about xxx_CONTIG_PAS?

Combining Jason's first comments here:
https://lore.kernel.org/linux-
iommu/[email protected]/

So, pinning should be optional too. And I think there would be unlikely a case where HW needs contiguous physical pages while not requiring to pin the pages, right?

So, we need an flag that could indicate to do both tests. Yet, "xxx_CONTIG_PAS" doesn't sound very fitting, compared to this "IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA".

Perhaps, we should just add some comments to clarify a bit. Or do you have some better naming?

Quoted 29 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 16, 2025 10:30 AM

Quoted 6 lines · Kevin

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

Add IOMMUFD_OBJ_HW_QUEUE with an iommufd_hw_queue structure, representing
a HW-accelerated queue type of IOMMU's physical queue that can be

passed

Quoted 10 lines

Quoted 10 lines

through to a user space VM for direct hardware control, such as:
- NVIDIA's Virtual Command Queue
- AMD vIOMMU's Command Buffer, Event Log Buffer, and PPR Log Buffer

Introduce an allocator iommufd_hw_queue_alloc(). And add a pair of viommu
ops for iommufd to forward user space ioctls to IOMMU drivers.

Given that the first user of this HW QUEUE (tegra241-cmdqv) will need to ensure the queue memory to be physically contiguous, add a flag

property

Quoted 6 lines

Quoted 4 lines

in iommufd_viommu_ops and IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA to allow
driver to flag it so that the core will validate the physical pages of a given guest queue.

'READS' is confusing here. What about xxx_CONTIG_PAS?

Quoted 7 lines

Combining Jason's first comments here:
https://lore.kernel.org/linux-
iommu/[email protected]/

So, pinning should be optional too. And I think there would be unlikely a case where HW needs contiguous physical pages while not requiring to pin the pages, right?

Quoted 5 lines

So, we need an flag that could indicate to do both tests. Yet, "xxx_CONTIG_PAS" doesn't sound very fitting, compared to this "IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA".

Quoted 3 lines

Perhaps, we should just add some comments to clarify a bit. Or do you have some better naming?

let's wait until that open is closed, i.e. whether we still let the core manage it and whether AMD requires pinning even when IOVA
is used.

include/linux/iommufd.h +7 -0

+#define iommufd_hw_queue_alloc(viommu, drv_struct, member)                     \
+	({                                                                     \
+		drv_struct *ret;                                               \
+									       \
+		static_assert(__same_type(struct iommufd_viommu, *viommu));    \
+		static_assert(__same_type(struct iommufd_hw_queue,             \
+					  ((drv_struct *)NULL)->member));      \

/* Helper for IOMMU driver to destroy structures created by allocators above */
#define iommufd_struct_destroy(drv_struct, member) \
({ \

Quoted 7 lines · Nicolin Chen

include/linux/iommufd.h +7 -0

+#define iommufd_hw_queue_alloc(viommu, drv_struct, member)                     \
+	({                                                                     \
+		drv_struct *ret;                                               \
+									       \
+		static_assert(__same_type(struct iommufd_viommu, *viommu));    \
+		static_assert(__same_type(struct iommufd_hw_queue,             \
+					  ((drv_struct *)NULL)->member));      \

include/linux/iommufd.h static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu, · line 338

Quoted 10 lines

include/linux/iommufd.h +10 -0

+		static_assert(offsetof(drv_struct, member.obj) == 0);          \
+		ret = (drv_struct *)_iommufd_object_alloc(                     \
+			viommu->ictx, sizeof(drv_struct),                      \
+			IOMMUFD_OBJ_HW_QUEUE);                                 \
+		if (!IS_ERR(ret)) {                                            \
+			ret->member.viommu = viommu;                           \
+			ret->member.ictx = viommu->ictx;                       \
+		}                                                              \
+		ret;                                                           \
+	})

This should just call

__iommufd_object_alloc(viommu->ictx, ret, IOMMUFD_OBJ_HW_QUEUE member.obj)

And all the casting and asserts should be in that macro, move it to this header

Quoted 3 lines

/* Helper for IOMMU driver to destroy structures created by allocators above */
#define iommufd_struct_destroy(drv_struct, member) \
({ \

This is abort not destroy, the names should be consistent. But looking more at the series I think it will be better to do the little rework I suggested and not give this function to the driver in the first place.

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

include/uapi/linux/iommufd.h +8 -0

+
+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a

multi-queue
+ * model

+ * @out_hw_queue_id: The ID of the new HW queue
+ * @base_addr: Base address of the queue memory in guest physical address space
+ * @length: Length of the queue memory in the guest physical address space

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

+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_hwpt_paging *hwpt;
+	struct iommufd_viommu *viommu;
+	struct page **pages;
+	int max_npages, i;
+	u64 end;

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

+
+	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
+	if (IS_ERR(viommu)) {
+		rc = PTR_ERR(viommu);
+		goto out_free;
+	}
+	hwpt = viommu->hwpt;
+
+	if (!viommu->ops || !viommu->ops->hw_queue_alloc) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}
+
+	/* Quick test on the base address */
+	if (!iommu_iova_to_phys(hwpt->common.domain, cmd->base_addr))

{

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

+
+	/*
+	 * The underlying physical pages must be pinned to prevent them

from

(quoted diff) +1 -0

+	 * being unmapped (via IOMMUFD_CMD_IOAS_UNMAP) during the

life cycle

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

+	 * of the HW QUEUE object.
+	 */
+	rc = iopt_pin_pages(&hwpt->ioas->iopt, cmd->base_addr, cmd-

Quoted 1 line

length,

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

+			    pages, 0);
+	if (rc)
+		goto out_put_viommu;
+
+	if (viommu->ops->flags &

IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA) {

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

+		/* Validate if the underlying physical pages are contiguous */
+		for (i = 1; i < max_npages && pages[i]; i++) {
+			if (page_to_pfn(pages[i]) ==
+			    page_to_pfn(pages[i - 1]) + 1)
+				continue;
+			rc = -EFAULT;
+			goto out_unpin;
+		}
+	}
+
+	hw_queue = viommu->ops->hw_queue_alloc(viommu, cmd->type,

cmd->index,

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

+	hw_queue->length = cmd->length;
+	hw_queue->base_addr = cmd->base_addr;
+	cmd->out_hw_queue_id = hw_queue->obj.id;
+	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
+	if (rc)
+		iommufd_object_abort_and_destroy(ucmd->ictx,

&hw_queue->obj);

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

+	else
+		iommufd_object_finalize(ucmd->ictx, &hw_queue->obj);
+	goto out_put_viommu;
+
+out_unpin:
+	iopt_unpin_pages(&hwpt->ioas->iopt, cmd->base_addr, cmd-

Quoted 1 line

length);

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

+out_put_viommu:
+	iommufd_put_object(ucmd->ictx, &viommu->obj);
+out_free:
+	kfree(pages);
+	return rc;
+}
--

2.43.0

Quoted 11 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

include/uapi/linux/iommufd.h +8 -0

+
+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a

Quoted 2 lines

multi-queue
+ * model

I'm thinking of an alternative way w/o having the user to assign index and allowing the driver to poke object dependency (next patch).

Let's say the index is internally assigned by the driver. so this cmd is just for allowing a hw queue and it's the driver to decide the allocation policy, e.g. in ascending order.

Introduce a new flag in viommu_ops to indicate to core that the new hw queue should hold a reference to the previous hw queue.

core maintains a last_queue field in viommu. Upon success return from @hw_queue_alloc() the core increments the users refcnt of last_queue, records the dependency in iommufd_hw_queue struct, and update viommu->last_queue.

Then the destroy order is naturally guaranteed.

Quoted 5 lines

+ * @out_hw_queue_id: The ID of the new HW queue
+ * @base_addr: Base address of the queue memory in guest physical address space
+ * @length: Length of the queue memory in the guest physical address space

length is agnostic to address space.

Quoted 14 lines

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

+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_hwpt_paging *hwpt;
+	struct iommufd_viommu *viommu;
+	struct page **pages;
+	int max_npages, i;
+	u64 end;
+	int rc;
+
+	if (cmd->flags || cmd->type == IOMMU_HW_QUEUE_TYPE_DEFAULT)
+		return -EOPNOTSUPP;
+	if (!cmd->base_addr || !cmd->length)

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

Quoted 5 lines

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

+		return -EINVAL;
+	if (check_add_overflow(cmd->base_addr, cmd->length - 1, &end))
+		return -EOVERFLOW;
+
+	max_npages = DIV_ROUND_UP(cmd->length, PAGE_SIZE);

what about [base_addr, base_addr+length) spanning two pages but 'length' is smaller than the size of one page?

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

Quoted 3 lines

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

+	pages = kcalloc(max_npages, sizeof(*pages), GFP_KERNEL);
+	if (!pages)
+		return -ENOMEM;

this could be moved to right before iopt_pin_pages().

Quoted 16 lines

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

+
+	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
+	if (IS_ERR(viommu)) {
+		rc = PTR_ERR(viommu);
+		goto out_free;
+	}
+	hwpt = viommu->hwpt;
+
+	if (!viommu->ops || !viommu->ops->hw_queue_alloc) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}
+
+	/* Quick test on the base address */
+	if (!iommu_iova_to_phys(hwpt->common.domain, cmd->base_addr))

{

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

Quoted 3 lines

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

+		rc = -ENXIO;
+		goto out_put_viommu;
+	}

this check is redundant. Actually it's not future proof, assuming that S2 must be pinned before the user attempts to call this cmd. But what if one day iommufd supports unpinned S2 (if a device is 100% PRI faultable) then this path will be broken.

Quoted 5 lines

(quoted diff) +1 -0

+	hw_queue = viommu->ops->hw_queue_alloc(viommu, cmd->type,

cmd->index,

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

+					       cmd->base_addr, cmd->length);
+	if (IS_ERR(hw_queue)) {
+		rc = PTR_ERR(hw_queue);

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

Quoted 6 lines

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

+		goto out_unpin;
+	}
+
+	hw_queue->viommu = viommu;
+	refcount_inc(&viommu->obj.users);
+	hw_queue->ictx = ucmd->ictx;

viommu/ictx are already set by iommufd_hw_queue_alloc().

From: Nicolin Chen <[email protected]>
Sent: Friday, May 16, 2025 2:45 AM

Quoted 24 lines · Kevin

Quoted 11 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

include/uapi/linux/iommufd.h +8 -0

+
+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a

Quoted 2 lines

multi-queue
+ * model

I'm thinking of an alternative way w/o having the user to assign index and allowing the driver to poke object dependency (next patch).

Let's say the index is internally assigned by the driver. so this cmd is just for allowing a hw queue and it's the driver to decide the allocation policy, e.g. in ascending order.

Introduce a new flag in viommu_ops to indicate to core that the new hw queue should hold a reference to the previous hw queue.

Quoted 6 lines

core maintains a last_queue field in viommu. Upon success return from @hw_queue_alloc() the core increments the users refcnt of last_queue, records the dependency in iommufd_hw_queue struct, and update viommu->last_queue.

Then the destroy order is naturally guaranteed.

I have thought about that too. It's nice that the core can easily maintain the dependency for the driver.

But there would still need an out_index to mark each dynamically allocated queue. So VMM would know where it should map the queue.

For example, if VMM wants to allocate a queue at its own index=1 without allocating index=0 first, kernel cannot fail that as VMM doesn't provide the index. The only way left for kernel would be to output the allocated queue with index=0 and then wish VMM can validate it, which doesn't sound safe..

Quoted 38 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 16, 2025 2:45 AM

Quoted 24 lines · Kevin

Quoted 11 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

include/uapi/linux/iommufd.h +8 -0

+
+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a

Quoted 2 lines

multi-queue
+ * model

I'm thinking of an alternative way w/o having the user to assign index and allowing the driver to poke object dependency (next patch).

Let's say the index is internally assigned by the driver. so this cmd is just for allowing a hw queue and it's the driver to decide the allocation policy, e.g. in ascending order.

Introduce a new flag in viommu_ops to indicate to core that the new hw queue should hold a reference to the previous hw queue.

Quoted 6 lines

core maintains a last_queue field in viommu. Upon success return from @hw_queue_alloc() the core increments the users refcnt of last_queue, records the dependency in iommufd_hw_queue struct, and update viommu->last_queue.

Then the destroy order is naturally guaranteed.

I have thought about that too. It's nice that the core can easily maintain the dependency for the driver.

Quoted 9 lines

But there would still need an out_index to mark each dynamically allocated queue. So VMM would know where it should map the queue.

For example, if VMM wants to allocate a queue at its own index=1 without allocating index=0 first, kernel cannot fail that as VMM doesn't provide the index. The only way left for kernel would be to output the allocated queue with index=0 and then wish VMM can validate it, which doesn't sound safe..

VMM's index is virtual which could be mapped to whatever queue object created at its own disposal.

the uAPI just requires VMM to remember a sequential list of allocated queue objects and destroy them in reverse order of allocation, instead of in the reverse order of virtual indexes.

From: Nicolin Chen <[email protected]>
Sent: Friday, May 16, 2025 11:17 AM

Quoted 7 lines · Kevin

Quoted 7 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 16, 2025 2:45 AM

Quoted 3 lines · Kevin

Quoted 3 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

include/uapi/linux/iommufd.h struct iommu_veventq_alloc {

Quoted 8 lines

include/uapi/linux/iommufd.h +8 -0

+
+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for

a

Quoted 33 lines

Quoted 23 lines

Quoted 13 lines

Quoted 2 lines

multi-queue
+ * model

I'm thinking of an alternative way w/o having the user to assign index and allowing the driver to poke object dependency (next patch).

Let's say the index is internally assigned by the driver. so this cmd is just for allowing a hw queue and it's the driver to decide the allocation policy, e.g. in ascending order.

Introduce a new flag in viommu_ops to indicate to core that the new hw queue should hold a reference to the previous hw queue.

Quoted 6 lines

core maintains a last_queue field in viommu. Upon success return from @hw_queue_alloc() the core increments the users refcnt of last_queue, records the dependency in iommufd_hw_queue struct, and update viommu->last_queue.

Then the destroy order is naturally guaranteed.

I have thought about that too. It's nice that the core can easily maintain the dependency for the driver.

Quoted 9 lines

But there would still need an out_index to mark each dynamically allocated queue. So VMM would know where it should map the queue.

For example, if VMM wants to allocate a queue at its own index=1 without allocating index=0 first, kernel cannot fail that as VMM doesn't provide the index. The only way left for kernel would be to output the allocated queue with index=0 and then wish VMM can validate it, which doesn't sound safe..

Quoted 6 lines

VMM's index is virtual which could be mapped to whatever queue object created at its own disposal.

the uAPI just requires VMM to remember a sequential list of allocated queue objects and destroy them in reverse order of allocation, instead of in the reverse order of virtual indexes.

But that's not going to work for VCMDQ.

VINTF mmaps only a single page that controls multiple queues. And all queues have to be mapped correctly between HW and VM indexes. Otherwise, it won't work if VMM maps:

HW-level VINTF1 LVCMDQ0 <==> VM-level VINTF0 LVCMDQ1 HW-level VINTF1 LVCMDQ1 <==> VM-level VINTF0 LVCMDQ0

So, one way or another, kernel has to ensure the static mappings of the indexes. And I think it's safer in the way that VMM tells what index to allocate..

Quoted 66 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 16, 2025 11:17 AM

Quoted 7 lines · Kevin

Quoted 7 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 16, 2025 2:45 AM

Quoted 3 lines · Kevin

Quoted 3 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, May 9, 2025 11:03 AM

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

include/uapi/linux/iommufd.h struct iommu_veventq_alloc {

Quoted 8 lines

include/uapi/linux/iommufd.h +8 -0

+
+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for

a

Quoted 33 lines

Quoted 23 lines

Quoted 13 lines

Quoted 2 lines

multi-queue
+ * model

I'm thinking of an alternative way w/o having the user to assign index and allowing the driver to poke object dependency (next patch).

Let's say the index is internally assigned by the driver. so this cmd is just for allowing a hw queue and it's the driver to decide the allocation policy, e.g. in ascending order.

Introduce a new flag in viommu_ops to indicate to core that the new hw queue should hold a reference to the previous hw queue.

Quoted 6 lines

core maintains a last_queue field in viommu. Upon success return from @hw_queue_alloc() the core increments the users refcnt of last_queue, records the dependency in iommufd_hw_queue struct, and update viommu->last_queue.

Then the destroy order is naturally guaranteed.

I have thought about that too. It's nice that the core can easily maintain the dependency for the driver.

Quoted 9 lines

But there would still need an out_index to mark each dynamically allocated queue. So VMM would know where it should map the queue.

For example, if VMM wants to allocate a queue at its own index=1 without allocating index=0 first, kernel cannot fail that as VMM doesn't provide the index. The only way left for kernel would be to output the allocated queue with index=0 and then wish VMM can validate it, which doesn't sound safe..

Quoted 6 lines

VMM's index is virtual which could be mapped to whatever queue object created at its own disposal.

the uAPI just requires VMM to remember a sequential list of allocated queue objects and destroy them in reverse order of allocation, instead of in the reverse order of virtual indexes.

But that's not going to work for VCMDQ.

VINTF mmaps only a single page that controls multiple queues. And all queues have to be mapped correctly between HW and VM indexes. Otherwise, it won't work if VMM maps:

Quoted 7 lines

HW-level VINTF1 LVCMDQ0 <==> VM-level VINTF0 LVCMDQ1 HW-level VINTF1 LVCMDQ1 <==> VM-level VINTF0 LVCMDQ0

So, one way or another, kernel has to ensure the static mappings of the indexes. And I think it's safer in the way that VMM tells what index to allocate..

Okay, that's a valid point.

But hey, we are already adding various restrictions to the uAPI about dependency, contiguity, etc. which the VMM should conform to. What hurts if we further say that the VMM should allocate virtual index in an ascending order along with hw queue allocation?

Quoted 4 lines · Kevin

But hey, we are already adding various restrictions to the uAPI about dependency, contiguity, etc. which the VMM should conform to. What hurts if we further say that the VMM should allocate virtual index in an ascending order along with hw queue allocation?

You mean adding another flag to manage the dependency in the core, right?

I talked with Jason offline when adding that depend API. He didn't want it to be in the core, saying that is a driver thing.

But that was before we added pin and contiguity, which he doesn't really enjoy being in the core either.

So, yea, I think you have a point here..

+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a multi-queue
+ * model
+ * @out_hw_queue_id: The ID of the new HW queue

+ * @base_addr: Base address of the queue memory in guest physical address space
+ * @length: Length of the queue memory in the guest physical address space
+ *
+ * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue, which
+ * allows HW to access a guest queue memory described by @base_addr and @length.
+ * Upon success, the underlying physical pages of the guest queue memory will be
+ * pinned to prevent VMM from unmapping them in the IOAS until the HW queue gets
+ * destroyed.

Quoted 9 lines · Nicolin Chen

+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a multi-queue
+ * model
+ * @out_hw_queue_id: The ID of the new HW queue

Quoted 8 lines

+ * @base_addr: Base address of the queue memory in guest physical address space
+ * @length: Length of the queue memory in the guest physical address space
+ *
+ * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue, which
+ * allows HW to access a guest queue memory described by @base_addr and @length.
+ * Upon success, the underlying physical pages of the guest queue memory will be
+ * pinned to prevent VMM from unmapping them in the IOAS until the HW queue gets
+ * destroyed.

Do we have way to make the pinning optional?

As I understand AMD's system the iommu HW itself translates the base_addr through the S2 page table automatically, so it doesn't need pinned memory and physical addresses but just the IOVA.

Perhaps for this reason the pinning should be done with a function call from the driver?

include/uapi/linux/iommufd.h struct iommu_veventq_alloc { · line 1188

Quoted 8 lines

include/uapi/linux/iommufd.h +8 -0

+struct iommu_hw_queue_alloc {
+	__u32 size;
+	__u32 flags;
+	__u32 viommu_id;
+	__u32 type;
+	__u32 index;
+	__u32 out_hw_queue_id;
+	__aligned_u64 base_addr;

base addr should probably be called nesting_parent_iova to match how we described the viommu hwpt:

* @hwpt_id: ID of a nesting parent HWPT to associate to

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

Quoted 7 lines

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

+	/*
+	 * The underlying physical pages must be pinned to prevent them from
+	 * being unmapped (via IOMMUFD_CMD_IOAS_UNMAP) during the life cycle
+	 * of the HW QUEUE object.
+	 */
+	rc = iopt_pin_pages(&hwpt->ioas->iopt, cmd->base_addr, cmd->length,
+			    pages, 0);

I don't think this actually works like this without an unmap callback. unmap will break:

			iommufd_access_notify_unmap(iopt, area_first, length);
			/* Something is not responding to unmap requests. */
			tries++;
			if (WARN_ON(tries > 100))
				return -EDEADLOCK;

If it can't shoot down the pinning.

Why did this need to change away from just a normal access? That ops and unmap callback are not optional things.

What vcmdq would do in the unmap callback is question I'm not sure of..

This is more reason to put the pin/access in the driver so it can provide an unmap callback that can fix it up.

I think this should have
been done just by using the normal access mechanism, maybe with a simplifying wrapper for in-driver use. ie no need for patch #9

Quoted 3 lines

As I understand AMD's system the iommu HW itself translates the base_addr through the S2 page table automatically, so it doesn't need pinned memory and physical addresses but just the IOVA.

Right. That's why we invented a flag, and it should be probably extended to cover the pin step as well.

Quoted 2 lines

Perhaps for this reason the pinning should be done with a function call from the driver?

But the whole point of doing in the core was to avoid the entire iopt related structure/function from being exposed to the driver, which would otherwise hugely increase the size of the driver.o?

Quoted 9 lines

I don't think this actually works like this without an unmap callback. unmap will break:

			iommufd_access_notify_unmap(iopt, area_first, length);
			/* Something is not responding to unmap requests. */
			tries++;
			if (WARN_ON(tries > 100))
				return -EDEADLOCK;

Quoted 1 line

If it can't shoot down the pinning.

Hmm, I thought we want the unmap to fail until VMM releases the HW QUEUE first? In what case, does VMM wants to unmap while holding the queue pages?

Quoted 2 lines

This is more reason to put the pin/access in the driver so it can provide an unmap callback that can fix it up.

As there are two types of "access" here.. you mean iommufd_access, i.e. vcmdq driver should hold an iommufd_access like an emulated vfio device driver?

Quoted 9 lines · Nicolin Chen

Quoted 9 lines

I don't think this actually works like this without an unmap callback. unmap will break:

			iommufd_access_notify_unmap(iopt, area_first, length);
			/* Something is not responding to unmap requests. */
			tries++;
			if (WARN_ON(tries > 100))
				return -EDEADLOCK;

Quoted 5 lines

Quoted 1 line

If it can't shoot down the pinning.

Hmm, I thought we want the unmap to fail until VMM releases the HW QUEUE first? In what case, does VMM wants to unmap while holding the queue pages?

Well, if that is what we want to do then this needs to be revised somehow..

Quoted 10 lines · Jason Gunthorpe

Quoted 9 lines · Nicolin Chen

Quoted 9 lines

I don't think this actually works like this without an unmap callback. unmap will break:

			iommufd_access_notify_unmap(iopt, area_first, length);
			/* Something is not responding to unmap requests. */
			tries++;
			if (WARN_ON(tries > 100))
				return -EDEADLOCK;

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

If it can't shoot down the pinning.

Hmm, I thought we want the unmap to fail until VMM releases the HW QUEUE first? In what case, does VMM wants to unmap while holding the queue pages?

Well, if that is what we want to do then this needs to be revised somehow..

Yea, unless we have a strong reason to allow unmap while holding the HW queue.

I think we could set a new flag:

enum {

(quoted diff) +1 -0

 	IOMMUFD_ACCESS_RW_READ = 0,
 	IOMMUFD_ACCESS_RW_WRITE = 1 << 0,
 	/* Set if the caller is in a kthread then rw will use kthread_use_mm() */
 	IOMMUFD_ACCESS_RW_KTHREAD = 1 << 1,
+	IOMMUFD_ACCESS_NO_UNMAP = 1 << 3,
 
 	/* Only for use by selftest */
 	__IOMMUFD_ACCESS_RW_SLOW_PATH = 1 << 2,
 };

and reject iopt_unmap_iova_range().

From: Jason Gunthorpe <[email protected]>
Sent: Friday, May 16, 2025 12:06 AM

Do we have way to make the pinning optional?

As I understand AMD's system the iommu HW itself translates the base_addr through the S2 page table automatically, so it doesn't need pinned memory and physical addresses but just the IOVA.

Quoted 5 lines

From: Jason Gunthorpe <[email protected]>
Sent: Friday, May 16, 2025 12:06 AM

Do we have way to make the pinning optional?

Quoted 4 lines

As I understand AMD's system the iommu HW itself translates the base_addr through the S2 page table automatically, so it doesn't need pinned memory and physical addresses but just the IOVA.

Though using IOVA could eliminate pinning conceptually, implementation wise an IOMMU may not tolerate translation errors in its access to guest queues with assumption that S2 is pinned.

Quoted 9 lines · Nicolin Chen

+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a multi-queue
+ * model
+ * @out_hw_queue_id: The ID of the new HW queue

Quoted 8 lines

+ * @base_addr: Base address of the queue memory in guest physical address space
+ * @length: Length of the queue memory in the guest physical address space
+ *
+ * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue, which
+ * allows HW to access a guest queue memory described by @base_addr and @length.
+ * Upon success, the underlying physical pages of the guest queue memory will be
+ * pinned to prevent VMM from unmapping them in the IOAS until the HW queue gets
+ * destroyed.

Do we have way to make the pinning optional?

As I understand AMD's system the iommu HW itself translates the base_addr through the S2 page table automatically, so it doesn't need pinned memory and physical addresses but just the IOVA.

Perhaps for this reason the pinning should be done with a function call from the driver?

Jason, Nicolin, Kevin,

Quoted 19 lines · Jason Gunthorpe

Quoted 9 lines · Nicolin Chen

+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a multi-queue
+ * model
+ * @out_hw_queue_id: The ID of the new HW queue

Quoted 8 lines

+ * @base_addr: Base address of the queue memory in guest physical address space
+ * @length: Length of the queue memory in the guest physical address space
+ *
+ * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue, which
+ * allows HW to access a guest queue memory described by @base_addr and @length.
+ * Upon success, the underlying physical pages of the guest queue memory will be
+ * pinned to prevent VMM from unmapping them in the IOAS until the HW queue gets
+ * destroyed.

Quoted 5 lines

Do we have way to make the pinning optional?

As I understand AMD's system the iommu HW itself translates the base_addr through the S2 page table automatically, so it doesn't need pinned memory and physical addresses but just the IOVA.

Correct. HW will translate GPA -> SPA automatically using below information.

AMD IOMMU need special device ID to setup with GPA -> SPA mapping per VM. and its programmed in VF Control BAR (VFCntlMMIO Offset {16’b[GuestID], 6’b01_0000} Guest Miscellaneous Control Register). IOMMU HW will use this address for GPA to SPA translation for buffers like command buffer.

So HW will use Base address (GPA), head/tail pointer to get the offset from Base. Then it will use GPA -> SPA translation.

Quoted 1 line

Quoted 2 lines

Perhaps for this reason the pinning should be done with a function call from the driver?

We still need to make sure memory allocated for page is present in memory so that IOMMU HW can access it.

Pinning at the time of guest boot is enough here -OR- do we need to increase reference in queue_alloc() path ?

Quoted 41 lines · Vasant Hegde

Jason, Nicolin, Kevin,

Quoted 19 lines · Jason Gunthorpe

Quoted 9 lines · Nicolin Chen

+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a multi-queue
+ * model
+ * @out_hw_queue_id: The ID of the new HW queue

Quoted 8 lines

+ * @base_addr: Base address of the queue memory in guest physical address space
+ * @length: Length of the queue memory in the guest physical address space
+ *
+ * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue, which
+ * allows HW to access a guest queue memory described by @base_addr and @length.
+ * Upon success, the underlying physical pages of the guest queue memory will be
+ * pinned to prevent VMM from unmapping them in the IOAS until the HW queue gets
+ * destroyed.

Quoted 5 lines

Do we have way to make the pinning optional?

As I understand AMD's system the iommu HW itself translates the base_addr through the S2 page table automatically, so it doesn't need pinned memory and physical addresses but just the IOVA.

Correct. HW will translate GPA -> SPA automatically using below information.

AMD IOMMU need special device ID to setup with GPA -> SPA mapping per VM. and its programmed in VF Control BAR (VFCntlMMIO Offset {16’b[GuestID], 6’b01_0000} Guest Miscellaneous Control Register). IOMMU HW will use this address for GPA to SPA translation for buffers like command buffer.

So HW will use Base address (GPA), head/tail pointer to get the offset from Base. Then it will use GPA -> SPA translation.

Quoted 1 line

Quoted 8 lines

Quoted 2 lines

Perhaps for this reason the pinning should be done with a function call from the driver?

We still need to make sure memory allocated for page is present in memory so that IOMMU HW can access it.

Pinning at the time of guest boot is enough here -OR- do we need to increase reference in queue_alloc() path ?

For NVIDIA's vCMDQ that reads host PA directly, pages should be pinned once when stage 2 mappings are created for the guest RAM, and iommu_hw_queue_alloc() should pin the pages again to prevent the gPA from being unmapped in the stage 2 page table. Otherwise it will be a security hole, as HW continues to read the unmapped memory through physical address space.

I understand that AMD Command Buffer also needs the S2 mappings to be present in order to work correctly. But what happens if a queue memory that isn't pinned (or even gets unmapped)? Will it raise a translation fault v.s. HW reading the unmapped memory?

If so, I think this is Jason's point: there would be unlikely a security hole, i.e. for AMD, iommu_hw_queue_alloc() pinning the physical pages is likely optional.

From: Vasant Hegde <[email protected]>
Sent: Tuesday, May 20, 2025 4:39 PM

Hi Nicolin,

Quoted 5 lines · Nicolin Chen

Quoted 4 lines · Vasant Hegde

Jason, Nicolin, Kevin,

On 5/15/2025 9:36 PM, Jason Gunthorpe wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines · Nicolin Chen

Quoted 7 lines

+/**
+ * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
+ * @size: sizeof(struct iommu_hw_queue_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the HW queue with
+ * @type: One of enum iommu_hw_queue_type
+ * @index: The logical index to the HW queue per virtual IOMMU for a

multi-queue

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

+ * model
+ * @out_hw_queue_id: The ID of the new HW queue
+ * @base_addr: Base address of the queue memory in guest physical

address space

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ * @length: Length of the queue memory in the guest physical address

space

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+ *
+ * Allocate a HW queue object for a vIOMMU-specific HW-accelerated

queue, which

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ * allows HW to access a guest queue memory described by

@base_addr and @length.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ * Upon success, the underlying physical pages of the guest queue

memory will be

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ * pinned to prevent VMM from unmapping them in the IOAS until the

HW queue gets

Quoted 9 lines

Quoted 9 lines

Quoted 7 lines

Quoted 1 line

+ * destroyed.

Do we have way to make the pinning optional?

As I understand AMD's system the iommu HW itself translates the base_addr through the S2 page table automatically, so it doesn't need pinned memory and physical addresses but just the IOVA.

Correct. HW will translate GPA -> SPA automatically using below

information.

Quoted 2 lines

Quoted 2 lines

AMD IOMMU need special device ID to setup with GPA -> SPA mapping

per VM.

Quoted 1 line

Quoted 1 line

and its programmed in VF Control BAR (VFCntlMMIO Offset

{16’b[GuestID],

Quoted 1 line

Quoted 1 line

6’b01_0000} Guest Miscellaneous Control Register). IOMMU HW will use

this

Quoted 3 lines

Quoted 3 lines

address for GPA to SPA translation for buffers like command buffer.

So HW will use Base address (GPA), head/tail pointer to get the offset

from

Quoted 8 lines

Quoted 8 lines

Base. Then it will use GPA -> SPA translation.

Quoted 3 lines

Perhaps for this reason the pinning should be done with a function call from the driver?

We still need to make sure memory allocated for page is present in

memory so

Quoted 3 lines

Quoted 3 lines

that IOMMU HW can access it.

Pinning at the time of guest boot is enough here -OR- do we need to

increase

Quoted 9 lines

Quoted 1 line

reference in queue_alloc() path ?

For NVIDIA's vCMDQ that reads host PA directly, pages should be pinned once when stage 2 mappings are created for the guest RAM, and iommu_hw_queue_alloc() should pin the pages again to prevent the gPA from being unmapped in the stage 2 page table. Otherwise it will be a security hole, as HW continues to read the unmapped memory through physical address space.

Quoted 4 lines

I understand that AMD Command Buffer also needs the S2 mappings to be present in order to work correctly. But what happens if a queue memory that isn't pinned (or even gets unmapped)? Will it raise a translation fault v.s. HW reading the unmapped memory?

If page is unmapped then stage 2 (Host page table) gets updated. IOMMU will not
be able to find page and logs fault.

Quoted 3 lines

If page is unmapped then stage 2 (Host page table) gets updated. IOMMU will not
be able to find page and logs fault.

As long as the fault is contained only for the relevant queue, yes we don't need another pinning from the driver.

include/linux/iommufd.h +5 -0

+/*
+ * Helpers for IOMMU driver to build/destroy a dependency between two sibling
+ * structures created by one of the allocators above
+ */
+#define iommufd_hw_queue_depend(dependent, depended, member)                   \

Quoted 5 lines · Nicolin Chen

include/linux/iommufd.h +5 -0

+/*
+ * Helpers for IOMMU driver to build/destroy a dependency between two sibling
+ * structures created by one of the allocators above
+ */
+#define iommufd_hw_queue_depend(dependent, depended, member)                   \

include/linux/iommufd.h static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu, · line 380

Quoted 10 lines

include/linux/iommufd.h +10 -0

+	({                                                                     \
+		static_assert(__same_type(struct iommufd_hw_queue,             \
+					  dependent->member));                 \
+		static_assert(offsetof(typeof(*dependent), member.obj) == 0);  \
+		static_assert(__same_type(struct iommufd_hw_queue,             \
+					  depended->member));                  \
+		static_assert(offsetof(typeof(*depended), member.obj) == 0);   \
+		_iommufd_object_depend(&dependent->member.obj,                 \
+				       &depended->member.obj);                 \
+	})

This doesn't need the offsetof == 0 checks, it isn't an allocator. And you want to check that the two structs have the same type:

static_assert(__same_type(struct iommufd_hw_queue, dependent->member)); static_assert(__same_type(typeof(*dependent), *dependend));