iommufd: Add vIOMMU infrastructure (Part-4 vCMDQ)

include/linux/iommufd.h +3 -0

+ * @vdevice_destroy: Clean up all driver-specific parts of an iommufd_vdevice.
+ *                   The memory of the vDEVICE will be free-ed by iommufd core
+ *                   after calling this op
   */
  struct iommufd_viommu_ops {
  	void (*destroy)(struct iommufd_viommu *viommu);

From: Baolu Lu <[email protected]>
Sent: Sunday, April 27, 2025 2:24 PM

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

Quoted 5 lines · Nicolin Chen

include/linux/iommufd.h +1 -0

@@ -120,6 +128,13 @@ 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_alloc: Allocate a vDEVICE object and init its driver-level

structure

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

Quoted 3 lines

include/linux/iommufd.h +3 -0

+ *                 or HW procedure. Note that the core-level structure is filled
+ *                 by the iommufd core after calling this op. @virt_id carries a
+ *                 per-vIOMMU virtual ID for the driver to initialize its HW.

I'm wondering whether the 'per-vIOMMU virtual ID' is intended to be generic for other features that might require a vdevice. I'm also not sure where this virtual ID originates when I read it here. Could it

potentially come from the KVM instance? If so, how about retrieving it directly from a struct kvm pointer? My understanding is that vIOMMU in IOMMUFD acts as a handle to KVM, so perhaps we should maintain a reference to the kvm pointer within the iommufd_viommu structure?

Quoted 14 lines

From: Baolu Lu <[email protected]>
Sent: Sunday, April 27, 2025 2:24 PM

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

Quoted 5 lines · Nicolin Chen

include/linux/iommufd.h +1 -0

@@ -120,6 +128,13 @@ 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_alloc: Allocate a vDEVICE object and init its driver-level

structure

Quoted 3 lines

include/linux/iommufd.h +3 -0

+ *                 or HW procedure. Note that the core-level structure is filled
+ *                 by the iommufd core after calling this op. @virt_id carries a
+ *                 per-vIOMMU virtual ID for the driver to initialize its HW.

Quoted 3 lines

I'm wondering whether the 'per-vIOMMU virtual ID' is intended to be generic for other features that might require a vdevice. I'm also not sure where this virtual ID originates when I read it here. Could it

for PCI it's the virtual BDF in the guest PCI topology, hence provided by the VMM when calling @vdevice_alloc:

The new type of vIOMMU for tegra241-cmdqv needs to pass in a driver-level data structure from user space via iommufd, so add a user_data to the op.

Reviewed-by Jason Gunthorpe<[email protected]>

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

Quoted 3 lines · Nicolin Chen

The new type of vIOMMU for tegra241-cmdqv needs to pass in a driver-level data structure from user space via iommufd, so add a user_data to the op.

Quoted 2 lines

Reviewed-by Jason Gunthorpe<[email protected]>

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

It would be better to add some words explaining what kind of user data can be passed when allocating a vIOMMU object and the reason why this might be necessary.

Reviewed-by Lu Baolu <[email protected]>

Quoted 4 lines · Baolu Lu

Quoted 3 lines · Nicolin Chen

The new type of vIOMMU for tegra241-cmdqv needs to pass in a driver-level data structure from user space via iommufd, so add a user_data to the op.

Quoted 8 lines

Quoted 2 lines

Reviewed-by Jason Gunthorpe<[email protected]>

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

It would be better to add some words explaining what kind of user data can be passed when allocating a vIOMMU object and the reason why this might be necessary.

Reviewed-by Lu Baolu <[email protected]>

Sure. Will do something like this:

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 (vm_pgoff and size), 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.

Thanks
Nicolin

The new type of vIOMMU for tegra241-cmdqv driver needs a driver-specific user data. So, add data_len/uptr to the iommu_viommu_alloc uAPI and pass it in via the viommu_alloc iommu op.

Reviewed-by Jason Gunthorpe<[email protected]>

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

The new type of vIOMMU for tegra241-cmdqv driver needs a driver-specific user data. So, add data_len/uptr to the iommu_viommu_alloc uAPI and pass it in via the viommu_alloc iommu op.

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 include/uapi/linux/iommufd.h   | 6 ++++++
 drivers/iommu/iommufd/viommu.c | 8 +++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

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

index f29b6c44655e..cc90299a08d9 100644
@@ -965,6 +965,9 @@ enum iommu_viommu_type {
  * @dev_id: The device's physical IOMMU will be used to back the virtual IOMMU
  * @hwpt_id: ID of a nesting parent HWPT to associate to
  * @out_viommu_id: Output virtual IOMMU ID for the allocated object
+ * @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
  *
  * Allocate a virtual IOMMU object, representing the underlying physical IOMMU's
  * virtualization support that is a security-isolated slice of the real IOMMU HW

@@ -985,6 +988,9 @@ struct iommu_viommu_alloc {
 	__u32 dev_id;
 	__u32 hwpt_id;
 	__u32 out_viommu_id;
+	__u32 data_len;
+	__u32 __reserved;
+	__aligned_u64 data_uptr;
 };
 #define IOMMU_VIOMMU_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VIOMMU_ALLOC)
 

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

index fffa57063c60..a65153458a26 100644
@@ -17,6 +17,11 @@ void iommufd_viommu_destroy(struct iommufd_object *obj)
 int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 {
 	struct iommu_viommu_alloc *cmd = ucmd->cmd;
+	const struct iommu_user_data user_data = {
+		.type = cmd->type,
+		.uptr = u64_to_user_ptr(cmd->data_uptr),
+		.len = cmd->data_len,
+	};
 	struct iommufd_hwpt_paging *hwpt_paging;
 	struct iommufd_viommu *viommu;
 	struct iommufd_device *idev;

@@ -48,7 +53,8 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	}
 
 	viommu = ops->viommu_alloc(idev->dev, hwpt_paging->common.domain,
-				   ucmd->ictx, cmd->type, NULL);
+				   ucmd->ictx, cmd->type,
+				   user_data.len ? &user_data : NULL);
 	if (IS_ERR(viommu)) {
 		rc = PTR_ERR(viommu);
 		goto out_put_hwpt;

Signature · 1 line

2.43.0

The new type of vIOMMU for tegra241-cmdqv driver needs a driver-specific user data. So, add data_len/uptr to the iommu_viommu_alloc uAPI and pass it in via the viommu_alloc iommu op.

Reviewed-by Jason Gunthorpe<[email protected]>

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

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]>

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

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]>

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

---
 include/linux/iommu.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

include/linux/iommu.h +0 -0

index ba7add27e9a0..634ff647888d 100644

include/linux/iommu.h +27 -0

+ * @data_len: Length of current user data structure, i.e. sizeof(struct _src)
+ * @min_len: Initial length of user data structure for backward compatibility.
+ *           This should be offsetofend using the last member in the user data
+ *           struct that was initially added to include/uapi/linux/iommufd.h
+ */
+static inline int
+__iommu_copy_struct_to_user(const struct iommu_user_data *dst_data,
+			    void *src_data, unsigned int data_type,
+			    size_t data_len, size_t min_len)
+{
+	if (WARN_ON(!dst_data || !src_data))
+		return -EINVAL;
+	if (dst_data->type != data_type)
+		return -EINVAL;
+	if (dst_data->len < min_len || data_len < dst_data->len)
+		return -EINVAL;
+	return copy_struct_to_user(dst_data->uptr, dst_data->len, src_data,
+				   data_len, NULL);
+}
+
+/**
+ * iommu_copy_struct_to_user - Report iommu driver specific user space data
+ * @user_data: Pointer to a struct iommu_user_data for user space data location
+ * @ksrc: Pointer to an iommu driver specific user data that is defined in
+ *        include/uapi/linux/iommufd.h
+ * @data_type: The data type of the @ksrc. Must match with @user_data->type
+ * @min_last: The last member of the data structure @ksrc points in the initial

Signature · 1 line

2.43.0

Quoted 18 lines · Nicolin Chen

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]>

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

---
 include/linux/iommu.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

include/linux/iommu.h +0 -0

index ba7add27e9a0..634ff647888d 100644
@@ -562,6 +562,46 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
 	return 0;
 }
 

include/linux/iommu.h iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size, · line 570

Quoted 6 lines

include/linux/iommu.h +6 -0

+/**
+ * __iommu_copy_struct_to_user - Report iommu driver specific user space data
+ * @dst_data: Pointer to a struct iommu_user_data for user space data location
+ * @src_data: Pointer to an iommu driver specific user data that is defined in
+ *            include/uapi/linux/iommufd.h
+ * @data_type: The data type of the @dst_data. Must match with @src_data.type

								   ^

Nit: Must match with @dst_data type.

Quoted 19 lines · Pranjal Shrivastava

Quoted 18 lines · Nicolin Chen

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]>

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

---
 include/linux/iommu.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

include/linux/iommu.h +0 -0

index ba7add27e9a0..634ff647888d 100644
@@ -562,6 +562,46 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
 	return 0;
 }
 

Quoted 8 lines

include/linux/iommu.h iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size, · line 570

Quoted 6 lines

include/linux/iommu.h +6 -0

+/**
+ * __iommu_copy_struct_to_user - Report iommu driver specific user space data
+ * @dst_data: Pointer to a struct iommu_user_data for user space data location
+ * @src_data: Pointer to an iommu driver specific user data that is defined in
+ *            include/uapi/linux/iommufd.h
+ * @data_type: The data type of the @dst_data. Must match with @src_data.type

								   ^

Nit: Must match with @dst_data type.

Oh, that's a copy-n-paste mistake. It should be:
* @data_type: The data type of the @src_data. Must match with @dst_data.type

Thanks!
Nicolin

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.

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]>

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

Quoted 3 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.

A brief explanation or a small code snippet illustrating a typical allocation and potential abort scenario would be helpful.

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

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

---
 drivers/iommu/iommufd/iommufd_test.h | 13 +++++++++++++
 drivers/iommu/iommufd/selftest.c     | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+)

drivers/iommu/iommufd/iommufd_test.h +13 -0

index 1cd7e8394129..fbf9ecb35a13 100644
@@ -227,6 +227,19 @@ struct iommu_hwpt_invalidate_selftest {
 
 #define IOMMU_VIOMMU_TYPE_SELFTEST 0xdeadbeef
 
+/**
+ * struct iommu_viommu_selftest - vIOMMU data for Mock driver
+ *                                (IOMMU_VIOMMU_TYPE_SELFTEST)
+ * @in_data: Input random data from user space
+ * @out_data: Output data (matching @in_data) to user space
+ *
+ * Simply set @out_data=@in_data for a loopback test
+ */
+struct iommu_viommu_selftest {
+	__u32 in_data;
+	__u32 out_data;
+};
+
 /* Should not be equal to any defined value in enum iommu_viommu_invalidate_data_type */
 #define IOMMU_VIOMMU_INVALIDATE_DATA_SELFTEST 0xdeadbeef
 #define IOMMU_VIOMMU_INVALIDATE_DATA_SELFTEST_INVALID 0xdadbeef

