Skip to contentSkip to conversation

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

PATCH Superseded Nicolin Chen 3 participants 79 messages May 18, 2025 – Jun 6, 2025

Subsystems (8)

v5 of 9 Superseded Latest is v9 →

lore
Share & tools
lore
#0 lore raw

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

The vIOMMU object is designed to represent a slice of an IOMMU HW for its virtualization features shared with or passed to user space (a VM mostly) in a way of HW acceleration. This extended the HWPT-based design for more advanced virtualization feature.

HW QUEUE introduced by this series as a part of the vIOMMU infrastructure represents a HW accelerated queue/buffer for VM to use exclusively, e.g.
- NVIDIA's Virtual Command Queue
- AMD vIOMMU's Command Buffer, Event Log Buffer, and PPR Log Buffer
each of which allows its IOMMU HW to directly access a queue memory owned by a guest VM and allows a guest OS to control the HW queue direclty, to avoid VM Exit overheads to improve the performance.

Introduce IOMMUFD_OBJ_HW_QUEUE and its pairing IOMMUFD_CMD_HW_QUEUE_ALLOC allowing VMM to forward the IOMMU-specific queue info, such as queue base address, size, and etc.

Meanwhile, a guest-owned queue needs the guest kernel to control the queue by reading/writing its consumer and producer indexes, via MMIO acceses to the hardware MMIO registers. Introduce an mmap infrastructure for iommufd to support passing through a piece of MMIO region from the host physical address space to the guest physical address space. The mmap info (offset/ length) used by an mmap syscall must be pre-allocated and returned to the user space via an output driver-data during an IOMMUFD_CMD_HW_QUEUE_ALLOC call. Thus, it requires a driver-specific user data support in the vIOMMU allocation flow.

As a real-world use case, this series implements a HW QUEUE support in the tegra241-cmdqv driver for VCMDQs on NVIDIA Grace CPU. In another word, it is also the Tegra CMDQV series Part-2 (user-space support), reworked from Previous RFCv1:

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

This enables the HW accelerated feature for NVIDIA Grace CPU. Compared to the standard SMMUv3 operating in the nested translation mode trapping CMDQ for TLBI and ATC_INV commands, this gives a huge performance improvement:
70% to 90% reductions of invalidation time were measured by various DMA unmap tests running in a guest OS.

// Unmap latencies from "dma_map_benchmark -g @granule -t @threads", // by toggling "/sys/kernel/debug/iommu/tegra241_cmdqv/bypass_vcmdq" @granule | @threads | bypass_vcmdq=1 | bypass_vcmdq=0

    4KB        1          35.7 us          5.3 us
   16KB        1          41.8 us          6.8 us
   64KB        1          68.9 us          9.9 us
  128KB        1         109.0 us         12.6 us
  256KB        1         187.1 us         18.0 us
    4KB        2          96.9 us          6.8 us
   16KB        2          97.8 us          7.5 us
   64KB        2         151.5 us         10.7 us
  128KB        2         257.8 us         12.7 us
  256KB        2         443.0 us         17.9 us

This is on Github:
https://github.com/nicolinc/iommufd/commits/iommufd_hw_queue-v5

Paring QEMU branch for testing:
https://github.com/nicolinc/qemu/commits/wip/for_iommufd_hw_queue-v5

Changelog
v5
* Rebase on v6.15-rc6
* Add Reviewed-by from Jason and Kevin
* Correct typos in kdoc and update commit logs
* [iommufd] Add a cosmetic fix
* [iommufd] Drop unused num_pfns
* [iommufd] Drop unnecessary check
* [iommufd] Reorder patch sequence
* [iommufd] Use io_remap_pfn_range()
* [iommufd] Use success oriented flow
* [iommufd] Fix max_npages calculation
* [iommufd] Add more selftest coverage
* [iommufd] Drop redundant static_assert
* [iommufd] Fix mmap pfn range validation
* [iommufd] Reject unmap on pinned iovas
* [iommufd] Drop redundant vm_flags_set()
* [iommufd] Drop iommufd_struct_destroy()
* [iommufd] Drop redundant queue iova test
* [iommufd] Use "mmio_addr" and "mmio_pfn"
* [iommufd] Rename to "nesting_parent_iova"
* [iommufd] Make iopt_pin_pages call option
* [iommufd] Add ictx comparison in depend()
* [iommufd] Add iommufd_object_alloc_ucmd()
* [iommufd] Move kcalloc() after validations
* [iommufd] Replace ictx setting with WARN_ON
* [iommufd] Make hw_info's type bidirectional
* [smmu] Add supported_vsmmu_type in impl_ops
* [smmu] Drop impl report in smmu vendor struct
* [tegra] Add IOMMU_HW_INFO_TYPE_TEGRA241_CMDQV
* [tegra] Replace "number of VINTFs" with a note
* [tegra] Drop the redundant lvcmdq pointer setting
* [tegra] Flag IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA
* [tegra] Use "vintf_alloc_vsid" for vdevice_alloc op
v4
https://lore.kernel.org/all/[email protected]/
* Rebase on v6.15-rc5
* Add Reviewed-by from Vasant
* Rename "vQUEUE" to "HW QUEUE"
* Use "offset" and "length" for all mmap-related variables
* [iommufd] Use u64 for guest PA
* [iommufd] Fix typo in uAPI doc
* [iommufd] Rename immap_id to offset
* [iommufd] Drop the partial-size mmap support
* [iommufd] Do not replace WARN_ON with WARN_ON_ONCE
* [iommufd] Use "u64 base_addr" for queue base address
* [iommufd] Use u64 base_pfn/num_pfns for immap structure
* [iommufd] Correct the size passed in to mtree_alloc_range()
* [iommufd] Add IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA to viommu_ops
v3
https://lore.kernel.org/all/[email protected]/
* Add Reviewed-by from Baolu, Pranjal, and Alok
* Revise kdocs, uAPI docs, and commit logs
* Rename "vCMDQ" back to "vQUEUE" for AMD cases
* [tegra] Add tegra241_vcmdq_hw_flush_timeout()
* [tegra] Rename vsmmu_alloc to alloc_vintf_user
* [tegra] Use writel for SID replacement registers
* [tegra] Move mmap removal call to vsmmu_destroy op
* [tegra] Fix revert in tegra241_vintf_alloc_lvcmdq_user()
* [iommufd] Replace "& ~PAGE_MASK" with PAGE_ALIGNED()
* [iommufd] Add an object-type "owner" to immap structure
* [iommufd] Drop the ictx input in the new for-driver APIs
* [iommufd] Add iommufd_vma_ops to keep track of mmap lifecycle
* [iommufd] Add viommu-based iommufd_viommu_alloc/destroy_mmap helpers
* [iommufd] Rename iommufd_ctx_alloc/free_mmap to _iommufd_alloc/destroy_mmap
v2
https://lore.kernel.org/all/[email protected]/
* Add Reviewed-by from Jason
* [smmu] Fix vsmmu initial value
* [smmu] Support impl for hw_info
* [tegra] Rename "slot" to "vsid"
* [tegra] Update kdocs and commit logs
* [tegra] Map/unmap LVCMDQ dynamically
* [tegra] Refcount the previous LVCMDQ
* [tegra] Return -EEXIST if LVCMDQ exists
* [tegra] Simplify VINTF cleanup routine
* [tegra] Use vmid and s2_domain in vsmmu
* [tegra] Rename "mmap_pgoff" to "immap_id"
* [tegra] Add more addr and length validation
* [iommufd] Add more narrative to mmap's kdoc
* [iommufd] Add iommufd_struct_depend/undepend()
* [iommufd] Rename vcmdq_free op to vcmdq_destroy
* [iommufd] Fix bug in iommu_copy_struct_to_user()
* [iommufd] Drop is_io from iommufd_ctx_alloc_mmap()
* [iommufd] Test the queue memory for its contiguity
* [iommufd] Return -ENXIO if address or length fails
* [iommufd] Do not change @min_last in mock_viommu_alloc()
* [iommufd] Generalize TEGRA241_VCMDQ data in core structure
* [iommufd] Add selftest coverage for IOMMUFD_CMD_VCMDQ_ALLOC
* [iommufd] Add iopt_pin_pages() to prevent queue memory from unmapping
v1
https://lore.kernel.org/all/[email protected]/

Thanks
Nicolin

Nicolin Chen (29):
iommufd: Apply obvious cosmetic fixes
iommufd: Introduce iommufd_object_alloc_ucmd helper
iommu: Apply the new iommufd_object_alloc_ucmd helper
iommu: Add iommu_copy_struct_to_user helper
iommu: Pass in a driver-level user data structure to viommu_alloc op iommufd/viommu: Allow driver-specific user data for a vIOMMU object iommufd/selftest: Support user_data in mock_viommu_alloc iommufd/selftest: Add coverage for viommu data
iommufd: Do not unmap an owned iopt_area
iommufd: Abstract iopt_pin_pages and iopt_unpin_pages helpers iommufd/driver: Let iommufd_viommu_alloc helper save ictx to

    viommu->ictx
  iommufd/viommu: Add driver-allocated vDEVICE support
  iommufd/viommu: Introduce IOMMUFD_OBJ_HW_QUEUE and its related struct
  iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl
  iommufd/driver: Add iommufd_hw_queue_depend/undepend() helpers
  iommufd/selftest: Add coverage for IOMMUFD_CMD_HW_QUEUE_ALLOC
  iommufd: Add mmap interface
  iommufd/selftest: Add coverage for the new mmap interface
  Documentation: userspace-api: iommufd: Update HW QUEUE
  iommu: Allow an input type in hw_info op
  iommufd: Allow an input data_type via iommu_hw_info
  iommufd/selftest: Update hw_info coverage for an input data_type
  iommu/arm-smmu-v3-iommufd: Add vsmmu_alloc impl op
  iommu/arm-smmu-v3-iommufd: Add hw_info to impl_ops
  iommu/tegra241-cmdqv: Use request_threaded_irq
  iommu/tegra241-cmdqv: Simplify deinit flow in
    tegra241_cmdqv_remove_vintf()

iommu/tegra241-cmdqv: Do not statically map LVCMDQs iommu/tegra241-cmdqv: Add user-space use support iommu/tegra241-cmdqv: Add IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV support

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  28 +-
 drivers/iommu/iommufd/io_pagetable.h          |  15 +-
 drivers/iommu/iommufd/iommufd_private.h       |  41 +-
 drivers/iommu/iommufd/iommufd_test.h          |  20 +
 include/linux/iommu.h                         |  53 +-
 include/linux/iommufd.h                       | 221 +++++++-
 include/uapi/linux/iommufd.h                  | 150 +++++-
 tools/testing/selftests/iommu/iommufd_utils.h |  91 +++-
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |  33 +-
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 496 +++++++++++++++++-
 drivers/iommu/intel/iommu.c                   |   4 +
 drivers/iommu/iommufd/device.c                | 137 +----
 drivers/iommu/iommufd/driver.c                |  97 ++++
 drivers/iommu/iommufd/eventq.c                |  14 +-
 drivers/iommu/iommufd/hw_pagetable.c          |   6 +-
 drivers/iommu/iommufd/io_pagetable.c          | 106 +++-
 drivers/iommu/iommufd/iova_bitmap.c           |   1 -
 drivers/iommu/iommufd/main.c                  |  80 ++-
 drivers/iommu/iommufd/pages.c                 |  19 +-
 drivers/iommu/iommufd/selftest.c              | 158 +++++-
 drivers/iommu/iommufd/viommu.c                | 146 +++++-
 tools/testing/selftests/iommu/iommufd.c       | 146 +++++-
 .../selftests/iommu/iommufd_fail_nth.c        |  15 +-
 Documentation/userspace-api/iommufd.rst       |  12 +
 24 files changed, 1794 insertions(+), 295 deletions(-)
Signature · 1 line
2.43.0
#1 lore raw

PATCH v5 1/29 iommufd: Apply obvious cosmetic fixes

Run clang-format but exclude those not so obvious ones, which leaves us:
- Align indentations
- Add missing spaces
- Remove unnecessary spaces
- Remove unnecessary line wrappings

---
 drivers/iommu/iommufd/io_pagetable.h    |  2 +-
 drivers/iommu/iommufd/iommufd_private.h |  6 ++----
 include/linux/iommufd.h                 |  5 +++--
 drivers/iommu/iommufd/device.c          |  3 +--
 drivers/iommu/iommufd/hw_pagetable.c    |  6 ++----
 drivers/iommu/iommufd/io_pagetable.c    |  3 +--
 drivers/iommu/iommufd/iova_bitmap.c     |  1 -
 drivers/iommu/iommufd/main.c            |  6 ++----
 drivers/iommu/iommufd/pages.c           |  9 ++++-----
 drivers/iommu/iommufd/selftest.c        | 24 +++++++++++-------------
 10 files changed, 27 insertions(+), 38 deletions(-)
drivers/iommu/iommufd/io_pagetable.h +1 -1
index 10c928a9a463..c115a51d9384 100644
@@ -240,7 +240,7 @@ int iopt_area_add_access(struct iopt_area *area, unsigned long start,
 			 unsigned long last, struct page **out_pages,
 			 unsigned int flags);
 void iopt_area_remove_access(struct iopt_area *area, unsigned long start,
-			    unsigned long last);
+			     unsigned long last);
 int iopt_pages_rw_access(struct iopt_pages *pages, unsigned long start_byte,
 			 void *data, unsigned long length, unsigned int flags);
 