drivers/iommu/iommufd/selftest.c +12 -0

index 8b8ba4fb91cd..b04bd2fbc53d 100644
@@ -740,16 +740,35 @@ mock_viommu_alloc(struct device *dev, struct iommu_domain *domain,
 {
 	struct mock_iommu_device *mock_iommu =
 		iommu_get_iommu_dev(dev, struct mock_iommu_device, iommu_dev);
+	struct iommu_viommu_selftest data;
 	struct mock_viommu *mock_viommu;
+	int rc;
 
 	if (viommu_type != IOMMU_VIOMMU_TYPE_SELFTEST)
 		return ERR_PTR(-EOPNOTSUPP);
 
+	if (user_data) {
+		rc = iommu_copy_struct_from_user(
+			&data, user_data, IOMMU_VIOMMU_TYPE_SELFTEST, out_data);
+		if (rc)
+			return ERR_PTR(rc);
+	}
+
 	mock_viommu = iommufd_viommu_alloc(ictx, struct mock_viommu, core,
 					   &mock_viommu_ops);
 	if (IS_ERR(mock_viommu))
 		return ERR_CAST(mock_viommu);
 
+	if (user_data) {
+		data.out_data = data.in_data;
+		rc = iommu_copy_struct_to_user(

Signature · 1 line

2.43.0

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

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

---
 tools/testing/selftests/iommu/iommufd_utils.h | 21 +++++++++-----
 tools/testing/selftests/iommu/iommufd.c       | 29 +++++++++++++++----
 .../selftests/iommu/iommufd_fail_nth.c        |  5 ++--
 3 files changed, 39 insertions(+), 16 deletions(-)

tools/testing/selftests/iommu/iommufd_utils.h +13 -8

index 72f6636e5d90..a5d4cbd089ba 100644
@@ -897,7 +897,8 @@ static int _test_cmd_trigger_iopf(int fd, __u32 device_id, __u32 pasid,
 					    pasid, fault_fd))
 
 static int _test_cmd_viommu_alloc(int fd, __u32 device_id, __u32 hwpt_id,
-				  __u32 type, __u32 flags, __u32 *viommu_id)
+				  __u32 flags, __u32 type, void *data,
+				  __u32 data_len, __u32 *viommu_id)
 {
 	struct iommu_viommu_alloc cmd = {
 		.size = sizeof(cmd),

@@ -905,6 +906,8 @@ static int _test_cmd_viommu_alloc(int fd, __u32 device_id, __u32 hwpt_id,
 		.type = type,
 		.dev_id = device_id,
 		.hwpt_id = hwpt_id,
+		.data_uptr = (uint64_t)data,
+		.data_len = data_len,
 	};
 	int ret;
 

@@ -916,13 +919,15 @@ static int _test_cmd_viommu_alloc(int fd, __u32 device_id, __u32 hwpt_id,
 	return 0;
 }
 
-#define test_cmd_viommu_alloc(device_id, hwpt_id, type, viommu_id)        \
-	ASSERT_EQ(0, _test_cmd_viommu_alloc(self->fd, device_id, hwpt_id, \
-					    type, 0, viommu_id))
-#define test_err_viommu_alloc(_errno, device_id, hwpt_id, type, viommu_id) \
-	EXPECT_ERRNO(_errno,                                               \
-		     _test_cmd_viommu_alloc(self->fd, device_id, hwpt_id,  \
-					    type, 0, viommu_id))
+#define test_cmd_viommu_alloc(device_id, hwpt_id, type, data, data_len,      \
+			      viommu_id)                                     \
+	ASSERT_EQ(0, _test_cmd_viommu_alloc(self->fd, device_id, hwpt_id, 0, \
+					    type, data, data_len, viommu_id))
+#define test_err_viommu_alloc(_errno, device_id, hwpt_id, type, data,        \
+			      data_len, viommu_id)                           \
+	EXPECT_ERRNO(_errno,                                                 \
+		     _test_cmd_viommu_alloc(self->fd, device_id, hwpt_id, 0, \
+					    type, data, data_len, viommu_id))
 
 static int _test_cmd_vdevice_alloc(int fd, __u32 viommu_id, __u32 idev_id,
 				   __u64 virt_id, __u32 *vdev_id)

tools/testing/selftests/iommu/iommufd.c +23 -6

index 1a8e85afe9aa..8ebbb7fda02d 100644
@@ -2688,7 +2688,7 @@ FIXTURE_SETUP(iommufd_viommu)
 
 		/* Allocate a vIOMMU taking refcount of the parent hwpt */
 		test_cmd_viommu_alloc(self->device_id, self->hwpt_id,
-				      IOMMU_VIOMMU_TYPE_SELFTEST,
+				      IOMMU_VIOMMU_TYPE_SELFTEST, NULL, 0,
 				      &self->viommu_id);
 
 		/* Allocate a regular nested hwpt */

@@ -2727,24 +2727,27 @@ TEST_F(iommufd_viommu, viommu_negative_tests)
 	if (self->device_id) {
 		/* Negative test -- invalid hwpt (hwpt_id=0) */
 		test_err_viommu_alloc(ENOENT, device_id, 0,
-				      IOMMU_VIOMMU_TYPE_SELFTEST, NULL);
+				      IOMMU_VIOMMU_TYPE_SELFTEST, NULL, 0,
+				      NULL);
 
 		/* Negative test -- not a nesting parent hwpt */
 		test_cmd_hwpt_alloc(device_id, ioas_id, 0, &hwpt_id);
 		test_err_viommu_alloc(EINVAL, device_id, hwpt_id,
-				      IOMMU_VIOMMU_TYPE_SELFTEST, NULL);
+				      IOMMU_VIOMMU_TYPE_SELFTEST, NULL, 0,
+				      NULL);
 		test_ioctl_destroy(hwpt_id);
 
 		/* Negative test -- unsupported viommu type */
 		test_err_viommu_alloc(EOPNOTSUPP, device_id, self->hwpt_id,
-				      0xdead, NULL);
+				      0xdead, NULL, 0, NULL);
 		EXPECT_ERRNO(EBUSY,
 			     _test_ioctl_destroy(self->fd, self->hwpt_id));
 		EXPECT_ERRNO(EBUSY,
 			     _test_ioctl_destroy(self->fd, self->viommu_id));
 	} else {
 		test_err_viommu_alloc(ENOENT, self->device_id, self->hwpt_id,
-				      IOMMU_VIOMMU_TYPE_SELFTEST, NULL);
+				      IOMMU_VIOMMU_TYPE_SELFTEST, NULL, 0,
+				      NULL);
 	}
 }
 

@@ -2791,6 +2794,20 @@ TEST_F(iommufd_viommu, viommu_alloc_nested_iopf)
 	}
 }
 
+TEST_F(iommufd_viommu, viommu_alloc_with_data)
+{
+	struct iommu_viommu_selftest data = {
+		.in_data = 0xbeef,
+	};
+
+	if (self->device_id) {
+		test_cmd_viommu_alloc(self->device_id, self->hwpt_id,
+				      IOMMU_VIOMMU_TYPE_SELFTEST, &data,
+				      sizeof(data), &self->viommu_id);
+		assert(data.out_data == data.in_data);
+	}
+}
+
 TEST_F(iommufd_viommu, vdevice_alloc)
 {
 	uint32_t viommu_id = self->viommu_id;

@@ -3105,7 +3122,7 @@ TEST_F(iommufd_device_pasid, pasid_attach)
 
 	/* Allocate a regular nested hwpt based on viommu */
 	test_cmd_viommu_alloc(self->device_id, parent_hwpt_id,
-			      IOMMU_VIOMMU_TYPE_SELFTEST,
+			      IOMMU_VIOMMU_TYPE_SELFTEST, NULL, 0,
 			      &viommu_id);
 	test_cmd_hwpt_alloc_nested(self->device_id, viommu_id,
 				   IOMMU_HWPT_ALLOC_PASID,

tools/testing/selftests/iommu/iommufd_fail_nth.c +3 -2

index e11ec4b121fc..f7ccf1822108 100644
@@ -688,8 +688,9 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 				 IOMMU_HWPT_DATA_NONE, 0, 0))
 		return -1;
 
-	if (_test_cmd_viommu_alloc(self->fd, idev_id, hwpt_id,
-				   IOMMU_VIOMMU_TYPE_SELFTEST, 0, &viommu_id))
+	if (_test_cmd_viommu_alloc(self->fd, idev_id, hwpt_id, 0,
+				   IOMMU_VIOMMU_TYPE_SELFTEST, NULL, 0,
+				   &viommu_id))
 		return -1;
 
 	if (_test_cmd_vdevice_alloc(self->fd, viommu_id, idev_id, 0, &vdev_id))

Signature · 1 line

2.43.0

The new vCMDQ object will be added for HW to access the guest memory for a 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 vCMDQ allocator. As the vCMDQ allocator will be a user-space triggered ioctl function, WARN_ON would not be a good fit in the new iopt_unpin_pages(), thus change them to use WARN_ON_ONCE instead.

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.

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(-)

Quoted 5 lines · Nicolin Chen

The new vCMDQ object will be added for HW to access the guest memory for a 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.

Quoted 8 lines

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 vCMDQ allocator. As the vCMDQ allocator will be a user-space triggered ioctl function, WARN_ON would not be a good fit in the new iopt_unpin_pages(), thus change them to use WARN_ON_ONCE instead.

I'm uncertain, but perhaps pr_warn_ratelimited() would be a better alternative to WARN_ON() here? WARN_ON_ONCE() generates warning messages with kernel call traces in the kernel messages, which might lead users to believe that something serious has happened in the kernel.

Quoted 4 lines

I'm uncertain, but perhaps pr_warn_ratelimited() would be a better alternative to WARN_ON() here? WARN_ON_ONCE() generates warning messages with kernel call traces in the kernel messages, which might lead users to believe that something serious has happened in the kernel.

We already have similar practice, e.g. iommufd_hwpt_nested_alloc.

In my review, a WARN_ON/WARN_ON_ONCE means there is a kernel bug, which shouldn't occur in the first place and isn't something that

Quoted 7 lines · Nicolin Chen

Quoted 4 lines

I'm uncertain, but perhaps pr_warn_ratelimited() would be a better alternative to WARN_ON() here? WARN_ON_ONCE() generates warning messages with kernel call traces in the kernel messages, which might lead users to believe that something serious has happened in the kernel.

We already have similar practice, e.g. iommufd_hwpt_nested_alloc.