drivers/iommu/iommufd/iommufd_private.h +2 -4
index 80e8c76d25f2..10899e2cd34a 100644
@@ -266,8 +266,7 @@ struct iommufd_ioas {
 static inline struct iommufd_ioas *iommufd_get_ioas(struct iommufd_ctx *ictx,
 						    u32 id)
 {
-	return container_of(iommufd_get_object(ictx, id,
-					       IOMMUFD_OBJ_IOAS),
+	return container_of(iommufd_get_object(ictx, id, IOMMUFD_OBJ_IOAS),
 			    struct iommufd_ioas, obj);
 }
 
@@ -455,8 +454,7 @@ struct iommufd_access {
 
 int iopt_add_access(struct io_pagetable *iopt, struct iommufd_access *access);
 void iopt_remove_access(struct io_pagetable *iopt,
-			struct iommufd_access *access,
-			u32 iopt_access_list_id);
+			struct iommufd_access *access, u32 iopt_access_list_id);
 void iommufd_access_destroy_object(struct iommufd_object *obj);
 
 struct iommufd_eventq {
include/linux/iommufd.h +3 -2
index 34b6e6ca4bfa..498c9a768506 100644
@@ -171,8 +171,9 @@ static inline void iommufd_access_unpin_pages(struct iommufd_access *access,
 {
 }
 
-static inline int iommufd_access_rw(struct iommufd_access *access, unsigned long iova,
-		      void *data, size_t len, unsigned int flags)
+static inline int iommufd_access_rw(struct iommufd_access *access,
+				    unsigned long iova, void *data, size_t len,
+				    unsigned int flags)
 {
 	return -EOPNOTSUPP;
 }
drivers/iommu/iommufd/device.c +1 -2
index 2111bad72c72..16c17ae1f5f2 100644
@@ -473,8 +473,7 @@ iommufd_device_get_attach_handle(struct iommufd_device *idev, ioasid_t pasid)
 
 	lockdep_assert_held(&idev->igroup->lock);
 
-	handle =
-		iommu_attach_handle_get(idev->igroup->group, pasid, 0);
+	handle = iommu_attach_handle_get(idev->igroup->group, pasid, 0);
 	if (IS_ERR(handle))
 		return NULL;
 	return to_iommufd_handle(handle);
drivers/iommu/iommufd/hw_pagetable.c +2 -4
index 487779470261..8565a6f596b2 100644
@@ -309,10 +309,8 @@ iommufd_viommu_alloc_hwpt_nested(struct iommufd_viommu *viommu, u32 flags,
 	refcount_inc(&viommu->obj.users);
 	hwpt_nested->parent = viommu->hwpt;
 
-	hwpt->domain =
-		viommu->ops->alloc_domain_nested(viommu,
-				flags & ~IOMMU_HWPT_FAULT_ID_VALID,
-				user_data);
+	hwpt->domain = viommu->ops->alloc_domain_nested(
+		viommu, flags & ~IOMMU_HWPT_FAULT_ID_VALID, user_data);
 	if (IS_ERR(hwpt->domain)) {
 		rc = PTR_ERR(hwpt->domain);
 		hwpt->domain = NULL;
drivers/iommu/iommufd/io_pagetable.c +1 -2
index 8a790e597e12..13d010f19ed1 100644
@@ -1410,8 +1410,7 @@ int iopt_add_access(struct io_pagetable *iopt, struct iommufd_access *access)
 }
 
 void iopt_remove_access(struct io_pagetable *iopt,
-			struct iommufd_access *access,
-			u32 iopt_access_list_id)
+			struct iommufd_access *access, u32 iopt_access_list_id)
 {
 	down_write(&iopt->domains_rwsem);
 	down_write(&iopt->iova_rwsem);
drivers/iommu/iommufd/iova_bitmap.c +0 -1
index 39a86a4a1d3a..4514575818fc 100644
@@ -407,7 +407,6 @@ void iova_bitmap_set(struct iova_bitmap *bitmap,
 
 update_indexes:
 	if (unlikely(!iova_bitmap_mapped_range(mapped, iova, length))) {
-
 		/*
 		 * The attempt to advance the base index to @iova
 		 * may fail if it's out of bounds, or pinning the pages
drivers/iommu/iommufd/main.c +2 -4
index 3df468f64e7d..347c56ef44d8 100644
@@ -102,9 +102,8 @@ static int iommufd_object_dec_wait_shortterm(struct iommufd_ctx *ictx,
 		return 0;
 
 	if (wait_event_timeout(ictx->destroy_wait,
-				refcount_read(&to_destroy->shortterm_users) ==
-					0,
-				msecs_to_jiffies(60000)))
+			       refcount_read(&to_destroy->shortterm_users) == 0,
+			       msecs_to_jiffies(60000)))
 		return 0;
 
 	pr_crit("Time out waiting for iommufd object to become free\n");
@@ -539,7 +538,6 @@ static struct miscdevice iommu_misc_dev = {
 	.mode = 0660,
 };
 
-
 static struct miscdevice vfio_misc_dev = {
 	.minor = VFIO_MINOR,
 	.name = "vfio",
drivers/iommu/iommufd/pages.c +4 -5
index 3427749bc5ce..cbdde642d2af 100644
@@ -1287,8 +1287,7 @@ static int pfn_reader_first(struct pfn_reader *pfns, struct iopt_pages *pages,
 }
 
 static struct iopt_pages *iopt_alloc_pages(unsigned long start_byte,
-					   unsigned long length,
-					   bool writable)
+					   unsigned long length, bool writable)
 {
 	struct iopt_pages *pages;
 
@@ -1328,7 +1327,7 @@ struct iopt_pages *iopt_alloc_user_pages(void __user *uptr,
 	struct iopt_pages *pages;
 	unsigned long end;
 	void __user *uptr_down =
-		(void __user *) ALIGN_DOWN((uintptr_t)uptr, PAGE_SIZE);
+		(void __user *)ALIGN_DOWN((uintptr_t)uptr, PAGE_SIZE);
 
 	if (check_add_overflow((unsigned long)uptr, length, &end))
 		return ERR_PTR(-EOVERFLOW);
@@ -2111,8 +2110,8 @@ iopt_pages_get_exact_access(struct iopt_pages *pages, unsigned long index,
  * This should be undone through a matching call to iopt_area_remove_access()
  */
 int iopt_area_add_access(struct iopt_area *area, unsigned long start_index,
-			  unsigned long last_index, struct page **out_pages,
-			  unsigned int flags)
+			 unsigned long last_index, struct page **out_pages,
+			 unsigned int flags)
 {
 	struct iopt_pages *pages = area->pages;
 	struct iopt_pages_access *access;
drivers/iommu/iommufd/selftest.c +11 -13
index 18d9a216eb30..8cd98a72767d 100644
@@ -1179,9 +1179,8 @@ static int iommufd_test_md_check_refs(struct iommufd_ucmd *ucmd,
 	return 0;
 }
 
-static int iommufd_test_md_check_iotlb(struct iommufd_ucmd *ucmd,
-				       u32 mockpt_id, unsigned int iotlb_id,
-				       u32 iotlb)
+static int iommufd_test_md_check_iotlb(struct iommufd_ucmd *ucmd, u32 mockpt_id,
+				       unsigned int iotlb_id, u32 iotlb)
 {
 	struct mock_iommu_domain_nested *mock_nested;
 	struct iommufd_hw_pagetable *hwpt;
@@ -1454,7 +1453,7 @@ static int iommufd_test_access_pages(struct iommufd_ucmd *ucmd,
 	int rc;
 
 	/* Prevent syzkaller from triggering a WARN_ON in kvzalloc() */
-	if (length > 16*1024*1024)
+	if (length > 16 * 1024 * 1024)
 		return -ENOMEM;
 
 	if (flags & ~(MOCK_FLAGS_ACCESS_WRITE | MOCK_FLAGS_ACCESS_SYZ))
@@ -1471,7 +1470,7 @@ static int iommufd_test_access_pages(struct iommufd_ucmd *ucmd,
 
 	if (flags & MOCK_FLAGS_ACCESS_SYZ)
 		iova = iommufd_test_syz_conv_iova(staccess->access,
-					&cmd->access_pages.iova);
+						  &cmd->access_pages.iova);
 
 	npages = (ALIGN(iova + length, PAGE_SIZE) -
 		  ALIGN_DOWN(iova, PAGE_SIZE)) /
@@ -1547,7 +1546,7 @@ static int iommufd_test_access_rw(struct iommufd_ucmd *ucmd,
 	int rc;
 
 	/* Prevent syzkaller from triggering a WARN_ON in kvzalloc() */
-	if (length > 16*1024*1024)
+	if (length > 16 * 1024 * 1024)
 		return -ENOMEM;
 
 	if (flags & ~(MOCK_ACCESS_RW_WRITE | MOCK_ACCESS_RW_SLOW_PATH |
@@ -1573,7 +1572,7 @@ static int iommufd_test_access_rw(struct iommufd_ucmd *ucmd,
 
 	if (flags & MOCK_FLAGS_ACCESS_SYZ)
 		iova = iommufd_test_syz_conv_iova(staccess->access,
-				&cmd->access_rw.iova);
+						  &cmd->access_rw.iova);
 
 	rc = iommufd_access_rw(staccess->access, iova, tmp, length, flags);
 	if (rc)
@@ -1628,7 +1627,7 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
 		goto out_put;
 	}
 
-	if (copy_from_user(tmp, uptr,DIV_ROUND_UP(max, BITS_PER_BYTE))) {
+	if (copy_from_user(tmp, uptr, DIV_ROUND_UP(max, BITS_PER_BYTE))) {
 		rc = -EFAULT;
 		goto out_free;
 	}
@@ -1664,7 +1663,7 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
 static int iommufd_test_trigger_iopf(struct iommufd_ucmd *ucmd,
 				     struct iommu_test_cmd *cmd)
 {
-	struct iopf_fault event = { };
+	struct iopf_fault event = {};
 	struct iommufd_device *idev;
 
 	idev = iommufd_get_device(ucmd, cmd->trigger_iopf.dev_id);
@@ -1795,8 +1794,7 @@ static int iommufd_test_pasid_attach(struct iommufd_ucmd *ucmd,
 
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 	if (rc)
-		iommufd_device_detach(sobj->idev.idev,
-				      cmd->pasid_attach.pasid);
+		iommufd_device_detach(sobj->idev.idev, cmd->pasid_attach.pasid);
 
 out_sobj:
 	iommufd_put_object(ucmd->ictx, &sobj->obj);
@@ -1967,8 +1965,8 @@ int __init iommufd_test_init(void)
 		goto err_bus;
 
 	rc = iommu_device_register_bus(&mock_iommu.iommu_dev, &mock_ops,
-				  &iommufd_mock_bus_type.bus,
-				  &iommufd_mock_bus_type.nb);
+				       &iommufd_mock_bus_type.bus,
+				       &iommufd_mock_bus_type.nb);
 	if (rc)
 		goto err_sysfs;
 
Signature · 1 line
2.43.0
maintainer Reviewed-by on v5 1/29
#3 lore raw

Re: [PATCH v5 01/29] iommufd: Apply obvious cosmetic fixes

In reply to Nicolin Chen
Quoted 7 lines · Nicolin Chen

Run clang-format but exclude those not so obvious ones, which leaves us:
- Align indentations
- Add missing spaces
- Remove unnecessary spaces
- Remove unnecessary line wrappings

Quoted 12 lines
---
 drivers/iommu/iommufd/io_pagetable.h    |  2 +-
 drivers/iommu/iommufd/iommufd_private.h |  6 ++----
 include/linux/iommufd.h                 |  5 +++--
 drivers/iommu/iommufd/device.c          |  3 +--
 drivers/iommu/iommufd/hw_pagetable.c    |  6 ++----
 drivers/iommu/iommufd/io_pagetable.c    |  3 +--
 drivers/iommu/iommufd/iova_bitmap.c     |  1 -
 drivers/iommu/iommufd/main.c            |  6 ++----
 drivers/iommu/iommufd/pages.c           |  9 ++++-----
 drivers/iommu/iommufd/selftest.c        | 24 +++++++++++-------------
 10 files changed, 27 insertions(+), 38 deletions(-)

I have mixed feelings, I know people don't like churn, but it doesn't seem so bad..

Jason

#4 lore raw

PATCH v5 2/29 iommufd: Introduce iommufd_object_alloc_ucmd helper

In reply to Nicolin Chen

An object allocator needs to call either iommufd_object_finalize() upon a success or iommufd_object_abort_and_destroy() upon an error code.

To reduce duplication, store a new_obj in the struct iommufd_ucmd and call iommufd_object_finalize/iommufd_object_abort_and_destroy() accordingly in the main function.

This will also ease a driver-allocated object so that a driver can simply return with an error code to trigger an iommufd_object_abort_and_destroy() instead of doing an abort itself.

Similar to iommufd_object_alloc() and __iommufd_object_alloc(), add a pair of helpers: __iommufd_object_alloc_ucmd() and iommufd_object_alloc_ucmd().

---
 drivers/iommu/iommufd/iommufd_private.h |  1 +
 include/linux/iommufd.h                 | 23 +++++++++++++++++++++++
 drivers/iommu/iommufd/driver.c          | 18 ++++++++++++++++++
 drivers/iommu/iommufd/main.c            |  7 +++++++
 4 files changed, 49 insertions(+)
drivers/iommu/iommufd/iommufd_private.h +1 -0
index 10899e2cd34a..16767c231580 100644
@@ -135,6 +135,7 @@ struct iommufd_ucmd {
 	void __user *ubuffer;
 	u32 user_size;
 	void *cmd;
+	struct iommufd_object *new_obj;
 };
 
 int iommufd_vfio_ioctl(struct iommufd_ctx *ictx, unsigned int cmd,
include/linux/iommufd.h +23 -0
index 498c9a768506..47f38034f5e7 100644
@@ -22,6 +22,7 @@ struct iommu_user_data_array;
 struct iommufd_access;
 struct iommufd_ctx;
 struct iommufd_device;
+struct iommufd_ucmd;
 struct iommufd_viommu_ops;
 struct page;
 
@@ -51,6 +52,18 @@ struct iommufd_object {
 	unsigned int id;
 };
 
+#define __iommufd_object_alloc_ucmd(ucmd, ptr, type, obj)                      \
+	container_of(_iommufd_object_alloc_ucmd(                               \
+			     ucmd,                                             \
+			     sizeof(*(ptr)) + BUILD_BUG_ON_ZERO(               \
+						      offsetof(typeof(*(ptr)), \
+							       obj) != 0),     \
+			     type),                                            \
+		     typeof(*(ptr)), obj)
+
+#define iommufd_object_alloc_ucmd(ucmd, ptr, type) \
+	__iommufd_object_alloc_ucmd(ucmd, ptr, type, obj)
+
 struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
 					   struct device *dev, u32 *id);
 void iommufd_device_unbind(struct iommufd_device *idev);
@@ -193,6 +206,9 @@ static inline int iommufd_vfio_compat_set_no_iommu(struct iommufd_ctx *ictx)
 struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
 					     size_t size,
 					     enum iommufd_object_type type);
+struct iommufd_object *
+_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size,
+			   enum iommufd_object_type type);
 struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 				       unsigned long vdev_id);
 int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
@@ -208,6 +224,13 @@ _iommufd_object_alloc(struct iommufd_ctx *ictx, size_t size,
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+static inline struct iommufd_object *
+_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size,
+			   enum iommufd_object_type type)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 static inline struct device *
 iommufd_viommu_find_dev(struct iommufd_viommu *viommu, unsigned long vdev_id)
 {
drivers/iommu/iommufd/driver.c +18 -0
index 922cd1fe7ec2..28dec9e09720 100644
@@ -36,6 +36,24 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
 }
 EXPORT_SYMBOL_NS_GPL(_iommufd_object_alloc, "IOMMUFD");
 
+struct iommufd_object *_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd,
+						  size_t size,
+						  enum iommufd_object_type type)
+{
+	struct iommufd_object *new_obj;
+
+	if (ucmd->new_obj)
+		return ERR_PTR(-EBUSY);
+
+	new_obj = _iommufd_object_alloc(ucmd->ictx, size, type);
+	if (IS_ERR(new_obj))
+		return new_obj;
+
+	ucmd->new_obj = new_obj;
+	return new_obj;
+}
+EXPORT_SYMBOL_NS_GPL(_iommufd_object_alloc_ucmd, "IOMMUFD");
+
 /* Caller should xa_lock(&viommu->vdevs) to protect the return value */
 struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 				       unsigned long vdev_id)
drivers/iommu/iommufd/main.c +7 -0
index 347c56ef44d8..c6d0b446e632 100644
@@ -416,6 +416,13 @@ static long iommufd_fops_ioctl(struct file *filp, unsigned int cmd,
 	if (ret)
 		return ret;
 	ret = op->execute(&ucmd);
+
+	if (ucmd.new_obj) {
+		if (ret)
+			iommufd_object_abort_and_destroy(ictx, ucmd.new_obj);
+		else
+			iommufd_object_finalize(ictx, ucmd.new_obj);
+	}
 	return ret;
 }
 
Signature · 1 line
2.43.0
maintainer Reviewed-by on v5 2/29
#5 lore raw

RE: [PATCH v5 02/29] iommufd: Introduce iommufd_object_alloc_ucmd helper

drivers/iommu/iommufd/iommufd_private.h struct iommufd_ucmd { · line 138

Quoted 8 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:21 AM

drivers/iommu/iommufd/iommufd_private.h +1 -0
@@ -135,6 +135,7 @@ struct iommufd_ucmd {
 	void __user *ubuffer;
 	u32 user_size;
 	void *cmd;
+	struct iommufd_object *new_obj;

Could just be called as 'obj'. but not a strong preference.

#6 lore raw

Re: [PATCH v5 02/29] iommufd: Introduce iommufd_object_alloc_ucmd helper

Quoted 10 lines · Kevin

drivers/iommu/iommufd/iommufd_private.h struct iommufd_ucmd { · line 138

Quoted 8 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:21 AM

drivers/iommu/iommufd/iommufd_private.h +1 -0
@@ -135,6 +135,7 @@ struct iommufd_ucmd {
 	void __user *ubuffer;
 	u32 user_size;
 	void *cmd;
+	struct iommufd_object *new_obj;

Could just be called as 'obj'. but not a strong preference.

Jason named it "alloced_obj" to clarify the purpose, I believe. I picked a shorter "new_obj" while keeping its purpose. I think I will keep it in this way.

Thanks
Nicolin

maintainer Reviewed-by on v5 2/29
#7 lore raw

Re: [PATCH v5 02/29] iommufd: Introduce iommufd_object_alloc_ucmd helper

In reply to Nicolin Chen
Quoted 14 lines · Nicolin Chen

An object allocator needs to call either iommufd_object_finalize() upon a success or iommufd_object_abort_and_destroy() upon an error code.

To reduce duplication, store a new_obj in the struct iommufd_ucmd and call iommufd_object_finalize/iommufd_object_abort_and_destroy() accordingly in the main function.

This will also ease a driver-allocated object so that a driver can simply return with an error code to trigger an iommufd_object_abort_and_destroy() instead of doing an abort itself.

Similar to iommufd_object_alloc() and __iommufd_object_alloc(), add a pair of helpers: __iommufd_object_alloc_ucmd() and iommufd_object_alloc_ucmd().

Quoted 8 lines
---
 drivers/iommu/iommufd/iommufd_private.h |  1 +
 include/linux/iommufd.h                 | 23 +++++++++++++++++++++++
 drivers/iommu/iommufd/driver.c          | 18 ++++++++++++++++++
 drivers/iommu/iommufd/main.c            |  7 +++++++
 4 files changed, 49 insertions(+)

Jason

#8 lore raw

PATCH v5 3/29 iommu: Apply the new iommufd_object_alloc_ucmd helper

In reply to Nicolin Chen

Now the new ucmd-based object allocator eases the finalize/abort routine, apply this to all existing allocators that aren't protected by any lock.

Upgrade the for-driver vIOMMU alloctor too, and pass down to all existing viommu_alloc op accordingly.

Note that __iommufd_object_alloc_ucmd() builds in some static tests that cover both static_asserts in the iommufd_viommu_alloc(). Thus drop them.

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
 include/linux/iommu.h                         |  6 +++---
 include/linux/iommufd.h                       |  9 +++------
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |  4 ++--
 drivers/iommu/iommufd/eventq.c                | 14 ++++---------
 drivers/iommu/iommufd/selftest.c              |  4 ++--
 drivers/iommu/iommufd/viommu.c                | 20 ++++---------------
 7 files changed, 19 insertions(+), 40 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -1
index dd1ad56ce863..4f9aacad0091 100644
@@ -1061,7 +1061,7 @@ struct arm_vsmmu {
 void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type);
 struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 				       struct iommu_domain *parent,
-				       struct iommufd_ctx *ictx,
+				       struct iommufd_ucmd *ucmd,
 				       unsigned int viommu_type);
 int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
 				    struct arm_smmu_nested_domain *nested_domain);
include/linux/iommu.h +3 -3
index 3a8d35d41fda..0db39f922775 100644
@@ -43,7 +43,7 @@ struct iommu_sva;
 struct iommu_dma_cookie;
 struct iommu_dma_msi_cookie;
 struct iommu_fault_param;
-struct iommufd_ctx;
+struct iommufd_ucmd;
 struct iommufd_viommu;
 struct msi_desc;
 struct msi_msg;
@@ -609,7 +609,7 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
  *                include/uapi/linux/iommufd.h
  *                It is required to call iommufd_viommu_alloc() helper for
  *                a bundled allocation of the core and the driver structures,
- *                using the given @ictx pointer.
+ *                using the given @ucmd pointer.
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  * @owner: Driver module providing these ops
  * @identity_domain: An always available, always attachable identity
@@ -662,7 +662,7 @@ struct iommu_ops {
 
 	struct iommufd_viommu *(*viommu_alloc)(
 		struct device *dev, struct iommu_domain *parent_domain,
-		struct iommufd_ctx *ictx, unsigned int viommu_type);
+		struct iommufd_ucmd *ucmd, unsigned int viommu_type);
 
 	const struct iommu_domain_ops *default_domain_ops;
 	unsigned long pgsize_bitmap;
include/linux/iommufd.h +3 -6
index 47f38034f5e7..d74c97feb9b5 100644
@@ -256,15 +256,12 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
  * Helpers for IOMMU driver to allocate driver structures that will be freed by
  * the iommufd core. The free op will be called prior to freeing the memory.
  */
-#define iommufd_viommu_alloc(ictx, drv_struct, member, viommu_ops)             \
+#define iommufd_viommu_alloc(ucmd, drv_struct, member, viommu_ops)             \
 	({                                                                     \
 		drv_struct *ret;                                               \
 									       \
-		static_assert(__same_type(struct iommufd_viommu,               \
-					  ((drv_struct *)NULL)->member));      \
-		static_assert(offsetof(drv_struct, member.obj) == 0);          \
-		ret = (drv_struct *)_iommufd_object_alloc(                     \
-			ictx, sizeof(drv_struct), IOMMUFD_OBJ_VIOMMU);         \
+		ret = (drv_struct *)__iommufd_object_alloc_ucmd(               \
+			ucmd, ret, IOMMUFD_OBJ_VIOMMU, member.obj);            \
 		if (!IS_ERR(ret))                                              \
 			ret->member.ops = viommu_ops;                          \
 		ret;                                                           \
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +2 -2
index e4fd8d522af8..5d50bd8f4c2c 100644
@@ -384,7 +384,7 @@ static const struct iommufd_viommu_ops arm_vsmmu_ops = {
 
 struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 				       struct iommu_domain *parent,
-				       struct iommufd_ctx *ictx,
+				       struct iommufd_ucmd *ucmd,
 				       unsigned int viommu_type)
 {
 	struct arm_smmu_device *smmu =
@@ -422,7 +422,7 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
 		return ERR_PTR(-EOPNOTSUPP);
 
-	vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, core,
+	vsmmu = iommufd_viommu_alloc(ucmd, struct arm_vsmmu, core,
 				     &arm_vsmmu_ops);
 	if (IS_ERR(vsmmu))
 		return ERR_CAST(vsmmu);
drivers/iommu/iommufd/eventq.c +4 -10
index f39cf0797347..269b667152b7 100644
@@ -473,8 +473,8 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 	if (cmd->flags)
 		return -EOPNOTSUPP;
 
-	fault = __iommufd_object_alloc(ucmd->ictx, fault, IOMMUFD_OBJ_FAULT,
-				       common.obj);
+	fault = __iommufd_object_alloc_ucmd(ucmd, fault, IOMMUFD_OBJ_FAULT,
+					    common.obj);
 	if (IS_ERR(fault))
 		return PTR_ERR(fault);
 
@@ -483,10 +483,8 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 
 	fdno = iommufd_eventq_init(&fault->common, "[iommufd-pgfault]",
 				   ucmd->ictx, &iommufd_fault_fops);
-	if (fdno < 0) {
-		rc = fdno;
-		goto out_abort;
-	}
+	if (fdno < 0)
+		return fdno;
 
 	cmd->out_fault_id = fault->common.obj.id;
 	cmd->out_fault_fd = fdno;
@@ -494,7 +492,6 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 	if (rc)
 		goto out_put_fdno;
-	iommufd_object_finalize(ucmd->ictx, &fault->common.obj);
 
 	fd_install(fdno, fault->common.filep);
 
@@ -502,9 +499,6 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 out_put_fdno:
 	put_unused_fd(fdno);
 	fput(fault->common.filep);
-out_abort:
-	iommufd_object_abort_and_destroy(ucmd->ictx, &fault->common.obj);
-
 	return rc;
 }
 
drivers/iommu/iommufd/selftest.c +2 -2
index 8cd98a72767d..4b021dc50758 100644
@@ -735,7 +735,7 @@ static struct iommufd_viommu_ops mock_viommu_ops = {
 
 static struct iommufd_viommu *mock_viommu_alloc(struct device *dev,
 						struct iommu_domain *domain,
-						struct iommufd_ctx *ictx,
+						struct iommufd_ucmd *ucmd,
 						unsigned int viommu_type)
 {
 	struct mock_iommu_device *mock_iommu =
@@ -745,7 +745,7 @@ static struct iommufd_viommu *mock_viommu_alloc(struct device *dev,
 	if (viommu_type != IOMMU_VIOMMU_TYPE_SELFTEST)
 		return ERR_PTR(-EOPNOTSUPP);
 
-	mock_viommu = iommufd_viommu_alloc(ictx, struct mock_viommu, core,
+	mock_viommu = iommufd_viommu_alloc(ucmd, struct mock_viommu, core,
 					   &mock_viommu_ops);
 	if (IS_ERR(mock_viommu))
 		return ERR_CAST(mock_viommu);
drivers/iommu/iommufd/viommu.c +4 -16
index 01df2b985f02..6d874d6a4c00 100644
@@ -47,8 +47,8 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		goto out_put_hwpt;
 	}
 
-	viommu = ops->viommu_alloc(idev->dev, hwpt_paging->common.domain,
-				   ucmd->ictx, cmd->type);
+	viommu = ops->viommu_alloc(idev->dev, hwpt_paging->common.domain, ucmd,
+				   cmd->type);
 	if (IS_ERR(viommu)) {
 		rc = PTR_ERR(viommu);
 		goto out_put_hwpt;
@@ -70,13 +70,7 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 
 	cmd->out_viommu_id = viommu->obj.id;
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
-	if (rc)
-		goto out_abort;
-	iommufd_object_finalize(ucmd->ictx, &viommu->obj);
-	goto out_put_hwpt;
 
-out_abort:
-	iommufd_object_abort_and_destroy(ucmd->ictx, &viommu->obj);
 out_put_hwpt:
 	iommufd_put_object(ucmd->ictx, &hwpt_paging->common.obj);
 out_put_idev:
@@ -124,7 +118,7 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		goto out_put_idev;
 	}
 
-	vdev = iommufd_object_alloc(ucmd->ictx, vdev, IOMMUFD_OBJ_VDEVICE);
+	vdev = iommufd_object_alloc_ucmd(ucmd, vdev, IOMMUFD_OBJ_VDEVICE);
 	if (IS_ERR(vdev)) {
 		rc = PTR_ERR(vdev);
 		goto out_put_idev;
@@ -139,18 +133,12 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	curr = xa_cmpxchg(&viommu->vdevs, virt_id, NULL, vdev, GFP_KERNEL);
 	if (curr) {
 		rc = xa_err(curr) ?: -EEXIST;
-		goto out_abort;
+		goto out_put_idev;
 	}
 
 	cmd->out_vdevice_id = vdev->obj.id;
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
-	if (rc)
-		goto out_abort;
-	iommufd_object_finalize(ucmd->ictx, &vdev->obj);
-	goto out_put_idev;
 
-out_abort:
-	iommufd_object_abort_and_destroy(ucmd->ictx, &vdev->obj);
 out_put_idev:
 	iommufd_put_object(ucmd->ictx, &idev->obj);
 out_put_viommu:
Signature · 1 line
2.43.0
maintainer
#9 lore raw

RE: [PATCH v5 03/29] iommu: Apply the new iommufd_object_alloc_ucmd helper

Quoted 11 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:21 AM

Now the new ucmd-based object allocator eases the finalize/abort routine, apply this to all existing allocators that aren't protected by any lock.

Upgrade the for-driver vIOMMU alloctor too, and pass down to all existing viommu_alloc op accordingly.

Note that __iommufd_object_alloc_ucmd() builds in some static tests that cover both static_asserts in the iommufd_viommu_alloc(). Thus drop them.

I may overlook something, but at a quick glance the following check Is not covered?

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

Btw this patch doesn't convert all object allocations to the new helper, e.g. ioas. What is the criteria for when to use the old helpers vs. the new helpers?

#10 lore raw

Re: [PATCH v5 03/29] iommu: Apply the new iommufd_object_alloc_ucmd helper

Quoted 15 lines · Kevin
Quoted 11 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:21 AM

Now the new ucmd-based object allocator eases the finalize/abort routine, apply this to all existing allocators that aren't protected by any lock.

Upgrade the for-driver vIOMMU alloctor too, and pass down to all existing viommu_alloc op accordingly.

Note that __iommufd_object_alloc_ucmd() builds in some static tests that cover both static_asserts in the iommufd_viommu_alloc(). Thus drop them.

I may overlook something, but at a quick glance the following check Is not covered?

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

Quoted 2 lines
include/linux/iommufd.h +0 -2
-		static_assert(__same_type(struct iommufd_viommu,               \
-					  ((drv_struct *)NULL)->member));      \

The container_of() inside __iommufd_object_alloc_ucmd() covers that:

#define container_of(ptr, type, member) ({				\
	void *__mptr = (void *)(ptr);					\
	static_assert(__same_type(*(ptr), ((type *)0)->member) ||	\ <<== here
		      __same_type(*(ptr), void),			\
		      "pointer type mismatch in container_of()");	\
	((type *)(__mptr - offsetof(type, member))); })
Quoted 3 lines

Btw this patch doesn't convert all object allocations to the new helper, e.g. ioas. What is the criteria for when to use the old helpers vs. the new helpers?

Criteria is "that aren't protected by any lock" mentioned in the line 2. The ioas has an ioas_creation_lock around finalize() :(

Thanks
Nic

maintainer
#11 lore raw

RE: [PATCH v5 03/29] iommu: Apply the new iommufd_object_alloc_ucmd helper

Quoted 29 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, May 24, 2025 5:35 AM

Quoted 4 lines · Kevin
Quoted 4 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:21 AM

Now the new ucmd-based object allocator eases the finalize/abort

routine,

Quoted 3 lines
Quoted 3 lines

apply this to all existing allocators that aren't protected by any lock.

Upgrade the for-driver vIOMMU alloctor too, and pass down to all

existing

Quoted 4 lines
Quoted 4 lines

viommu_alloc op accordingly.

Note that __iommufd_object_alloc_ucmd() builds in some static tests that cover both static_asserts in the iommufd_viommu_alloc(). Thus drop

them.

Review

Quoted 5 lines

I may overlook something, but at a quick glance the following check Is not covered?

(quoted diff) +0 -1
-		static_assert(__same_type(struct iommufd_viommu,

\

Review

Quoted 1 line
(quoted diff) +0 -1
-					  ((drv_struct *)NULL)->member));

\

The container_of() inside __iommufd_object_alloc_ucmd() covers that:

Quoted 7 lines
#define container_of(ptr, type, member) ({				\
	void *__mptr = (void *)(ptr);					\
	static_assert(__same_type(*(ptr), ((type *)0)->member) ||	\
<<== here
		      __same_type(*(ptr), void),			\
		      "pointer type mismatch in container_of()");	\
	((type *)(__mptr - offsetof(type, member))); })

Yeah it's there.

maintainer Reviewed-by on v5 3/29
#12 lore raw

Re: [PATCH v5 03/29] iommu: Apply the new iommufd_object_alloc_ucmd helper

In reply to Nicolin Chen
Quoted 11 lines · Nicolin Chen

Now the new ucmd-based object allocator eases the finalize/abort routine, apply this to all existing allocators that aren't protected by any lock.

Upgrade the for-driver vIOMMU alloctor too, and pass down to all existing viommu_alloc op accordingly.

Note that __iommufd_object_alloc_ucmd() builds in some static tests that cover both static_asserts in the iommufd_viommu_alloc(). Thus drop them.

Quoted 9 lines
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
 include/linux/iommu.h                         |  6 +++---
 include/linux/iommufd.h                       |  9 +++------
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |  4 ++--
 drivers/iommu/iommufd/eventq.c                | 14 ++++---------
 drivers/iommu/iommufd/selftest.c              |  4 ++--
 drivers/iommu/iommufd/viommu.c                | 20 ++++---------------
 7 files changed, 19 insertions(+), 40 deletions(-)

Good diffstat..

Jason

#13 lore raw

PATCH v5 4/29 iommu: Add iommu_copy_struct_to_user helper

In reply to 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.

---
 include/linux/iommu.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
include/linux/iommu.h +40 -0
index 0db39f922775..5f9c4eaaaaff 100644
@@ -562,6 +562,46 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
 	return 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 @src_data. Must match with @dst_data.type
+ * @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
+ *            version.
+ * Return 0 for success, otherwise -error.
+ */
+#define iommu_copy_struct_to_user(user_data, ksrc, data_type, min_last)        \
+	__iommu_copy_struct_to_user(user_data, ksrc, data_type, sizeof(*ksrc), \
+				    offsetofend(typeof(*ksrc), min_last))
+
 /**
  * struct iommu_ops - iommu ops and capabilities
  * @capable: check capability
Signature · 1 line
2.43.0
#14 lore raw

PATCH v5 5/29 iommu: Pass in a driver-level user data structure to viommu_alloc op

In reply to Nicolin Chen

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.

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h         | 3 ++-
 include/linux/iommu.h                               | 3 ++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 3 ++-
 drivers/iommu/iommufd/selftest.c                    | 8 ++++----
 drivers/iommu/iommufd/viommu.c                      | 2 +-
 5 files changed, 11 insertions(+), 8 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -1
index 4f9aacad0091..cb93eff4d9ab 100644
@@ -1062,7 +1062,8 @@ void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type);
 struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 				       struct iommu_domain *parent,
 				       struct iommufd_ucmd *ucmd,
-				       unsigned int viommu_type);
+				       unsigned int viommu_type,
+				       const struct iommu_user_data *user_data);
 int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
 				    struct arm_smmu_nested_domain *nested_domain);
 void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state);
include/linux/iommu.h +2 -1
index 5f9c4eaaaaff..4c54dd8f94e4 100644
@@ -702,7 +702,8 @@ struct iommu_ops {
 
 	struct iommufd_viommu *(*viommu_alloc)(
 		struct device *dev, struct iommu_domain *parent_domain,
-		struct iommufd_ucmd *ucmd, unsigned int viommu_type);
+		struct iommufd_ucmd *ucmd, unsigned int viommu_type,
+		const struct iommu_user_data *user_data);
 
 	const struct iommu_domain_ops *default_domain_ops;
 	unsigned long pgsize_bitmap;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +2 -1
index 5d50bd8f4c2c..adef56db4028 100644
@@ -385,7 +385,8 @@ static const struct iommufd_viommu_ops arm_vsmmu_ops = {
 struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 				       struct iommu_domain *parent,
 				       struct iommufd_ucmd *ucmd,
-				       unsigned int viommu_type)
+				       unsigned int viommu_type,
+				       const struct iommu_user_data *user_data)
 {
 	struct arm_smmu_device *smmu =
 		iommu_get_iommu_dev(dev, struct arm_smmu_device, iommu);
drivers/iommu/iommufd/selftest.c +4 -4
index 4b021dc50758..3d56e2bddb0b 100644
@@ -733,10 +733,10 @@ static struct iommufd_viommu_ops mock_viommu_ops = {
 	.cache_invalidate = mock_viommu_cache_invalidate,
 };
 
-static struct iommufd_viommu *mock_viommu_alloc(struct device *dev,
-						struct iommu_domain *domain,
-						struct iommufd_ucmd *ucmd,
-						unsigned int viommu_type)
+static struct iommufd_viommu *
+mock_viommu_alloc(struct device *dev, struct iommu_domain *domain,
+		  struct iommufd_ucmd *ucmd, unsigned int viommu_type,
+		  const struct iommu_user_data *user_data)
 {
 	struct mock_iommu_device *mock_iommu =
 		iommu_get_iommu_dev(dev, struct mock_iommu_device, iommu_dev);
drivers/iommu/iommufd/viommu.c +1 -1
index 6d874d6a4c00..b50b50ec87da 100644
@@ -48,7 +48,7 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	}
 
 	viommu = ops->viommu_alloc(idev->dev, hwpt_paging->common.domain, ucmd,
-				   cmd->type);
+				   cmd->type, NULL);
 	if (IS_ERR(viommu)) {
 		rc = PTR_ERR(viommu);
 		goto out_put_hwpt;
Signature · 1 line
2.43.0
#15 lore raw

PATCH v5 6/29 iommufd/viommu: Allow driver-specific user data for a vIOMMU object

In reply to Nicolin Chen

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.

---
 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..272da7324a2b 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 a 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 b50b50ec87da..2b30627d1d8e 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,
-				   cmd->type, NULL);
+				   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
#16 lore raw

PATCH v5 7/29 iommufd/selftest: Support user_data in mock_viommu_alloc

In reply to Nicolin Chen

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

---
 drivers/iommu/iommufd/iommufd_test.h | 13 +++++++++++++
 drivers/iommu/iommufd/selftest.c     | 17 +++++++++++++++++
 2 files changed, 30 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 +17 -0
index 3d56e2bddb0b..ea2e4c626ad5 100644
@@ -740,16 +740,33 @@ 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(ucmd, 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(
+			user_data, &data, IOMMU_VIOMMU_TYPE_SELFTEST, out_data);
+		if (rc)
+			return ERR_PTR(rc);
+	}
+
 	refcount_inc(&mock_iommu->users);
 	return &mock_viommu->core;
 }
Signature · 1 line
2.43.0
#17 lore raw

PATCH v5 8/29 iommufd/selftest: Add coverage for viommu data

In reply to Nicolin Chen

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

---
 tools/testing/selftests/iommu/iommufd_utils.h | 21 ++++++++-----
 tools/testing/selftests/iommu/iommufd.c       | 30 ++++++++++++++-----
 .../selftests/iommu/iommufd_fail_nth.c        |  5 ++--
 3 files changed, 39 insertions(+), 17 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 -7
index 1a8e85afe9aa..f22388dfacad 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_EQ(data.out_data, data.in_data);
+	}
+}
+
 TEST_F(iommufd_viommu, vdevice_alloc)
 {
 	uint32_t viommu_id = self->viommu_id;
@@ -3105,8 +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,
-			      &viommu_id);
+			      IOMMU_VIOMMU_TYPE_SELFTEST, NULL, 0, &viommu_id);
 	test_cmd_hwpt_alloc_nested(self->device_id, viommu_id,
 				   IOMMU_HWPT_ALLOC_PASID,
 				   &nested_hwpt_id[2],
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
#18 lore raw

PATCH v5 9/29 iommufd: Do not unmap an owned iopt_area

In reply to Nicolin Chen

Do not allow to unmap an iopt_area that is owned by an object, giving this owner a choice to lock the mapping in the iopt.

It will be used by vIOMMU's HW QUEUE structure that must prevent the queue memory from being unmapped in the nesting parent IO page table, until user space destroys the HW QUEUE object first.

---
 drivers/iommu/iommufd/io_pagetable.h |  7 ++++---
 drivers/iommu/iommufd/device.c       |  8 +++++---
 drivers/iommu/iommufd/io_pagetable.c |  6 ++++++
 drivers/iommu/iommufd/pages.c        | 12 ++++++++++--
 4 files changed, 25 insertions(+), 8 deletions(-)
drivers/iommu/iommufd/io_pagetable.h +4 -3
index c115a51d9384..2f2a47a2f9ee 100644
@@ -48,6 +48,7 @@ struct iopt_area {
 	int iommu_prot;
 	bool prevent_access : 1;
 	unsigned int num_accesses;
+	unsigned int num_owners;
 };
 
 struct iopt_allowed {
@@ -235,12 +236,11 @@ int iopt_pages_fill_xarray(struct iopt_pages *pages, unsigned long start,
 			   unsigned long last, struct page **out_pages);
 void iopt_pages_unfill_xarray(struct iopt_pages *pages, unsigned long start,
 			      unsigned long last);
-
 int iopt_area_add_access(struct iopt_area *area, unsigned long start,
 			 unsigned long last, struct page **out_pages,
-			 unsigned int flags);
+			 unsigned int flags, bool is_owner);
 void iopt_area_remove_access(struct iopt_area *area, unsigned long start,
-			     unsigned long last);
+			     unsigned long last, bool is_owner);
 int iopt_pages_rw_access(struct iopt_pages *pages, unsigned long start_byte,
 			 void *data, unsigned long length, unsigned int flags);
 
@@ -250,6 +250,7 @@ int iopt_pages_rw_access(struct iopt_pages *pages, unsigned long start_byte,
  */
 struct iopt_pages_access {
 	struct interval_tree_node node;
+	unsigned int owners;
 	unsigned int users;
 };
 
drivers/iommu/iommufd/device.c +5 -3
index 16c17ae1f5f2..2b1ed5c8dc5b 100644
@@ -1265,7 +1265,8 @@ void iommufd_access_unpin_pages(struct iommufd_access *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))));
+				min(last_iova, iopt_area_last_iova(area))),
+			false);
 	WARN_ON(!iopt_area_contig_done(&iter));
 	up_read(&iopt->iova_rwsem);
 	mutex_unlock(&access->ioas_lock);
@@ -1356,7 +1357,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 		}
 
 		rc = iopt_area_add_access(area, index, last_index, out_pages,
-					  flags);
+					  flags, false);
 		if (rc)
 			goto err_remove;
 		out_pages += last_index - index + 1;
@@ -1379,7 +1380,8 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova,
 				iopt_area_iova_to_index(area, iter.cur_iova),
 				iopt_area_iova_to_index(
 					area, min(last_iova,
-						  iopt_area_last_iova(area))));
+						  iopt_area_last_iova(area))),
+				false);
 	}
 	up_read(&iopt->iova_rwsem);
 	mutex_unlock(&access->ioas_lock);
drivers/iommu/iommufd/io_pagetable.c +6 -0
index 13d010f19ed1..51efebb1c6ed 100644
@@ -719,6 +719,12 @@ static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start,
 			goto out_unlock_iova;
 		}
 