Quoted 2 lines

In my review, a WARN_ON/WARN_ON_ONCE means there is a kernel bug, which shouldn't occur in the first place and isn't something that

Right, so it should never happen from any ioctl path and syzkaller should never trigger it based on system call randomization

Is that what this achieves?

Quoted 14 lines · Jason Gunthorpe

Quoted 9 lines · Nicolin Chen

Quoted 4 lines

I'm uncertain, but perhaps pr_warn_ratelimited() would be a better alternative to WARN_ON() here? WARN_ON_ONCE() generates warning messages with kernel call traces in the kernel messages, which might lead users to believe that something serious has happened in the kernel.

We already have similar practice, e.g. iommufd_hwpt_nested_alloc.

In my review, a WARN_ON/WARN_ON_ONCE means there is a kernel bug, which shouldn't occur in the first place and isn't something that

Right, so it should never happen from any ioctl path and syzkaller should never trigger it based on system call randomization

Quoted 1 line

Is that what this achieves?

The functions would be still used in the kernel path. So, I think we need to retain these warnings for that. But given that an ioctl could trigger a series of WARN_ONs, WARN_ON_ONCE is something that wouldn't bother user space a lot while it provides the kernel path enough info to debug.

Quoted 17 lines · Nicolin Chen

Quoted 15 lines · Jason Gunthorpe

Quoted 9 lines · Nicolin Chen

Quoted 4 lines

I'm uncertain, but perhaps pr_warn_ratelimited() would be a better alternative to WARN_ON() here? WARN_ON_ONCE() generates warning messages with kernel call traces in the kernel messages, which might lead users to believe that something serious has happened in the kernel.

We already have similar practice, e.g. iommufd_hwpt_nested_alloc.

In my review, a WARN_ON/WARN_ON_ONCE means there is a kernel bug, which shouldn't occur in the first place and isn't something that

Right, so it should never happen from any ioctl path and syzkaller should never trigger it based on system call randomization

Is that what this achieves?

Quoted 5 lines

The functions would be still used in the kernel path. So, I think we need to retain these warnings for that. But given that an ioctl could trigger a series of WARN_ONs, WARN_ON_ONCE is something that wouldn't bother user space a lot while it provides the kernel path enough info to debug.

No, it does bother userspace, we must not have ioctl triggerable WARN_ON at all.

Quoted 24 lines · Jason Gunthorpe

Quoted 22 lines · Nicolin Chen

Quoted 8 lines · Jason Gunthorpe

Quoted 7 lines · Nicolin Chen

Quoted 4 lines

I'm uncertain, but perhaps pr_warn_ratelimited() would be a better alternative to WARN_ON() here? WARN_ON_ONCE() generates warning messages with kernel call traces in the kernel messages, which might lead users to believe that something serious has happened in the kernel.

We already have similar practice, e.g. iommufd_hwpt_nested_alloc.

Quoted 7 lines

Quoted 2 lines

In my review, a WARN_ON/WARN_ON_ONCE means there is a kernel bug, which shouldn't occur in the first place and isn't something that

Right, so it should never happen from any ioctl path and syzkaller should never trigger it based on system call randomization

Is that what this achieves?

The functions would be still used in the kernel path. So, I think we need to retain these warnings for that. But given that an ioctl could trigger a series of WARN_ONs, WARN_ON_ONCE is something that wouldn't bother user space a lot while it provides the kernel path enough info to debug.

Quoted 2 lines

No, it does bother userspace, we must not have ioctl triggerable WARN_ON at all.

You mean we have to eliminate any WARN_ON in a call path of an ioctl?

We can drop them, just that would mute any kernel bug.

Btw, IIRC, the destroy ioctl could trigger some WARN_ON in the remove() when the object's refcount isn't correctly decreased. Should that be a problem too?

Quoted 25 lines · Nicolin Chen

Quoted 24 lines · Jason Gunthorpe

Quoted 22 lines · Nicolin Chen

Quoted 8 lines · Jason Gunthorpe

Quoted 7 lines · Nicolin Chen

Quoted 4 lines

I'm uncertain, but perhaps pr_warn_ratelimited() would be a better alternative to WARN_ON() here? WARN_ON_ONCE() generates warning messages with kernel call traces in the kernel messages, which might lead users to believe that something serious has happened in the kernel.

We already have similar practice, e.g. iommufd_hwpt_nested_alloc.

Quoted 7 lines

Quoted 2 lines

In my review, a WARN_ON/WARN_ON_ONCE means there is a kernel bug, which shouldn't occur in the first place and isn't something that

Right, so it should never happen from any ioctl path and syzkaller should never trigger it based on system call randomization

Is that what this achieves?

The functions would be still used in the kernel path. So, I think we need to retain these warnings for that. But given that an ioctl could trigger a series of WARN_ONs, WARN_ON_ONCE is something that wouldn't bother user space a lot while it provides the kernel path enough info to debug.

Quoted 5 lines

Quoted 2 lines

No, it does bother userspace, we must not have ioctl triggerable WARN_ON at all.

You mean we have to eliminate any WARN_ON in a call path of an ioctl?

Yes, not one that derives from user information. You can WARN_ON if internal kernel structures are corrupted but not if user ioctl arguments are bad.

Quoted 1 line

We can drop them, just that would mute any kernel bug.

That maybe the right answer

Quoted 3 lines

Btw, IIRC, the destroy ioctl could trigger some WARN_ON in the remove() when the object's refcount isn't correctly decreased. Should that be a problem too?

That is kernel data structures being corrupted.

In this case, I think it's similar, so we should retain them, as they were?

The new vCMDQ object will be added for HW to access the guest memory for a 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 vCMDQ allocator. As the vCMDQ allocator will be a user-space triggered ioctl function, WARN_ON would not be a good fit in the new iopt_unpin_pages(), thus change them to use WARN_ON_ONCE instead.

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.

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(-)

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

index 10c928a9a463..4288a2b1a90f 100644
@@ -114,6 +114,14 @@ static inline unsigned long iopt_area_iova_to_index(struct iopt_area *area,
 	return iopt_area_start_byte(area, iova) / PAGE_SIZE;
 }
 
+static inline bool iopt_area_check_prot(struct iopt_area *area,
+					unsigned int flags)
+{
+	if (flags & IOMMUFD_ACCESS_RW_WRITE)
+		return area->iommu_prot & IOMMU_WRITE;
+	return area->iommu_prot & IOMMU_READ;
+}
+
 #define __make_iopt_iter(name)                                                 \
 	static inline struct iopt_##name *iopt_##name##_iter_first(            \
 		struct io_pagetable *iopt, unsigned long start,                \

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

index 8d96aa514033..79160b039bc7 100644
@@ -130,6 +130,12 @@ int iopt_cut_iova(struct io_pagetable *iopt, unsigned long *iovas,
 void iopt_enable_large_pages(struct io_pagetable *iopt);
 int iopt_disable_large_pages(struct io_pagetable *iopt);
 
+int iopt_pin_pages(struct io_pagetable *iopt, unsigned long iova,
+		   unsigned long length, struct page **out_pages,
+		   unsigned int flags);
+void iopt_unpin_pages(struct io_pagetable *iopt, unsigned long iova,
+		      unsigned long length);
+
 struct iommufd_ucmd {
 	struct iommufd_ctx *ictx;
 	void __user *ubuffer;

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

index 2111bad72c72..a5c6be164254 100644
@@ -1240,58 +1240,17 @@ void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
 void iommufd_access_unpin_pages(struct iommufd_access *access,
 				unsigned long iova, unsigned long length)
 {
-	struct iopt_area_contig_iter iter;
-	struct io_pagetable *iopt;
-	unsigned long last_iova;
-	struct iopt_area *area;
-
-	if (WARN_ON(!length) ||
-	    WARN_ON(check_add_overflow(iova, length - 1, &last_iova)))
-		return;
-
-	mutex_lock(&access->ioas_lock);
+	guard(mutex)(&access->ioas_lock);
 	/*
 	 * The driver must be doing something wrong if it calls this before an
 	 * iommufd_access_attach() or after an iommufd_access_detach().
 	 */
-	if (WARN_ON(!access->ioas_unpin)) {
-		mutex_unlock(&access->ioas_lock);
+	if (WARN_ON(!access->ioas_unpin))
 		return;
-	}
-	iopt = &access->ioas_unpin->iopt;
-
-	down_read(&iopt->iova_rwsem);
-	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova)
-		iopt_area_remove_access(
-			area, iopt_area_iova_to_index(area, iter.cur_iova),
-			iopt_area_iova_to_index(
-				area,
-				min(last_iova, iopt_area_last_iova(area))));
-	WARN_ON(!iopt_area_contig_done(&iter));
-	up_read(&iopt->iova_rwsem);
-	mutex_unlock(&access->ioas_lock);
+	iopt_unpin_pages(&access->ioas_unpin->iopt, iova, length);
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_unpin_pages, "IOMMUFD");
 
-static bool iopt_area_contig_is_aligned(struct iopt_area_contig_iter *iter)
-{
-	if (iopt_area_start_byte(iter->area, iter->cur_iova) % PAGE_SIZE)
-		return false;
-
-	if (!iopt_area_contig_done(iter) &&
-	    (iopt_area_start_byte(iter->area, iopt_area_last_iova(iter->area)) %
-	     PAGE_SIZE) != (PAGE_SIZE - 1))
-		return false;
-	return true;
-}
-
-static bool check_area_prot(struct iopt_area *area, unsigned int flags)
-{
-	if (flags & IOMMUFD_ACCESS_RW_WRITE)
-		return area->iommu_prot & IOMMU_WRITE;
-	return area->iommu_prot & IOMMU_READ;
-}
-
 /**
  * iommufd_access_pin_pages() - Return a list of pages under the iova
  * @access: IOAS access to act on

@@ -1315,76 +1274,16 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 			     unsigned long length, struct page **out_pages,
 			     unsigned int flags)
 {
-	struct iopt_area_contig_iter iter;
-	struct io_pagetable *iopt;
-	unsigned long last_iova;
-	struct iopt_area *area;
-	int rc;
-
 	/* Driver's ops don't support pin_pages */
 	if (IS_ENABLED(CONFIG_IOMMUFD_TEST) &&
 	    WARN_ON(access->iova_alignment != PAGE_SIZE || !access->ops->unmap))
 		return -EINVAL;
 
-	if (!length)
-		return -EINVAL;
-	if (check_add_overflow(iova, length - 1, &last_iova))
-		return -EOVERFLOW;
-
-	mutex_lock(&access->ioas_lock);
-	if (!access->ioas) {
-		mutex_unlock(&access->ioas_lock);
+	guard(mutex)(&access->ioas_lock);
+	if (!access->ioas)
 		return -ENOENT;
-	}
-	iopt = &access->ioas->iopt;
-
-	down_read(&iopt->iova_rwsem);
-	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
-		unsigned long last = min(last_iova, iopt_area_last_iova(area));
-		unsigned long last_index = iopt_area_iova_to_index(area, last);
-		unsigned long index =
-			iopt_area_iova_to_index(area, iter.cur_iova);
-
-		if (area->prevent_access ||
-		    !iopt_area_contig_is_aligned(&iter)) {
-			rc = -EINVAL;
-			goto err_remove;
-		}
-
-		if (!check_area_prot(area, flags)) {
-			rc = -EPERM;
-			goto err_remove;
-		}
-
-		rc = iopt_area_add_access(area, index, last_index, out_pages,
-					  flags);
-		if (rc)
-			goto err_remove;
-		out_pages += last_index - index + 1;
-	}
-	if (!iopt_area_contig_done(&iter)) {
-		rc = -ENOENT;
-		goto err_remove;
-	}
-
-	up_read(&iopt->iova_rwsem);
-	mutex_unlock(&access->ioas_lock);
-	return 0;
-
-err_remove:
-	if (iova < iter.cur_iova) {
-		last_iova = iter.cur_iova - 1;
-		iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova)
-			iopt_area_remove_access(
-				area,
-				iopt_area_iova_to_index(area, iter.cur_iova),
-				iopt_area_iova_to_index(
-					area, min(last_iova,
-						  iopt_area_last_iova(area))));
-	}
-	up_read(&iopt->iova_rwsem);
-	mutex_unlock(&access->ioas_lock);
-	return rc;
+	return iopt_pin_pages(&access->ioas->iopt, iova, length, out_pages,
+			      flags);
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_access_pin_pages, "IOMMUFD");
 