+		/* The area is held by an object that has not been destroyed */
+		if (area->num_owners) {
+			rc = -EBUSY;
+			goto out_unlock_iova;
+		}
+
 		if (area_first < start || area_last > last) {
 			rc = -ENOENT;
 			goto out_unlock_iova;
drivers/iommu/iommufd/pages.c +10 -2
index cbdde642d2af..4793b4de5fca 100644
@@ -2111,7 +2111,7 @@ iopt_pages_get_exact_access(struct iopt_pages *pages, unsigned long index,
  */
 int iopt_area_add_access(struct iopt_area *area, unsigned long start_index,
 			 unsigned long last_index, struct page **out_pages,
-			 unsigned int flags)
+			 unsigned int flags, bool is_owner)
 {
 	struct iopt_pages *pages = area->pages;
 	struct iopt_pages_access *access;
@@ -2124,6 +2124,8 @@ int iopt_area_add_access(struct iopt_area *area, unsigned long start_index,
 	access = iopt_pages_get_exact_access(pages, start_index, last_index);
 	if (access) {
 		area->num_accesses++;
+		if (is_owner)
+			area->num_owners++;
 		access->users++;
 		iopt_pages_fill_from_xarray(pages, start_index, last_index,
 					    out_pages);
@@ -2145,6 +2147,8 @@ int iopt_area_add_access(struct iopt_area *area, unsigned long start_index,
 	access->node.last = last_index;
 	access->users = 1;
 	area->num_accesses++;
+	if (is_owner)
+		area->num_owners++;
 	interval_tree_insert(&access->node, &pages->access_itree);
 	mutex_unlock(&pages->mutex);
 	return 0;
@@ -2166,7 +2170,7 @@ int iopt_area_add_access(struct iopt_area *area, unsigned long start_index,
  * must stop using the PFNs before calling this.
  */
 void iopt_area_remove_access(struct iopt_area *area, unsigned long start_index,
-			     unsigned long last_index)
+			     unsigned long last_index, bool is_owner)
 {
 	struct iopt_pages *pages = area->pages;
 	struct iopt_pages_access *access;
@@ -2177,6 +2181,10 @@ void iopt_area_remove_access(struct iopt_area *area, unsigned long start_index,
 		goto out_unlock;
 
 	WARN_ON(area->num_accesses == 0 || access->users == 0);
+	if (is_owner) {
+		WARN_ON(area->num_owners == 0);
+		area->num_owners--;
+	}
 	area->num_accesses--;
 	access->users--;
 	if (access->users)
Signature · 1 line
2.43.0
maintainer Reviewed-by on v5 9/29
#19 lore raw

RE: [PATCH v5 09/29] iommufd: Do not unmap an owned iopt_area

drivers/iommu/iommufd/io_pagetable.h struct iopt_area { · line 51

Quoted 8 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:21 AM

drivers/iommu/iommufd/io_pagetable.h +1 -0
@@ -48,6 +48,7 @@ struct iopt_area {
 	int iommu_prot;
 	bool prevent_access : 1;
 	unsigned int num_accesses;
+	unsigned int num_owners;

What about 'num_locked' to directly mark out that this area is locked hence cannot be removed?

#20 lore raw

Re: [PATCH v5 09/29] iommufd: Do not unmap an owned iopt_area

Quoted 11 lines · Kevin

drivers/iommu/iommufd/io_pagetable.h struct iopt_area { · line 51

Quoted 8 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:21 AM

drivers/iommu/iommufd/io_pagetable.h +1 -0
@@ -48,6 +48,7 @@ struct iopt_area {
 	int iommu_prot;
 	bool prevent_access : 1;
 	unsigned int num_accesses;
+	unsigned int num_owners;

What about 'num_locked' to directly mark out that this area is locked hence cannot be removed?

Yea. That sounds more straightforward to me. I will keep it plural "num_locks". And "is_owner" can be just "locks_area".

Thanks
Nicolin

#21 lore raw

Re: [PATCH v5 09/29] iommufd: Do not unmap an owned iopt_area

In reply to Nicolin Chen

drivers/iommu/iommufd/io_pagetable.h int iopt_pages_rw_access(struct iopt_pages *pages, unsigned long start_byte, · line 254

Quoted 6 lines · Nicolin Chen
drivers/iommu/iommufd/io_pagetable.h +1 -0
@@ -250,6 +250,7 @@ int iopt_pages_rw_access(struct iopt_pages *pages, unsigned long start_byte,
  */
 struct iopt_pages_access {
 	struct interval_tree_node node;
+	unsigned int owners;
 	unsigned int users;

This isn't used anywhere, and I forgot to delete.

Will drop in v6.

Thanks
Nicolin

maintainer
#22 lore raw

Re: [PATCH v5 09/29] iommufd: Do not unmap an owned iopt_area

In reply to Nicolin Chen
Quoted 20 lines · Nicolin Chen

Do not allow to unmap an iopt_area that is owned by an object, giving this owner a choice to lock the mapping in the iopt.

It will be used by vIOMMU's HW QUEUE structure that must prevent the queue memory from being unmapped in the nesting parent IO page table, until user space destroys the HW QUEUE object first.

---
 drivers/iommu/iommufd/io_pagetable.h |  7 ++++---
 drivers/iommu/iommufd/device.c       |  8 +++++---
 drivers/iommu/iommufd/io_pagetable.c |  6 ++++++
 drivers/iommu/iommufd/pages.c        | 12 ++++++++++--
 4 files changed, 25 insertions(+), 8 deletions(-)
drivers/iommu/iommufd/io_pagetable.h +0 -0
index c115a51d9384..2f2a47a2f9ee 100644

drivers/iommu/iommufd/io_pagetable.h struct iopt_area { · line 52

Quoted 5 lines
drivers/iommu/iommufd/io_pagetable.h +1 -0
@@ -48,6 +48,7 @@ struct iopt_area {
 	int iommu_prot;
 	bool prevent_access : 1;
 	unsigned int num_accesses;
+	unsigned int num_owners;
 };

Owner seems like the wrong word here..

Fundamentally this is supporting accesses that don't have an unmap op callback.

It could be done differently, with the below just create your access with a null ops. Prevent mdev drivers from doing this when they create their accesses. We don't need to optimize this failure case so it is fine to scan the xarray. All we really need to do here is eliminate the WARN_ON.

drivers/iommu/iommufd/device.c +8 -2
index 2111bad72c720f..b88354de299449 100644
@@ -1206,13 +1206,14 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_replace, "IOMMUFD");
  * run in the future. Due to this a driver must not create locking that prevents
  * unmap to complete while iommufd_access_destroy() is running.
  */
-void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
+int iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
 				 unsigned long length)
 {
 	struct iommufd_ioas *ioas =
 		container_of(iopt, struct iommufd_ioas, iopt);
 	struct iommufd_access *access;
 	unsigned long index;
+	int ret = 0;
 
 	xa_lock(&ioas->iopt.access_list);
 	xa_for_each(&ioas->iopt.access_list, index, access) {
@@ -1220,12 +1221,17 @@ void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
 			continue;
 		xa_unlock(&ioas->iopt.access_list);
 
-		access->ops->unmap(access->data, iova, length);
+		if (access->ops && access->ops->unmap)
+			access->ops->unmap(access->data, iova, length);
+		else
+			ret = -EBUSY;
 
 		iommufd_put_object(access->ictx, &access->obj);
 		xa_lock(&ioas->iopt.access_list);
 	}
 	xa_unlock(&ioas->iopt.access_list);
+
+	return ret;
 }
 
 /**
drivers/iommu/iommufd/io_pagetable.c +9 -3
index 8a790e597e1253..f1cb809500fa19 100644
@@ -740,11 +740,16 @@ static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start,
 			up_write(&iopt->iova_rwsem);
 			up_read(&iopt->domains_rwsem);
 
-			iommufd_access_notify_unmap(iopt, area_first, length);
+			rc = iommufd_access_notify_unmap(iopt, area_first, length);
+			if (rc)
+				goto out_unmapped;
+
 			/* Something is not responding to unmap requests. */
 			tries++;
-			if (WARN_ON(tries > 100))
-				return -EDEADLOCK;
+			if (WARN_ON(tries > 100)) {
+				rc = -EDEADLOCK;
+				goto out_unmapped;
+			}
 			goto again;
 		}
 
@@ -766,6 +771,7 @@ static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start,
 out_unlock_iova:
 	up_write(&iopt->iova_rwsem);
 	up_read(&iopt->domains_rwsem);
+out_unmapped:
 	if (unmapped)
 		*unmapped = unmapped_bytes;
 	return rc;
#23 lore raw

Re: [PATCH v5 09/29] iommufd: Do not unmap an owned iopt_area

Quoted 2 lines · Jason Gunthorpe

Fundamentally this is supporting accesses that don't have an unmap op callback.

Ah, I see.

Quoted 5 lines

It could be done differently, with the below just create your access with a null ops. Prevent mdev drivers from doing this when they create their accesses. We don't need to optimize this failure case so it is fine to scan the xarray. All we really need to do here is eliminate the WARN_ON.

Yes. That approach looks much cleaner.

Will replace the iopt_pin_pages patch.

Thanks
Nicolin

#24 lore raw

Re: [PATCH v5 09/29] iommufd: Do not unmap an owned iopt_area

In reply to Jason Gunthorpe

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

Quoted 9 lines · Jason Gunthorpe
drivers/iommu/iommufd/io_pagetable.c +4 -1
@@ -740,11 +740,16 @@ static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start,
 			up_write(&iopt->iova_rwsem);
 			up_read(&iopt->domains_rwsem);
 
-			iommufd_access_notify_unmap(iopt, area_first, length);
+			rc = iommufd_access_notify_unmap(iopt, area_first, length);
+			if (rc)
+				goto out_unmapped;
+

I found that we have to re-take those rwsems to unset a flag:

			rc = iommufd_access_notify_unmap(iopt, area_first, length);
			if (rc) {
				down_read(&iopt->domains_rwsem);
  				down_write(&iopt->iova_rwsem);
				area->prevent_access = false;
				goto out_unlock_iova;
			}

Will include this in the patch.

Thanks
Nicolin

#25 lore raw

PATCH v5 10/29 iommufd: Abstract iopt_pin_pages and iopt_unpin_pages helpers

In reply to Nicolin Chen

The new HW QUEUE object will be added for HW to access the guest queue for HW-accelerated virtualization feature. Some of HW QUEUEs are designed in a way of accessing the guest queue via a host physical address without doing a translation using the nesting parent IO page table, while others can use the guest physical address. For the former case, kernel working with a VMM needs to pin the physical pages backing the guest memory to lock them when HW QUEUE is accessing, and to ensure those physical pages to be contiguous in the 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.

---
 drivers/iommu/iommufd/io_pagetable.h    |   8 ++
 drivers/iommu/iommufd/iommufd_private.h |   6 ++
 drivers/iommu/iommufd/device.c          | 119 ++----------------------
 drivers/iommu/iommufd/io_pagetable.c    |  97 +++++++++++++++++++
 4 files changed, 119 insertions(+), 111 deletions(-)
drivers/iommu/iommufd/io_pagetable.h +8 -0
index 2f2a47a2f9ee..e04dcc79d309 100644
@@ -115,6 +115,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 16767c231580..cef3e0e0bbb2 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, bool is_owner);
+void iopt_unpin_pages(struct io_pagetable *iopt, unsigned long iova,
+		      unsigned long length, bool is_owner);
+
 struct iommufd_ucmd {
 	struct iommufd_ctx *ictx;
 	void __user *ubuffer;
drivers/iommu/iommufd/device.c +8 -111

151 lines not on this page · raw message

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

100 lines not on this page · raw message

Signature · 1 line
2.43.0
maintainer
#26 lore raw

Re: [PATCH v5 10/29] iommufd: Abstract iopt_pin_pages and iopt_unpin_pages helpers

Quoted 22 lines · Nicolin Chen

The new HW QUEUE object will be added for HW to access the guest queue for HW-accelerated virtualization feature. Some of HW QUEUEs are designed in a way of accessing the guest queue via a host physical address without doing a translation using the nesting parent IO page table, while others can use the guest physical address. For the former case, kernel working with a VMM needs to pin the physical pages backing the guest memory to lock them when HW QUEUE is accessing, and to ensure those physical pages to be contiguous in the 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.

Quoted 6 lines
---
 drivers/iommu/iommufd/io_pagetable.h    |   8 ++
 drivers/iommu/iommufd/iommufd_private.h |   6 ++
 drivers/iommu/iommufd/device.c          | 119 ++----------------------
 drivers/iommu/iommufd/io_pagetable.c    |  97 +++++++++++++++++++
 4 files changed, 119 insertions(+), 111 deletions(-)

And if you do what was suggested do we need this patch at all? Just use the normal access sequence:

iommufd_access_create(ops=NULL)
iommufd_access_attach(viommu->hwpt->ioas)
iommufd_access_pin_pages()

And store a viommu->access pointer to undo it all.

This avoids making it all special with different internal behavior from a mdev. The only difference is we allowe ops=null for viommu but not for mdev.

I don't think it is worth doing all these changes just to eliminate the access memory allocation.. viommu allocation is not fast path.

Jason

#27 lore raw

Re: [PATCH v5 10/29] iommufd: Abstract iopt_pin_pages and iopt_unpin_pages helpers

Quoted 30 lines · Jason Gunthorpe
Quoted 22 lines · Nicolin Chen

The new HW QUEUE object will be added for HW to access the guest queue for HW-accelerated virtualization feature. Some of HW QUEUEs are designed in a way of accessing the guest queue via a host physical address without doing a translation using the nesting parent IO page table, while others can use the guest physical address. For the former case, kernel working with a VMM needs to pin the physical pages backing the guest memory to lock them when HW QUEUE is accessing, and to ensure those physical pages to be contiguous in the 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.

Quoted 6 lines
---
 drivers/iommu/iommufd/io_pagetable.h    |   8 ++
 drivers/iommu/iommufd/iommufd_private.h |   6 ++
 drivers/iommu/iommufd/device.c          | 119 ++----------------------
 drivers/iommu/iommufd/io_pagetable.c    |  97 +++++++++++++++++++
 4 files changed, 119 insertions(+), 111 deletions(-)
Quoted 8 lines

And if you do what was suggested do we need this patch at all? Just use the normal access sequence:

iommufd_access_create(ops=NULL)
iommufd_access_attach(viommu->hwpt->ioas)
iommufd_access_pin_pages()

And store a viommu->access pointer to undo it all.

I found the entire ictx would be locked by iommufd_access_create(), then the release fop couldn't even get invoked to destroy objects.

I added a new flag to address this:
-----------------------------------------------------------------

drivers/iommu/iommufd/device.c +4 -2
index f25e272ae378c..a3e0ace583a66 100644
@@ -1085,7 +1085,8 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
        if (access->ioas)
                WARN_ON(iommufd_access_change_ioas(access, NULL));
        mutex_unlock(&access->ioas_lock);
-       iommufd_ctx_put(access->ictx);
+       if (!access->ops->internal_use)
+               iommufd_ctx_put(access->ictx);
 }
 
 /**
@@ -1126,7 +1127,8 @@ iommufd_access_create(struct iommufd_ctx *ictx,
        /* The calling driver is a user until iommufd_access_destroy() */
        refcount_inc(&access->obj.users);
        access->ictx = ictx;
-       iommufd_ctx_get(ictx);
+       if (!ops->internal_use)
+               iommufd_ctx_get(ictx);
        iommufd_object_finalize(ictx, &access->obj);
        *id = access->obj.id;
        mutex_init(&access->ioas_lock);

-----------------------------------------------------------------

Btw, I think we can have an ops but only set unmap to NULL:

 static const struct iommufd_access_ops hw_queue_access_ops = {
        .needs_pin_pages = 1,
+       .internal_use = 1,
        /* NULL unmap to reject IOMMUFD_CMD_IOAS_UNMAP */

};

Having two flags makes the code slightly more readable. After all, HW queue does need to pin pages.

Thanks
Nicolin

maintainer
#28 lore raw

Re: [PATCH v5 10/29] iommufd: Abstract iopt_pin_pages and iopt_unpin_pages helpers

Quoted 2 lines · Nicolin Chen

I found the entire ictx would be locked by iommufd_access_create(), then the release fop couldn't even get invoked to destroy objects.

Yes, that makes sense..

It looks to me like you can safely leave ictx as NULL instead of adding a flag? That would be nicer than leaving a unrefcounted pointer floating around..

Jason

#29 lore raw

Re: [PATCH v5 10/29] iommufd: Abstract iopt_pin_pages and iopt_unpin_pages helpers

Quoted 10 lines · Jason Gunthorpe
Quoted 2 lines · Nicolin Chen

I found the entire ictx would be locked by iommufd_access_create(), then the release fop couldn't even get invoked to destroy objects.

Yes, that makes sense..

It looks to me like you can safely leave ictx as NULL instead of adding a flag? That would be nicer than leaving a unrefcounted pointer floating around..

Hmm, there are a few iommufd_get_object calls using access->ictx in iommufd_access_attach() and iommufd_access_destroy().

We could have a set of internal access APIs to leave access->ictx as NULL, as an internal caller has an ictx to pass in. It's going to be a larger change though..

Thanks
Nicolin

maintainer
#30 lore raw

Re: [PATCH v5 10/29] iommufd: Abstract iopt_pin_pages and iopt_unpin_pages helpers

Quoted 6 lines · Nicolin Chen
Quoted 5 lines · Jason Gunthorpe
Quoted 2 lines · Nicolin Chen

I found the entire ictx would be locked by iommufd_access_create(), then the release fop couldn't even get invoked to destroy objects.

Quoted 8 lines
Quoted 5 lines

Yes, that makes sense..

It looks to me like you can safely leave ictx as NULL instead of adding a flag? That would be nicer than leaving a unrefcounted pointer floating around..

Hmm, there are a few iommufd_get_object calls using access->ictx in iommufd_access_attach() and iommufd_access_destroy().

I counted:

iommufd_access_change_ioas_id
* Don't call this
iommufd_access_destroy_object
* Don't put if null
iommufd_access_create
* Don't set it
iommufd_access_destroy
* Call iommufd_object_destroy_user directly
iommufd_access_notify_unmap
* Check for null access->ops->unmap and skip the lock_obj/put_obj

Jason

#31 lore raw

Re: [PATCH v5 10/29] iommufd: Abstract iopt_pin_pages and iopt_unpin_pages helpers

Quoted 18 lines · Jason Gunthorpe
Quoted 6 lines · Nicolin Chen
Quoted 5 lines · Jason Gunthorpe
Quoted 2 lines · Nicolin Chen

I found the entire ictx would be locked by iommufd_access_create(), then the release fop couldn't even get invoked to destroy objects.

Quoted 8 lines
Quoted 5 lines

Yes, that makes sense..

It looks to me like you can safely leave ictx as NULL instead of adding a flag? That would be nicer than leaving a unrefcounted pointer floating around..

Hmm, there are a few iommufd_get_object calls using access->ictx in iommufd_access_attach() and iommufd_access_destroy().

I counted:

Quoted 10 lines

iommufd_access_change_ioas_id
* Don't call this
iommufd_access_destroy_object
* Don't put if null
iommufd_access_create
* Don't set it
iommufd_access_destroy
* Call iommufd_object_destroy_user directly
iommufd_access_notify_unmap
* Check for null access->ops->unmap and skip the lock_obj/put_obj

Yea. And I added a set of lighter "_internal" helpers so the caller side looks consistent:

	access = iommufd_access_create_internal(viommu->ictx);
	rc = iommufd_access_attach_internal(access, viommu->hwpt->ioas);
	...
	iommufd_access_detach_internal(access);
	iommufd_access_destroy_internal(ictx, access);

Thanks
Nicolin

#32 lore raw

PATCH v5 11/29 iommufd/driver: Let iommufd_viommu_alloc helper save ictx to viommu->ictx

In reply to 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.

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.

---
 drivers/iommu/iommufd/iommufd_private.h |  8 --------
 include/linux/iommufd.h                 | 12 +++++++++++-
 drivers/iommu/iommufd/viommu.c          |  7 ++++++-
 3 files changed, 17 insertions(+), 10 deletions(-)
drivers/iommu/iommufd/iommufd_private.h +0 -8
index cef3e0e0bbb2..44286216ac5a 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, bool is_owner);
 
-struct iommufd_ucmd {
-	struct iommufd_ctx *ictx;
-	void __user *ubuffer;
-	u32 user_size;
-	void *cmd;
-	struct iommufd_object *new_obj;
-};
-
 int iommufd_vfio_ioctl(struct iommufd_ctx *ictx, unsigned int cmd,
 		       unsigned long arg);
 
include/linux/iommufd.h +11 -1
index d74c97feb9b5..a07466333c48 100644
@@ -52,6 +52,14 @@ struct iommufd_object {
 	unsigned int id;
 };
 
+struct iommufd_ucmd {
+	struct iommufd_ctx *ictx;
+	void __user *ubuffer;
+	u32 user_size;
+	void *cmd;
+	struct iommufd_object *new_obj;
+};
+
 #define __iommufd_object_alloc_ucmd(ucmd, ptr, type, obj)                      \
 	container_of(_iommufd_object_alloc_ucmd(                               \
 			     ucmd,                                             \
@@ -262,8 +270,10 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 									       \
 		ret = (drv_struct *)__iommufd_object_alloc_ucmd(               \
 			ucmd, ret, IOMMUFD_OBJ_VIOMMU, member.obj);            \
-		if (!IS_ERR(ret))                                              \
+		if (!IS_ERR(ret)) {                                            \
 			ret->member.ops = viommu_ops;                          \
+			ret->member.ictx = ucmd->ictx;                         \
+		}                                                              \
 		ret;                                                           \
 	})
 #endif
drivers/iommu/iommufd/viommu.c +6 -1
index 2b30627d1d8e..662f5c1b1935 100644
@@ -60,9 +60,14 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		goto out_put_hwpt;
 	}
 
+	/* The iommufd_viommu_alloc helper saves ucmd->ictx in viommu->ictx */
+	if (WARN_ON_ONCE(viommu->ictx != ucmd->ictx)) {
+		rc = -EINVAL;
+		goto out_put_hwpt;
+	}
+
 	xa_init(&viommu->vdevs);
 	viommu->type = cmd->type;
-	viommu->ictx = ucmd->ictx;
 	viommu->hwpt = hwpt_paging;
 	refcount_inc(&viommu->hwpt->common.obj.users);
 	INIT_LIST_HEAD(&viommu->veventqs);
Signature · 1 line
2.43.0
#33 lore raw

PATCH v5 12/29 iommufd/viommu: Add driver-allocated vDEVICE support

In reply to Nicolin Chen

To allow IOMMU drivers to allocate own vDEVICE structures, move the struct iommufd_vdevice to the public header and provide a pair of viommu ops.

The iommufd_vdevice_alloc_ioctl will prioritize the callback function from the viommu ops, i.e. a driver-allocated vDEVICE.

---
 drivers/iommu/iommufd/iommufd_private.h |  8 ------
 include/linux/iommufd.h                 | 36 +++++++++++++++++++++++++
 drivers/iommu/iommufd/viommu.c          | 10 ++++++-
 3 files changed, 45 insertions(+), 9 deletions(-)
drivers/iommu/iommufd/iommufd_private.h +0 -8
index 44286216ac5a..35088478cb07 100644
@@ -604,14 +604,6 @@ void iommufd_viommu_destroy(struct iommufd_object *obj);
 int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd);
 void iommufd_vdevice_destroy(struct iommufd_object *obj);
 
-struct iommufd_vdevice {
-	struct iommufd_object obj;
-	struct iommufd_ctx *ictx;
-	struct iommufd_viommu *viommu;
-	struct device *dev;
-	u64 id; /* per-vIOMMU virtual ID */
-};
-
 #ifdef CONFIG_IOMMUFD_TEST
 int iommufd_test(struct iommufd_ucmd *ucmd);
 void iommufd_selftest_destroy(struct iommufd_object *obj);
include/linux/iommufd.h +36 -0
index a07466333c48..266ac6805213 100644
@@ -125,6 +125,14 @@ struct iommufd_viommu {
 	unsigned int type;
 };
 
+struct iommufd_vdevice {
+	struct iommufd_object obj;
+	struct iommufd_ctx *ictx;
+	struct iommufd_viommu *viommu;
+	struct device *dev;
+	u64 id; /* per-vIOMMU virtual ID */
+};
+
 /**
  * struct iommufd_viommu_ops - vIOMMU specific operations
  * @destroy: Clean up all driver-specific parts of an iommufd_viommu. The memory
@@ -141,6 +149,15 @@ 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
+ *                 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 (refer to struct iommu_vdevice_alloc in
+ *                 include/uapi/linux/iommufd.h) for the driver to initialize HW
+ *                 for an attached physical device.
+ * @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);
@@ -149,6 +166,11 @@ struct iommufd_viommu_ops {
 		const struct iommu_user_data *user_data);
 	int (*cache_invalidate)(struct iommufd_viommu *viommu,
 				struct iommu_user_data_array *array);
+	struct iommufd_vdevice *(*vdevice_alloc)(struct iommufd_ucmd *ucmd,
+						 struct iommufd_viommu *viommu,
+						 struct device *dev,
+						 u64 virt_id);
+	void (*vdevice_destroy)(struct iommufd_vdevice *vdev);
 };
 
 #if IS_ENABLED(CONFIG_IOMMUFD)
@@ -276,4 +298,18 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 		}                                                              \
 		ret;                                                           \
 	})
+
+#define iommufd_vdevice_alloc(ucmd, viommu, drv_struct, member)                \
+	({                                                                     \
+		drv_struct *ret;                                               \
+									       \
+		static_assert(__same_type(struct iommufd_viommu, *viommu));    \
+		ret = (drv_struct *)__iommufd_object_alloc_ucmd(               \
+			ucmd, ret, IOMMUFD_OBJ_VDEVICE, member.obj);           \
+		if (!IS_ERR(ret)) {                                            \
+			ret->member.viommu = viommu;                           \
+			ret->member.ictx = viommu->ictx;                       \
+		}                                                              \
+		ret;                                                           \
+	})
 #endif
drivers/iommu/iommufd/viommu.c +9 -1
index 662f5c1b1935..7248fb7c7baf 100644
@@ -95,6 +95,9 @@ void iommufd_vdevice_destroy(struct iommufd_object *obj)
 		container_of(obj, struct iommufd_vdevice, obj);
 	struct iommufd_viommu *viommu = vdev->viommu;
 
+	if (viommu->ops && viommu->ops->vdevice_destroy)
+		viommu->ops->vdevice_destroy(vdev);
+
 	/* xa_cmpxchg is okay to fail if alloc failed xa_cmpxchg previously */
 	xa_cmpxchg(&viommu->vdevs, vdev->id, vdev, NULL, GFP_KERNEL);
 	refcount_dec(&viommu->obj.users);
@@ -129,7 +132,12 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		goto out_put_idev;
 	}
 
-	vdev = iommufd_object_alloc_ucmd(ucmd, vdev, IOMMUFD_OBJ_VDEVICE);
+	if (viommu->ops && viommu->ops->vdevice_alloc)
+		vdev = viommu->ops->vdevice_alloc(ucmd, viommu, idev->dev,
+						  virt_id);
+	else
+		vdev = iommufd_object_alloc_ucmd(ucmd, vdev,
+						 IOMMUFD_OBJ_VDEVICE);
 	if (IS_ERR(vdev)) {
 		rc = PTR_ERR(vdev);
 		goto out_put_idev;
Signature · 1 line
2.43.0
#34 lore raw

PATCH v5 13/29 iommufd/viommu: Introduce IOMMUFD_OBJ_HW_QUEUE and its related struct

In reply to Nicolin Chen

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.

---
 include/linux/iommufd.h | 47 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)
include/linux/iommufd.h +47 -0
index 266ac6805213..923c66ccc15a 100644
@@ -38,6 +38,7 @@ enum iommufd_object_type {
 	IOMMUFD_OBJ_VIOMMU,
 	IOMMUFD_OBJ_VDEVICE,
 	IOMMUFD_OBJ_VEVENTQ,
+	IOMMUFD_OBJ_HW_QUEUE,
 #ifdef CONFIG_IOMMUFD_TEST
 	IOMMUFD_OBJ_SELFTEST,
 #endif
@@ -133,6 +134,24 @@ struct iommufd_vdevice {
 	u64 id; /* per-vIOMMU virtual ID */
 };
 
+struct iommufd_hw_queue {
+	struct iommufd_object obj;
+	struct iommufd_ctx *ictx;
+	struct iommufd_viommu *viommu;
+	u64 base_addr; /* in guest physical address space */
+	size_t length;
+};
+
+enum iommufd_viommu_flags {
+	/*
+	 * The HW does not go through an address translation table but reads the
+	 * physical address space directly: iommufd core should pin the physical
+	 * pages backing the queue memory that's allocated for the HW QUEUE, and
+	 * ensure those physical pages are contiguous in the physical space.
+	 */
+	IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA = 1 << 0,
+};
+
 /**
  * struct iommufd_viommu_ops - vIOMMU specific operations
  * @destroy: Clean up all driver-specific parts of an iommufd_viommu. The memory
@@ -158,8 +177,18 @@ 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
+ * @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 model; @base_addr
+ *                  carries the guest physical base address of the queue memory;
+ *                  @length carries the size of the queue
+ * @hw_queue_destroy: Clean up all driver-specific parts of an iommufd_hw_queue.
+ *                    The memory of the HW QUEUE will be free-ed by iommufd core
+ *                    after calling this op
  */
 struct iommufd_viommu_ops {
+	u32 flags;
 	void (*destroy)(struct iommufd_viommu *viommu);
 	struct iommu_domain *(*alloc_domain_nested)(
 		struct iommufd_viommu *viommu, u32 flags,
@@ -171,6 +200,10 @@ struct iommufd_viommu_ops {
 						 struct device *dev,
 						 u64 virt_id);
 	void (*vdevice_destroy)(struct iommufd_vdevice *vdev);
+	struct iommufd_hw_queue *(*hw_queue_alloc)(
+		struct iommufd_ucmd *ucmd, struct iommufd_viommu *viommu,
+		unsigned int type, u32 index, u64 base_addr, size_t length);
+	void (*hw_queue_destroy)(struct iommufd_hw_queue *hw_queue);
 };
 
 #if IS_ENABLED(CONFIG_IOMMUFD)
@@ -312,4 +345,18 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 		}                                                              \
 		ret;                                                           \
 	})
+
+#define iommufd_hw_queue_alloc(ucmd, viommu, drv_struct, member)               \
+	({                                                                     \
+		drv_struct *ret;                                               \
+									       \
+		static_assert(__same_type(struct iommufd_viommu, *viommu));    \
+		ret = (drv_struct *)__iommufd_object_alloc_ucmd(               \
+			ucmd, ret, IOMMUFD_OBJ_HW_QUEUE, member.obj);          \
+		if (!IS_ERR(ret)) {                                            \
+			ret->member.viommu = viommu;                           \
+			ret->member.ictx = viommu->ictx;                       \
+		}                                                              \
+		ret;                                                           \
+	})
 #endif
Signature · 1 line
2.43.0
maintainer Reviewed-by on v5 13/29
#35 lore raw

RE: [PATCH v5 13/29] iommufd/viommu: Introduce IOMMUFD_OBJ_HW_QUEUE and its related struct

Quoted 12 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:22 AM

include/linux/iommufd.h +4 -0
+
+enum iommufd_viommu_flags {
+	/*
+	 * The HW does not go through an address translation table but

reads the

(quoted diff) +1 -0
+	 * physical address space directly: iommufd core should pin the

physical

(quoted diff) +1 -0
+	 * pages backing the queue memory that's allocated for the HW

QUEUE, and

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

Quoted 4 lines
include/linux/iommufd.h +4 -0
+	 * ensure those physical pages are contiguous in the physical space.
+	 */
+	IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA = 1 << 0,
+};

The queue itself doesn't read an address.

What about 'QUEUE_BASE_PA'?

#36 lore raw

Re: [PATCH v5 13/29] iommufd/viommu: Introduce IOMMUFD_OBJ_HW_QUEUE and its related struct

Quoted 19 lines · Kevin
Quoted 12 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:22 AM

include/linux/iommufd.h +4 -0
+
+enum iommufd_viommu_flags {
+	/*
+	 * The HW does not go through an address translation table but

reads the

(quoted diff) +1 -0
+	 * physical address space directly: iommufd core should pin the

physical

(quoted diff) +1 -0
+	 * pages backing the queue memory that's allocated for the HW

QUEUE, and

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

Quoted 4 lines
include/linux/iommufd.h +4 -0
+	 * ensure those physical pages are contiguous in the physical space.
+	 */
+	IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA = 1 << 0,
+};

The queue itself doesn't read an address.

Quoted 1 line

What about 'QUEUE_BASE_PA'?

But the HW queue object represents the HW feature, not the guest queue memory. So, it is accurate to say that it reads an address?

We have this in doc:
- IOMMUFD_OBJ_HW_QUEUE, representing a hardware accelerated queue, as a subset of IOMMU's virtualization features, for the IOMMU HW to directly read or write the virtual queue memory owned by a guest OS. This HW-acceleration feature can
...

Thanks
Nic

maintainer
#37 lore raw

RE: [PATCH v5 13/29] iommufd/viommu: Introduce IOMMUFD_OBJ_HW_QUEUE and its related struct

Quoted 28 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, May 24, 2025 5:46 AM

Quoted 19 lines · Kevin
Quoted 12 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:22 AM

include/linux/iommufd.h +4 -0
+
+enum iommufd_viommu_flags {
+	/*
+	 * The HW does not go through an address translation table but

reads the

(quoted diff) +1 -0
+	 * physical address space directly: iommufd core should pin the

physical

(quoted diff) +1 -0
+	 * pages backing the queue memory that's allocated for the HW

QUEUE, and

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

Quoted 4 lines
include/linux/iommufd.h +4 -0
+	 * ensure those physical pages are contiguous in the physical space.
+	 */
+	IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA = 1 << 0,
+};

The queue itself doesn't read an address.

Quoted 1 line

What about 'QUEUE_BASE_PA'?

But the HW queue object represents the HW feature, not the guest queue memory. So, it is accurate to say that it reads an address?

Quoted 9 lines

We have this in doc:
- IOMMUFD_OBJ_HW_QUEUE, representing a hardware accelerated queue, as a subset
of IOMMU's virtualization features, for the IOMMU HW to directly read or
write
the virtual queue memory owned by a guest OS. This HW-acceleration
feature can
...

Okay. Then ACCESS_PA means both read/write?

#38 lore raw

Re: [PATCH v5 13/29] iommufd/viommu: Introduce IOMMUFD_OBJ_HW_QUEUE and its related struct

Quoted 38 lines · Kevin
Quoted 28 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, May 24, 2025 5:46 AM

Quoted 19 lines · Kevin
Quoted 12 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:22 AM

include/linux/iommufd.h +4 -0
+
+enum iommufd_viommu_flags {
+	/*
+	 * The HW does not go through an address translation table but

reads the

(quoted diff) +1 -0
+	 * physical address space directly: iommufd core should pin the

physical

(quoted diff) +1 -0
+	 * pages backing the queue memory that's allocated for the HW

QUEUE, and

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

Quoted 4 lines
include/linux/iommufd.h +4 -0
+	 * ensure those physical pages are contiguous in the physical space.
+	 */
+	IOMMUFD_VIOMMU_FLAG_HW_QUEUE_READS_PA = 1 << 0,
+};

The queue itself doesn't read an address.

Quoted 1 line

What about 'QUEUE_BASE_PA'?

But the HW queue object represents the HW feature, not the guest queue memory. So, it is accurate to say that it reads an address?

Quoted 9 lines

We have this in doc:
- IOMMUFD_OBJ_HW_QUEUE, representing a hardware accelerated queue, as a subset
of IOMMU's virtualization features, for the IOMMU HW to directly read or
write
the virtual queue memory owned by a guest OS. This HW-acceleration
feature can
...

Quoted 1 line

Okay. Then ACCESS_PA means both read/write?

OK. IOMMUFD_VIOMMU_FLAG_HW_QUEUE_ACCESS_PA

maintainer
#39 lore raw

Re: [PATCH v5 13/29] iommufd/viommu: Introduce IOMMUFD_OBJ_HW_QUEUE and its related struct

In reply to Nicolin Chen

include/linux/iommufd.h struct iommufd_viommu_ops { · line 205

Quoted 12 lines · Nicolin Chen

struct iommufd_viommu_ops {

include/linux/iommufd.h +4 -0
+	u32 flags;
 	void (*destroy)(struct iommufd_viommu *viommu);
 	struct iommu_domain *(*alloc_domain_nested)(
 		struct iommufd_viommu *viommu, u32 flags,
@@ -171,6 +200,10 @@ struct iommufd_viommu_ops {
 						 struct device *dev,
 						 u64 virt_id);
 	void (*vdevice_destroy)(struct iommufd_vdevice *vdev);
+	struct iommufd_hw_queue *(*hw_queue_alloc)(
+		struct iommufd_ucmd *ucmd, struct iommufd_viommu *viommu,
+		unsigned int type, u32 index, u64 base_addr, size_t length);

I think it would better to have two function pointers here than the flags:

+ struct iommufd_hw_queue *(*hw_queue_alloc)(
+ struct iommufd_ucmd *ucmd, struct iommufd_viommu *viommu,
+ unsigned int type, u32 index, u64 s2_iova, size_t length);

+ struct iommufd_hw_queue *(*hw_queue_alloc_phys)(
+ struct iommufd_ucmd *ucmd, struct iommufd_viommu *viommu,
+ unsigned int type, u32 index, phys_addr_t phys, size_t length);

Jason

#40 lore raw

Re: [PATCH v5 13/29] iommufd/viommu: Introduce IOMMUFD_OBJ_HW_QUEUE and its related struct

Quoted 16 lines · Jason Gunthorpe

include/linux/iommufd.h struct iommufd_viommu_ops { · line 205

Quoted 12 lines · Nicolin Chen

struct iommufd_viommu_ops {

include/linux/iommufd.h +4 -0
+	u32 flags;
 	void (*destroy)(struct iommufd_viommu *viommu);
 	struct iommu_domain *(*alloc_domain_nested)(
 		struct iommufd_viommu *viommu, u32 flags,
@@ -171,6 +200,10 @@ struct iommufd_viommu_ops {
 						 struct device *dev,
 						 u64 virt_id);
 	void (*vdevice_destroy)(struct iommufd_vdevice *vdev);
+	struct iommufd_hw_queue *(*hw_queue_alloc)(
+		struct iommufd_ucmd *ucmd, struct iommufd_viommu *viommu,
+		unsigned int type, u32 index, u64 base_addr, size_t length);

I think it would better to have two function pointers here than the flags:

Quoted 8 lines

+ struct iommufd_hw_queue *(*hw_queue_alloc)(
+ struct iommufd_ucmd *ucmd, struct iommufd_viommu *viommu,
+ unsigned int type, u32 index, u64 s2_iova, size_t length);

+ struct iommufd_hw_queue *(*hw_queue_alloc_phys)(
+ struct iommufd_ucmd *ucmd, struct iommufd_viommu *viommu,
+ unsigned int type, u32 index, phys_addr_t phys, size_t length);

OK. I think these two should be exclusive then. Maybe it needs a WARN_ON in iommufd_viommu_alloc.

Thanks
Nicolin

#41 lore raw

PATCH v5 14/29 iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

In reply to Nicolin Chen

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

This is a vIOMMU based ioctl. Simply increase the refcount of the vIOMMU.

---
 drivers/iommu/iommufd/iommufd_private.h |   2 +
 include/uapi/linux/iommufd.h            |  45 ++++++++++
 drivers/iommu/iommufd/main.c            |   6 ++
 drivers/iommu/iommufd/viommu.c          | 105 ++++++++++++++++++++++++
 4 files changed, 158 insertions(+)
drivers/iommu/iommufd/iommufd_private.h +2 -0
index 35088478cb07..a94d04ab0d2c 100644
@@ -603,6 +603,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_hw_queue_alloc_ioctl(struct iommufd_ucmd *ucmd);
+void iommufd_hw_queue_destroy(struct iommufd_object *obj);
 
 #ifdef CONFIG_IOMMUFD_TEST
 int iommufd_test(struct iommufd_ucmd *ucmd);
include/uapi/linux/iommufd.h +45 -0

55 lines not on this page · raw message

drivers/iommu/iommufd/main.c +6 -0
index c6d0b446e632..0750e740fa1d 100644
@@ -304,6 +304,7 @@ union ucmd_buffer {
 	struct iommu_destroy destroy;
 	struct iommu_fault_alloc fault;
 	struct iommu_hw_info info;
+	struct iommu_hw_queue_alloc hw_queue;
 	struct iommu_hwpt_alloc hwpt;
 	struct iommu_hwpt_get_dirty_bitmap get_dirty_bitmap;
 	struct iommu_hwpt_invalidate cache;
@@ -346,6 +347,8 @@ static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
 		 struct iommu_fault_alloc, out_fault_fd),
 	IOCTL_OP(IOMMU_GET_HW_INFO, iommufd_get_hw_info, struct iommu_hw_info,
 		 __reserved),
+	IOCTL_OP(IOMMU_HW_QUEUE_ALLOC, iommufd_hw_queue_alloc_ioctl,
+		 struct iommu_hw_queue_alloc, length),
 	IOCTL_OP(IOMMU_HWPT_ALLOC, iommufd_hwpt_alloc, struct iommu_hwpt_alloc,
 		 __reserved),
 	IOCTL_OP(IOMMU_HWPT_GET_DIRTY_BITMAP, iommufd_hwpt_get_dirty_bitmap,
@@ -509,6 +512,9 @@ static const struct iommufd_object_ops iommufd_object_ops[] = {
 	[IOMMUFD_OBJ_FAULT] = {
 		.destroy = iommufd_fault_destroy,
 	},
+	[IOMMUFD_OBJ_HW_QUEUE] = {
+		.destroy = iommufd_hw_queue_destroy,
+	},
 	[IOMMUFD_OBJ_HWPT_PAGING] = {
 		.destroy = iommufd_hwpt_paging_destroy,
 		.abort = iommufd_hwpt_paging_abort,
drivers/iommu/iommufd/viommu.c +105 -0

108 lines not on this page · raw message

Signature · 1 line
2.43.0
maintainer Reviewed-by on v5 14/29
#42 lore raw

RE: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:22 AM

+ * @nesting_parent_iova: Base address of the queue memory in the guest physical
+ * address space

'nesting_parent' is a bit redundant. 'iova' should be sufficient as it implies a s2 input address.

Quoted 5 lines

+ * @length: Length of the queue memory
+ *
+ * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue, which
+ * allows HW to access a guest queue memory described using

Quoted 8 lines

@nesting_parent_iova
+ * 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.

This is conditional.

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

Quoted 11 lines
drivers/iommu/iommufd/viommu.c +9 -0
+void iommufd_hw_queue_destroy(struct iommufd_object *obj)
+{
+	struct iommufd_hw_queue *hw_queue =
+		container_of(obj, struct iommufd_hw_queue, obj);
+	struct iommufd_viommu *viommu = hw_queue->viommu;
+
+	if (viommu->ops->hw_queue_destroy)
+		viommu->ops->hw_queue_destroy(hw_queue);
+	iopt_unpin_pages(&viommu->hwpt->ioas->iopt, hw_queue-
Quoted 1 line

base_addr,

drivers/iommu/iommufd/viommu.c +1 -0
+			 hw_queue->length, true);

Check the flag

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

Quoted 3 lines
drivers/iommu/iommufd/viommu.c +3 -0
+
+		/* Validate if the underlying physical pages are contiguous */
+		for (i = 1; i < max_npages && pages[i]; i++) {

Page[i] must be valid otherwise it's a bug?

#43 lore raw

Re: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

Quoted 9 lines · Kevin
Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:22 AM

+ * @nesting_parent_iova: Base address of the queue memory in the guest physical
+ * address space

'nesting_parent' is a bit redundant. 'iova' should be sufficient as it implies a s2 input address.

Hmm, Jason suggested to highlight "nesting_parent":
https://lore.kernel.org/all/[email protected]/

Quoted 15 lines
Quoted 14 lines

+ * @length: Length of the queue memory
+ *
+ * Allocate a HW queue object for a vIOMMU-specific HW-accelerated queue, which
+ * allows HW to access a guest queue memory described using @nesting_parent_iova
+ * 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 1 line

This is conditional.

Yea, forgot to delete that.

Quoted 10 lines
Quoted 10 lines
drivers/iommu/iommufd/viommu.c +9 -0
+void iommufd_hw_queue_destroy(struct iommufd_object *obj)
+{
+	struct iommufd_hw_queue *hw_queue =
+		container_of(obj, struct iommufd_hw_queue, obj);
+	struct iommufd_viommu *viommu = hw_queue->viommu;
+
+	if (viommu->ops->hw_queue_destroy)
+		viommu->ops->hw_queue_destroy(hw_queue);
+	iopt_unpin_pages(&viommu->hwpt->ioas->iopt, hw_queue-
Quoted 4 lines

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

Quoted 2 lines
Quoted 1 line

base_addr,

drivers/iommu/iommufd/viommu.c +1 -0
+			 hw_queue->length, true);

Check the flag

Oh, right! Will fix.

Quoted 5 lines

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

Quoted 3 lines
drivers/iommu/iommufd/viommu.c +3 -0
+
+		/* Validate if the underlying physical pages are contiguous */
+		for (i = 1; i < max_npages && pages[i]; i++) {

Page[i] must be valid otherwise it's a bug?

Yea, I think so. We can drop this pages[i].

Thanks!
Nicolin

maintainer
#44 lore raw

Re: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

In reply to Nicolin Chen
Quoted 4 lines · Nicolin Chen
drivers/iommu/iommufd/viommu.c +4 -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;

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

Quoted 10 lines
drivers/iommu/iommufd/viommu.c +10 -0
+	struct iommufd_viommu *viommu;
+	struct page **pages;
+	int max_npages, i;
+	u64 last, offset;
+	int rc;
+
+	if (cmd->flags || cmd->type == IOMMU_HW_QUEUE_TYPE_DEFAULT)
+		return -EOPNOTSUPP;
+	if (!cmd->nesting_parent_iova || !cmd->length)
+		return -EINVAL;

0 is a legal nesting_parent_iova

Quoted 6 lines
drivers/iommu/iommufd/viommu.c +6 -0
+	if (check_add_overflow(cmd->nesting_parent_iova, cmd->length - 1,
+			       &last))
+		return -EOVERFLOW;
+
+	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
+	if (IS_ERR(viommu))

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

Quoted 10 lines
drivers/iommu/iommufd/viommu.c +10 -0
+		return PTR_ERR(viommu);
+
+	if (!viommu->ops || !viommu->ops->hw_queue_alloc) {
+		rc = -EOPNOTSUPP;
+		goto out_put_viommu;
+	}
+
+	offset =
+		cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova);
+	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);

This should probably be capped to PAGE_SIZE/sizeof(void *), return EINVAL if not

And we don't need the allocation if we are not doing an access.

Quoted 4 lines
drivers/iommu/iommufd/viommu.c +4 -0
+	hw_queue = viommu->ops->hw_queue_alloc(ucmd, viommu, cmd->type,
+					       cmd->index,
+					       cmd->nesting_parent_iova,
+					       cmd->length);

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

Quoted 10 lines
drivers/iommu/iommufd/viommu.c +10 -0
+	if (IS_ERR(hw_queue)) {
+		rc = PTR_ERR(hw_queue);
+		goto out_unpin;
+	}
+
+	/* The iommufd_hw_queue_alloc helper saves ictx in hw_queue->ictx */
+	if (WARN_ON_ONCE(hw_queue->ictx != ucmd->ictx)) {
+		rc = -EINVAL;
+		goto out_unpin;
+	}

There is another technique from RDMA which may actually be very helpful here considering how things are getting split up..

Put a size_t in the driver ops:

size_t size_viommu;
size_t size_hw_queue;

Have the driver set it via a macro like INIT_RDMA_OBJ_SIZE

#define INIT_RDMA_OBJ_SIZE(ib_struct, drv_struct, member)                      \
	.size_##ib_struct =                                                    \
		(sizeof(struct drv_struct) +                                   \
		 BUILD_BUG_ON_ZERO(offsetof(struct drv_struct, member)) +      \
		 BUILD_BUG_ON_ZERO(                                            \
			 !__same_type(((struct drv_struct *)NULL)->member,     \
				      struct ib_struct)))

Which proves the core structure is at the front.

Then the core code can allocate the object along with enough space for the driver and call a driver function to init the driver portion of the already allocated object.

Then you don't need these dances where the driver helper has to do things like set uctx, or the core structure is partially initialized:

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

Quoted 4 lines
drivers/iommu/iommufd/viommu.c +4 -0
+	hw_queue->viommu = viommu;
+	refcount_inc(&viommu->obj.users);
+	hw_queue->length = cmd->length;
+	hw_queue->base_addr = cmd->nesting_parent_iova;

When the driver is running, which can be a source of bugs.

This would be useful for the existing ops too.

May reduce the size of the linked in code.

Jason

#45 lore raw

Re: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

Quoted 7 lines · Jason Gunthorpe

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

Quoted 3 lines · Nicolin Chen
drivers/iommu/iommufd/viommu.c +3 -0
+	offset =
+		cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova);
+	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);

This should probably be capped to PAGE_SIZE/sizeof(void *), return EINVAL if not

Hmm, mind elaborating where this PAGE_SIZE/sizeof comes from?

Quoted 33 lines
Quoted 4 lines
drivers/iommu/iommufd/viommu.c +4 -0
+	hw_queue = viommu->ops->hw_queue_alloc(ucmd, viommu, cmd->type,
+					       cmd->index,
+					       cmd->nesting_parent_iova,
+					       cmd->length);

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

Quoted 10 lines
drivers/iommu/iommufd/viommu.c +10 -0
+	if (IS_ERR(hw_queue)) {
+		rc = PTR_ERR(hw_queue);
+		goto out_unpin;
+	}
+
+	/* The iommufd_hw_queue_alloc helper saves ictx in hw_queue->ictx */
+	if (WARN_ON_ONCE(hw_queue->ictx != ucmd->ictx)) {
+		rc = -EINVAL;
+		goto out_unpin;
+	}

There is another technique from RDMA which may actually be very helpful here considering how things are getting split up..

Put a size_t in the driver ops:

size_t size_viommu;
size_t size_hw_queue;

Have the driver set it via a macro like INIT_RDMA_OBJ_SIZE

#define INIT_RDMA_OBJ_SIZE(ib_struct, drv_struct, member)                      \
	.size_##ib_struct =                                                    \
		(sizeof(struct drv_struct) +                                   \
		 BUILD_BUG_ON_ZERO(offsetof(struct drv_struct, member)) +      \
		 BUILD_BUG_ON_ZERO(                                            \
			 !__same_type(((struct drv_struct *)NULL)->member,     \
				      struct ib_struct)))
Quoted 5 lines

Which proves the core structure is at the front.

Then the core code can allocate the object along with enough space for the driver and call a driver function to init the driver portion of the already allocated object.

That's interesting! Then all "_alloc" ops would be "_init" instead.

Quoted 9 lines

Then you don't need these dances where the driver helper has to do things like set uctx, or the core structure is partially initialized:

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

Quoted 4 lines
drivers/iommu/iommufd/viommu.c +4 -0
+	hw_queue->viommu = viommu;
+	refcount_inc(&viommu->obj.users);
+	hw_queue->length = cmd->length;
+	hw_queue->base_addr = cmd->nesting_parent_iova;

When the driver is running, which can be a source of bugs.

Hmm, I don't quite follow the "bugs" here. Any example?

Quoted 3 lines

This would be useful for the existing ops too.

May reduce the size of the linked in code.

Yes! Haven't tried that yet, but sounds like we could move quite a few things back to the private header.

Perhaps a smaller cleanup series first to the existing code.

Thanks!
Nicolin

maintainer
#46 lore raw

Re: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

Quoted 10 lines · Nicolin Chen
Quoted 7 lines · Jason Gunthorpe

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

Quoted 3 lines · Nicolin Chen
drivers/iommu/iommufd/viommu.c +3 -0
+	offset =
+		cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova);
+	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);

This should probably be capped to PAGE_SIZE/sizeof(void *), return EINVAL if not

Hmm, mind elaborating where this PAGE_SIZE/sizeof comes from?

We can usually allocate up to a PAGE_SIZE without too much trouble. Beyond that it gets more likely to fail.

Quoted 8 lines
Quoted 6 lines

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

Quoted 4 lines
drivers/iommu/iommufd/viommu.c +4 -0
+	hw_queue->viommu = viommu;
+	refcount_inc(&viommu->obj.users);
+	hw_queue->length = cmd->length;
+	hw_queue->base_addr = cmd->nesting_parent_iova;

When the driver is running, which can be a source of bugs.

Hmm, I don't quite follow the "bugs" here. Any example?

Like if the driver thinks that hw_queue->length should be valid during init, it turns out it isn't.

Jason

#47 lore raw

Re: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

Quoted 10 lines · Jason Gunthorpe
Quoted 9 lines · Nicolin Chen
Quoted 7 lines · Jason Gunthorpe

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

Quoted 3 lines · Nicolin Chen
drivers/iommu/iommufd/viommu.c +3 -0
+	offset =
+		cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova);
+	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);

This should probably be capped to PAGE_SIZE/sizeof(void *), return EINVAL if not

Quoted 4 lines
Quoted 1 line

Hmm, mind elaborating where this PAGE_SIZE/sizeof comes from?

We can usually allocate up to a PAGE_SIZE without too much trouble. Beyond that it gets more likely to fail.

If PAGE_SIZE=4096, the upper limit for max_npages is 512, i.e. the max size of a guest queue is 2MB? It seems to be too small, as the VMM can use a larger huge page size to back the guest queue?

Quoted 11 lines
Quoted 8 lines
Quoted 6 lines

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

Quoted 4 lines
drivers/iommu/iommufd/viommu.c +4 -0
+	hw_queue->viommu = viommu;
+	refcount_inc(&viommu->obj.users);
+	hw_queue->length = cmd->length;
+	hw_queue->base_addr = cmd->nesting_parent_iova;

When the driver is running, which can be a source of bugs.

Hmm, I don't quite follow the "bugs" here. Any example?

Like if the driver thinks that hw_queue->length should be valid during init, it turns out it isn't.

Ah, I see. Yes.

Nicolin

maintainer
#48 lore raw

Re: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

Quoted 11 lines · Nicolin Chen
Quoted 10 lines · Jason Gunthorpe
Quoted 9 lines · Nicolin Chen
Quoted 7 lines · Jason Gunthorpe

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

Quoted 3 lines · Nicolin Chen
drivers/iommu/iommufd/viommu.c +3 -0
+	offset =
+		cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova);
+	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);

This should probably be capped to PAGE_SIZE/sizeof(void *), return EINVAL if not

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

Hmm, mind elaborating where this PAGE_SIZE/sizeof comes from?

We can usually allocate up to a PAGE_SIZE without too much trouble. Beyond that it gets more likely to fail.

If PAGE_SIZE=4096, the upper limit for max_npages is 512, i.e. the max size of a guest queue is 2MB? It seems to be too small, as the VMM can use a larger huge page size to back the guest queue?

May need to make a new API that returns a bio_vec or something else more efficient then :\

Jason

#49 lore raw

Re: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

Quoted 17 lines · Jason Gunthorpe
Quoted 16 lines · Nicolin Chen
Quoted 14 lines · Jason Gunthorpe
Quoted 10 lines · Nicolin Chen
Quoted 7 lines · Jason Gunthorpe

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

Quoted 3 lines · Nicolin Chen
drivers/iommu/iommufd/viommu.c +3 -0
+	offset =
+		cmd->nesting_parent_iova - PAGE_ALIGN(cmd->nesting_parent_iova);
+	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);

This should probably be capped to PAGE_SIZE/sizeof(void *), return EINVAL if not

Hmm, mind elaborating where this PAGE_SIZE/sizeof comes from?

We can usually allocate up to a PAGE_SIZE without too much trouble. Beyond that it gets more likely to fail.

Quoted 6 lines
Quoted 3 lines

If PAGE_SIZE=4096, the upper limit for max_npages is 512, i.e. the max size of a guest queue is 2MB? It seems to be too small, as the VMM can use a larger huge page size to back the guest queue?

May need to make a new API that returns a bio_vec or something else more efficient then :\

Hmm, that sounds like a rabbit hole :-\

Let me leave a FIXME at this max_npages calculation instead..

Thanks
Nicolin

#50 lore raw

Re: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

In reply to Jason Gunthorpe
Quoted 15 lines · Jason Gunthorpe

Put a size_t in the driver ops:

size_t size_viommu;
size_t size_hw_queue;

Have the driver set it via a macro like INIT_RDMA_OBJ_SIZE

#define INIT_RDMA_OBJ_SIZE(ib_struct, drv_struct, member)                      \
	.size_##ib_struct =                                                    \
		(sizeof(struct drv_struct) +                                   \
		 BUILD_BUG_ON_ZERO(offsetof(struct drv_struct, member)) +      \
		 BUILD_BUG_ON_ZERO(                                            \
			 !__same_type(((struct drv_struct *)NULL)->member,     \
				      struct ib_struct)))
Quoted 5 lines

Which proves the core structure is at the front.

Then the core code can allocate the object along with enough space for the driver and call a driver function to init the driver portion of the already allocated object.

I found that the size_viommu or size_hw_queue might not work using a static macro as that RDMA one does:

- The size in vIOMMU case is type dependent. E.g. smmuv3 driver uses one iommu_ops to support two types: vSMMU and vCMDQ

- Changing to a type-indexed size array would eventually result some driver having a big size array, as the type number grows

I came up with two alternatives:

1) Define a get_viommu_size(unsigned int type) op: use a similar macro in the driver function to return with:

#define VIOMMU_STRUCT_SIZE(ib_struct, drv_struct, member)                      \
	    (sizeof(drv_struct) +                                              \

BUILD_BUG_ON_ZERO(offsetof(drv_struct, member)) + \
BUILD_BUG_ON_ZERO(!__same_type(((drv_struct *)NULL)->member, \
ib_struct)))

	if (type == SMMU)
		return VIOMMU_STRUCT_SIZE(
			struct arm_vsmmu, struct iommufd_viommu, core);
	return 0;

2) Let core allocate with sizeof(struct iommufd_viommu), then let driver krealloc during the viommu_init op call:

	viommu = kzalloc(sizeof(struct iommufd_viommu), ...);
	...
	viommu = ops->viommu_init(viommu, dev, parent_dom, type);

I am guessing that you may prefer 1 over 2? Or any better idea?

Thanks
Nicolin

maintainer
#51 lore raw

Re: [PATCH v5 14/29] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

Quoted 5 lines · Nicolin Chen

I found that the size_viommu or size_hw_queue might not work using a static macro as that RDMA one does:

- The size in vIOMMU case is type dependent. E.g. smmuv3 driver uses one iommu_ops to support two types: vSMMU and vCMDQ

Maybe they can just be max()'d?

Quoted 9 lines

1) Define a get_viommu_size(unsigned int type) op: use a similar macro in the driver function to return with:

#define VIOMMU_STRUCT_SIZE(ib_struct, drv_struct, member)                      \
	    (sizeof(drv_struct) +                                              \
(quoted diff) +0 -0
 		 BUILD_BUG_ON_ZERO(offsetof(drv_struct, member)) +             \
 		 BUILD_BUG_ON_ZERO(!__same_type(((drv_struct *)NULL)->member,  \
 						ib_struct)))
Quoted 4 lines
	if (type == SMMU)
		return VIOMMU_STRUCT_SIZE(
			struct arm_vsmmu, struct iommufd_viommu, core);
	return 0;

I guess this is best?

Quoted 2 lines

2) Let core allocate with sizeof(struct iommufd_viommu), then let driver krealloc during the viommu_init op call:

No.. memmoving things like locks doesn't work.

Jason

#52 lore raw

PATCH v5 15/29 iommufd/driver: Add iommufd_hw_queue_depend/undepend() helpers

In reply to Nicolin Chen

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

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

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

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

---
 include/linux/iommufd.h        | 44 ++++++++++++++++++++++++++++++++++
 drivers/iommu/iommufd/driver.c | 28 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)
include/linux/iommufd.h +44 -0
index 923c66ccc15a..a89797307379 100644
@@ -272,6 +272,10 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
 struct iommufd_object *
 _iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size,
 			   enum iommufd_object_type type);
+int _iommufd_object_depend(struct iommufd_object *obj_dependent,
+			   struct iommufd_object *obj_depended);
+void _iommufd_object_undepend(struct iommufd_object *obj_dependent,
+			      struct iommufd_object *obj_depended);
 struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 				       unsigned long vdev_id);
 int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
@@ -294,6 +298,18 @@ _iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size,
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+static inline int _iommufd_object_depend(struct iommufd_object *obj_dependent,
+					 struct iommufd_object *obj_depended)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void
+_iommufd_object_undepend(struct iommufd_object *obj_dependent,
+			 struct iommufd_object *obj_depended)
+{
+}
+
 static inline struct device *
 iommufd_viommu_find_dev(struct iommufd_viommu *viommu, unsigned long vdev_id)
 {
@@ -359,4 +375,32 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 		}                                                              \
 		ret;                                                           \
 	})
+
+/*
+ * 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)                   \
+	({                                                                     \
+		int ret = -EINVAL;                                             \
+									       \
+		static_assert(__same_type(struct iommufd_hw_queue,             \
+					  dependent->member));                 \
+		static_assert(__same_type(typeof(*dependent), *depended));     \
+		if (!WARN_ON_ONCE(dependent->member.ictx !=                    \
+				  depended->member.ictx))                      \
+			ret = _iommufd_object_depend(&dependent->member.obj,   \
+						     &depended->member.obj);   \
+		ret;                                                           \
+	})
+
+#define iommufd_hw_queue_undepend(dependent, depended, member)                 \
+	({                                                                     \
+		static_assert(__same_type(struct iommufd_hw_queue,             \
+					  dependent->member));                 \
+		static_assert(__same_type(typeof(*dependent), *depended));     \
+		WARN_ON_ONCE(dependent->member.ictx != depended->member.ictx); \
+		_iommufd_object_undepend(&dependent->member.obj,               \
+					 &depended->member.obj);               \
+	})
 #endif
drivers/iommu/iommufd/driver.c +28 -0
index 28dec9e09720..529968126f85 100644
@@ -54,6 +54,34 @@ struct iommufd_object *_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd,
 }
 EXPORT_SYMBOL_NS_GPL(_iommufd_object_alloc_ucmd, "IOMMUFD");
 
+/* Driver should use a per-structure helper in include/linux/iommufd.h */
+int _iommufd_object_depend(struct iommufd_object *obj_dependent,
+			   struct iommufd_object *obj_depended)
+{
+	/* Reject self dependency that dead locks */
+	if (obj_dependent == obj_depended)
+		return -EINVAL;
+	/* Only support dependency between two objects of the same type */
+	if (obj_dependent->type != obj_depended->type)
+		return -EINVAL;
+
+	refcount_inc(&obj_depended->users);
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(_iommufd_object_depend, "IOMMUFD");
+
+/* Driver should use a per-structure helper in include/linux/iommufd.h */
+void _iommufd_object_undepend(struct iommufd_object *obj_dependent,
+			      struct iommufd_object *obj_depended)
+{
+	if (WARN_ON_ONCE(obj_dependent == obj_depended ||
+			 obj_dependent->type != obj_depended->type))
+		return;
+
+	refcount_dec(&obj_depended->users);
+}
+EXPORT_SYMBOL_NS_GPL(_iommufd_object_undepend, "IOMMUFD");
+
 /* Caller should xa_lock(&viommu->vdevs) to protect the return value */
 struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 				       unsigned long vdev_id)