@@ -1431,7 +1330,7 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
 			goto err_out;
 		}
 
-		if (!check_area_prot(area, flags)) {
+		if (!iopt_area_check_prot(area, flags)) {
 			rc = -EPERM;
 			goto err_out;
 		}

drivers/iommu/iommufd/io_pagetable.c +25 -0

index 8a790e597e12..160eec49af1b 100644
@@ -1472,3 +1472,98 @@ int iopt_table_enforce_dev_resv_regions(struct io_pagetable *iopt,
 	up_write(&iopt->iova_rwsem);
 	return rc;
 }
+
+static bool iopt_area_contig_is_aligned(struct iopt_area_contig_iter *iter)
+{
+	if (iopt_area_start_byte(iter->area, iter->cur_iova) % PAGE_SIZE)
+		return false;
+
+	if (!iopt_area_contig_done(iter) &&
+	    (iopt_area_start_byte(iter->area, iopt_area_last_iova(iter->area)) %
+	     PAGE_SIZE) != (PAGE_SIZE - 1))
+		return false;
+	return true;
+}
+
+int iopt_pin_pages(struct io_pagetable *iopt, unsigned long iova,
+		   unsigned long length, struct page **out_pages,
+		   unsigned int flags)
+{
+	struct iopt_area_contig_iter iter;
+	unsigned long last_iova;
+	struct iopt_area *area;
+	int rc;
+
+	if (!length)
+		return -EINVAL;
+	if (check_add_overflow(iova, length - 1, &last_iova))

drivers/iommu/iommufd/io_pagetable.c +60 -0

+		    !iopt_area_contig_is_aligned(&iter)) {
+			rc = -EINVAL;
+			goto err_remove;
+		}
+
+		if (!iopt_area_check_prot(area, flags)) {
+			rc = -EPERM;
+			goto err_remove;
+		}
+
+		rc = iopt_area_add_access(area, index, last_index, out_pages,
+					  flags);
+		if (rc)
+			goto err_remove;
+		out_pages += last_index - index + 1;
+	}
+	if (!iopt_area_contig_done(&iter)) {
+		rc = -ENOENT;
+		goto err_remove;
+	}
+
+	up_read(&iopt->iova_rwsem);
+	return 0;
+
+err_remove:
+	if (iova < iter.cur_iova) {
+		last_iova = iter.cur_iova - 1;
+		iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova)
+			iopt_area_remove_access(
+				area,
+				iopt_area_iova_to_index(area, iter.cur_iova),
+				iopt_area_iova_to_index(
+					area, min(last_iova,
+						  iopt_area_last_iova(area))));
+	}
+	up_read(&iopt->iova_rwsem);
+	return rc;
+}
+
+void iopt_unpin_pages(struct io_pagetable *iopt, unsigned long iova,
+		      unsigned long length)
+{
+	struct iopt_area_contig_iter iter;
+	unsigned long last_iova;
+	struct iopt_area *area;
+
+	if (WARN_ON_ONCE(!length) ||
+	    WARN_ON_ONCE(check_add_overflow(iova, length - 1, &last_iova)))
+		return;
+
+	down_read(&iopt->iova_rwsem);
+	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova)
+		iopt_area_remove_access(
+			area, iopt_area_iova_to_index(area, iter.cur_iova),
+			iopt_area_iova_to_index(
+				area,
+				min(last_iova, iopt_area_last_iova(area))));
+	WARN_ON_ONCE(!iopt_area_contig_done(&iter));
+	up_read(&iopt->iova_rwsem);
+}

Signature · 1 line

2.43.0