Signature · 1 line
2.43.0
#53 lore raw

PATCH v5 16/29 iommufd/selftest: Add coverage for IOMMUFD_CMD_HW_QUEUE_ALLOC

In reply to Nicolin Chen

Some simple tests for IOMMUFD_CMD_HW_QUEUE_ALLOC infrastructure covering the new iommufd_hw_queue_depend/undepend() helpers.

Since the iommufd_hw_queue_alloc_ioctl() will call iommu_iova_to_phys(), for input address validation. It might intentionally pass in a bad input for a negative test. So drop the WARN_ON() in mock_domain_iova_to_phys().

---
 drivers/iommu/iommufd/iommufd_test.h          |  3 +
 tools/testing/selftests/iommu/iommufd_utils.h | 31 ++++++++
 drivers/iommu/iommufd/selftest.c              | 71 ++++++++++++++++++-
 tools/testing/selftests/iommu/iommufd.c       | 62 ++++++++++++++++
 .../selftests/iommu/iommufd_fail_nth.c        |  6 ++
 5 files changed, 172 insertions(+), 1 deletion(-)
drivers/iommu/iommufd/iommufd_test.h +3 -0
index fbf9ecb35a13..51cd744a354f 100644
@@ -265,4 +265,7 @@ struct iommu_viommu_event_selftest {
 	__u32 virt_id;
 };
 