drivers/iommu/iommufd/io_pagetable.c int iopt_table_enforce_dev_resv_regions(struct io_pagetable *iopt, · line 1509

Quoted 7 lines · Nicolin Chen

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

+	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
+		unsigned long last = min(last_iova, iopt_area_last_iova(area));
+		unsigned long last_index = iopt_area_iova_to_index(area, last);
+		unsigned long index =
+			iopt_area_iova_to_index(area, iter.cur_iova);
+
+		if (area->prevent_access ||

Nit:
Shouldn't we return -EBUSY or something if (area->prevent_access == 1) ? IIUC, this just means that an unmap attempt is in progress, hence avoid accessing the area.

Quoted 13 lines · Pranjal Shrivastava

drivers/iommu/iommufd/io_pagetable.c int iopt_table_enforce_dev_resv_regions(struct io_pagetable *iopt, · line 1509

Quoted 7 lines · Nicolin Chen

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

+	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
+		unsigned long last = min(last_iova, iopt_area_last_iova(area));
+		unsigned long last_index = iopt_area_iova_to_index(area, last);
+		unsigned long index =
+			iopt_area_iova_to_index(area, iter.cur_iova);
+
+		if (area->prevent_access ||

Nit:
Shouldn't we return -EBUSY or something if (area->prevent_access == 1) ? IIUC, this just means that an unmap attempt is in progress, hence avoid accessing the area.

Maybe. But this is what it was. So we need a different patch to change that.

Quoted 15 lines · Nicolin Chen

Quoted 13 lines · Pranjal Shrivastava

drivers/iommu/iommufd/io_pagetable.c int iopt_table_enforce_dev_resv_regions(struct io_pagetable *iopt, · line 1509

Quoted 7 lines · Nicolin Chen

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

+	iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) {
+		unsigned long last = min(last_iova, iopt_area_last_iova(area));
+		unsigned long last_index = iopt_area_iova_to_index(area, last);
+		unsigned long index =
+			iopt_area_iova_to_index(area, iter.cur_iova);
+
+		if (area->prevent_access ||

Nit:
Shouldn't we return -EBUSY or something if (area->prevent_access == 1) ? IIUC, this just means that an unmap attempt is in progress, hence avoid accessing the area.

Quoted 2 lines

Maybe. But this is what it was. So we need a different patch to change that.

Rereading the code. The prevent_access is set by an unmap(), which means there shouldn't be any pin() and rw() as the caller should finish unmap() first.

In the newer use case of vCMDQ, it's similar. If VMM is unmapping the stage-2 mapping, it shouldn't try to allocate a vCMDQ.

-EBUSY makes some sense, but -EINVAL could still stand.

So, I am leaving it as is, since this patch is just about moving the functions for sharing.

Nicolin

From: Nicolin Chen <[email protected]>
Sent: Saturday, April 26, 2025 1:58 PM

The new vCMDQ object will be added for HW to access the guest memory for a
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 vCMDQ allocator. As the vCMDQ allocator will be a user-space triggered ioctl function, WARN_ON would not be a good fit in the new iopt_unpin_pages(), thus change them to use WARN_ON_ONCE instead.

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.

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

Quoted 20 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, April 26, 2025 1:58 PM

The new vCMDQ object will be added for HW to access the guest memory for a
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 vCMDQ allocator. As the vCMDQ allocator will be a user-space triggered ioctl function, WARN_ON would not be a good fit in the new iopt_unpin_pages(), thus change them to use WARN_ON_ONCE instead.

Quoted 4 lines

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.

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

any reason why this cannot be done by the core? all types of vcmd queues need to pin the guest buffer pages, no matter the IOMMU accesses GPA or HPA.

Jason made a similar comment earlier [1].

check of continuity is still done by the driver, if HPA is being accessed.

[1] https://lore.kernel.org/all/[email protected]/

From: Nicolin Chen <[email protected]>
Sent: Wednesday, May 7, 2025 3:18 AM

Quoted 4 lines · Kevin

Quoted 4 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, April 26, 2025 1:58 PM

The new vCMDQ object will be added for HW to access the guest memory

for

Quoted 2 lines

Quoted 2 lines

a
HW-accelerated virtualization feature. It needs to ensure the guest

memory

Quoted 5 lines

Quoted 5 lines

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

Quoted 6 lines

Quoted 6 lines

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 vCMDQ allocator. As the vCMDQ allocator will be a user-space triggered ioctl function,

WARN_ON

Quoted 1 line

Quoted 1 line

would not be a good fit in the new iopt_unpin_pages(), thus change them

to

Quoted 3 lines

Quoted 3 lines

use WARN_ON_ONCE instead.

Rename check_area_prot() to align with the existing iopt_area helpers,

and

Quoted 12 lines

Quoted 3 lines

inline it to the header since iommufd_access_rw() still uses it.

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

any reason why this cannot be done by the core? all types of vcmd queues need to pin the guest buffer pages, no matter the IOMMU accesses GPA or HPA.

Jason made a similar comment earlier [1].

check of continuity is still done by the driver, if HPA is being accessed.

Quoted 1 line

[1] https://lore.kernel.org/all/[email protected]/

But I am doing in the core. I have iopt_pin_pages() called in the core ioctl handler iommufd_vqueue_alloc_ioctl():

https://lore.kernel.org/linux-
iommu/1ef2e242ee1d844f823581a5365823d78c67ec6a.1746139811.git.nicoli [email protected]/

Quoted 6 lines

Quoted 3 lines

any reason why this cannot be done by the core? all types of vcmd queues need to pin the guest buffer pages, no matter the IOMMU accesses GPA or HPA.

But I am doing in the core. I have iopt_pin_pages() called in the core ioctl handler iommufd_vqueue_alloc_ioctl():

IMHO we just want to keep the pin logic in the core while leaving the check of PFN continuity to the driver (or have a way for the driver to communicate such need to the core).

It's possible for have an implementation with IOMMU accessing GPA of the queue which further goes through the stage-2 translation. In such case it's fine to have disjointed PFNs.

From: Nicolin Chen <[email protected]>
Sent: Wednesday, May 7, 2025 3:37 PM

Quoted 11 lines · Kevin

Quoted 6 lines

Quoted 3 lines

any reason why this cannot be done by the core? all types of vcmd queues need to pin the guest buffer pages, no matter the IOMMU accesses GPA or HPA.

But I am doing in the core. I have iopt_pin_pages() called in the core ioctl handler iommufd_vqueue_alloc_ioctl():

IMHO we just want to keep the pin logic in the core while leaving the check of PFN continuity to the driver (or have a way for the driver to communicate such need to the core).

Quoted 3 lines

It's possible for have an implementation with IOMMU accessing GPA of the queue which further goes through the stage-2 translation. In such case it's fine to have disjointed PFNs.

That makes sense. In the iommufd_viommu_ops structure, we can add "u32 flag" where driver can define IOMMUFD_VQUEUE_CONTIGUOUS_PFNS?

Add a new IOMMUFD_OBJ_VCMDQ with an iommufd_vcmdq structure, representing a command queue type of physical HW passed to a user space VM. This vCMDQ object, is a subset of vIOMMU resources of a physical IOMMU's, such as:
- NVIDIA's virtual command queue
- AMD vIOMMU's command buffer

Inroduce a struct iommufd_vcmdq and its allocator iommufd_vcmdq_alloc(). Also add a pair of viommu ops for iommufd to forward user space ioctls to IOMMU drivers.

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

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

include/linux/iommufd.h +1 -0

index ef0d3c4765cf..e91381aaec5a 100644
@@ -37,6 +37,7 @@ enum iommufd_object_type {
  	IOMMUFD_OBJ_VIOMMU,
  	IOMMUFD_OBJ_VDEVICE,
  	IOMMUFD_OBJ_VEVENTQ,
+	IOMMUFD_OBJ_VCMDQ,
  #ifdef CONFIG_IOMMUFD_TEST
  	IOMMUFD_OBJ_SELFTEST,
  #endif

Quoted 11 lines · Nicolin Chen

Add a new IOMMUFD_OBJ_VCMDQ with an iommufd_vcmdq structure, representing a command queue type of physical HW passed to a user space VM. This vCMDQ object, is a subset of vIOMMU resources of a physical IOMMU's, such as:
- NVIDIA's virtual command queue
- AMD vIOMMU's command buffer

Inroduce a struct iommufd_vcmdq and its allocator iommufd_vcmdq_alloc(). Also add a pair of viommu ops for iommufd to forward user space ioctls to IOMMU drivers.

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

Reviewed-by Lu Baolu <[email protected]>

with a small nit below ...

Quoted 19 lines

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

include/linux/iommufd.h +1 -0

index ef0d3c4765cf..e91381aaec5a 100644
@@ -37,6 +37,7 @@ enum iommufd_object_type {
  	IOMMUFD_OBJ_VIOMMU,
  	IOMMUFD_OBJ_VDEVICE,
  	IOMMUFD_OBJ_VEVENTQ,
+	IOMMUFD_OBJ_VCMDQ,
  #ifdef CONFIG_IOMMUFD_TEST
  	IOMMUFD_OBJ_SELFTEST,
  #endif

@@ -112,6 +113,14 @@ struct iommufd_vdevice {
  	u64 id; /* per-vIOMMU virtual ID */
  };

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

Quoted 5 lines

include/linux/iommufd.h +5 -0

+struct iommufd_vcmdq {
+	struct iommufd_object obj;
+	struct iommufd_ctx *ictx;
+	struct iommufd_viommu *viommu;
+	dma_addr_t addr;

It's better to add a comment to state that @addr is a guest physical address. Or not?

Quoted 8 lines

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

Quoted 5 lines

include/linux/iommufd.h +5 -0

+struct iommufd_vcmdq {
+	struct iommufd_object obj;
+	struct iommufd_ctx *ictx;
+	struct iommufd_viommu *viommu;
+	dma_addr_t addr;

It's better to add a comment to state that @addr is a guest physical address. Or not?

Yea. Let's add one:

dma_addr_t addr; /* in guest physical address space */

Quoted 9 lines · Nicolin Chen

Quoted 8 lines

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

Quoted 5 lines

include/linux/iommufd.h +5 -0

+struct iommufd_vcmdq {
+	struct iommufd_object obj;
+	struct iommufd_ctx *ictx;
+	struct iommufd_viommu *viommu;
+	dma_addr_t addr;

It's better to add a comment to state that @addr is a guest physical address. Or not?

Quoted 3 lines

Yea. Let's add one:

dma_addr_t addr; /* in guest physical address space */

If it is a guest physical in a nested page table it should technically be u64 not dma_addr_t

Add a new IOMMUFD_OBJ_VCMDQ with an iommufd_vcmdq structure, representing a command queue type of physical HW passed to a user space VM. This vCMDQ object, is a subset of vIOMMU resources of a physical IOMMU's, such as:
- NVIDIA's virtual command queue
- AMD vIOMMU's command buffer

Inroduce a struct iommufd_vcmdq and its allocator iommufd_vcmdq_alloc(). Also add a pair of viommu ops for iommufd to forward user space ioctls to IOMMU drivers.

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

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

include/linux/iommufd.h +35 -0

index ef0d3c4765cf..e91381aaec5a 100644
@@ -37,6 +37,7 @@ enum iommufd_object_type {
 	IOMMUFD_OBJ_VIOMMU,
 	IOMMUFD_OBJ_VDEVICE,
 	IOMMUFD_OBJ_VEVENTQ,
+	IOMMUFD_OBJ_VCMDQ,
 #ifdef CONFIG_IOMMUFD_TEST
 	IOMMUFD_OBJ_SELFTEST,
 #endif

@@ -112,6 +113,14 @@ struct iommufd_vdevice {
 	u64 id; /* per-vIOMMU virtual ID */
 };
 
+struct iommufd_vcmdq {
+	struct iommufd_object obj;
+	struct iommufd_ctx *ictx;
+	struct iommufd_viommu *viommu;
+	dma_addr_t addr;
+	size_t length;
+};
+
 /**
  * struct iommufd_viommu_ops - vIOMMU specific operations
  * @destroy: Clean up all driver-specific parts of an iommufd_viommu. The memory

@@ -135,6 +144,13 @@ struct iommufd_vdevice {
  * @vdevice_destroy: Clean up all driver-specific parts of an iommufd_vdevice.
  *                   The memory of the vDEVICE will be free-ed by iommufd core
  *                   after calling this op
+ * @vcmdq_alloc: Allocate a @type of iommufd_vcmdq as a user space command queue
+ *               for a @viommu. @index carries the logical vcmdq ID (for a multi-
+ *               queue case); @addr carries the guest physical base address of
+ *               the queue memory; @length carries the size of the queue memory
+ * @vcmdq_destroy: Clean up all driver-specific parts of an iommufd_vcmdq. The
+ *                 memory of the iommufd_vcmdq will be free-ed by iommufd core
+ *                 after calling this op
  */
 struct iommufd_viommu_ops {
 	void (*destroy)(struct iommufd_viommu *viommu);

@@ -147,6 +163,10 @@ struct iommufd_viommu_ops {
 						 struct device *dev,
 						 u64 virt_id);
 	void (*vdevice_destroy)(struct iommufd_vdevice *vdev);
+	struct iommufd_vcmdq *(*vcmdq_alloc)(struct iommufd_viommu *viommu,
+					     unsigned int type, u32 index,
+					     dma_addr_t addr, size_t length);
+	void (*vcmdq_destroy)(struct iommufd_vcmdq *vcmdq);
 };
 
 #if IS_ENABLED(CONFIG_IOMMUFD)

@@ -286,6 +306,21 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 		ret;                                                           \
 	})
 
+#define iommufd_vcmdq_alloc(viommu, drv_struct, member)                        \
+	({                                                                     \
+		drv_struct *ret;                                               \
+									       \
+		static_assert(__same_type(struct iommufd_viommu, *viommu));    \
+		static_assert(__same_type(struct iommufd_vcmdq,                \
+					  ((drv_struct *)NULL)->member));      \
+		static_assert(offsetof(drv_struct, member.obj) == 0);          \
+		ret = (drv_struct *)_iommufd_object_alloc(                     \
+			viommu->ictx, sizeof(drv_struct), IOMMUFD_OBJ_VCMDQ);  \
+		if (!IS_ERR(ret))                                              \
+			ret->member.viommu = viommu;                           \
+		ret;                                                           \
+	})
+
 /* Helper for IOMMU driver to destroy structures created by allocators above */
 #define iommufd_struct_destroy(ictx, drv_struct, member)                       \
 	({                                                                     \

Signature · 1 line

2.43.0

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

index a65153458a26..02a111710ffe 100644
@@ -170,3 +170,97 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
  	iommufd_put_object(ucmd->ictx, &viommu->obj);
  	return rc;
  }
+
+void iommufd_vcmdq_destroy(struct iommufd_object *obj)
+{
+	struct iommufd_vcmdq *vcmdq =
+		container_of(obj, struct iommufd_vcmdq, obj);
+	struct iommufd_viommu *viommu = vcmdq->viommu;
+
+	if (viommu->ops->vcmdq_destroy)
+		viommu->ops->vcmdq_destroy(vcmdq);
+	iopt_unpin_pages(&viommu->hwpt->ioas->iopt, vcmdq->addr, vcmdq->length);
+	refcount_dec(&viommu->obj.users);
+}
+
+int iommufd_vcmdq_alloc_ioctl(struct iommufd_ucmd *ucmd)

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

Quoted 11 lines · Nicolin Chen

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

+int iommufd_vcmdq_alloc_ioctl(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_vcmdq_alloc *cmd = ucmd->cmd;
+	struct iommufd_viommu *viommu;
+	struct iommufd_vcmdq *vcmdq;
+	struct page **pages;
+	int max_npages, i;
+	dma_addr_t end;
+	int rc;
+
+	if (cmd->flags || cmd->type == IOMMU_VCMDQ_TYPE_DEFAULT)

I don't follow the check of 'cmd->type == IOMMU_VCMDQ_TYPE_DEFAULT' here. My understanding is that it states that "other values of type are not supported". If so, shouldn't it be,

	if (cmd->flags || cmd->type != IOMMU_VCMDQ_TYPE_DEFAULT)

?

Quoted 18 lines · Baolu Lu

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

Quoted 11 lines · Nicolin Chen

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

+int iommufd_vcmdq_alloc_ioctl(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_vcmdq_alloc *cmd = ucmd->cmd;
+	struct iommufd_viommu *viommu;
+	struct iommufd_vcmdq *vcmdq;
+	struct page **pages;
+	int max_npages, i;
+	dma_addr_t end;
+	int rc;
+
+	if (cmd->flags || cmd->type == IOMMU_VCMDQ_TYPE_DEFAULT)

I don't follow the check of 'cmd->type == IOMMU_VCMDQ_TYPE_DEFAULT' here. My understanding is that it states that "other values of type are not supported". If so, shouldn't it be,

	if (cmd->flags || cmd->type != IOMMU_VCMDQ_TYPE_DEFAULT)

Quoted 1 line

?

No. Only other (new) types will be supported. We have this:
"* @IOMMU_VCMDQ_TYPE_DEFAULT: Reserved for future use" which means driver should define a new type.

We have the same DEFAULT type in vIOMMU/vEVENTQ allocators by the way.

Introduce a new IOMMUFD_CMD_VCMDQ_ALLOC ioctl for user space to allocate a vCMDQ for a vIOMMU object. Simply increase the refcount of the vIOMMU.

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

---
 drivers/iommu/iommufd/iommufd_private.h |  2 +
 include/uapi/linux/iommufd.h            | 41 +++++++++++
 drivers/iommu/iommufd/main.c            |  6 ++
 drivers/iommu/iommufd/viommu.c          | 94 +++++++++++++++++++++++++
 4 files changed, 143 insertions(+)

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

index 79160b039bc7..b974c207ae8a 100644
@@ -611,6 +611,8 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd);
 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);
+int iommufd_vcmdq_alloc_ioctl(struct iommufd_ucmd *ucmd);
+void iommufd_vcmdq_destroy(struct iommufd_object *obj);
 
 #ifdef CONFIG_IOMMUFD_TEST
 int iommufd_test(struct iommufd_ucmd *ucmd);

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

index cc90299a08d9..06a763fda47f 100644
@@ -56,6 +56,7 @@ enum {
 	IOMMUFD_CMD_VDEVICE_ALLOC = 0x91,
 	IOMMUFD_CMD_IOAS_CHANGE_PROCESS = 0x92,
 	IOMMUFD_CMD_VEVENTQ_ALLOC = 0x93,
+	IOMMUFD_CMD_VCMDQ_ALLOC = 0x94,
 };
 
 /**

@@ -1147,4 +1148,44 @@ struct iommu_veventq_alloc {
 	__u32 __reserved;
 };
 #define IOMMU_VEVENTQ_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VEVENTQ_ALLOC)
+
+/**
+ * enum iommu_vcmdq_type - Virtual Command Queue Type
+ * @IOMMU_VCMDQ_TYPE_DEFAULT: Reserved for future use
+ */
+enum iommu_vcmdq_type {
+	IOMMU_VCMDQ_TYPE_DEFAULT = 0,
+};
+
+/**
+ * struct iommu_vcmdq_alloc - ioctl(IOMMU_VCMDQ_ALLOC)

+ * @length: Length of the queue memory in the guest physical address space
+ *
+ * Allocate a virtual command queue object for a vIOMMU-specific HW-accelerated
+ * feature that can access a guest queue memory described by @addr and @length.
+ * It's suggested for VMM to back the queue memory using a single huge page with
+ * a proper alignment for its contiguity in the host physical address space. The
+ * call will fail, if the queue memory is not contiguous in the physical address
+ * space. Upon success, its underlying physical pages will be pinned to prevent
+ * VMM from unmapping them in the IOAS, until the virtual CMDQ gets destroyed.
+ */

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

+struct iommu_vcmdq_alloc {
+	__u32 size;
+	__u32 flags;
+	__u32 viommu_id;
+	__u32 type;
+	__u32 index;
+	__u32 out_vcmdq_id;
+	__aligned_u64 addr;
+	__aligned_u64 length;
+};
+#define IOMMU_VCMDQ_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VCMDQ_ALLOC)
 #endif

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

index 2b9ee9b4a424..ac51d5cfaa61 100644
@@ -303,6 +303,7 @@ union ucmd_buffer {
 	struct iommu_ioas_map map;
 	struct iommu_ioas_unmap unmap;
 	struct iommu_option option;
+	struct iommu_vcmdq_alloc vcmdq;
 	struct iommu_vdevice_alloc vdev;
 	struct iommu_veventq_alloc veventq;
 	struct iommu_vfio_ioas vfio_ioas;

@@ -358,6 +359,8 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
 	IOCTL_OP(IOMMU_IOAS_UNMAP, iommufd_ioas_unmap, struct iommu_ioas_unmap,
 		 length),
 	IOCTL_OP(IOMMU_OPTION, iommufd_option, struct iommu_option, val64),
+	IOCTL_OP(IOMMU_VCMDQ_ALLOC, iommufd_vcmdq_alloc_ioctl,
+		 struct iommu_vcmdq_alloc, length),
 	IOCTL_OP(IOMMU_VDEVICE_ALLOC, iommufd_vdevice_alloc_ioctl,
 		 struct iommu_vdevice_alloc, virt_id),
 	IOCTL_OP(IOMMU_VEVENTQ_ALLOC, iommufd_veventq_alloc,

@@ -501,6 +504,9 @@ static const struct iommufd_object_ops iommufd_object_ops[] = {
 	[IOMMUFD_OBJ_IOAS] = {
 		.destroy = iommufd_ioas_destroy,
 	},
+	[IOMMUFD_OBJ_VCMDQ] = {
+		.destroy = iommufd_vcmdq_destroy,
+	},
 	[IOMMUFD_OBJ_VDEVICE] = {
 		.destroy = iommufd_vdevice_destroy,
 	},

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

index a65153458a26..02a111710ffe 100644

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

+	struct iommufd_vcmdq *vcmdq =
+		container_of(obj, struct iommufd_vcmdq, obj);
+	struct iommufd_viommu *viommu = vcmdq->viommu;
+
+	if (viommu->ops->vcmdq_destroy)
+		viommu->ops->vcmdq_destroy(vcmdq);
+	iopt_unpin_pages(&viommu->hwpt->ioas->iopt, vcmdq->addr, vcmdq->length);
+	refcount_dec(&viommu->obj.users);
+}
+
+int iommufd_vcmdq_alloc_ioctl(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_vcmdq_alloc *cmd = ucmd->cmd;
+	struct iommufd_viommu *viommu;
+	struct iommufd_vcmdq *vcmdq;
+	struct page **pages;
+	int max_npages, i;
+	dma_addr_t end;
+	int rc;
+
+	if (cmd->flags || cmd->type == IOMMU_VCMDQ_TYPE_DEFAULT)
+		return -EOPNOTSUPP;
+	if (!cmd->addr || !cmd->length)
+		return -EINVAL;
+	if (check_add_overflow(cmd->addr, cmd->length - 1, &end))
+		return -EOVERFLOW;
+
+	max_npages = DIV_ROUND_UP(cmd->length, PAGE_SIZE);
+	pages = kcalloc(max_npages, sizeof(*pages), GFP_KERNEL);
+	if (!pages)
+		return -ENOMEM;
+
+	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
+	if (IS_ERR(viommu)) {
+		rc = PTR_ERR(viommu);
+		goto out_free;
+	}
+
+	if (!viommu->ops || !viommu->ops->vcmdq_alloc) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}

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

+	if (rc)
+		goto out_put_viommu;
+
+	/* 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;
+	}
+
+	vcmdq = viommu->ops->vcmdq_alloc(viommu, cmd->type, cmd->index,
+					 cmd->addr, cmd->length);
+	if (IS_ERR(vcmdq)) {
+		rc = PTR_ERR(vcmdq);
+		goto out_unpin;
+	}
+
+	vcmdq->viommu = viommu;
+	refcount_inc(&viommu->obj.users);
+	vcmdq->addr = cmd->addr;
+	vcmdq->ictx = ucmd->ictx;
+	vcmdq->length = cmd->length;
+	cmd->out_vcmdq_id = vcmdq->obj.id;
+	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
+	if (rc)
+		iommufd_object_abort_and_destroy(ucmd->ictx, &vcmdq->obj);
+	else
+		iommufd_object_finalize(ucmd->ictx, &vcmdq->obj);
+	goto out_put_viommu;
+
+out_unpin:
+	iopt_unpin_pages(&viommu->hwpt->ioas->iopt, cmd->addr, cmd->length);
+out_put_viommu:
+	iommufd_put_object(ucmd->ictx, &viommu->obj);
+out_free:
+	kfree(pages);
+	return rc;
+}

Quoted 10 lines

+/**
+ * struct iommu_vcmdq_alloc - ioctl(IOMMU_VCMDQ_ALLOC)
+ * @size: sizeof(struct iommu_vcmdq_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the virtual command queue with
+ * @type: One of enum iommu_vcmdq_type
+ * @index: The logical index to the virtual command queue per virtual IOMMU, for
+ * a multi-queue model
+ * @out_vcmdq_id: The ID of the new virtual command queue
+ * @addr: Base address of the queue memory in the guest physical address space

Sorry. I didn't get this part.

So here addr is command queue base address like
- NVIDIA's virtual command queue
- AMD vIOMMU's command buffer

.. and it will allocate vcmdq for each buffer type. Is that the correct understanding?

In case of AMD vIOMMU, buffer base address is programmed in different register (ex: MMIO Offset 0008h Command Buffer Base Address Register) and buffer enable/disable is done via different register (ex: MMIO Offset 0018h IOMMU Control Register). And we need to communicate both to hypervisor. Not sure this API can accommodate this as addr seems to be mandatory.

[1]
https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/specifications/48882_IOMMU.pdf

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

Quoted 7 lines

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

@@ -170,3 +170,97 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	iommufd_put_object(ucmd->ictx, &viommu->obj);
 	return rc;
 }
+
+void iommufd_vcmdq_destroy(struct iommufd_object *obj)
+{

I didn't understood destroy flow in general. Can you please help me to understand:

VMM is expected to track all buffers and call this interface? OR iommufd will take care of it? What happens if VM crashes ?

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

Quoted 3 lines

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

+	/* The underlying physical pages must be pinned in the IOAS */
+	rc = iopt_pin_pages(&viommu->hwpt->ioas->iopt, cmd->addr, cmd->length,
+			    pages, 0);

Why do we need this? is it not pinned already as part of vfio binding?

Quoted 17 lines · Vasant Hegde

Quoted 10 lines

+/**
+ * struct iommu_vcmdq_alloc - ioctl(IOMMU_VCMDQ_ALLOC)
+ * @size: sizeof(struct iommu_vcmdq_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the virtual command queue with
+ * @type: One of enum iommu_vcmdq_type
+ * @index: The logical index to the virtual command queue per virtual IOMMU, for
+ * a multi-queue model
+ * @out_vcmdq_id: The ID of the new virtual command queue
+ * @addr: Base address of the queue memory in the guest physical address space

Sorry. I didn't get this part.

So here addr is command queue base address like
- NVIDIA's virtual command queue
- AMD vIOMMU's command buffer

Quoted 2 lines

.. and it will allocate vcmdq for each buffer type. Is that the correct understanding?

Yes. For AMD "vIOMMU", it needs a new type for iommufd vIOMMU:
IOMMU_VIOMMU_TYPE_AMD_VIOMMU,

For AMD "vIOMMU" command buffer, it needs a new type too:
IOMMU_VCMDQ_TYPE_AMD_VIOMMU, /* Kdoc it to be Command Buffer */

Then, use IOMMUFD_CMD_VIOMMU_ALLOC ioctl to allocate an vIOMMU obj, and use IOMMUFD_CMD_VCMDQ_ALLOC ioctl(s) to allocate vCMDQ objs.

Quoted 5 lines

In case of AMD vIOMMU, buffer base address is programmed in different register (ex: MMIO Offset 0008h Command Buffer Base Address Register) and buffer enable/disable is done via different register (ex: MMIO Offset 0018h IOMMU Control Register). And we need to communicate both to hypervisor. Not sure this API can accommodate this as addr seems to be mandatory.

NVIDIA's CMDQV has all three of them too. What we do here is to let VMM trap the buffer base address (in guest physical address space) and forward it to kernel using this @addr. Then, kernel will translate this @addr to host physical address space, and program the physical address and size to the register.

Quoted 3 lines

[1]
https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/specifications/48882_IOMMU.pdf

Thanks for the doc. So, AMD has:

Command Buffer Base Address Register [MMIO Offset 0008h] "used to program the system physical base address and size of the
command buffer. The command buffer occupies contiguous physical memory starting at the programmed base address, up to the programmed size."
Command Buffer Head Pointer Register [MMIO Offset 2000h] Command Buffer Tail Pointer Register [MMIO Offset 2008h]

IIUIC, AMD should do the same: VMM traps VM's Command Buffer Base Address register when the guest kernel allocates a command buffer by programming the VM's Command Buffer Base Address register, to capture the guest PA and size. Then, VMM allocates a vCMDQ object (for this command buffer) forwarding its buffer address and size via @addr and @length to the host kernel. Then, the kernel should translate the guest PA to host PA to program the HW.

We can see that the Head/Tail registers are in a different MMIO page (offset by two 4K pages), which is very like NVIDIA CMDQV that allows VMM to mmap that MMIO page of the Head/Tail registers for guest OS to directly control the HW (i.e. VMM doesn't trap these two registers.

When guest OS wants to issue a new command, the guest kernel can just fill the guest command buffer at the entry that the Head register points to, and program the Tail register (backed by an mmap'd MMIO page), then the HW will read the programmed physical address from the entry (Head) till the entry (Tail) in the guest command buffer.

Quoted 11 lines

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

Quoted 8 lines

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

@@ -170,3 +170,97 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	iommufd_put_object(ucmd->ictx, &viommu->obj);
 	return rc;
 }
+
+void iommufd_vcmdq_destroy(struct iommufd_object *obj)
+{

I didn't understood destroy flow in general. Can you please help me to understand:

Quoted 2 lines

VMM is expected to track all buffers and call this interface? OR iommufd will take care of it? What happens if VM crashes ?

In a normal routine, VMM gets a vCMDQ object ID for each vCMDQ object it allocated. So, it should track all the IDs and release them when VM shuts down.

The iommufd core does track all the objects that belong to an iommufd context (ictx), and automatically release them. But, it can't resolve certain dependency on other FD, e.g. vEVENTQ and FAULT QUEUE would return another FD that user space listens to and must be closed properly to destroy the QUEUE object.

Quoted 6 lines

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

Quoted 4 lines

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

+	/* The underlying physical pages must be pinned in the IOAS */
+	rc = iopt_pin_pages(&viommu->hwpt->ioas->iopt, cmd->addr, cmd->length,
+			    pages, 0);

Why do we need this? is it not pinned already as part of vfio binding?

I think this could be clearer:
/*
* The underlying physical pages must be pinned to prevent them from
* being unmapped (via IOMMUFD_CMD_IOAS_UNMAP) during the life cycle
* of the vCMDQ object.
*/

Quoted 24 lines · Nicolin Chen

Quoted 11 lines · Vasant Hegde

Quoted 10 lines

+/**
+ * struct iommu_vcmdq_alloc - ioctl(IOMMU_VCMDQ_ALLOC)
+ * @size: sizeof(struct iommu_vcmdq_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the virtual command queue with
+ * @type: One of enum iommu_vcmdq_type
+ * @index: The logical index to the virtual command queue per virtual IOMMU, for
+ * a multi-queue model
+ * @out_vcmdq_id: The ID of the new virtual command queue
+ * @addr: Base address of the queue memory in the guest physical address space

Quoted 8 lines

Sorry. I didn't get this part.

So here addr is command queue base address like
- NVIDIA's virtual command queue
- AMD vIOMMU's command buffer

.. and it will allocate vcmdq for each buffer type. Is that the correct understanding?

Yes. For AMD "vIOMMU", it needs a new type for iommufd vIOMMU:
IOMMU_VIOMMU_TYPE_AMD_VIOMMU,

Quoted 2 lines

For AMD "vIOMMU" command buffer, it needs a new type too:
IOMMU_VCMDQ_TYPE_AMD_VIOMMU, /* Kdoc it to be Command Buffer */

You are suggesting we define one type for AMD and use it for all buffers like command buffer, event log, PPR buffet etc? and use iommu_vcmdq_alloc->index to identity different buffer type?

Quoted 10 lines

Then, use IOMMUFD_CMD_VIOMMU_ALLOC ioctl to allocate an vIOMMU obj, and use IOMMUFD_CMD_VCMDQ_ALLOC ioctl(s) to allocate vCMDQ objs.

Quoted 5 lines

In case of AMD vIOMMU, buffer base address is programmed in different register (ex: MMIO Offset 0008h Command Buffer Base Address Register) and buffer enable/disable is done via different register (ex: MMIO Offset 0018h IOMMU Control Register). And we need to communicate both to hypervisor. Not sure this API can accommodate this as addr seems to be mandatory.

Quoted 5 lines

NVIDIA's CMDQV has all three of them too. What we do here is to let VMM trap the buffer base address (in guest physical address space) and forward it to kernel using this @addr. Then, kernel will translate this @addr to host physical address space, and program the physical address and size to the register.

Right. For AMD IOMMU 1st 4K of MMIO space (which contains all buffer base address registers) is not accelerated. So we can trap it and pass GPA, size to iommufd.

.. but programming base register (like Command buffer base addr) is not sufficient. We have to enable the command buffer by setting particular bit in Control register. So at high level flow is something like below (@Suravee, correct me if I missed something here).

From guest side :
Write command bufer base addr, size (MMIO offset 0x08)
Set MMIO Offset 0x18[bit 12]
Also we need to program few other bits that are not related to these buffers like Completion wait interrupt enable.

From VMM side:
We need to trap both register and pass it to iommufd

From Host AMD IOMMU driver:
We have to program VFCntlMMIO Offset {16’b[GuestID], 6’b10_0000}

We need a way to pass Control register details to iommufd -> AMD driver so that we can program the VF control MMIO register.

Since iommu_vcmdq_alloc structure doesn't have user_data, how do we communicate control register?

Quoted 25 lines · Vasant Hegde

Quoted 24 lines · Nicolin Chen

Quoted 11 lines · Vasant Hegde

Quoted 10 lines

+/**
+ * struct iommu_vcmdq_alloc - ioctl(IOMMU_VCMDQ_ALLOC)
+ * @size: sizeof(struct iommu_vcmdq_alloc)
+ * @flags: Must be 0
+ * @viommu_id: Virtual IOMMU ID to associate the virtual command queue with
+ * @type: One of enum iommu_vcmdq_type
+ * @index: The logical index to the virtual command queue per virtual IOMMU, for
+ * a multi-queue model
+ * @out_vcmdq_id: The ID of the new virtual command queue
+ * @addr: Base address of the queue memory in the guest physical address space

Quoted 8 lines

Sorry. I didn't get this part.

So here addr is command queue base address like
- NVIDIA's virtual command queue
- AMD vIOMMU's command buffer

.. and it will allocate vcmdq for each buffer type. Is that the correct understanding?

Yes. For AMD "vIOMMU", it needs a new type for iommufd vIOMMU:
IOMMU_VIOMMU_TYPE_AMD_VIOMMU,

Quoted 6 lines

Quoted 2 lines

For AMD "vIOMMU" command buffer, it needs a new type too:
IOMMU_VCMDQ_TYPE_AMD_VIOMMU, /* Kdoc it to be Command Buffer */

You are suggesting we define one type for AMD and use it for all buffers like command buffer, event log, PPR buffet etc? and use iommu_vcmdq_alloc->index to identity different buffer type?

We have vEVENTQ for event logging and FAULT_QUEUE for PRI, but both are not for hardware accelerated use cases.

I didn't check the details of AMD's event log and PPR buffers. But they seem to be the same ring buffers and can be consumed by guest kernel directly?

Will the hardware replace the physical device ID in the event with the virtual device ID when injecting the event to a guest event/PPR queue?
If so, yea, I think you can define them separately using the> vCMDQ

- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_CMDBUF
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_EVENTLOG
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_PPRLOG
(@Kevin @Jason Hmm, in this case we might want to revert the naming
"vCMDQ" back to "vQEUEUE", once Vasant confirms.)

Each of them will be allocated on top of one vIOMMU object.

As for index, it really depends on how vIOMMU manages those vCMDQ objects. In NVIDIA case, each VINTF can support multiple vCMDQs of the same type (like smp in CPU term). If AMD is a similar case, yea, apply an index to each of them is a good idea. Otherwise, if vIOMMU could only have three queues and their types are different, perhaps the driver-level vIOMMU structure could just hold three pointers to manage them without using the index.

Quoted 17 lines

Quoted 11 lines

Then, use IOMMUFD_CMD_VIOMMU_ALLOC ioctl to allocate an vIOMMU obj, and use IOMMUFD_CMD_VCMDQ_ALLOC ioctl(s) to allocate vCMDQ objs.

Quoted 5 lines

In case of AMD vIOMMU, buffer base address is programmed in different register (ex: MMIO Offset 0008h Command Buffer Base Address Register) and buffer enable/disable is done via different register (ex: MMIO Offset 0018h IOMMU Control Register). And we need to communicate both to hypervisor. Not sure this API can accommodate this as addr seems to be mandatory.

Quoted 5 lines

NVIDIA's CMDQV has all three of them too. What we do here is to let VMM trap the buffer base address (in guest physical address space) and forward it to kernel using this @addr. Then, kernel will translate this @addr to host physical address space, and program the physical address and size to the register.

Quoted 3 lines

Right. For AMD IOMMU 1st 4K of MMIO space (which contains all buffer base address registers) is not accelerated. So we can trap it and pass GPA, size to iommufd.

Yes.

Quoted 18 lines

.. but programming base register (like Command buffer base addr) is not sufficient. We have to enable the command buffer by setting particular bit in Control register. So at high level flow is something like below (@Suravee, correct me if I missed something here).

From guest side :
Write command bufer base addr, size (MMIO offset 0x08)
Set MMIO Offset 0x18[bit 12]
Also we need to program few other bits that are not related to these buffers like Completion wait interrupt enable.

From VMM side:
We need to trap both register and pass it to iommufd

From Host AMD IOMMU driver:
We have to program VFCntlMMIO Offset {16’b[GuestID], 6’b10_0000}

Quoted 5 lines

We need a way to pass Control register details to iommufd -> AMD driver so that we can program the VF control MMIO register.

Since iommu_vcmdq_alloc structure doesn't have user_data, how do we communicate control register?

BIT(12) is the CMD enable bit. VMM can trap that as the trigger to forward the base address/length and other info.

And you'd likely need to define a driver structure:

// Add this to struct iommu_vcmdq_alloc;
+ * @data_len: Length of the type specific data
+ * @data_uptr: User pointer to the type specific data ..

(quoted diff) +2 -0

+	__u32 data_len;
+	__aligned_u64 data_uptr;

// Refer to my patch in the v1 series that handles the user_data:
// https://lore.kernel.org/linux-iommu/5cd2c7c4d92c79baf0cfc59e2a6b3e1db4e86ab8.1744353300.git.nicolinc@nvidia.com/

// Define a driver structure

struct iommu_vcmdq_amd_viommu_cmdbuf {
	__u32 flags;
	__u32 data;
};

Thanks
Nicolin

Quoted 13 lines · Nicolin Chen

Quoted 11 lines · Vasant Hegde

Quoted 6 lines · Nicolin Chen

On Mon, Apr 28, 2025 at 05:42:27PM +0530, Vasant Hegde wrote:
Yes. For AMD "vIOMMU", it needs a new type for iommufd vIOMMU:
IOMMU_VIOMMU_TYPE_AMD_VIOMMU,

For AMD "vIOMMU" command buffer, it needs a new type too:
IOMMU_VCMDQ_TYPE_AMD_VIOMMU, /* Kdoc it to be Command Buffer */

You are suggesting we define one type for AMD and use it for all buffers like command buffer, event log, PPR buffet etc? and use iommu_vcmdq_alloc->index to identity different buffer type?

Quoted 6 lines

We have vEVENTQ for event logging and FAULT_QUEUE for PRI, but both are not for hardware accelerated use cases.

I didn't check the details of AMD's event log and PPR buffers. But they seem to be the same ring buffers and can be consumed by guest kernel directly?

Right. Event log is accelerated and consumed by guest directly. Also we have Event Log B !

Quoted 1 line

Quoted 4 lines

Will the hardware replace the physical device ID in the event with the virtual device ID when injecting the event to a guest event/PPR queue?
If so, yea, I think you can define them separately using the> vCMDQ

infrastructures:

Quoted 5 lines

- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_CMDBUF
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_EVENTLOG
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_PPRLOG
(@Kevin @Jason Hmm, in this case we might want to revert the naming
"vCMDQ" back to "vQEUEUE", once Vasant confirms.)

Quoted 30 lines · Vasant Hegde

Quoted 13 lines · Nicolin Chen

Quoted 11 lines · Vasant Hegde

Quoted 6 lines · Nicolin Chen

On Mon, Apr 28, 2025 at 05:42:27PM +0530, Vasant Hegde wrote:
Yes. For AMD "vIOMMU", it needs a new type for iommufd vIOMMU:
IOMMU_VIOMMU_TYPE_AMD_VIOMMU,

For AMD "vIOMMU" command buffer, it needs a new type too:
IOMMU_VCMDQ_TYPE_AMD_VIOMMU, /* Kdoc it to be Command Buffer */

You are suggesting we define one type for AMD and use it for all buffers like command buffer, event log, PPR buffet etc? and use iommu_vcmdq_alloc->index to identity different buffer type?

Quoted 6 lines

We have vEVENTQ for event logging and FAULT_QUEUE for PRI, but both are not for hardware accelerated use cases.

I didn't check the details of AMD's event log and PPR buffers. But they seem to be the same ring buffers and can be consumed by guest kernel directly?

Right. Event log is accelerated and consumed by guest directly. Also we have Event Log B !

Quoted 5 lines

Will the hardware replace the physical device ID in the event with the virtual device ID when injecting the event to a guest event/PPR queue?
If so, yea, I think you can define them separately using the> vCMDQ

infrastructures:

Quoted 5 lines

Quoted 5 lines

- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_CMDBUF
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_EVENTLOG
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_PPRLOG
(@Kevin @Jason Hmm, in this case we might want to revert the naming
"vCMDQ" back to "vQEUEUE", once Vasant confirms.)

I think I should rename IOMMUFD_OBJ_VCMDQ back to IOMMUFD_OBJ_VQUEUE since the same object fits three types of queue now in the AMD case.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Will the hardware replace the physical device ID in the event with the virtual device ID when injecting the event to a guest event/PPR queue?
If so, yea, I think you can define them separately using the> vCMDQ

infrastructures:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_CMDBUF
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_EVENTLOG
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_PPRLOG
(@Kevin @Jason Hmm, in this case we might want to revert the naming
"vCMDQ" back to "vQEUEUE", once Vasant confirms.)

I think I should rename IOMMUFD_OBJ_VCMDQ back to IOMMUFD_OBJ_VQUEUE since the same object fits three types of queue now in the AMD case.

Makes sense.

AMD architecture supports 5 buffers. In practice we have not implemented event log B / PPR Log B in Linux.
Command buffer
Event Log A / B
PPR Log A / B

Quoted 16 lines · Vasant Hegde

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Will the hardware replace the physical device ID in the event with the virtual device ID when injecting the event to a guest event/PPR queue?
If so, yea, I think you can define them separately using the> vCMDQ

infrastructures:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_CMDBUF
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_EVENTLOG
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_PPRLOG
(@Kevin @Jason Hmm, in this case we might want to revert the naming
"vCMDQ" back to "vQEUEUE", once Vasant confirms.)

I think I should rename IOMMUFD_OBJ_VCMDQ back to IOMMUFD_OBJ_VQUEUE since the same object fits three types of queue now in the AMD case.

Makes sense.

Quoted 5 lines

AMD architecture supports 5 buffers. In practice we have not implemented event log B / PPR Log B in Linux.
Command buffer
Event Log A / B
PPR Log A / B

I think those A/B should be dual buffers of the same type?

Anyway, I have renamed all the core "vCMDQ" back to "vQUEUE" in my local branch. Will send v3 in the next days.

From: Nicolin Chen <[email protected]>
Sent: Wednesday, April 30, 2025 1:15 AM

Quoted 13 lines · Vasant Hegde

Quoted 6 lines · Nicolin Chen

On Tue, Apr 29, 2025 at 11:04:06AM +0530, Vasant Hegde wrote:

Will the hardware replace the physical device ID in the event with the virtual device ID when injecting the event to a guest event/PPR queue?
If so, yea, I think you can define them separately using the> vCMDQ

infrastructures:

Quoted 5 lines

- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_CMDBUF
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_EVENTLOG
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_PPRLOG
(@Kevin @Jason Hmm, in this case we might want to revert the naming
"vCMDQ" back to "vQEUEUE", once Vasant confirms.)

I think I should rename IOMMUFD_OBJ_VCMDQ back to IOMMUFD_OBJ_VQUEUE
since the same object fits three types of queue now in the AMD case.

Or any better naming suggestion?

Quoted 22 lines

From: Nicolin Chen <[email protected]>
Sent: Wednesday, April 30, 2025 1:15 AM

Quoted 13 lines · Vasant Hegde

Quoted 6 lines · Nicolin Chen

On Tue, Apr 29, 2025 at 11:04:06AM +0530, Vasant Hegde wrote:

Will the hardware replace the physical device ID in the event with the virtual device ID when injecting the event to a guest event/PPR queue?
If so, yea, I think you can define them separately using the> vCMDQ

infrastructures:

Quoted 5 lines

- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_CMDBUF
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_EVENTLOG
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_PPRLOG
(@Kevin @Jason Hmm, in this case we might want to revert the naming
"vCMDQ" back to "vQEUEUE", once Vasant confirms.)

I think I should rename IOMMUFD_OBJ_VCMDQ back to IOMMUFD_OBJ_VQUEUE
since the same object fits three types of queue now in the AMD case.

Quoted 2 lines

Or any better naming suggestion?

What about IOMMUFD_OBJ_HQUEUE to differentiate from other pure software queue structs? 'H" stands for direct hw access to the queue object.

From: Nicolin Chen <[email protected]>
Sent: Wednesday, May 7, 2025 4:13 AM

Quoted 11 lines · Kevin

Quoted 11 lines

From: Nicolin Chen <[email protected]>
Sent: Wednesday, April 30, 2025 1:15 AM

Quoted 7 lines · Vasant Hegde

Quoted 6 lines · Nicolin Chen

On Tue, Apr 29, 2025 at 11:04:06AM +0530, Vasant Hegde wrote:

Will the hardware replace the physical device ID in the event with the virtual device ID when injecting the event to a guest event/PPR queue?
If so, yea, I think you can define them separately using the> vCMDQ

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

infrastructures:

Quoted 4 lines

- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_CMDBUF
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_EVENTLOG
- IOMMU_VCMDQ_TYPE_AMD_VIOMMU_PPRLOG
(@Kevin @Jason Hmm, in this case we might want to revert the

naming

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

"vCMDQ" back to "vQEUEUE", once Vasant confirms.)

I think I should rename IOMMUFD_OBJ_VCMDQ back to IOMMUFD_OBJ_VQUEUE
since the same object fits three types of queue now in the AMD case.

Quoted 6 lines

Quoted 2 lines

Or any better naming suggestion?

What about IOMMUFD_OBJ_HQUEUE to differentiate from other pure software queue structs? 'H" stands for direct hw access to the queue object.