+#define IOMMU_HW_QUEUE_TYPE_SELFTEST 0xdeadbeef
+#define IOMMU_TEST_HW_QUEUE_MAX 2
+
 #endif
tools/testing/selftests/iommu/iommufd_utils.h +31 -0
index a5d4cbd089ba..9a556f99d992 100644
@@ -956,6 +956,37 @@ static int _test_cmd_vdevice_alloc(int fd, __u32 viommu_id, __u32 idev_id,
 		     _test_cmd_vdevice_alloc(self->fd, viommu_id, idev_id,   \
 					     virt_id, vdev_id))
 
+static int _test_cmd_hw_queue_alloc(int fd, __u32 viommu_id, __u32 type,
+				    __u32 idx, __u64 base_addr, __u64 length,
+				    __u32 *hw_queue_id)
+{
+	struct iommu_hw_queue_alloc cmd = {
+		.size = sizeof(cmd),
+		.viommu_id = viommu_id,
+		.type = type,
+		.index = idx,
+		.nesting_parent_iova = base_addr,
+		.length = length,
+	};
+	int ret;
+
+	ret = ioctl(fd, IOMMU_HW_QUEUE_ALLOC, &cmd);
+	if (ret)
+		return ret;
+	if (hw_queue_id)
+		*hw_queue_id = cmd.out_hw_queue_id;
+	return 0;
+}
+
+#define test_cmd_hw_queue_alloc(viommu_id, type, idx, base_addr, len, out_qid) \
+	ASSERT_EQ(0, _test_cmd_hw_queue_alloc(self->fd, viommu_id, type, idx,  \
+					      base_addr, len, out_qid))
+#define test_err_hw_queue_alloc(_errno, viommu_id, type, idx, base_addr, len, \
+				out_qid)                                      \
+	EXPECT_ERRNO(_errno,                                                  \
+		     _test_cmd_hw_queue_alloc(self->fd, viommu_id, type, idx, \
+					      base_addr, len, out_qid))
+
 static int _test_cmd_veventq_alloc(int fd, __u32 viommu_id, __u32 type,
 				   __u32 *veventq_id, __u32 *veventq_fd)
 {
drivers/iommu/iommufd/selftest.c +70 -1

99 lines not on this page · raw message

tools/testing/selftests/iommu/iommufd.c +62 -0

68 lines not on this page · raw message

tools/testing/selftests/iommu/iommufd_fail_nth.c +6 -0
index f7ccf1822108..41c685bbd252 100644
@@ -634,6 +634,7 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 	uint32_t idev_id;
 	uint32_t hwpt_id;
 	uint32_t viommu_id;
+	uint32_t hw_queue_id;
 	uint32_t vdev_id;
 	__u64 iova;
 
@@ -696,6 +697,11 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 	if (_test_cmd_vdevice_alloc(self->fd, viommu_id, idev_id, 0, &vdev_id))
 		return -1;
 
+	if (_test_cmd_hw_queue_alloc(self->fd, viommu_id,
+				     IOMMU_HW_QUEUE_TYPE_SELFTEST, 0, iova,
+				     PAGE_SIZE, &hw_queue_id))
+		return -1;
+
 	if (_test_ioctl_fault_alloc(self->fd, &fault_id, &fault_fd))
 		return -1;
 	close(fault_fd);
Signature · 1 line
2.43.0
#54 lore raw

PATCH v5 17/29 iommufd: Add mmap interface

In reply to Nicolin Chen

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

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

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

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

---
 drivers/iommu/iommufd/iommufd_private.h | 14 ++++++
 include/linux/iommufd.h                 | 45 ++++++++++++++++++
 drivers/iommu/iommufd/driver.c          | 51 +++++++++++++++++++++
 drivers/iommu/iommufd/main.c            | 61 +++++++++++++++++++++++++
 4 files changed, 171 insertions(+)
drivers/iommu/iommufd/iommufd_private.h +14 -0
index a94d04ab0d2c..718a3161d3fa 100644
@@ -7,6 +7,7 @@
 #include <linux/iommu.h>
 #include <linux/iommufd.h>
 #include <linux/iova_bitmap.h>
+#include <linux/maple_tree.h>
 #include <linux/rwsem.h>
 #include <linux/uaccess.h>
 #include <linux/xarray.h>
@@ -44,6 +45,7 @@ struct iommufd_ctx {
 	struct xarray groups;
 	wait_queue_head_t destroy_wait;
 	struct rw_semaphore ioas_creation_lock;
+	struct maple_tree mt_mmap;
 
 	struct mutex sw_msi_lock;
 	struct list_head sw_msi_list;
@@ -55,6 +57,18 @@ struct iommufd_ctx {
 	struct iommufd_ioas *vfio_ioas;
 };
 
+/* Entry for iommufd_ctx::mt_mmap */
+struct iommufd_mmap {
+	struct iommufd_object *owner;
+
+	/* Allocated start position in mt_mmap tree */
+	unsigned long startp;
+
+	/* Physical range for io_remap_pfn_range() */
+	unsigned long mmio_pfn;
+	unsigned long num_pfns;
+};
+
 /*
  * The IOVA to PFN map. The map automatically copies the PFNs into multiple
  * domains and permits sharing of PFNs between io_pagetable instances. This
include/linux/iommufd.h +45 -0

61 lines not on this page · raw message

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

57 lines not on this page · raw message

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

78 lines not on this page · raw message

Signature · 1 line
2.43.0
maintainer
#55 lore raw

Re: [PATCH v5 17/29] iommufd: Add mmap interface

Quoted 6 lines · Nicolin Chen
include/linux/iommufd.h +6 -0
+/*
+ * Helpers for IOMMU driver to alloc/destroy an mmapable area for a structure.
+ *
+ * To support an mmappable MMIO region, kernel driver must first register it to
+ * iommufd core to allocate an @out_offset, in the context of an driver-struct
+ * allocation (e.g. viommu_alloc op). Then, it should report to user space this

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

Quoted 10 lines
include/linux/iommufd.h +10 -0
+ * @out_offset and the @length of the MMIO region for mmap syscall.
+ */
+#define iommufd_viommu_alloc_mmap(viommu, member, mmio, length, out_offset)    \
+	({                                                                     \
+		static_assert(__same_type(struct iommufd_viommu,               \
+					  viommu->member));                    \
+		static_assert(offsetof(typeof(*viommu), member.obj) == 0);     \
+		_iommufd_alloc_mmap(viommu->member.ictx, &viommu->member.obj,  \
+				    mmio, length, out_offset);                 \
+	})

Why is this like this? It's weird, just

static inline int iommufd_alloc_viommu_mmap(struct iommufd_viommu *viommu,
phys_addr_t mmio_addr, size_t length, unsigned long *offset);

Quoted 12 lines
drivers/iommu/iommufd/main.c +12 -0
+/* The vm_pgoff must be pre-allocated from mt_mmap, and given to user space */
+static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct iommufd_ctx *ictx = filp->private_data;
+	size_t length = vma->vm_end - vma->vm_start;
+	struct iommufd_mmap *immap;
+	int rc;
+
+	if (!PAGE_ALIGNED(length))
+		return -EINVAL;
+	if (!(vma->vm_flags & VM_SHARED))
+		return -EINVAL;

drivers/iommu/iommufd/main.c static long iommufd_fops_ioctl(struct file *filp, unsigned int cmd, · line 473

Quoted 10 lines
drivers/iommu/iommufd/main.c +10 -0
+	if (vma->vm_flags & VM_EXEC)
+		return -EPERM;
+
+	/* vma->vm_pgoff carries an index to an mtree entry (immap) */
+	immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff);
+	if (!immap)
+		return -ENXIO;
+	/* Validate the vm_pgoff and length against the registered region */
+	if (vma->vm_pgoff != immap->startp)
+		return -ENXIO;

This check seems redundant

Jason

#56 lore raw

Re: [PATCH v5 17/29] iommufd: Add mmap interface

Quoted 11 lines · Jason Gunthorpe
Quoted 10 lines · Nicolin Chen
include/linux/iommufd.h +10 -0
+/*
+ * Helpers for IOMMU driver to alloc/destroy an mmapable area for a structure.
+ *
+ * To support an mmappable MMIO region, kernel driver must first register it to
+ * iommufd core to allocate an @out_offset, in the context of an driver-struct
+ * allocation (e.g. viommu_alloc op). Then, it should report to user space this
+ * @out_offset and the @length of the MMIO region for mmap syscall.
+ */
+#define iommufd_viommu_alloc_mmap(viommu, member, mmio, length, out_offset)    \
+	({                                                                     \
Quoted 8 lines

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

Quoted 6 lines
include/linux/iommufd.h +6 -0
+		static_assert(__same_type(struct iommufd_viommu,               \
+					  viommu->member));                    \
+		static_assert(offsetof(typeof(*viommu), member.obj) == 0);     \
+		_iommufd_alloc_mmap(viommu->member.ictx, &viommu->member.obj,  \
+				    mmio, length, out_offset);                 \
+	})

Why is this like this? It's weird, just

I was trying to follow the pattern of the other two macros that asks for driver-level structure. But it seems not necessary, as we have iommufd_hw_queue_alloc taking the core-level viommu..

Quoted 3 lines

static inline int iommufd_alloc_viommu_mmap(struct iommufd_viommu *viommu,
phys_addr_t mmio_addr, size_t length, unsigned long *offset);

OK.

Quoted 16 lines
Quoted 16 lines
drivers/iommu/iommufd/main.c +16 -0
+/* The vm_pgoff must be pre-allocated from mt_mmap, and given to user space */
+static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct iommufd_ctx *ictx = filp->private_data;
+	size_t length = vma->vm_end - vma->vm_start;
+	struct iommufd_mmap *immap;
+	int rc;
+
+	if (!PAGE_ALIGNED(length))
+		return -EINVAL;
+	if (!(vma->vm_flags & VM_SHARED))
+		return -EINVAL;
+	if (vma->vm_flags & VM_EXEC)
+		return -EPERM;
+
+	/* vma->vm_pgoff carries an index to an mtree entry (immap) */
Quoted 8 lines

drivers/iommu/iommufd/main.c static long iommufd_fops_ioctl(struct file *filp, unsigned int cmd, · line 473

Quoted 6 lines
drivers/iommu/iommufd/main.c +6 -0
+	immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff);
+	if (!immap)
+		return -ENXIO;
+	/* Validate the vm_pgoff and length against the registered region */
+	if (vma->vm_pgoff != immap->startp)
+		return -ENXIO;

This check seems redundant

Hmm, I was trying to follow your remarks:
"This needs to validate that vm_pgoff is at the start of the immap"
https://lore.kernel.org/all/[email protected]/

I also added a selftest coverage for this check.

Thanks
Nicolin

maintainer
#57 lore raw

Re: [PATCH v5 17/29] iommufd: Add mmap interface

Quoted 25 lines · Nicolin Chen
Quoted 16 lines
Quoted 16 lines
drivers/iommu/iommufd/main.c +16 -0
+/* The vm_pgoff must be pre-allocated from mt_mmap, and given to user space */
+static int iommufd_fops_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	struct iommufd_ctx *ictx = filp->private_data;
+	size_t length = vma->vm_end - vma->vm_start;
+	struct iommufd_mmap *immap;
+	int rc;
+
+	if (!PAGE_ALIGNED(length))
+		return -EINVAL;
+	if (!(vma->vm_flags & VM_SHARED))
+		return -EINVAL;
+	if (vma->vm_flags & VM_EXEC)
+		return -EPERM;
+
+	/* vma->vm_pgoff carries an index to an mtree entry (immap) */
Quoted 8 lines

drivers/iommu/iommufd/main.c static long iommufd_fops_ioctl(struct file *filp, unsigned int cmd, · line 473

Quoted 6 lines
drivers/iommu/iommufd/main.c +6 -0
+	immap = mtree_load(&ictx->mt_mmap, vma->vm_pgoff);
+	if (!immap)
+		return -ENXIO;
+	/* Validate the vm_pgoff and length against the registered region */
+	if (vma->vm_pgoff != immap->startp)
+		return -ENXIO;

This check seems redundant

Quoted 3 lines

Hmm, I was trying to follow your remarks:
"This needs to validate that vm_pgoff is at the start of the immap"
https://lore.kernel.org/all/[email protected]/

Oh, right I forgot how mtree_load works again. :\ Maybe add a little note

/* mtree_load returns the immap for any contained pgoff, only allow
the immap thing to be mapped. */

Jason

#58 lore raw

PATCH v5 18/29 iommufd/selftest: Add coverage for the new mmap interface

In reply to Nicolin Chen

Extend the loopback test to a new mmap page.

---
 drivers/iommu/iommufd/iommufd_test.h          |  4 +++
 tools/testing/selftests/iommu/iommufd_utils.h |  4 +++
 drivers/iommu/iommufd/selftest.c              | 34 ++++++++++++++++++-
 tools/testing/selftests/iommu/iommufd.c       | 22 ++++++++++++
 4 files changed, 63 insertions(+), 1 deletion(-)
drivers/iommu/iommufd/iommufd_test.h +4 -0
index 51cd744a354f..8fc618b2bcf9 100644
@@ -232,12 +232,16 @@ struct iommu_hwpt_invalidate_selftest {
  *                                (IOMMU_VIOMMU_TYPE_SELFTEST)
  * @in_data: Input random data from user space
  * @out_data: Output data (matching @in_data) to user space
+ * @out_mmap_offset: The offset argument for mmap syscall
+ * @out_mmap_length: The length argument for mmap syscall
  *
  * Simply set @out_data=@in_data for a loopback test
  */
 struct iommu_viommu_selftest {
 	__u32 in_data;
 	__u32 out_data;
+	__aligned_u64 out_mmap_offset;
+	__aligned_u64 out_mmap_length;
 };
 
 /* Should not be equal to any defined value in enum iommu_viommu_invalidate_data_type */
tools/testing/selftests/iommu/iommufd_utils.h +4 -0
index 9a556f99d992..4a1b2bade018 100644
@@ -56,6 +56,10 @@ static unsigned long PAGE_SIZE;
 #define offsetofend(TYPE, MEMBER) \
 	(offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
 
+#define test_err_mmap(_errno, length, offset)                                 \
+	EXPECT_ERRNO(_errno, (long)mmap(NULL, length, PROT_READ | PROT_WRITE, \
+					MAP_SHARED, self->fd, offset))
+
 static inline void *memfd_mmap(size_t length, int prot, int flags, int *mfd_p)
 {
 	int mfd_flags = (flags & MAP_HUGETLB) ? MFD_HUGETLB : 0;
drivers/iommu/iommufd/selftest.c +33 -1
index eca1a66fefed..45276c137cee 100644
@@ -149,6 +149,9 @@ struct mock_viommu {
 	struct iommufd_viommu core;
 	struct mock_iommu_domain *s2_parent;
 	struct mock_hw_queue *hw_queue[IOMMU_TEST_HW_QUEUE_MAX];
+
+	unsigned long mmap_offset;
+	u32 *page; /* Mmap page to test u32 type of in_data */
 };
 
 static inline struct mock_viommu *to_mock_viommu(struct iommufd_viommu *viommu)
@@ -647,9 +650,14 @@ static void mock_viommu_destroy(struct iommufd_viommu *viommu)
 {
 	struct mock_iommu_device *mock_iommu = container_of(
 		viommu->iommu_dev, struct mock_iommu_device, iommu_dev);
+	struct mock_viommu *mock_viommu = to_mock_viommu(viommu);
 
 	if (refcount_dec_and_test(&mock_iommu->users))
 		complete(&mock_iommu->complete);
+	if (mock_viommu->mmap_offset)
+		iommufd_viommu_destroy_mmap(mock_viommu, core,
+					    mock_viommu->mmap_offset);
+	free_page((unsigned long)mock_viommu->page);
 
 	/* iommufd core frees mock_viommu and viommu */
 }
@@ -829,15 +837,39 @@ mock_viommu_alloc(struct device *dev, struct iommu_domain *domain,
 		return ERR_CAST(mock_viommu);
 
 	if (user_data) {
+		/* Allocate two pages */
+		mock_viommu->page =
+			(u32 *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
+		if (!mock_viommu->page)
+			return ERR_PTR(-ENOMEM);
+
+		rc = iommufd_viommu_alloc_mmap(mock_viommu, core,
+					       __pa(mock_viommu->page),
+					       PAGE_SIZE * 2,
+					       &mock_viommu->mmap_offset);
+		if (rc)
+			goto err_free_page;
+
+		/* For loopback tests on both the page and out_data */
+		*mock_viommu->page = data.in_data;
 		data.out_data = data.in_data;
+		data.out_mmap_length = PAGE_SIZE * 2;
+		data.out_mmap_offset = mock_viommu->mmap_offset;
 		rc = iommu_copy_struct_to_user(
 			user_data, &data, IOMMU_VIOMMU_TYPE_SELFTEST, out_data);
 		if (rc)
-			return ERR_PTR(rc);
+			goto err_destroy_mmap;
 	}
 
 	refcount_inc(&mock_iommu->users);
 	return &mock_viommu->core;
+
+err_destroy_mmap:
+	iommufd_viommu_destroy_mmap(mock_viommu, core,
+				    mock_viommu->mmap_offset);
+err_free_page:
+	free_page((unsigned long)mock_viommu->page);
+	return ERR_PTR(rc);
 }
 
 static const struct iommu_ops mock_ops = {
tools/testing/selftests/iommu/iommufd.c +22 -0
index fabec1b551c6..2b3274015832 100644
@@ -2799,12 +2799,34 @@ TEST_F(iommufd_viommu, viommu_alloc_with_data)
 	struct iommu_viommu_selftest data = {
 		.in_data = 0xbeef,
 	};
+	uint32_t *test;
 
 	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_EQ(data.out_data, data.in_data);
+
+		/* Negative mmap tests -- offset and length cannot be changed */
+		test_err_mmap(ENXIO, data.out_mmap_length,
+			      data.out_mmap_offset + PAGE_SIZE);
+		test_err_mmap(ENXIO, data.out_mmap_length,
+			      data.out_mmap_offset + PAGE_SIZE * 2);
+		test_err_mmap(ENXIO, data.out_mmap_length / 2,
+			      data.out_mmap_offset);
+		test_err_mmap(ENXIO, data.out_mmap_length * 2,
+			      data.out_mmap_offset);
+
+		/* Now do a correct mmap for a loopback test */
+		test = mmap(NULL, data.out_mmap_length, PROT_READ | PROT_WRITE,
+			    MAP_SHARED, self->fd, data.out_mmap_offset);
+		ASSERT_NE(MAP_FAILED, test);
+		ASSERT_EQ(data.in_data, *test);
+
+		/* The owner of the mmap region should be blocked */
+		EXPECT_ERRNO(EBUSY,
+			     _test_ioctl_destroy(self->fd, self->viommu_id));
+		munmap(test, data.out_mmap_length);
 	}
 }
 
Signature · 1 line
2.43.0
#59 lore raw

PATCH v5 19/29 Documentation: userspace-api: iommufd: Update HW QUEUE

In reply to Nicolin Chen

With the introduction of the new object and its infrastructure, update the doc to reflect that.

---
 Documentation/userspace-api/iommufd.rst | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Documentation/userspace-api/iommufd.rst +12 -0
index b0df15865dec..03f7510384d2 100644
@@ -124,6 +124,17 @@ Following IOMMUFD objects are exposed to userspace:
   used to allocate a vEVENTQ. Each vIOMMU can support multiple types of vEVENTS,
   but is confined to one vEVENTQ per vEVENTQ type.
 
+- IOMMUFD_OBJ_HW_QUEUE, representing a hardware accelerated queue, as a subset
+  of IOMMU's virtualization features, for the IOMMU HW to directly read or write
+  the virtual queue memory owned by a guest OS. This HW-acceleration feature can
+  allow VM to work with the IOMMU HW directly without a VM Exit, so as to reduce
+  overhead from the hypercalls. Along with the HW QUEUE object, iommufd provides
+  user space an mmap interface for VMM to mmap a physical MMIO region from the
+  host physical address space to the guest physical address space, allowing the
+  guest OS to directly control the allocated HW QUEUE. Thus, when allocating a
+  HW QUEUE, the VMM must request a pair of mmap info (offset/length) and pass in
+  exactly to an mmap syscall via its offset and length arguments.
+
 All user-visible objects are destroyed via the IOMMU_DESTROY uAPI.
 
 The diagrams below show relationships between user-visible objects and kernel
@@ -270,6 +281,7 @@ User visible objects are backed by following datastructures:
 - iommufd_viommu for IOMMUFD_OBJ_VIOMMU.
 - iommufd_vdevice for IOMMUFD_OBJ_VDEVICE.
 - iommufd_veventq for IOMMUFD_OBJ_VEVENTQ.
+- iommufd_hw_queue for IOMMUFD_OBJ_HW_QUEUE.
 
 Several terminologies when looking at these datastructures:
 
Signature · 1 line
2.43.0
#60 lore raw

PATCH v5 20/29 iommu: Allow an input type in hw_info op

In reply to Nicolin Chen

The hw_info uAPI will support a bidirectional data_type field that can be used as an input field for user space to request for a specific info data.

To prepare for the uAPI update, change the iommu layer first:
- Add a new IOMMU_HW_INFO_TYPE_DEFAULT as an input, for which driver can output its only (or firstly) supported type
- Update the kdoc accordingly
- Roll out the type validation in the existing drivers

---
 include/linux/iommu.h                               | 6 ++++--
 include/uapi/linux/iommufd.h                        | 4 +++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 4 ++++
 drivers/iommu/intel/iommu.c                         | 4 ++++
 drivers/iommu/iommufd/device.c                      | 3 +++
 drivers/iommu/iommufd/selftest.c                    | 4 ++++
 6 files changed, 22 insertions(+), 3 deletions(-)
include/linux/iommu.h +4 -2
index 4c54dd8f94e4..01a19b68a0ea 100644
@@ -607,8 +607,10 @@ __iommu_copy_struct_to_user(const struct iommu_user_data *dst_data,
  * @capable: check capability
  * @hw_info: report iommu hardware information. The data buffer returned by this
  *           op is allocated in the iommu driver and freed by the caller after
- *           use. The information type is one of enum iommu_hw_info_type defined
- *           in include/uapi/linux/iommufd.h.
+ *           use. @type can input a requested type and output a supported type,
+ *           either of which must be defined in enum iommu_hw_info_type in
+ *           include/uapi/linux/iommufd.h. Driver should reject an unsupported
+ *           data @type input
  * @domain_alloc: allocate and return an iommu domain if success. Otherwise
  *                NULL is returned. The domain is not fully initialized until
  *                the caller iommu_domain_alloc() returns.
include/uapi/linux/iommufd.h +3 -1
index aeed0127553e..1fc546acb231 100644
@@ -593,13 +593,15 @@ struct iommu_hw_info_arm_smmuv3 {
 
 /**
  * enum iommu_hw_info_type - IOMMU Hardware Info Types
- * @IOMMU_HW_INFO_TYPE_NONE: Used by the drivers that do not report hardware
+ * @IOMMU_HW_INFO_TYPE_NONE: Output by the drivers that do not report hardware
  *                           info
+ * @IOMMU_HW_INFO_TYPE_DEFAULT: Input to request for a default type
  * @IOMMU_HW_INFO_TYPE_INTEL_VTD: Intel VT-d iommu info type
  * @IOMMU_HW_INFO_TYPE_ARM_SMMUV3: ARM SMMUv3 iommu info type
  */
 enum iommu_hw_info_type {
 	IOMMU_HW_INFO_TYPE_NONE = 0,
+	IOMMU_HW_INFO_TYPE_DEFAULT = 0,
 	IOMMU_HW_INFO_TYPE_INTEL_VTD = 1,
 	IOMMU_HW_INFO_TYPE_ARM_SMMUV3 = 2,
 };
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +4 -0
index adef56db4028..61a3f9134a9b 100644
@@ -14,6 +14,10 @@ void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type)
 	u32 __iomem *base_idr;
 	unsigned int i;
 
+	if (*type != IOMMU_HW_INFO_TYPE_DEFAULT &&
+	    *type != IOMMU_HW_INFO_TYPE_ARM_SMMUV3)
+		return ERR_PTR(-EOPNOTSUPP);
+
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return ERR_PTR(-ENOMEM);
drivers/iommu/intel/iommu.c +4 -0
index cb0b993bebb4..8ad8714ece5e 100644
@@ -4149,6 +4149,10 @@ static void *intel_iommu_hw_info(struct device *dev, u32 *length, u32 *type)
 	struct intel_iommu *iommu = info->iommu;
 	struct iommu_hw_info_vtd *vtd;
 
+	if (*type != IOMMU_HW_INFO_TYPE_DEFAULT &&
+	    *type != IOMMU_HW_INFO_TYPE_INTEL_VTD)
+		return ERR_PTR(-EOPNOTSUPP);
+
 	vtd = kzalloc(sizeof(*vtd), GFP_KERNEL);
 	if (!vtd)
 		return ERR_PTR(-ENOMEM);
drivers/iommu/iommufd/device.c +3 -0
index 0f2bda9e9d84..50337183eb1c 100644
@@ -1365,6 +1365,9 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
 	    cmd->__reserved[2])
 		return -EOPNOTSUPP;
 
+	/* Clear the type field since drivers don't support a random input */
+	cmd->out_data_type = IOMMU_HW_INFO_TYPE_DEFAULT;
+
 	idev = iommufd_get_device(ucmd, cmd->dev_id);
 	if (IS_ERR(idev))
 		return PTR_ERR(idev);
drivers/iommu/iommufd/selftest.c +4 -0
index 45276c137cee..299a8e1e7f6c 100644
@@ -290,6 +290,10 @@ static void *mock_domain_hw_info(struct device *dev, u32 *length, u32 *type)
 {
 	struct iommu_test_hw_info *info;
 
+	if (*type != IOMMU_HW_INFO_TYPE_DEFAULT &&
+	    *type != IOMMU_HW_INFO_TYPE_SELFTEST)
+		return ERR_PTR(-EOPNOTSUPP);
+
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return ERR_PTR(-ENOMEM);
Signature · 1 line
2.43.0
maintainer Reviewed-by on v5 20/29
#61 lore raw

RE: [PATCH v5 20/29] iommu: Allow an input type in hw_info op

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:22 AM

The hw_info uAPI will support a bidirectional data_type field that can be used as an input field for user space to request for a specific info data.

Quoted 8 lines

To prepare for the uAPI update, change the iommu layer first:
- Add a new IOMMU_HW_INFO_TYPE_DEFAULT as an input, for which driver
can
output its only (or firstly) supported type
- Update the kdoc accordingly
- Roll out the type validation in the existing drivers

maintainer Reviewed-by on v5 20/29
#62 lore raw

Re: [PATCH v5 20/29] iommu: Allow an input type in hw_info op

In reply to Nicolin Chen
Quoted 10 lines · Nicolin Chen

The hw_info uAPI will support a bidirectional data_type field that can be used as an input field for user space to request for a specific info data.

To prepare for the uAPI update, change the iommu layer first:
- Add a new IOMMU_HW_INFO_TYPE_DEFAULT as an input, for which driver can output its only (or firstly) supported type
- Update the kdoc accordingly
- Roll out the type validation in the existing drivers

Quoted 8 lines
---
 include/linux/iommu.h                               | 6 ++++--
 include/uapi/linux/iommufd.h                        | 4 +++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 4 ++++
 drivers/iommu/intel/iommu.c                         | 4 ++++
 drivers/iommu/iommufd/device.c                      | 3 +++
 drivers/iommu/iommufd/selftest.c                    | 4 ++++
 6 files changed, 22 insertions(+), 3 deletions(-)

Jason

#63 lore raw

PATCH v5 21/29 iommufd: Allow an input data_type via iommu_hw_info

In reply to Nicolin Chen

The iommu_hw_info can output via the out_data_type field the vendor data type from a driver, but this only allows driver to report one data type.

Now, with SMMUv3 having a Tegra241 CMDQV implementation, it has two sets of types and data structs to report.

One way to support that is to use the same type field bidirectionally.

Rename "out_data_type" to simply "data_type", to allow an input for user space to request for a specific type and to get the corresponding data.

For backward compatibility, since the ioctl handler has never checked an input value, add a new IOMMU_HW_INFO_FLAG_INPUT_TYPE to switch between the old output-only field and the new bidirectional field.

---
 include/uapi/linux/iommufd.h   | 22 +++++++++++++++++-----
 drivers/iommu/iommufd/device.c | 16 +++++++++-------
 2 files changed, 26 insertions(+), 12 deletions(-)
include/uapi/linux/iommufd.h +17 -5
index 1fc546acb231..7bcd3912180a 100644
@@ -628,6 +628,15 @@ enum iommufd_hw_capabilities {
 	IOMMU_HW_CAP_PCI_PASID_PRIV = 1 << 2,
 };
 
+/**
+ * enum iommufd_hw_info_flags - Flags for iommu_hw_info
+ * @IOMMU_HW_INFO_FLAG_INPUT_TYPE: If set, @data_type carries an input type for
+ *                                 user space to request for a specific info
+ */
+enum iommufd_hw_info_flags {
+	IOMMU_HW_INFO_FLAG_INPUT_TYPE = 1 << 0,
+};
+
 /**
  * struct iommu_hw_info - ioctl(IOMMU_GET_HW_INFO)
  * @size: sizeof(struct iommu_hw_info)
@@ -637,8 +646,11 @@ enum iommufd_hw_capabilities {
  *            data that kernel supports
  * @data_uptr: User pointer to a user-space buffer used by the kernel to fill
  *             the iommu type specific hardware information data
- * @out_data_type: Output the iommu hardware info type as defined in the enum
- *                 iommu_hw_info_type.
+ * @data_type: Output the iommu hardware info type as defined in the enum
+ *             iommu_hw_info_type. If IOMMU_HW_INFO_FLAG_INPUT_TYPE is set, an
+ *             input type via @data_type will be valid, requesting for the info
+ *             data to the given type. Otherwise, any input value carried via
+ *             @data_type will be seen as IOMMU_HW_INFO_TYPE_DEFAULT
  * @out_capabilities: Output the generic iommu capability info type as defined
  *                    in the enum iommu_hw_capabilities.
  * @out_max_pasid_log2: Output the width of PASIDs. 0 means no PASID support.
@@ -657,8 +669,8 @@ enum iommufd_hw_capabilities {
  * user buffer is larger than the data that kernel has. Otherwise, kernel only
  * fills the buffer using the given length in @data_len. If the ioctl succeeds,
  * @data_len will be updated to the length that kernel actually supports,
- * @out_data_type will be filled to decode the data filled in the buffer
- * pointed by @data_uptr. Input @data_len == zero is allowed.
+ * @data_type will be filled to decode the data filled in the buffer pointed by
+ * @data_uptr. Input @data_len == zero is allowed.
  */
 struct iommu_hw_info {
 	__u32 size;
@@ -666,7 +678,7 @@ struct iommu_hw_info {
 	__u32 dev_id;
 	__u32 data_len;
 	__aligned_u64 data_uptr;
-	__u32 out_data_type;
+	__u32 data_type;
 	__u8 out_max_pasid_log2;
 	__u8 __reserved[3];
 	__aligned_u64 out_capabilities;
drivers/iommu/iommufd/device.c +9 -7
index 50337183eb1c..68e8b8e36907 100644
@@ -1352,6 +1352,7 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_rw, "IOMMUFD");
 
 int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
 {
+	const u32 SUPPORTED_FLAGS = IOMMU_HW_INFO_FLAG_INPUT_TYPE;
 	struct iommu_hw_info *cmd = ucmd->cmd;
 	void __user *user_ptr = u64_to_user_ptr(cmd->data_uptr);
 	const struct iommu_ops *ops;
@@ -1361,12 +1362,14 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
 	void *data;
 	int rc;
 
-	if (cmd->flags || cmd->__reserved[0] || cmd->__reserved[1] ||
-	    cmd->__reserved[2])
+	if (cmd->flags & ~SUPPORTED_FLAGS)
+		return -EOPNOTSUPP;
+	if (cmd->__reserved[0] || cmd->__reserved[1] || cmd->__reserved[2])
 		return -EOPNOTSUPP;
 
 	/* Clear the type field since drivers don't support a random input */
-	cmd->out_data_type = IOMMU_HW_INFO_TYPE_DEFAULT;
+	if (!(cmd->flags & IOMMU_HW_INFO_FLAG_INPUT_TYPE))
+		cmd->data_type = IOMMU_HW_INFO_TYPE_DEFAULT;
 
 	idev = iommufd_get_device(ucmd, cmd->dev_id);
 	if (IS_ERR(idev))
@@ -1374,7 +1377,7 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
 
 	ops = dev_iommu_ops(idev->dev);
 	if (ops->hw_info) {
-		data = ops->hw_info(idev->dev, &data_len, &cmd->out_data_type);
+		data = ops->hw_info(idev->dev, &data_len, &cmd->data_type);
 		if (IS_ERR(data)) {
 			rc = PTR_ERR(data);
 			goto out_put;
@@ -1384,13 +1387,12 @@ int iommufd_get_hw_info(struct iommufd_ucmd *ucmd)
 		 * drivers that have hw_info callback should have a unique
 		 * iommu_hw_info_type.
 		 */
-		if (WARN_ON_ONCE(cmd->out_data_type ==
-				 IOMMU_HW_INFO_TYPE_NONE)) {
+		if (WARN_ON_ONCE(cmd->data_type == IOMMU_HW_INFO_TYPE_NONE)) {
 			rc = -ENODEV;
 			goto out_free;
 		}
 	} else {
-		cmd->out_data_type = IOMMU_HW_INFO_TYPE_NONE;
+		cmd->data_type = IOMMU_HW_INFO_TYPE_NONE;
 		data_len = 0;
 		data = NULL;
 	}
Signature · 1 line
2.43.0
maintainer Reviewed-by on v5 21/29
#64 lore raw

RE: [PATCH v5 21/29] iommufd: Allow an input data_type via iommu_hw_info

Quoted 15 lines

From: Nicolin Chen <[email protected]>
Sent: Sunday, May 18, 2025 11:22 AM

The iommu_hw_info can output via the out_data_type field the vendor data type from a driver, but this only allows driver to report one data type.

Now, with SMMUv3 having a Tegra241 CMDQV implementation, it has two sets
of types and data structs to report.

One way to support that is to use the same type field bidirectionally.

Rename "out_data_type" to simply "data_type", to allow an input for user space to request for a specific type and to get the corresponding data.

Quoted 6 lines

For backward compatibility, since the ioctl handler has never checked an input value, add a new IOMMU_HW_INFO_FLAG_INPUT_TYPE to switch between
the old output-only field and the new bidirectional field.

maintainer
#65 lore raw

Re: [PATCH v5 21/29] iommufd: Allow an input data_type via iommu_hw_info

In reply to Nicolin Chen

include/uapi/linux/iommufd.h struct iommu_hw_info { · line 681

Quoted 6 lines · Nicolin Chen
include/uapi/linux/iommufd.h +1 -1
@@ -666,7 +678,7 @@ struct iommu_hw_info {
 	__u32 dev_id;
 	__u32 data_len;
 	__aligned_u64 data_uptr;
-	__u32 out_data_type;
+	__u32 data_type;

It would be better to avoid changing this name, maybe stick a union around it?

Jason

#66 lore raw

Re: [PATCH v5 21/29] iommufd: Allow an input data_type via iommu_hw_info

Quoted 10 lines · Jason Gunthorpe

include/uapi/linux/iommufd.h struct iommu_hw_info { · line 681

Quoted 6 lines · Nicolin Chen
include/uapi/linux/iommufd.h +1 -1
@@ -666,7 +678,7 @@ struct iommu_hw_info {
 	__u32 dev_id;
 	__u32 data_len;
 	__aligned_u64 data_uptr;
-	__u32 out_data_type;
+	__u32 data_type;

It would be better to avoid changing this name, maybe stick a union around it?

Yes. I think a union is nicer.

Thanks!
Nicolin

#67 lore raw

PATCH v5 22/29 iommufd/selftest: Update hw_info coverage for an input data_type

In reply to Nicolin Chen

Test both IOMMU_HW_INFO_TYPE_DEFAULT and IOMMU_HW_INFO_TYPE_SELFTEST, and add a negative test for an unsupported type.

Also drop the unused mask in test_cmd_get_hw_capabilities() as checkpatch is complaining.

---
 tools/testing/selftests/iommu/iommufd_utils.h | 35 +++++++++++--------
 tools/testing/selftests/iommu/iommufd.c       | 32 ++++++++++++-----
 .../selftests/iommu/iommufd_fail_nth.c        |  4 +--
 3 files changed, 47 insertions(+), 24 deletions(-)
tools/testing/selftests/iommu/iommufd_utils.h +21 -14
index 4a1b2bade018..399d0dd18daf 100644
@@ -761,25 +761,29 @@ static void teardown_iommufd(int fd, struct __test_metadata *_metadata)
 #endif
 
 /* @data can be NULL */
-static int _test_cmd_get_hw_info(int fd, __u32 device_id, void *data,
-				 size_t data_len, uint32_t *capabilities,
-				 uint8_t *max_pasid)
+static int _test_cmd_get_hw_info(int fd, __u32 device_id, __u32 data_type,
+				 void *data, size_t data_len,
+				 uint32_t *capabilities, uint8_t *max_pasid)
 {
 	struct iommu_test_hw_info *info = (struct iommu_test_hw_info *)data;
 	struct iommu_hw_info cmd = {
 		.size = sizeof(cmd),
 		.dev_id = device_id,
 		.data_len = data_len,
+		.data_type = data_type,
 		.data_uptr = (uint64_t)data,
 		.out_capabilities = 0,
 	};
 	int ret;
 
+	if (data_type != IOMMU_HW_INFO_TYPE_DEFAULT)
+		cmd.flags |= IOMMU_HW_INFO_FLAG_INPUT_TYPE;
+
 	ret = ioctl(fd, IOMMU_GET_HW_INFO, &cmd);
 	if (ret)
 		return ret;
 
-	assert(cmd.out_data_type == IOMMU_HW_INFO_TYPE_SELFTEST);
+	assert(cmd.data_type == IOMMU_HW_INFO_TYPE_SELFTEST);
 
 	/*
 	 * The struct iommu_test_hw_info should be the one defined
@@ -817,20 +821,23 @@ static int _test_cmd_get_hw_info(int fd, __u32 device_id, void *data,
 	return 0;
 }
 
-#define test_cmd_get_hw_info(device_id, data, data_len)               \
-	ASSERT_EQ(0, _test_cmd_get_hw_info(self->fd, device_id, data, \
-					   data_len, NULL, NULL))
+#define test_cmd_get_hw_info(device_id, data_type, data, data_len)         \
+	ASSERT_EQ(0, _test_cmd_get_hw_info(self->fd, device_id, data_type, \
+					   data, data_len, NULL, NULL))
 
-#define test_err_get_hw_info(_errno, device_id, data, data_len)               \
-	EXPECT_ERRNO(_errno, _test_cmd_get_hw_info(self->fd, device_id, data, \
-						   data_len, NULL, NULL))
+#define test_err_get_hw_info(_errno, device_id, data_type, data, data_len) \
+	EXPECT_ERRNO(_errno,                                               \
+		     _test_cmd_get_hw_info(self->fd, device_id, data_type, \
+					   data, data_len, NULL, NULL))
 
-#define test_cmd_get_hw_capabilities(device_id, caps, mask) \
-	ASSERT_EQ(0, _test_cmd_get_hw_info(self->fd, device_id, NULL, \
+#define test_cmd_get_hw_capabilities(device_id, caps)                        \
+	ASSERT_EQ(0, _test_cmd_get_hw_info(self->fd, device_id,              \
+					   IOMMU_HW_INFO_TYPE_DEFAULT, NULL, \
 					   0, &caps, NULL))
 
-#define test_cmd_get_hw_info_pasid(device_id, max_pasid)              \
-	ASSERT_EQ(0, _test_cmd_get_hw_info(self->fd, device_id, NULL, \
+#define test_cmd_get_hw_info_pasid(device_id, max_pasid)                     \
+	ASSERT_EQ(0, _test_cmd_get_hw_info(self->fd, device_id,              \
+					   IOMMU_HW_INFO_TYPE_DEFAULT, NULL, \
 					   0, NULL, max_pasid))
 
 static int _test_ioctl_fault_alloc(int fd, __u32 *fault_id, __u32 *fault_fd)
tools/testing/selftests/iommu/iommufd.c +24 -8
index 2b3274015832..eb5b0adb16a5 100644
@@ -764,19 +764,34 @@ TEST_F(iommufd_ioas, get_hw_info)
 		uint8_t max_pasid = 0;
 
 		/* Provide a zero-size user_buffer */
-		test_cmd_get_hw_info(self->device_id, NULL, 0);
+		test_cmd_get_hw_info(self->device_id,
+				     IOMMU_HW_INFO_TYPE_DEFAULT, NULL, 0);
 		/* Provide a user_buffer with exact size */
-		test_cmd_get_hw_info(self->device_id, &buffer_exact, sizeof(buffer_exact));
+		test_cmd_get_hw_info(self->device_id,
+				     IOMMU_HW_INFO_TYPE_DEFAULT, &buffer_exact,
+				     sizeof(buffer_exact));
+
+		/* Request for a wrong data_type, and a correct one */
+		test_err_get_hw_info(EOPNOTSUPP, self->device_id,
+				     IOMMU_HW_INFO_TYPE_SELFTEST + 1,
+				     &buffer_exact, sizeof(buffer_exact));
+		test_cmd_get_hw_info(self->device_id,
+				     IOMMU_HW_INFO_TYPE_SELFTEST, &buffer_exact,
+				     sizeof(buffer_exact));
 		/*
 		 * Provide a user_buffer with size larger than the exact size to check if
 		 * kernel zero the trailing bytes.
 		 */
-		test_cmd_get_hw_info(self->device_id, &buffer_larger, sizeof(buffer_larger));
+		test_cmd_get_hw_info(self->device_id,
+				     IOMMU_HW_INFO_TYPE_DEFAULT, &buffer_larger,
+				     sizeof(buffer_larger));
 		/*
 		 * Provide a user_buffer with size smaller than the exact size to check if
 		 * the fields within the size range still gets updated.
 		 */
-		test_cmd_get_hw_info(self->device_id, &buffer_smaller, sizeof(buffer_smaller));
+		test_cmd_get_hw_info(self->device_id,
+				     IOMMU_HW_INFO_TYPE_DEFAULT,
+				     &buffer_smaller, sizeof(buffer_smaller));
 		test_cmd_get_hw_info_pasid(self->device_id, &max_pasid);
 		ASSERT_EQ(0, max_pasid);
 		if (variant->pasid_capable) {
@@ -786,9 +801,11 @@ TEST_F(iommufd_ioas, get_hw_info)
 		}
 	} else {
 		test_err_get_hw_info(ENOENT, self->device_id,
-				     &buffer_exact, sizeof(buffer_exact));
+				     IOMMU_HW_INFO_TYPE_DEFAULT, &buffer_exact,
+				     sizeof(buffer_exact));
 		test_err_get_hw_info(ENOENT, self->device_id,
-				     &buffer_larger, sizeof(buffer_larger));
+				     IOMMU_HW_INFO_TYPE_DEFAULT, &buffer_larger,
+				     sizeof(buffer_larger));
 	}
 }
 
@@ -2175,8 +2192,7 @@ TEST_F(iommufd_dirty_tracking, device_dirty_capability)
 
 	test_cmd_hwpt_alloc(self->idev_id, self->ioas_id, 0, &hwpt_id);
 	test_cmd_mock_domain(hwpt_id, &stddev_id, NULL, NULL);
-	test_cmd_get_hw_capabilities(self->idev_id, caps,
-				     IOMMU_HW_CAP_DIRTY_TRACKING);
+	test_cmd_get_hw_capabilities(self->idev_id, caps);
 	ASSERT_EQ(IOMMU_HW_CAP_DIRTY_TRACKING,
 		  caps & IOMMU_HW_CAP_DIRTY_TRACKING);
 
tools/testing/selftests/iommu/iommufd_fail_nth.c +2 -2
index 41c685bbd252..651fc9f13c08 100644
@@ -667,8 +667,8 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 					&self->stdev_id, NULL, &idev_id))
 		return -1;
 
-	if (_test_cmd_get_hw_info(self->fd, idev_id, &info,
-				  sizeof(info), NULL, NULL))
+	if (_test_cmd_get_hw_info(self->fd, idev_id, IOMMU_HW_INFO_TYPE_DEFAULT,
+				  &info, sizeof(info), NULL, NULL))
 		return -1;
 
 	if (_test_cmd_hwpt_alloc(self->fd, idev_id, ioas_id, 0,
Signature · 1 line
2.43.0
#68 lore raw

PATCH v5 23/29 iommu/arm-smmu-v3-iommufd: Add vsmmu_alloc impl op

In reply to Nicolin Chen

An impl driver might want to allocate its own type of vIOMMU object or the standard IOMMU_VIOMMU_TYPE_ARM_SMMUV3 by setting up its own SW/HW bits, as the tegra241-cmdqv driver will add IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV.

Add a vsmmu_alloc op and prioritize it in arm_vsmmu_alloc().

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h        |  7 +++++++
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c    | 14 ++++++++------
 2 files changed, 15 insertions(+), 6 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +7 -0
index cb93eff4d9ab..a455ff97e53e 100644
@@ -16,6 +16,7 @@
 #include <linux/sizes.h>
 
 struct arm_smmu_device;
+struct arm_smmu_domain;
 
 /* MMIO registers */
 #define ARM_SMMU_IDR0			0x0
@@ -715,11 +716,17 @@ struct arm_smmu_strtab_cfg {
 };
 
 struct arm_smmu_impl_ops {
+	const unsigned int supported_vsmmu_type;
 	int (*device_reset)(struct arm_smmu_device *smmu);
 	void (*device_remove)(struct arm_smmu_device *smmu);
 	int (*init_structures)(struct arm_smmu_device *smmu);
 	struct arm_smmu_cmdq *(*get_secondary_cmdq)(
 		struct arm_smmu_device *smmu, struct arm_smmu_cmdq_ent *ent);
+	struct arm_vsmmu *(*vsmmu_alloc)(
+		struct arm_smmu_device *smmu,
+		struct arm_smmu_domain *smmu_domain, struct iommufd_ucmd *ucmd,
+		unsigned int viommu_type,
+		const struct iommu_user_data *user_data);
 };
 
 /* An SMMUv3 instance */
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +8 -6
index 61a3f9134a9b..8a13d0102c05 100644
@@ -396,10 +396,7 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 		iommu_get_iommu_dev(dev, struct arm_smmu_device, iommu);
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 	struct arm_smmu_domain *s2_parent = to_smmu_domain(parent);
-	struct arm_vsmmu *vsmmu;
-
-	if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
-		return ERR_PTR(-EOPNOTSUPP);
+	struct arm_vsmmu *vsmmu = ERR_PTR(-EOPNOTSUPP);
 
 	if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
 		return ERR_PTR(-EOPNOTSUPP);
@@ -427,8 +424,13 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
 		return ERR_PTR(-EOPNOTSUPP);
 
-	vsmmu = iommufd_viommu_alloc(ucmd, struct arm_vsmmu, core,
-				     &arm_vsmmu_ops);
+	if (smmu->impl_ops && smmu->impl_ops->vsmmu_alloc &&
+	    viommu_type == smmu->impl_ops->supported_vsmmu_type)
+		vsmmu = smmu->impl_ops->vsmmu_alloc(smmu, s2_parent, ucmd,
+						    viommu_type, user_data);
+	else if (viommu_type == IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
+		vsmmu = iommufd_viommu_alloc(ucmd, struct arm_vsmmu, core,
+					     &arm_vsmmu_ops);
 	if (IS_ERR(vsmmu))
 		return ERR_CAST(vsmmu);
 
Signature · 1 line
2.43.0
#69 lore raw

PATCH v5 24/29 iommu/arm-smmu-v3-iommufd: Add hw_info to impl_ops

In reply to Nicolin Chen

This will be used by Tegra241 CMDQV implementation to report a non-default HW info data.

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h         | 1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
index a455ff97e53e..8328c8cc1899 100644
@@ -727,6 +727,7 @@ struct arm_smmu_impl_ops {
 		struct arm_smmu_domain *smmu_domain, struct iommufd_ucmd *ucmd,
 		unsigned int viommu_type,
 		const struct iommu_user_data *user_data);
+	void *(*hw_info)(struct arm_smmu_device *smmu, u32 *length, u32 *type);
 };
 
 /* An SMMUv3 instance */
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +6 -2
index 8a13d0102c05..837f15e2d40f 100644
@@ -10,13 +10,17 @@
 void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type)
 {
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	const struct arm_smmu_impl_ops *impl_ops = master->smmu->impl_ops;
 	struct iommu_hw_info_arm_smmuv3 *info;
 	u32 __iomem *base_idr;
 	unsigned int i;
 
 	if (*type != IOMMU_HW_INFO_TYPE_DEFAULT &&
-	    *type != IOMMU_HW_INFO_TYPE_ARM_SMMUV3)
-		return ERR_PTR(-EOPNOTSUPP);
+	    *type != IOMMU_HW_INFO_TYPE_ARM_SMMUV3) {
+		if (!impl_ops || !impl_ops->hw_info)
+			return ERR_PTR(-EOPNOTSUPP);
+		return impl_ops->hw_info(master->smmu, length, type);
+	}
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
Signature · 1 line
2.43.0
maintainer Reviewed-by on v5 24/29
#70 lore raw

Re: [PATCH v5 24/29] iommu/arm-smmu-v3-iommufd: Add hw_info to impl_ops

Quoted 8 lines · Nicolin Chen

This will be used by Tegra241 CMDQV implementation to report a non-default HW info data.

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h         | 1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

Jason

#71 lore raw

PATCH v5 25/29 iommu/tegra241-cmdqv: Use request_threaded_irq

In reply to Nicolin Chen

A vEVENT can be reported only from a threaded IRQ context. Change to using request_threaded_irq to support that.

---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +3 -2
index dd7d030d2e89..ba029f7d24ce 100644
@@ -824,8 +824,9 @@ __tegra241_cmdqv_probe(struct arm_smmu_device *smmu, struct resource *res,
 	cmdqv->dev = smmu->impl_dev;
 
 	if (cmdqv->irq > 0) {
-		ret = request_irq(irq, tegra241_cmdqv_isr, 0, "tegra241-cmdqv",
-				  cmdqv);
+		ret = request_threaded_irq(irq, NULL, tegra241_cmdqv_isr,
+					   IRQF_ONESHOT, "tegra241-cmdqv",
+					   cmdqv);
 		if (ret) {
 			dev_err(cmdqv->dev, "failed to request irq (%d): %d\n",
 				cmdqv->irq, ret);
Signature · 1 line
2.43.0
#72 lore raw

PATCH v5 26/29 iommu/tegra241-cmdqv: Simplify deinit flow in tegra241_cmdqv_remove_vintf()

In reply to Nicolin Chen

The current flow of tegra241_cmdqv_remove_vintf() is:
1. For each LVCMDQ, tegra241_vintf_remove_lvcmdq():
a. Disable the LVCMDQ HW
b. Release the LVCMDQ SW resource
2. For current VINTF, tegra241_vintf_hw_deinit():
c. Disable all LVCMDQ HWs
d. Disable VINTF HW

Obviously, the step 1.a and the step 2.c are redundant.

Since tegra241_vintf_hw_deinit() disables all of its LVCMDQ HWs, it could simplify the flow in tegra241_cmdqv_remove_vintf() by calling that first:
1. For current VINTF, tegra241_vintf_hw_deinit():
a. Disable all LVCMDQ HWs
b. Disable VINTF HW
2. Release all LVCMDQ SW resources

Drop tegra241_vintf_remove_lvcmdq(), and move tegra241_vintf_free_lvcmdq() as the new step 2.

---
 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +3 -10
index ba029f7d24ce..8d418c131b1b 100644
@@ -628,24 +628,17 @@ static int tegra241_cmdqv_init_vintf(struct tegra241_cmdqv *cmdqv, u16 max_idx,
 
 /* Remove Helpers */
 
-static void tegra241_vintf_remove_lvcmdq(struct tegra241_vintf *vintf, u16 lidx)
-{
-	tegra241_vcmdq_hw_deinit(vintf->lvcmdqs[lidx]);
-	tegra241_vintf_free_lvcmdq(vintf, lidx);
-}
-
 static void tegra241_cmdqv_remove_vintf(struct tegra241_cmdqv *cmdqv, u16 idx)
 {
 	struct tegra241_vintf *vintf = cmdqv->vintfs[idx];
 	u16 lidx;
 
+	tegra241_vintf_hw_deinit(vintf);
+
 	/* Remove LVCMDQ resources */
 	for (lidx = 0; lidx < vintf->cmdqv->num_lvcmdqs_per_vintf; lidx++)
 		if (vintf->lvcmdqs[lidx])
-			tegra241_vintf_remove_lvcmdq(vintf, lidx);
-
-	/* Remove VINTF resources */
-	tegra241_vintf_hw_deinit(vintf);
+			tegra241_vintf_free_lvcmdq(vintf, lidx);
 
 	dev_dbg(cmdqv->dev, "VINTF%u: deallocated\n", vintf->idx);
 	tegra241_cmdqv_deinit_vintf(cmdqv, idx);
Signature · 1 line
2.43.0
#73 lore raw

PATCH v5 27/29 iommu/tegra241-cmdqv: Do not statically map LVCMDQs

In reply to Nicolin Chen

To simplify the mappings from global VCMDQs to VINTFs' LVCMDQs, the design chose to do static allocations and mappings in the global reset function.

However, with the user-owned VINTF support, it exposes a security concern:
if user space VM only wants one LVCMDQ for a VINTF, statically mapping two or more LVCMDQs creates a hidden VCMDQ that user space could DoS attack by writing random stuff to overwhelm the kernel with unhandleable IRQs.

Thus, to support the user-owned VINTF feature, a LVCMDQ mapping has to be done dynamically.

HW allows pre-assigning global VCMDQs in the CMDQ_ALLOC registers, without finalizing the mappings by keeping CMDQV_CMDQ_ALLOCATED=0. So, add a pair of map/unmap helper that simply sets/clears that bit.

For kernel-owned VINTF0, move LVCMDQ mappings to tegra241_vintf_hw_init(), and the unmappings to tegra241_vintf_hw_deinit().

For user-owned VINTFs that will be added, the mappings/unmappings will be on demand upon an LVCMDQ allocation from the user space.

However, the dynamic LVCMDQ mapping/unmapping can complicate the timing of calling tegra241_vcmdq_hw_init/deinit(), which write LVCMDQ address space, i.e. requiring LVCMDQ to be mapped. Highlight that with a note to the top of either of them.

---
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 37 +++++++++++++++++--
 1 file changed, 33 insertions(+), 4 deletions(-)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +33 -4
index 8d418c131b1b..869c90b660c1 100644
@@ -351,6 +351,7 @@ tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu,
 
 /* HW Reset Functions */
 
+/* This function is for LVCMDQ, so @vcmdq must not be unmapped yet */
 static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq)
 {
 	char header[64], *h = lvcmdq_error_header(vcmdq, header, 64);
@@ -379,6 +380,7 @@ static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq)
 	dev_dbg(vcmdq->cmdqv->dev, "%sdeinited\n", h);
 }
 
+/* This function is for LVCMDQ, so @vcmdq must be mapped prior */
 static int tegra241_vcmdq_hw_init(struct tegra241_vcmdq *vcmdq)
 {
 	char header[64], *h = lvcmdq_error_header(vcmdq, header, 64);
@@ -404,16 +406,42 @@ static int tegra241_vcmdq_hw_init(struct tegra241_vcmdq *vcmdq)
 	return 0;
 }
 
+/* Unmap a global VCMDQ from the pre-assigned LVCMDQ */
+static void tegra241_vcmdq_unmap_lvcmdq(struct tegra241_vcmdq *vcmdq)
+{
+	u32 regval = readl(REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx)));
+	char header[64], *h = lvcmdq_error_header(vcmdq, header, 64);
+
+	writel(regval & ~CMDQV_CMDQ_ALLOCATED,
+	       REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx)));
+	dev_dbg(vcmdq->cmdqv->dev, "%sunmapped\n", h);
+}
+
 static void tegra241_vintf_hw_deinit(struct tegra241_vintf *vintf)
 {
-	u16 lidx;
+	u16 lidx = vintf->cmdqv->num_lvcmdqs_per_vintf;
 
-	for (lidx = 0; lidx < vintf->cmdqv->num_lvcmdqs_per_vintf; lidx++)
-		if (vintf->lvcmdqs && vintf->lvcmdqs[lidx])
+	/* HW requires to unmap LVCMDQs in descending order */
+	while (lidx--) {
+		if (vintf->lvcmdqs && vintf->lvcmdqs[lidx]) {
 			tegra241_vcmdq_hw_deinit(vintf->lvcmdqs[lidx]);
+			tegra241_vcmdq_unmap_lvcmdq(vintf->lvcmdqs[lidx]);
+		}
+	}
 	vintf_write_config(vintf, 0);
 }
 
+/* Map a global VCMDQ to the pre-assigned LVCMDQ */
+static void tegra241_vcmdq_map_lvcmdq(struct tegra241_vcmdq *vcmdq)
+{
+	u32 regval = readl(REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx)));
+	char header[64], *h = lvcmdq_error_header(vcmdq, header, 64);
+
+	writel(regval | CMDQV_CMDQ_ALLOCATED,
+	       REG_CMDQV(vcmdq->cmdqv, CMDQ_ALLOC(vcmdq->idx)));
+	dev_dbg(vcmdq->cmdqv->dev, "%smapped\n", h);
+}
+
 static int tegra241_vintf_hw_init(struct tegra241_vintf *vintf, bool hyp_own)
 {
 	u32 regval;
@@ -441,8 +469,10 @@ static int tegra241_vintf_hw_init(struct tegra241_vintf *vintf, bool hyp_own)
 	 */
 	vintf->hyp_own = !!(VINTF_HYP_OWN & readl(REG_VINTF(vintf, CONFIG)));
 
+	/* HW requires to map LVCMDQs in ascending order */
 	for (lidx = 0; lidx < vintf->cmdqv->num_lvcmdqs_per_vintf; lidx++) {
 		if (vintf->lvcmdqs && vintf->lvcmdqs[lidx]) {
+			tegra241_vcmdq_map_lvcmdq(vintf->lvcmdqs[lidx]);
 			ret = tegra241_vcmdq_hw_init(vintf->lvcmdqs[lidx]);
 			if (ret) {
 				tegra241_vintf_hw_deinit(vintf);
@@ -476,7 +506,6 @@ static int tegra241_cmdqv_hw_reset(struct arm_smmu_device *smmu)
 		for (lidx = 0; lidx < cmdqv->num_lvcmdqs_per_vintf; lidx++) {
 			regval  = FIELD_PREP(CMDQV_CMDQ_ALLOC_VINTF, idx);
 			regval |= FIELD_PREP(CMDQV_CMDQ_ALLOC_LVCMDQ, lidx);
-			regval |= CMDQV_CMDQ_ALLOCATED;
 			writel_relaxed(regval,
 				       REG_CMDQV(cmdqv, CMDQ_ALLOC(qidx++)));
 		}
Signature · 1 line
2.43.0
#74 lore raw

PATCH v5 28/29 iommu/tegra241-cmdqv: Add user-space use support

In reply to Nicolin Chen

The CMDQV HW supports a user-space use for virtualization cases. It allows the VM to issue guest-level TLBI or ATC_INV commands directly to the queue and executes them without a VMEXIT, as HW will replace the VMID field in a TLBI command and the SID field in an ATC_INV command with the preset VMID and SID.

This is built upon the vIOMMU infrastructure by allowing VMM to allocate a VINTF (as a vIOMMU object) and assign VCMDQs (HW QUEUE objs) to the VINTF.

So firstly, replace the standard vSMMU model with the VINTF implementation but reuse the standard cache_invalidate op (for unsupported commands) and the standard alloc_domain_nested op (for standard nested STE).

Each VINTF has two 64KB MMIO pages (128B per logical VCMDQ):
- Page0 (directly accessed by guest) has all the control and status bits.
- Page1 (trapped by VMM) has guest-owned queue memory location/size info.

VMM should trap the emulated VINTF0's page1 of the guest VM for the guest- level VCMDQ location/size info and forward that to the kernel to translate to a physical memory location to program the VCMDQ HW during an allocation call. Then, it should mmap the assigned VINTF's page0 to the VINTF0 page0 of the guest VM. This allows the guest OS to read and write the guest-own VINTF's page0 for direct control of the VCMDQ HW.

For ATC invalidation commands that hold an SID, it requires all devices to register their virtual SIDs to the SID_MATCH registers and their physical SIDs to the pairing SID_REPLACE registers, so that HW can use those as a lookup table to replace those virtual SIDs with the correct physical SIDs. Thus, implement the driver-allocated vDEVICE op with a tegra241_vintf_sid structure to allocate SID_REPLACE and to program the SIDs accordingly.

This enables the HW accelerated feature for NVIDIA Grace CPU. Compared to the standard SMMUv3 operating in the nested translation mode trapping CMDQ for TLBI and ATC_INV commands, this gives a huge performance improvement:
70% to 90% reductions of invalidation time were measured by various DMA unmap tests running in a guest OS.

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  15 +
 include/uapi/linux/iommufd.h                  |  58 +++
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |   6 +-
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 419 +++++++++++++++++-
 4 files changed, 491 insertions(+), 7 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +15 -0
index 8328c8cc1899..187c47370652 100644
@@ -1001,6 +1001,14 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
 				struct arm_smmu_cmdq *cmdq, u64 *cmds, int n,
 				bool sync);
 
+static inline phys_addr_t
+arm_smmu_domain_ipa_to_pa(struct arm_smmu_domain *smmu_domain, u64 ipa)
+{
+	if (WARN_ON_ONCE(smmu_domain->stage != ARM_SMMU_DOMAIN_S2))
+		return 0;
+	return iommu_iova_to_phys(&smmu_domain->domain, ipa);
+}
+
 #ifdef CONFIG_ARM_SMMU_V3_SVA
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
 bool arm_smmu_master_sva_supported(struct arm_smmu_master *master);
@@ -1077,9 +1085,16 @@ int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
 void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state);
 void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master);
 int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt);
+struct iommu_domain *
+arm_vsmmu_alloc_domain_nested(struct iommufd_viommu *viommu, u32 flags,
+			      const struct iommu_user_data *user_data);
+int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
+			       struct iommu_user_data_array *array);
 #else
 #define arm_smmu_hw_info NULL
 #define arm_vsmmu_alloc NULL
+#define arm_vsmmu_alloc_domain_nested NULL
+#define arm_vsmmu_cache_invalidate NULL
 
 static inline int
 arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
include/uapi/linux/iommufd.h +58 -0

95 lines not on this page · raw message

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +3 -3
index 837f15e2d40f..4917e5b6f562 100644
@@ -224,7 +224,7 @@ static int arm_smmu_validate_vste(struct iommu_hwpt_arm_smmuv3 *arg,
 	return 0;
 }
 
-static struct iommu_domain *
+struct iommu_domain *
 arm_vsmmu_alloc_domain_nested(struct iommufd_viommu *viommu, u32 flags,
 			      const struct iommu_user_data *user_data)
 {
@@ -335,8 +335,8 @@ static int arm_vsmmu_convert_user_cmd(struct arm_vsmmu *vsmmu,
 	return 0;
 }
 
-static int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
-				      struct iommu_user_data_array *array)
+int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
+			       struct iommu_user_data_array *array)
 {
 	struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core);
 	struct arm_smmu_device *smmu = vsmmu->smmu;
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +415 -4

585 lines not on this page · raw message

Signature · 1 line
2.43.0
maintainer
#75 lore raw

Re: [PATCH v5 28/29] iommu/tegra241-cmdqv: Add user-space use support

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu, · line 1010

Quoted 7 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +7 -0
+static inline phys_addr_t
+arm_smmu_domain_ipa_to_pa(struct arm_smmu_domain *smmu_domain, u64 ipa)
+{
+	if (WARN_ON_ONCE(smmu_domain->stage != ARM_SMMU_DOMAIN_S2))
+		return 0;
+	return iommu_iova_to_phys(&smmu_domain->domain, ipa);
+}

It is not allowed to call this function unless you are holding a range lock on the IOAS that covers ipa, and this path does not.

This is why it has to go through an access to get the physical as that has the right locking.

Jason

#76 lore raw

Re: [PATCH v5 28/29] iommu/tegra241-cmdqv: Add user-space use support

Quoted 12 lines · Jason Gunthorpe

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu, · line 1010

Quoted 7 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +7 -0
+static inline phys_addr_t
+arm_smmu_domain_ipa_to_pa(struct arm_smmu_domain *smmu_domain, u64 ipa)
+{
+	if (WARN_ON_ONCE(smmu_domain->stage != ARM_SMMU_DOMAIN_S2))
+		return 0;
+	return iommu_iova_to_phys(&smmu_domain->domain, ipa);
+}

It is not allowed to call this function unless you are holding a range lock on the IOAS that covers ipa, and this path does not.

Quoted 2 lines

This is why it has to go through an access to get the physical as that has the right locking.

OK. It would not be needed either once the core passes in type phys_addr_t as you suggested in the other patch.

Thanks
Nicolin

#77 lore raw

PATCH v5 29/29 iommu/tegra241-cmdqv: Add IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV support

In reply to Nicolin Chen

Add a new vEVENTQ type for VINTFs that are assigned to the user space. Simply report the two 64-bit LVCMDQ_ERR_MAPs register values.

---
 include/uapi/linux/iommufd.h                  | 15 +++++++++++++
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 22 +++++++++++++++++++
 2 files changed, 37 insertions(+)
include/uapi/linux/iommufd.h +15 -0
index 8a9090cc938f..2080594db39b 100644
@@ -1139,10 +1139,12 @@ struct iommufd_vevent_header {
  * enum iommu_veventq_type - Virtual Event Queue Type
  * @IOMMU_VEVENTQ_TYPE_DEFAULT: Reserved for future use
  * @IOMMU_VEVENTQ_TYPE_ARM_SMMUV3: ARM SMMUv3 Virtual Event Queue
+ * @IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV: NVIDIA Tegra241 CMDQV Extension IRQ
  */
 enum iommu_veventq_type {
 	IOMMU_VEVENTQ_TYPE_DEFAULT = 0,
 	IOMMU_VEVENTQ_TYPE_ARM_SMMUV3 = 1,
+	IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV = 2,
 };
 
 /**
@@ -1166,6 +1168,19 @@ struct iommu_vevent_arm_smmuv3 {
 	__aligned_le64 evt[4];
 };
 
+/**
+ * struct iommu_vevent_tegra241_cmdqv - Tegra241 CMDQV IRQ
+ *                                      (IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV)
+ * @lvcmdq_err_map: 128-bit logical vcmdq error map, little-endian.
+ *                  (Refer to register LVCMDQ_ERR_MAPs per VINTF )
+ *
+ * The 128-bit register value from HW exclusively reflect the error bits for a
+ * Virtual Interface represented by a vIOMMU object. Read and report directly.
+ */
+struct iommu_vevent_tegra241_cmdqv {
+	__aligned_le64 lvcmdq_err_map[2];
+};
+
 /**
  * struct iommu_veventq_alloc - ioctl(IOMMU_VEVENTQ_ALLOC)
  * @size: sizeof(struct iommu_veventq_alloc)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +22 -0
index 49f07f61673c..7f3b7887478b 100644
@@ -292,6 +292,20 @@ static inline int vcmdq_write_config(struct tegra241_vcmdq *vcmdq, u32 regval)
 
 /* ISR Functions */
 
+static void tegra241_vintf_user_handle_error(struct tegra241_vintf *vintf)
+{
+	struct iommufd_viommu *viommu = &vintf->vsmmu.core;
+	struct iommu_vevent_tegra241_cmdqv vevent_data;
+	int i;
+
+	for (i = 0; i < LVCMDQ_ERR_MAP_NUM_64; i++)
+		vevent_data.lvcmdq_err_map[i] =
+			readq_relaxed(REG_VINTF(vintf, LVCMDQ_ERR_MAP_64(i)));
+
+	iommufd_viommu_report_event(viommu, IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
+				    &vevent_data, sizeof(vevent_data));
+}
+
 static void tegra241_vintf0_handle_error(struct tegra241_vintf *vintf)
 {
 	int i;
@@ -337,6 +351,14 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)
 		vintf_map &= ~BIT_ULL(0);
 	}
 
+	/* Handle other user VINTFs and their LVCMDQs */
+	while (vintf_map) {
+		unsigned long idx = __ffs64(vintf_map);
+
+		tegra241_vintf_user_handle_error(cmdqv->vintfs[idx]);
+		vintf_map &= ~BIT_ULL(idx);
+	}
+
 	return IRQ_HANDLED;
 }
 
Signature · 1 line
2.43.0
maintainer Reviewed-by on v5 29/29
#78 lore raw

Re: [PATCH v5 29/29] iommu/tegra241-cmdqv: Add IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV support

Quoted 10 lines · Nicolin Chen

Add a new vEVENTQ type for VINTFs that are assigned to the user space. Simply report the two 64-bit LVCMDQ_ERR_MAPs register values.

---
 include/uapi/linux/iommufd.h                  | 15 +++++++++++++
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 22 +++++++++++++++++++
 2 files changed, 37 insertions(+)

Jason