iommufd: Repare for IOMMUFD_OBJ_HW_QUEUE

PATCH Superseded Nicolin Chen 4 participants 46 messages Jun 9, 2025 – Jun 13, 2025

Subsystems (3)

IOMMU Arm SMMU drivers Iommufd

lore

Share & tools

lore

Nicolin Chen

author

2025-06-09 17:14 UTC #0 lore raw

cover iommufd: Repare for IOMMUFD_OBJ_HW_QUEUE

The new HW Queue object will require more interactions with IOMMU drivers, with a few more for-driver APIs. This will complicate the driver-allocated structure design like the viommu_alloc op: since the core structure is not initialized during the driver allocation stage, a new for-driver API can't reference any member in the core vIOMMU structure.

Make a preparatory series doing:
- Cosmetic fixes and clean ups
- Replace viommu_alloc design with get_viommu_size + viommu_init
- Add a new iommufd_object_alloc_ucmd

Some of the patches are included from:
[PATCH v5 00/29] iommufd: Add vIOMMU infrastructure (Part-4 HW QUEUE)
https://lore.kernel.org/all/[email protected]/

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

Thanks
Nicolin

Nicolin Chen (12):
iommufd: Apply obvious cosmetic fixes
iommufd: Drop unused ictx in struct iommufd_vdevice
iommufd: Use enum iommu_viommu_type for type in struct iommufd_viommu
iommufd: Use enum iommu_veventq_type for type in struct
iommufd_veventq
iommu: Introduce get_viommu_size and viommu_init ops iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init iommu/arm-smmu-v3: Implement arm_smmu_get_viommu_size and
arm_vsmmu_init
iommufd/viommu: Replace ops->viommu_alloc with ops->viommu_init
iommu: Deprecate viommu_alloc op
iommufd: Move _iommufd_object_alloc out of driver.c
iommufd: Introduce iommufd_object_alloc_ucmd helper
iommufd: Apply the new iommufd_object_alloc_ucmd helper

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 11 ++--
 drivers/iommu/iommufd/io_pagetable.h          |  2 +-
 drivers/iommu/iommufd/iommufd_private.h       | 33 +++++++---
 include/linux/iommu.h                         | 26 ++++----
 include/linux/iommufd.h                       | 39 +++---------
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 47 +++++++-------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  3 +-
 drivers/iommu/iommufd/device.c                |  3 +-
 drivers/iommu/iommufd/driver.c                | 33 ----------
 drivers/iommu/iommufd/eventq.c                | 14 ++---
 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                  | 62 +++++++++++++++++--
 drivers/iommu/iommufd/pages.c                 |  9 ++-
 drivers/iommu/iommufd/selftest.c              | 57 ++++++++---------
 drivers/iommu/iommufd/viommu.c                | 46 +++++++++-----
 17 files changed, 212 insertions(+), 183 deletions(-)

Signature · 1 line

2.43.0

Nicolin Chen

author

2025-06-09 17:14 UTC #1 lore raw

PATCH 1/12 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

Reviewed-by Kevin Tian <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 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 9ccc83341f32..190ceb5dada3 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);
 }

@@ -452,8 +451,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 86244403b532..ed0dc539d490 100644
@@ -485,8 +485,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 6bd0abf9a641..4d5dca8027b1 100644
@@ -1216,9 +1216,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;

@@ -1491,7 +1490,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))

@@ -1508,7 +1507,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)) /

@@ -1584,7 +1583,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 |

@@ -1610,7 +1609,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)

@@ -1665,7 +1664,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;
 	}

@@ -1701,7 +1700,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);

@@ -1832,8 +1831,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);

@@ -2004,8 +2002,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

Nicolin Chen

author

2025-06-09 17:14 UTC #2 lore raw

PATCH 2/12 iommufd: Drop unused ictx in struct iommufd_vdevice

In reply to Nicolin Chen

The core code can always get the ictx pointer via vdev->viommu->ictx, thus drop this unused one.

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

---
 drivers/iommu/iommufd/iommufd_private.h | 1 -
 1 file changed, 1 deletion(-)

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

index 190ceb5dada3..4619285c09b7 100644
@@ -601,7 +601,6 @@ 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 */

Signature · 1 line

2.43.0

Tian, Kevin

maintainer Reviewed-by on v1 2/12

2025-06-12 08:06 UTC #3 lore raw

RE: [PATCH v1 02/12] iommufd: Drop unused ictx in struct iommufd_vdevice

Quoted 7 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:13 AM
The core code can always get the ictx pointer via vdev->viommu->ictx, thus drop this unused one.
Signed-off-by Nicolin Chen <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Jason Gunthorpe

maintainer Reviewed-by on v1 2/12

2025-06-13 13:36 UTC #4 lore raw

Re: [PATCH v1 02/12] iommufd: Drop unused ictx in struct iommufd_vdevice

In reply to Nicolin Chen

Quoted 7 lines · Nicolin Chen

The core code can always get the ictx pointer via vdev->viommu->ictx, thus drop this unused one.
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/iommufd/iommufd_private.h | 1 -
 1 file changed, 1 deletion(-)

Reviewed-by Jason Gunthorpe <[email protected]>

Jason

Nicolin Chen

author

2025-06-09 17:14 UTC #5 lore raw

PATCH 3/12 iommufd: Use enum iommu_viommu_type for type in struct iommufd_viommu

In reply to Nicolin Chen

Replace unsigned init, to make it clear. No functional changes.

The viommu_alloc iommu op will be deprecated, so don't change that.

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

---
 include/linux/iommufd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/iommufd.h +1 -1

index 498c9a768506..ac98e49e44fe 100644
@@ -101,7 +101,7 @@ struct iommufd_viommu {
 	struct list_head veventqs;
 	struct rw_semaphore veventqs_rwsem;
 
-	unsigned int type;
+	enum iommu_viommu_type type;
 };
 
 /**

Signature · 1 line

2.43.0

Tian, Kevin

maintainer Reviewed-by on v1 3/12

2025-06-12 08:07 UTC #6 lore raw

RE: [PATCH v1 03/12] iommufd: Use enum iommu_viommu_type for type in struct iommufd_viommu

Quoted 8 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:13 AM
Replace unsigned init, to make it clear. No functional changes.
The viommu_alloc iommu op will be deprecated, so don't change that.
Signed-off-by Nicolin Chen <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Jason Gunthorpe

maintainer Reviewed-by on v1 3/12

2025-06-13 13:36 UTC #7 lore raw

Re: [PATCH v1 03/12] iommufd: Use enum iommu_viommu_type for type in struct iommufd_viommu

In reply to Nicolin Chen

Quoted 8 lines · Nicolin Chen

Replace unsigned init, to make it clear. No functional changes.
The viommu_alloc iommu op will be deprecated, so don't change that.
Signed-off-by Nicolin Chen <[email protected]>
---
 include/linux/iommufd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by Jason Gunthorpe <[email protected]>

Jason

Nicolin Chen

author

2025-06-09 17:14 UTC #8 lore raw

PATCH 4/12 iommufd: Use enum iommu_veventq_type for type in struct iommufd_veventq

In reply to Nicolin Chen

Replace unsigned init, to make it clear. No functional changes.

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

---
 drivers/iommu/iommufd/iommufd_private.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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

index 4619285c09b7..32f0631368e1 100644
@@ -526,7 +526,7 @@ struct iommufd_veventq {
 	struct list_head node; /* for iommufd_viommu::veventqs */
 	struct iommufd_vevent lost_events_header;
 
-	unsigned int type;
+	enum iommu_veventq_type type;
 	unsigned int depth;
 
 	/* Use common.lock for protection */

@@ -581,7 +581,8 @@ iommufd_get_viommu(struct iommufd_ucmd *ucmd, u32 id)
 }
 
 static inline struct iommufd_veventq *
-iommufd_viommu_find_veventq(struct iommufd_viommu *viommu, u32 type)
+iommufd_viommu_find_veventq(struct iommufd_viommu *viommu,
+			    enum iommu_veventq_type type)
 {
 	struct iommufd_veventq *veventq, *next;

Signature · 1 line

2.43.0

Tian, Kevin

maintainer Reviewed-by on v1 4/12

2025-06-12 08:07 UTC #9 lore raw

RE: [PATCH v1 04/12] iommufd: Use enum iommu_veventq_type for type in struct iommufd_veventq

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:13 AM
Replace unsigned init, to make it clear. No functional changes.
Signed-off-by Nicolin Chen <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Jason Gunthorpe

maintainer Reviewed-by on v1 4/12

2025-06-13 13:36 UTC #10 lore raw

Re: [PATCH v1 04/12] iommufd: Use enum iommu_veventq_type for type in struct iommufd_veventq

In reply to Nicolin Chen

Quoted 6 lines · Nicolin Chen

Replace unsigned init, to make it clear. No functional changes.
Signed-off-by Nicolin Chen <[email protected]>
---
 drivers/iommu/iommufd/iommufd_private.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by Jason Gunthorpe <[email protected]>

Jason

Nicolin Chen

author

2025-06-09 17:14 UTC #11 lore raw

PATCH 5/12 iommu: Introduce get_viommu_size and viommu_init ops

In reply to Nicolin Chen

So far, a vIOMMU object has been allocated by IOMMU driver and initialized with the driver-level structure, before it returns to the iommufd core for core-level structure initialization. It has been requiring iommufd core to expose some core structure/helpers in its driver.c file, which result in a size increase of this driver module.

Meanwhile, IOMMU drivers are now requiring more vIOMMU-base structures for some advanced feature, such as the existing vDEVICE and a future HW_QUEUE. Initializing a core-structure later than driver-structure gives for-driver helpers some trouble, when they are used by IOMMU driver assuming that the new structure (including core) are fully initialized, for example:

    // my_viommu is successfully allocated
    my_viommu = iommufd_viommu_alloc(...);
    // This may crash if it reads viommu->ictx
    new = iommufd_new_viommu_helper(my_viommu->core ...);

To ease such a condition, allow the IOMMU driver to report the size of its vIOMMU structure, let the core allocate a vIOMMU object and initialize the core-level structure first, and then hand it over the driver to initialize its driver-level structure.

Thus, this requires two new iommu ops, get_viommu_size and viommu_init, so iommufd core can communicate with drivers to replace the viommu_alloc op.

This also adds a VIOMMU_STRUCT_SIZE macro, for drivers to use, which would statically sanitize the driver structure.

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

---
 include/linux/iommu.h   | 15 +++++++++++++++
 include/linux/iommufd.h |  6 ++++++
 2 files changed, 21 insertions(+)

include/linux/iommu.h +15 -0

index 156732807994..e42a28971182 100644
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/of.h>
 #include <linux/iova_bitmap.h>
+#include <uapi/linux/iommufd.h>
 
 #define IOMMU_READ	(1 << 0)
 #define IOMMU_WRITE	(1 << 1)

@@ -596,6 +597,16 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
  *		- IOMMU_DOMAIN_DMA: must use a dma domain
  *		- 0: use the default setting
  * @default_domain_ops: the default ops for domains
+ * @get_viommu_size: Get the size of a driver-level vIOMMU structure for a given
+ *                   @dev corresponding to @viommu_type. Driver should return an
+ *                   errno if vIOMMU isn't supported accordingly. It is required
+ *                   for driver to use the VIOMMU_STRUCT_SIZE macro to sanitize
+ *                   a driver-level vIOMMU structure related to the core one
+ * @viommu_init: Init the driver-level struct of an iommufd_viommu on a physical
+ *               IOMMU instance @viommu->iommu_dev, as the set of virtualization
+ *               resources shared/passed to user space IOMMU instance. Associate
+ *               it with a nesting @parent_domain. It is required for driver to
+ *               set @viommu->ops pointing to its own viommu_ops
  * @viommu_alloc: Allocate an iommufd_viommu on a physical IOMMU instance behind
  *                the @dev, as the set of virtualization resources shared/passed
  *                to user space IOMMU instance. And associate it with a nesting

@@ -654,6 +665,10 @@ struct iommu_ops {
 
 	int (*def_domain_type)(struct device *dev);
 
+	int (*get_viommu_size)(enum iommu_viommu_type viommu_type,
+			       struct device *dev, size_t *viommu_size);
+	int (*viommu_init)(struct iommufd_viommu *viommu,
+			   struct iommu_domain *parent_domain);
 	struct iommufd_viommu *(*viommu_alloc)(
 		struct device *dev, struct iommu_domain *parent_domain,
 		struct iommufd_ctx *ictx, unsigned int viommu_type);

include/linux/iommufd.h +6 -0

index ac98e49e44fe..423e08963d90 100644
@@ -229,6 +229,12 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 }
 #endif /* CONFIG_IOMMUFD_DRIVER_CORE */
 
+#define VIOMMU_STRUCT_SIZE(drv_struct, member)                                 \
+	(sizeof(drv_struct) +                                                  \
+	 BUILD_BUG_ON_ZERO(offsetof(drv_struct, member)) +                     \
+	 BUILD_BUG_ON_ZERO(!__same_type(struct iommufd_viommu,                 \
+					((drv_struct *)NULL)->member)))
+
 /*
  * 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.

Signature · 1 line

2.43.0

Tian, Kevin

maintainer

2025-06-12 08:12 UTC #12 lore raw

RE: [PATCH v1 05/12] iommu: Introduce get_viommu_size and viommu_init ops

Quoted 9 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:13 AM
+ * @get_viommu_size: Get the size of a driver-level vIOMMU structure for a given
+ * @dev corresponding to @viommu_type. Driver should return
an
+ *                   errno if vIOMMU isn't supported accordingly. It is required
+ *                   for driver to use the VIOMMU_STRUCT_SIZE macro to sanitize
+ *                   a driver-level vIOMMU structure related to the core one

What about returning size with '0' indicating NOSUPPORT?

Nicolin Chen

author

2025-06-12 17:07 UTC #13 lore raw

Re: [PATCH v1 05/12] iommu: Introduce get_viommu_size and viommu_init ops

Quoted 11 lines · Kevin

Quoted 9 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:13 AM
+ * @get_viommu_size: Get the size of a driver-level vIOMMU structure for a given
+ * @dev corresponding to @viommu_type. Driver should return
an
+ *                   errno if vIOMMU isn't supported accordingly. It is required
+ *                   for driver to use the VIOMMU_STRUCT_SIZE macro to sanitize
+ *                   a driver-level vIOMMU structure related to the core one
What about returning size with '0' indicating NOSUPPORT?

I tend to support other errnos here. Basically this moves all the driver-level sanity checks from viommu_alloc to get_viommu_size, to reject an incompatible case immediately before allocating any structure. And there can be another errno than EOPNOTSUPP.

Thanks
Nicolin

Tian, Kevin

maintainer

2025-06-13 07:33 UTC #14 lore raw

RE: [PATCH v1 05/12] iommu: Introduce get_viommu_size and viommu_init ops

Quoted 15 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, June 13, 2025 1:07 AM
Quoted 3 lines · Kevin
Quoted 3 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:13 AM
+ * @get_viommu_size: Get the size of a driver-level vIOMMU structure
for a
Quoted 2 lines
Quoted 2 lines
given
+ * @dev corresponding to @viommu_type. Driver should
return
Quoted 3 lines
Quoted 3 lines
an
+ * errno if vIOMMU isn't supported accordingly. It is required
+ * for driver to use the VIOMMU_STRUCT_SIZE macro to
sanitize

Quoted 9 lines

Quoted 3 lines
Quoted 1 line
+ * a driver-level vIOMMU structure related to the core one
What about returning size with '0' indicating NOSUPPORT?
I tend to support other errnos here. Basically this moves all the driver-level sanity checks from viommu_alloc to get_viommu_size, to reject an incompatible case immediately before allocating any structure. And there can be another errno than EOPNOTSUPP.

Okay. Make sense.

Jason Gunthorpe

maintainer

2025-06-13 13:41 UTC #15 lore raw

Re: [PATCH v1 05/12] iommu: Introduce get_viommu_size and viommu_init ops

In reply to Nicolin Chen

include/linux/iommu.h struct iommu_ops { · line 669

Quoted 6 lines · Nicolin Chen

include/linux/iommu.h +2 -0

@@ -654,6 +665,10 @@ struct iommu_ops {
 
 	int (*def_domain_type)(struct device *dev);
 
+	int (*get_viommu_size)(enum iommu_viommu_type viommu_type,
+			       struct device *dev, size_t *viommu_size);

I'd return the size in a size_t instead of using an output pointer. Make 0 mean not supported..

Jason

Nicolin Chen

author

2025-06-13 20:45 UTC #16 lore raw

Re: [PATCH v1 05/12] iommu: Introduce get_viommu_size and viommu_init ops

Quoted 10 lines · Jason Gunthorpe

include/linux/iommu.h struct iommu_ops { · line 669
Quoted 6 lines · Nicolin Chen
include/linux/iommu.h +2 -0
@@ -654,6 +665,10 @@ struct iommu_ops {
 
 	int (*def_domain_type)(struct device *dev);
 
+	int (*get_viommu_size)(enum iommu_viommu_type viommu_type,
+			       struct device *dev, size_t *viommu_size);
I'd return the size in a size_t instead of using an output pointer. Make 0 mean not supported..

Yes. Kevin pointed out the same.

I found that the final version only has EOPNOTSUPP errno (had one of WIP versions reporting EINVAL). So, yea, doing a ssize_t would be cleaner and EOPNOTSUPP is probably enough. Will change in v2.

Thanks
Nicolin

Nicolin Chen

author

2025-06-09 17:14 UTC #17 lore raw

PATCH 6/12 iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init

In reply to Nicolin Chen

Sanitize the inputs and report the size of struct mock_viommu on success, in mock_get_viommu_size().

The core will ensure the viommu_type is set to the core vIOMMU object, so simply init the driver part in mock_viommu_init().

The mock_viommu_alloc() will be cleaned up once the transition is done.

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

---
 drivers/iommu/iommufd/selftest.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

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

index 4d5dca8027b1..b0de205a2303 100644
@@ -772,6 +772,29 @@ static struct iommufd_viommu_ops mock_viommu_ops = {
 	.cache_invalidate = mock_viommu_cache_invalidate,
 };
 
+static int mock_get_viommu_size(enum iommu_viommu_type viommu_type,
+				struct device *dev, size_t *viommu_size)
+{
+	if (viommu_type != IOMMU_VIOMMU_TYPE_SELFTEST)
+		return -EOPNOTSUPP;
+	*viommu_size = VIOMMU_STRUCT_SIZE(struct mock_viommu, core);
+	return 0;
+}
+
+static int mock_viommu_init(struct iommufd_viommu *viommu,
+			    struct iommu_domain *parent_domain)
+{
+	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);
+
+	refcount_inc(&mock_iommu->users);
+	mock_viommu->s2_parent = to_mock_domain(parent_domain);
+
+	viommu->ops = &mock_viommu_ops;
+	return 0;
+}
+
 static struct iommufd_viommu *mock_viommu_alloc(struct device *dev,
 						struct iommu_domain *domain,
 						struct iommufd_ctx *ictx,

@@ -810,6 +833,8 @@ static const struct iommu_ops mock_ops = {
 	.probe_device = mock_probe_device,
 	.page_response = mock_domain_page_response,
 	.user_pasid_table = true,
+	.get_viommu_size = mock_get_viommu_size,
+	.viommu_init = mock_viommu_init,
 	.viommu_alloc = mock_viommu_alloc,
 	.default_domain_ops =
 		&(struct iommu_domain_ops){

Signature · 1 line

2.43.0

Tian, Kevin

maintainer Reviewed-by on v1 6/12

2025-06-12 08:17 UTC #18 lore raw

RE: [PATCH v1 06/12] iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:13 AM
Sanitize the inputs and report the size of struct mock_viommu on success, in mock_get_viommu_size().

Quoted 6 lines

The core will ensure the viommu_type is set to the core vIOMMU object, so simply init the driver part in mock_viommu_init().
The mock_viommu_alloc() will be cleaned up once the transition is done.
Signed-off-by Nicolin Chen <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

btw I didn't see where mock_viommu->s2_parent is set in the original code. Is it a bug or oversight?

Nicolin Chen

author

2025-06-12 17:12 UTC #19 lore raw

Re: [PATCH v1 06/12] iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init

Quoted 9 lines · Kevin

Quoted 9 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:13 AM
Sanitize the inputs and report the size of struct mock_viommu on success, in mock_get_viommu_size().
The core will ensure the viommu_type is set to the core vIOMMU object, so simply init the driver part in mock_viommu_init().

Quoted 8 lines

Quoted 3 lines
The mock_viommu_alloc() will be cleaned up once the transition is done.
Signed-off-by Nicolin Chen <[email protected]>
Reviewed-by Kevin Tian <[email protected]>
btw I didn't see where mock_viommu->s2_parent is set in the original code. Is it a bug or oversight?

Looks like that was missing.. But it shouldn't trigger any bug since mock_nested->parent doesn't have any meaningful value in the code..

Perhaps we should think of adding some use case out of it. Or, we'd need to clean it away..

Thanks
Nicolin

Tian, Kevin

maintainer

2025-06-13 07:34 UTC #20 lore raw

RE: [PATCH v1 06/12] iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init

Quoted 23 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, June 13, 2025 1:12 AM
Quoted 7 lines · Kevin
Quoted 7 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:13 AM
Sanitize the inputs and report the size of struct mock_viommu on success, in mock_get_viommu_size().
The core will ensure the viommu_type is set to the core vIOMMU object,
so
Quoted 10 lines
Quoted 5 lines
simply init the driver part in mock_viommu_init().
The mock_viommu_alloc() will be cleaned up once the transition is done.
Signed-off-by Nicolin Chen <[email protected]>
Reviewed-by Kevin Tian <[email protected]>
btw I didn't see where mock_viommu->s2_parent is set in the original code. Is it a bug or oversight?

Quoted 7 lines

Looks like that was missing.. But it shouldn't trigger any bug since mock_nested->parent doesn't have any meaningful value in the code..
Perhaps we should think of adding some use case out of it. Or, we'd need to clean it away..

Then it's kind of dead code. If we cannot find meaningful use then better to remove it.

Jason Gunthorpe

maintainer

2025-06-13 13:45 UTC #21 lore raw

Re: [PATCH v1 06/12] iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init

In reply to Nicolin Chen

Quoted 7 lines · Nicolin Chen

Sanitize the inputs and report the size of struct mock_viommu on success, in mock_get_viommu_size().
The core will ensure the viommu_type is set to the core vIOMMU object, so simply init the driver part in mock_viommu_init().
The mock_viommu_alloc() will be cleaned up once the transition is done.

Note the addition of the missed s2_parent store

Quoted 25 lines

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

---
 drivers/iommu/iommufd/selftest.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

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

index 4d5dca8027b1..b0de205a2303 100644
@@ -772,6 +772,29 @@ static struct iommufd_viommu_ops mock_viommu_ops = {
 	.cache_invalidate = mock_viommu_cache_invalidate,
 };
 
+static int mock_get_viommu_size(enum iommu_viommu_type viommu_type,
+				struct device *dev, size_t *viommu_size)
+{
+	if (viommu_type != IOMMU_VIOMMU_TYPE_SELFTEST)
+		return -EOPNOTSUPP;
+	*viommu_size = VIOMMU_STRUCT_SIZE(struct mock_viommu, core);
+	return 0;
+}
+
+static int mock_viommu_init(struct iommufd_viommu *viommu,
+			    struct iommu_domain *parent_domain)
+{

drivers/iommu/iommufd/selftest.c static struct iommufd_viommu_ops mock_viommu_ops = { · line 796

Quoted 10 lines

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

+	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);
+
+	refcount_inc(&mock_iommu->users);
+	mock_viommu->s2_parent = to_mock_domain(parent_domain);
+
+	viommu->ops = &mock_viommu_ops;
+	return 0;
+}

The patches will read better if you add the call logic for init along side alloc based on init or alloc ops being non-NULL in the prior patch and then have these driver patches replace alloc with init.

Duplicating alloc into init and leaving both makes the patch harder to check.

But the change looks right

Jason

Nicolin Chen

author

2025-06-13 20:20 UTC #22 lore raw

Re: [PATCH v1 06/12] iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init

Quoted 10 lines · Jason Gunthorpe

Quoted 7 lines · Nicolin Chen
Sanitize the inputs and report the size of struct mock_viommu on success, in mock_get_viommu_size().
The core will ensure the viommu_type is set to the core vIOMMU object, so simply init the driver part in mock_viommu_init().
The mock_viommu_alloc() will be cleaned up once the transition is done.
Note the addition of the missed s2_parent store

I am adding a new patch in prep-v2 dropping the unused s2_parent:

-------------------------------------------------------------------
iommufd/selftest: Drop parent domain from mock_iommu_domain_nested

This is no use of this parent domain. Delete the dead code.

Note that the s2_parent in struct mock_viommu will be a deadcode too. Yet, keep it because it will be soon used by HW queue objects, i.e. no point in adding it back and forth in such a short window. Besides, keeping it could cover the majority of vIOMMU use cases where a driver-level structure will be larger in size than the core structure.

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

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

In this case, we don't need to assign mock_viommu->s2_parent here since it's a dead code. The HW queue series will assign it and add a use case:

+/* Test iommufd_hw_queue_depend/undepend() */
+static int mock_hw_queue_init_phys(struct iommufd_hw_queue *hw_queue, u32 index,
+ phys_addr_t base_addr_pa)
...
+ /*

+        * Test to catch a kernel bug if the core converted the physical address
+        * incorrectly. Let mock_domain_iova_to_phys() WARN_ON if it fails.
+        */

+ if (base_addr_pa != iommu_iova_to_phys(&mock_viommu->s2_parent->domain,
+ hw_queue->base_addr)) {
+ rc = -EFAULT;
+ goto unlock;
+ }

Quoted 18 lines

drivers/iommu/iommufd/selftest.c static struct iommufd_viommu_ops mock_viommu_ops = { · line 796
Quoted 13 lines
drivers/iommu/iommufd/selftest.c +13 -0
+static int mock_viommu_init(struct iommufd_viommu *viommu,
+			    struct iommu_domain *parent_domain)
+{
+	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);
+
+	refcount_inc(&mock_iommu->users);
+	mock_viommu->s2_parent = to_mock_domain(parent_domain);
+
+	viommu->ops = &mock_viommu_ops;
+	return 0;
+}
The patches will read better if you add the call logic for init along side alloc based on init or alloc ops being non-NULL in the prior patch and then have these driver patches replace alloc with init.

Quoted 2 lines

Duplicating alloc into init and leaving both makes the patch harder to check.

I see. That will add an additional patch tentatively supporting both ops.

Thanks
Nicolin

Jason Gunthorpe

maintainer

2025-06-13 23:23 UTC #23 lore raw

Re: [PATCH v1 06/12] iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init

Quoted 9 lines · Nicolin Chen

Quoted 6 lines
The patches will read better if you add the call logic for init along side alloc based on init or alloc ops being non-NULL in the prior patch and then have these driver patches replace alloc with init.
Duplicating alloc into init and leaving both makes the patch harder to check.
I see. That will add an additional patch tentatively supporting both ops.

An additional patch is not needed, just add calls to the ops in the same patch where you add the ops.

Then the patch that removes the old ops removes the extra logic deciding which set to call. This is the preferred way to do driver conversions.

Jason

Nicolin Chen

author

2025-06-13 23:37 UTC #24 lore raw

Re: [PATCH v1 06/12] iommufd/selftest: Implement mock_get_viommu_size and mock_viommu_init

Quoted 5 lines · Jason Gunthorpe

Quoted 4 lines · Nicolin Chen
Quoted 4 lines
The patches will read better if you add the call logic for init along side alloc based on init or alloc ops being non-NULL in the prior patch and then have these driver patches replace alloc with init.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Duplicating alloc into init and leaving both makes the patch harder to check.
I see. That will add an additional patch tentatively supporting both ops.
An additional patch is not needed, just add calls to the ops in the same patch where you add the ops.

I have a long writing in that iommu ops patch, as Don requested per an offline email. So separating them would be a bit cleaner.

Yet, I am merging two cleanup patches (viommu.c + iommu.h):

iommu: Introduce get_viommu_size and viommu_init ops iommufd/viommu: Support get_viommu_size and viommu_init ops iommufd/selftest: Drop parent domain from mock_iommu_domain_nested iommufd/selftest: Replace mock_viommu_alloc with mock_viommu_init iommu/arm-smmu-v3: Replace arm_vsmmu_alloc with arm_vsmmu_init
iommu: Deprecate viommu_alloc op

Net-net will be the same. And the driver patch has viommu_alloc and viommu_init parts side by side.

Quoted 3 lines

Then the patch that removes the old ops removes the extra logic deciding which set to call. This is the preferred way to do driver conversions.

Yes. I made a very small diff in the last patch:

@@ -33,8 +33,6 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)

        ops = dev_iommu_ops(idev->dev);
        if (!ops->get_viommu_size || !ops->viommu_init) {
-               if (ops->viommu_alloc)
-                       goto get_hwpt_paging;
                rc = -EOPNOTSUPP;
                goto out_put_idev;
        }
@@ -54,7 +52,6 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
                goto out_put_idev;
        }

-get_hwpt_paging:

        hwpt_paging = iommufd_get_hwpt_paging(ucmd, cmd->hwpt_id);
        if (IS_ERR(hwpt_paging)) {
                rc = PTR_ERR(hwpt_paging);
@@ -66,13 +63,8 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
                goto out_put_hwpt;
        }

- if (!ops->viommu_init)
- viommu = ops->viommu_alloc(idev->dev,
- hwpt_paging->common.domain,
- ucmd->ictx, cmd->type);
- else
- viommu = (struct iommufd_viommu *)_iommufd_object_alloc(
- ucmd->ictx, viommu_size, IOMMUFD_OBJ_VIOMMU);
+ viommu = (struct iommufd_viommu *)_iommufd_object_alloc(

+               ucmd->ictx, viommu_size, IOMMUFD_OBJ_VIOMMU);
        if (IS_ERR(viommu)) {
                rc = PTR_ERR(viommu);
                goto out_put_hwpt;

Thanks!
Nicolin

Nicolin Chen

author

2025-06-09 17:14 UTC #25 lore raw

PATCH 7/12 iommu/arm-smmu-v3: Implement arm_smmu_get_viommu_size and arm_vsmmu_init

In reply to Nicolin Chen

Sanitize the inputs and report the size of struct arm_vsmmu on success, in arm_smmu_get_viommu_size().

The core will ensure the viommu_type is set to the core vIOMMU object, and pass in the same dev pointer, so arm_vsmmu_init() won't need to repeat the same sanity tests but to simply init the arm_vsmmu struct.

The arm_vsmmu_alloc() will be cleaned up once the transition is done.

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  6 ++
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 56 +++++++++++++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  2 +
 3 files changed, 64 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +6 -0

index ea41d790463e..2357459099f4 100644
@@ -1034,6 +1034,10 @@ struct arm_vsmmu {
 
 #if IS_ENABLED(CONFIG_ARM_SMMU_V3_IOMMUFD)
 void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type);
+int arm_smmu_get_viommu_size(enum iommu_viommu_type viommu_type,
+			     struct device *dev, size_t *viommu_size);
+int arm_vsmmu_init(struct iommufd_viommu *viommu,
+		   struct iommu_domain *parent_domain);
 struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 				       struct iommu_domain *parent,
 				       struct iommufd_ctx *ictx,

@@ -1044,8 +1048,10 @@ 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);
 #else
+#define arm_smmu_get_viommu_size NULL
 #define arm_smmu_hw_info NULL
 #define arm_vsmmu_alloc NULL
+#define arm_vsmmu_init NULL
 
 static inline int
 arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +56 -0

62 lines not on this page · raw message

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -0

index 10cc6dc26b7b..eef1fbd68914 100644
@@ -3688,6 +3688,8 @@ static struct iommu_ops arm_smmu_ops = {
 	.get_resv_regions	= arm_smmu_get_resv_regions,
 	.page_response		= arm_smmu_page_response,
 	.def_domain_type	= arm_smmu_def_domain_type,
+	.get_viommu_size	= arm_smmu_get_viommu_size,
+	.viommu_init		= arm_vsmmu_init,
 	.viommu_alloc		= arm_vsmmu_alloc,
 	.user_pasid_table	= 1,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */

Signature · 1 line

2.43.0

Tian, Kevin

maintainer

2025-06-12 08:20 UTC #26 lore raw

RE: [PATCH v1 07/12] iommu/arm-smmu-v3: Implement arm_smmu_get_viommu_size and arm_vsmmu_init

Quoted 34 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:14 AM
+int arm_smmu_get_viommu_size(enum iommu_viommu_type viommu_type,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +10 -0
+			     struct device *dev, size_t *viommu_size)
+{
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_device *smmu = master->smmu;
+
+	if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
+		return -EOPNOTSUPP;
+
+	/*
+	 * FORCE_SYNC is not set with FEAT_NESTING. Some study of the
exact HW
(quoted diff) +1 -0
+	 * defect is needed to determine if arm_vsmmu_cache_invalidate()
needs
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +3 -0
+	 * any change to remove this.
+	 */
+	if (WARN_ON(smmu->options &
ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +4 -0
+		return -EOPNOTSUPP;
+
+	/*
+	 * Must support some way to prevent the VM from bypassing the
cache
(quoted diff) +1 -0
+	 * because VFIO currently does not do any cache maintenance.
canwbs
(quoted diff) +2 -0
+	 * indicates the device is fully coherent and no cache maintenance is
+	 * ever required, even for PCI No-Snoop. S2FWB means the S1 can't
make
(quoted diff) +1 -0
+	 * things non-coherent using the memattr, but No-Snoop behavior is
not

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c static const struct iommufd_viommu_ops arm_vsmmu_ops = { · line 415

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +8 -0

+	 * effected.
+	 */
+	if (!arm_smmu_master_canwbs(master) &&
+	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
+		return -EOPNOTSUPP;
+
+	if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
+		return -EOPNOTSUPP;

it's more intuitive to check it first.

btw does it make sense to also add below here?

	if (s2_parent->smmu != master->smmu)
		return ERR_PTR(-EINVAL);

Nicolin Chen

author

2025-06-12 17:18 UTC #27 lore raw

Re: [PATCH v1 07/12] iommu/arm-smmu-v3: Implement arm_smmu_get_viommu_size and arm_vsmmu_init

Quoted 36 lines · Kevin

Quoted 36 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:14 AM
+int arm_smmu_get_viommu_size(enum iommu_viommu_type viommu_type,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +10 -0
+			     struct device *dev, size_t *viommu_size)
+{
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_device *smmu = master->smmu;
+
+	if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
+		return -EOPNOTSUPP;
+
+	/*
+	 * FORCE_SYNC is not set with FEAT_NESTING. Some study of the
exact HW
(quoted diff) +1 -0
+	 * defect is needed to determine if arm_vsmmu_cache_invalidate()
needs
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +3 -0
+	 * any change to remove this.
+	 */
+	if (WARN_ON(smmu->options &
ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +4 -0
+		return -EOPNOTSUPP;
+
+	/*
+	 * Must support some way to prevent the VM from bypassing the
cache
(quoted diff) +1 -0
+	 * because VFIO currently does not do any cache maintenance.
canwbs
(quoted diff) +2 -0
+	 * indicates the device is fully coherent and no cache maintenance is
+	 * ever required, even for PCI No-Snoop. S2FWB means the S1 can't
make
(quoted diff) +1 -0
+	 * things non-coherent using the memattr, but No-Snoop behavior is
not
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +2 -0
+	 * effected.
+	 */

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c static const struct iommufd_viommu_ops arm_vsmmu_ops = { · line 415
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +6 -0
+	if (!arm_smmu_master_canwbs(master) &&
+	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
+		return -EOPNOTSUPP;
+
+	if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
+		return -EOPNOTSUPP;
it's more intuitive to check it first.

Agreed. But I kinda intentionally left it here. The SMMU driver
will have something like an impl_op->get_viommu_size in the HW
queue series. That can simply insert a piece:
===============================================================
@@ -415,6 +415,12 @@ int arm_smmu_get_viommu_size(enum iommu_viommu_type viommu_type,

            !(smmu->features & ARM_SMMU_FEAT_S2FWB))
                return -EOPNOTSUPP;

+ if (smmu->impl_ops && smmu->impl_ops->vsmmu_size &&
+ viommu_type == smmu->impl_ops->vsmmu_type) {
+ *viommu_size = smmu->impl_ops->vsmmu_size;
+ return 0;
+ }
+

        if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
                return -EOPNOTSUPP;

===============================================================

Otherwise, this following patch has to move the type check again.

Quoted 3 lines

btw does it make sense to also add below here?
	if (s2_parent->smmu != master->smmu)
		return ERR_PTR(-EINVAL);

I can't find a legit reason to forward the s2_parent to run this sanity. "struct device *" is forwarded since the driver needs to know the smmu pointer: A for the compatibility checks; b for the smmu->impl_ops mentioned above.

Thanks
Nicolin

Tian, Kevin

maintainer

2025-06-13 07:36 UTC #28 lore raw

RE: [PATCH v1 07/12] iommu/arm-smmu-v3: Implement arm_smmu_get_viommu_size and arm_vsmmu_init

Quoted 67 lines

From: Nicolin Chen <[email protected]>
Sent: Friday, June 13, 2025 1:18 AM
Quoted 36 lines · Kevin
Quoted 36 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:14 AM
+int arm_smmu_get_viommu_size(enum iommu_viommu_type viommu_type,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +10 -0
+			     struct device *dev, size_t *viommu_size)
+{
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_device *smmu = master->smmu;
+
+	if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
+		return -EOPNOTSUPP;
+
+	/*
+	 * FORCE_SYNC is not set with FEAT_NESTING. Some study of the
exact HW
(quoted diff) +1 -0
+	 * defect is needed to determine if arm_vsmmu_cache_invalidate()
needs
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +3 -0
+	 * any change to remove this.
+	 */
+	if (WARN_ON(smmu->options &
ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +4 -0
+		return -EOPNOTSUPP;
+
+	/*
+	 * Must support some way to prevent the VM from bypassing the
cache
(quoted diff) +1 -0
+	 * because VFIO currently does not do any cache maintenance.
canwbs
(quoted diff) +2 -0
+	 * indicates the device is fully coherent and no cache maintenance is
+	 * ever required, even for PCI No-Snoop. S2FWB means the S1 can't
make
(quoted diff) +1 -0
+	 * things non-coherent using the memattr, but No-Snoop behavior is
not
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +2 -0
+	 * effected.
+	 */
Quoted 8 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c static const struct iommufd_viommu_ops arm_vsmmu_ops = { · line 415
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +6 -0
+	if (!arm_smmu_master_canwbs(master) &&
+	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
+		return -EOPNOTSUPP;
+
+	if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
+		return -EOPNOTSUPP;
it's more intuitive to check it first.
Agreed. But I kinda intentionally left it here. The SMMU driver will have something like an impl_op->get_viommu_size in the HW queue series. That can simply insert a piece:
===============================================================
(quoted diff) +6 -0
@@ -415,6 +415,12 @@ int arm_smmu_get_viommu_size(enum
 iommu_viommu_type viommu_type,
            !(smmu->features & ARM_SMMU_FEAT_S2FWB))
                return -EOPNOTSUPP;
 
+       if (smmu->impl_ops && smmu->impl_ops->vsmmu_size &&
+           viommu_type == smmu->impl_ops->vsmmu_type) {
+               *viommu_size = smmu->impl_ops->vsmmu_size;
+               return 0;
+       }
+
        if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
                return -EOPNOTSUPP;

Quoted 1 line

===============================================================

Oh I see.

Quoted 12 lines

Otherwise, this following patch has to move the type check again.
Quoted 3 lines
btw does it make sense to also add below here?
	if (s2_parent->smmu != master->smmu)
		return ERR_PTR(-EINVAL);
I can't find a legit reason to forward the s2_parent to run this sanity. "struct device *" is forwarded since the driver needs to know the smmu pointer: A for the compatibility checks; b for the smmu->impl_ops mentioned above.

yes

Nicolin Chen

author

2025-06-09 17:14 UTC #29 lore raw

PATCH 8/12 iommufd/viommu: Replace ops->viommu_alloc with ops->viommu_init

In reply to Nicolin Chen

To ease the for-driver iommufd APIs, get_viommu_size and viommu_init ops are introduced. Now, those existing vIOMMU supported drivers implemented these two ops too.

Relace the ops->viommu_alloc call with the two new ones.

Note that this will fail a !viommu->ops case from now on, since a vIOMMU is expected to support alloc_domain_nested at least.

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

---
 drivers/iommu/iommufd/viommu.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

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

index 01df2b985f02..63a92fb27ef4 100644
@@ -21,6 +21,7 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	struct iommufd_viommu *viommu;
 	struct iommufd_device *idev;
 	const struct iommu_ops *ops;
+	size_t viommu_size;
 	int rc;
 
 	if (cmd->flags || cmd->type == IOMMU_VIOMMU_TYPE_DEFAULT)

@@ -31,11 +32,24 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		return PTR_ERR(idev);
 
 	ops = dev_iommu_ops(idev->dev);
-	if (!ops->viommu_alloc) {
+	if (!ops->get_viommu_size || !ops->viommu_init) {
 		rc = -EOPNOTSUPP;
 		goto out_put_idev;
 	}
 
+	rc = ops->get_viommu_size(cmd->type, idev->dev, &viommu_size);
+	if (rc)
+		goto out_put_idev;
+
+	/*
+	 * It is a driver bug for providing a viommu_size smaller than the core
+	 * vIOMMU structure size
+	 */
+	if (WARN_ON_ONCE(viommu_size < sizeof(*viommu))) {
+		rc = -EINVAL;
+		goto out_put_idev;
+	}
+
 	hwpt_paging = iommufd_get_hwpt_paging(ucmd, cmd->hwpt_id);
 	if (IS_ERR(hwpt_paging)) {
 		rc = PTR_ERR(hwpt_paging);

@@ -47,8 +61,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 = (struct iommufd_viommu *)_iommufd_object_alloc(
+		ucmd->ictx, viommu_size, IOMMUFD_OBJ_VIOMMU);
 	if (IS_ERR(viommu)) {
 		rc = PTR_ERR(viommu);
 		goto out_put_hwpt;

@@ -68,6 +82,16 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	 */
 	viommu->iommu_dev = __iommu_get_iommu_dev(idev->dev);
 
+	rc = ops->viommu_init(viommu, hwpt_paging->common.domain);
+	if (rc)
+		goto out_abort;
+
+	/* It is a driver bug that viommu->ops isn't filled */
+	if (WARN_ON_ONCE(!viommu->ops)) {
+		rc = -EINVAL;
+		goto out_abort;
+	}
+
 	cmd->out_viommu_id = viommu->obj.id;
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 	if (rc)

Signature · 1 line

2.43.0

Baolu Lu

2025-06-10 05:56 UTC #30 lore raw

Re: [PATCH v1 08/12] iommufd/viommu: Replace ops->viommu_alloc with ops->viommu_init

Quoted 8 lines · Nicolin Chen

To ease the for-driver iommufd APIs, get_viommu_size and viommu_init ops are introduced. Now, those existing vIOMMU supported drivers implemented these two ops too.
Relace the ops->viommu_alloc call with the two new ones.
Note that this will fail a !viommu->ops case from now on, since a vIOMMU is expected to support alloc_domain_nested at least.

Does this mean that the viommu implementation in the iommu driver is required to implement alloc_domain_nested? I suppose viommu should soon be extended to support TEE/IO.

Quoted 7 lines

Suggested-by Jason Gunthorpe<[email protected]>
Signed-off-by Nicolin Chen<[email protected]>
---
  drivers/iommu/iommufd/viommu.c | 30 +++++++++++++++++++++++++++---
  1 file changed, 27 insertions(+), 3 deletions(-)

<...>

Quoted 7 lines

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

@@ -68,6 +82,16 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
  	 */
  	viommu->iommu_dev = __iommu_get_iommu_dev(idev->dev);
 
+	rc = ops->viommu_init(viommu, hwpt_paging->common.domain);
+	if (rc)
+		goto out_abort;

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

Quoted 10 lines

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

+
+	/* It is a driver bug that viommu->ops isn't filled */
+	if (WARN_ON_ONCE(!viommu->ops)) {
+		rc = -EINVAL;
+		goto out_abort;
+	}
+
  	cmd->out_viommu_id = viommu->obj.id;
  	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
  	if (rc)

Thanks,
baolu

Nicolin Chen

author

2025-06-10 06:20 UTC #31 lore raw

Re: [PATCH v1 08/12] iommufd/viommu: Replace ops->viommu_alloc with ops->viommu_init

Quoted 5 lines · Baolu Lu

Quoted 4 lines · Nicolin Chen
To ease the for-driver iommufd APIs, get_viommu_size and viommu_init ops are introduced. Now, those existing vIOMMU supported drivers implemented these two ops too.

Quoted 8 lines

Quoted 4 lines
Relace the ops->viommu_alloc call with the two new ones.
Note that this will fail a !viommu->ops case from now on, since a vIOMMU is expected to support alloc_domain_nested at least.
Does this mean that the viommu implementation in the iommu driver is required to implement alloc_domain_nested? I suppose viommu should soon be extended to support TEE/IO.

It's a good point that CCA might not need a nested domain. So, it's inaccurate to say that, although I suspect that CCA would need some other viommu op then the check here would be sane.

With that being said, it's probably not worth adding that until we are 100% sure that no case will work with a !viommu->ops, so let's drop this new rejection, since we haven't had it so far.

Thanks
Nicolin

Tian, Kevin

maintainer Reviewed-by

2025-06-12 08:23 UTC #32 lore raw

RE: [PATCH v1 08/12] iommufd/viommu: Replace ops->viommu_alloc with ops->viommu_init

Quoted 22 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 2:20 PM
Quoted 2 lines · Baolu Lu
Quoted 1 line · Nicolin Chen
To ease the for-driver iommufd APIs, get_viommu_size and viommu_init
ops
Quoted 1 line
Quoted 1 line
are introduced. Now, those existing vIOMMU supported drivers
implemented
Quoted 5 lines
Quoted 5 lines
these two ops too.
Relace the ops->viommu_alloc call with the two new ones.
Note that this will fail a !viommu->ops case from now on, since a
vIOMMU
Quoted 4 lines
Quoted 1 line
is expected to support alloc_domain_nested at least.
Does this mean that the viommu implementation in the iommu driver is required to implement alloc_domain_nested? I suppose viommu should
soon
Quoted 1 line
be extended to support TEE/IO.

Quoted 8 lines

It's a good point that CCA might not need a nested domain. So, it's inaccurate to say that, although I suspect that CCA would need some other viommu op then the check here would be sane.
With that being said, it's probably not worth adding that until we are 100% sure that no case will work with a !viommu->ops, so let's drop this new rejection, since we haven't had it so far.

WARN_ON_ONCE() is built on the current context, not the future usage. So I'd prefer to keeping it until there is a real need to revert.

Reviewed-by Kevin Tian <[email protected]>

Nicolin Chen

author

2025-06-12 17:36 UTC #33 lore raw

Re: [PATCH v1 08/12] iommufd/viommu: Replace ops->viommu_alloc with ops->viommu_init

Quoted 31 lines · Kevin

Quoted 22 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 2:20 PM
Quoted 2 lines · Baolu Lu
Quoted 1 line · Nicolin Chen
To ease the for-driver iommufd APIs, get_viommu_size and viommu_init
ops
Quoted 1 line
Quoted 1 line
are introduced. Now, those existing vIOMMU supported drivers
implemented
Quoted 5 lines
Quoted 5 lines
these two ops too.
Relace the ops->viommu_alloc call with the two new ones.
Note that this will fail a !viommu->ops case from now on, since a
vIOMMU
Quoted 4 lines
Quoted 1 line
is expected to support alloc_domain_nested at least.
Does this mean that the viommu implementation in the iommu driver is required to implement alloc_domain_nested? I suppose viommu should
soon
Quoted 1 line
be extended to support TEE/IO.
Quoted 8 lines
It's a good point that CCA might not need a nested domain. So, it's inaccurate to say that, although I suspect that CCA would need some other viommu op then the check here would be sane.
With that being said, it's probably not worth adding that until we are 100% sure that no case will work with a !viommu->ops, so let's drop this new rejection, since we haven't had it so far.

Quoted 4 lines

WARN_ON_ONCE() is built on the current context, not the future usage. So I'd prefer to keeping it until there is a real need to revert.
Reviewed-by Kevin Tian <[email protected]>

Okay. And I can make the commit message clearer.

Thanks
Nicolin

Tian, Kevin

maintainer

2025-06-12 08:27 UTC #34 lore raw

RE: [PATCH v1 08/12] iommufd/viommu: Replace ops->viommu_alloc with ops->viommu_init

In reply to Nicolin Chen

Quoted 6 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:14 AM
drivers/iommu/iommufd/viommu.c +3 -0
+	rc = ops->get_viommu_size(cmd->type, idev->dev, &viommu_size);
+	if (rc)
+		goto out_put_idev;

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

Quoted 10 lines

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

+
+	/*
+	 * It is a driver bug for providing a viommu_size smaller than the core
+	 * vIOMMU structure size
+	 */
+	if (WARN_ON_ONCE(viommu_size < sizeof(*viommu))) {
+		rc = -EINVAL;
+		goto out_put_idev;
+	}
+

It's not about user providing an invalid argument. Sounds cleaner to return NOSUPPORT in such case.

Nicolin Chen

author

2025-06-12 17:25 UTC #35 lore raw

Re: [PATCH v1 08/12] iommufd/viommu: Replace ops->viommu_alloc with ops->viommu_init

Quoted 19 lines · Kevin

Quoted 6 lines
From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:14 AM
drivers/iommu/iommufd/viommu.c +3 -0
+	rc = ops->get_viommu_size(cmd->type, idev->dev, &viommu_size);
+	if (rc)
+		goto out_put_idev;
drivers/iommu/iommufd/viommu.c int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 52
Quoted 10 lines
drivers/iommu/iommufd/viommu.c +10 -0
+
+	/*
+	 * It is a driver bug for providing a viommu_size smaller than the core
+	 * vIOMMU structure size
+	 */
+	if (WARN_ON_ONCE(viommu_size < sizeof(*viommu))) {
+		rc = -EINVAL;
+		goto out_put_idev;
+	}
+
It's not about user providing an invalid argument. Sounds cleaner to return NOSUPPORT in such case.

Yea, I think that's legit. The driver is broken to support vIOMMU allocations.

Maybe we should change this one too?
226 static struct iommufd_hwpt_nested *
227 iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,

228                           struct iommufd_hwpt_paging *parent,
229                           struct iommufd_device *idev, u32 flags,
230                           const struct iommu_user_data *user_data)

231 {
...
266 if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
267 rc = -EINVAL;
268 goto out_abort;
269 }

Similarly, the driver is broken to support nested HWPT allocations.

Thanks
Nicolin

Nicolin Chen

author

2025-06-09 17:14 UTC #36 lore raw

PATCH 9/12 iommu: Deprecate viommu_alloc op

In reply to Nicolin Chen

Now, iommufd uses the get_viommu_size and viommu_init ops instead.

Remove the iommu_alloc op and all the drivers implementaitons.

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

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  5 --
 include/linux/iommu.h                         | 11 ----
 include/linux/iommufd.h                       | 18 -------
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 53 -------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
 drivers/iommu/iommufd/selftest.c              | 22 --------
 6 files changed, 110 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0 -5

index 2357459099f4..3f5e3db7ae1c 100644
@@ -1038,10 +1038,6 @@ int arm_smmu_get_viommu_size(enum iommu_viommu_type viommu_type,
 			     struct device *dev, size_t *viommu_size);
 int arm_vsmmu_init(struct iommufd_viommu *viommu,
 		   struct iommu_domain *parent_domain);
-struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
-				       struct iommu_domain *parent,
-				       struct iommufd_ctx *ictx,
-				       unsigned int viommu_type);
 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);

@@ -1050,7 +1046,6 @@ int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt);
 #else
 #define arm_smmu_get_viommu_size NULL
 #define arm_smmu_hw_info NULL
-#define arm_vsmmu_alloc NULL
 #define arm_vsmmu_init NULL
 
 static inline int

include/linux/iommu.h +0 ~~-11~~

index e42a28971182..6389bb1915dc 100644
@@ -607,14 +607,6 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
  *               resources shared/passed to user space IOMMU instance. Associate
  *               it with a nesting @parent_domain. It is required for driver to
  *               set @viommu->ops pointing to its own viommu_ops
- * @viommu_alloc: Allocate an iommufd_viommu on a physical IOMMU instance behind
- *                the @dev, as the set of virtualization resources shared/passed
- *                to user space IOMMU instance. And associate it with a nesting
- *                @parent_domain. The @viommu_type must be defined in the header
- *                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.
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  * @owner: Driver module providing these ops
  * @identity_domain: An always available, always attachable identity

@@ -669,9 +661,6 @@ struct iommu_ops {
 			       struct device *dev, size_t *viommu_size);
 	int (*viommu_init)(struct iommufd_viommu *viommu,
 			   struct iommu_domain *parent_domain);
-	struct iommufd_viommu *(*viommu_alloc)(
-		struct device *dev, struct iommu_domain *parent_domain,
-		struct iommufd_ctx *ictx, unsigned int viommu_type);
 
 	const struct iommu_domain_ops *default_domain_ops;
 	unsigned long pgsize_bitmap;

include/linux/iommufd.h +0 ~~-18~~

index 423e08963d90..bf41b242b9f6 100644
@@ -234,22 +234,4 @@ static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 	 BUILD_BUG_ON_ZERO(offsetof(drv_struct, member)) +                     \
 	 BUILD_BUG_ON_ZERO(!__same_type(struct iommufd_viommu,                 \
 					((drv_struct *)NULL)->member)))
-
-/*
- * 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)             \
-	({                                                                     \
-		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);         \
-		if (!IS_ERR(ret))                                              \
-			ret->member.ops = viommu_ops;                          \
-		ret;                                                           \
-	})
 #endif

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +0 ~~-53~~

59 lines not on this page · raw message

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -1

index eef1fbd68914..181d07bc1a9d 100644
@@ -3690,7 +3690,6 @@ static struct iommu_ops arm_smmu_ops = {
 	.def_domain_type	= arm_smmu_def_domain_type,
 	.get_viommu_size	= arm_smmu_get_viommu_size,
 	.viommu_init		= arm_vsmmu_init,
-	.viommu_alloc		= arm_vsmmu_alloc,
 	.user_pasid_table	= 1,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 	.owner			= THIS_MODULE,

drivers/iommu/iommufd/selftest.c +0 ~~-22~~

index b0de205a2303..2f072f1b1dad 100644
@@ -795,27 +795,6 @@ static int mock_viommu_init(struct iommufd_viommu *viommu,
 	return 0;
 }
 
-static struct iommufd_viommu *mock_viommu_alloc(struct device *dev,
-						struct iommu_domain *domain,
-						struct iommufd_ctx *ictx,
-						unsigned int viommu_type)
-{
-	struct mock_iommu_device *mock_iommu =
-		iommu_get_iommu_dev(dev, struct mock_iommu_device, iommu_dev);
-	struct mock_viommu *mock_viommu;
-
-	if (viommu_type != IOMMU_VIOMMU_TYPE_SELFTEST)
-		return ERR_PTR(-EOPNOTSUPP);
-
-	mock_viommu = iommufd_viommu_alloc(ictx, struct mock_viommu, core,
-					   &mock_viommu_ops);
-	if (IS_ERR(mock_viommu))
-		return ERR_CAST(mock_viommu);
-
-	refcount_inc(&mock_iommu->users);
-	return &mock_viommu->core;
-}
-
 static const struct iommu_ops mock_ops = {
 	/*
 	 * IOMMU_DOMAIN_BLOCKED cannot be returned from def_domain_type()

@@ -835,7 +814,6 @@ static const struct iommu_ops mock_ops = {
 	.user_pasid_table = true,
 	.get_viommu_size = mock_get_viommu_size,
 	.viommu_init = mock_viommu_init,
-	.viommu_alloc = mock_viommu_alloc,
 	.default_domain_ops =
 		&(struct iommu_domain_ops){
 			.free = mock_domain_free,

Signature · 1 line

2.43.0

Tian, Kevin

maintainer Reviewed-by on v1 9/12

2025-06-12 08:23 UTC #37 lore raw

RE: [PATCH v1 09/12] iommu: Deprecate viommu_alloc op

Quoted 8 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:14 AM
Now, iommufd uses the get_viommu_size and viommu_init ops instead.
Remove the iommu_alloc op and all the drivers implementaitons.
Signed-off-by Nicolin Chen <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Nicolin Chen

author

2025-06-09 17:14 UTC #38 lore raw

PATCH 10/12 iommufd: Move _iommufd_object_alloc out of driver.c

In reply to Nicolin Chen

Now, all driver structures will be allocated by the core, i.e. no longer a need of driver calling _iommufd_object_alloc. Thus, move it back.

Before:
text data bss dec hex filename

   3024	    180	      0	   3204	    c84	drivers/iommu/iommufd/driver.o
   9074	    610	     64	   9748	   2614	drivers/iommu/iommufd/main.o

After:
text data bss dec hex filename
2665 164 0 2829 b0d drivers/iommu/iommufd/driver.o
9410 618 64 10092 276c drivers/iommu/iommufd/main.o

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

---
 drivers/iommu/iommufd/iommufd_private.h |  4 +++
 include/linux/iommufd.h                 | 10 --------
 drivers/iommu/iommufd/driver.c          | 33 -------------------------
 drivers/iommu/iommufd/main.c            | 32 ++++++++++++++++++++++++
 4 files changed, 36 insertions(+), 43 deletions(-)

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

index 32f0631368e1..ec5b499d139c 100644
@@ -230,6 +230,10 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx,
 	iommufd_object_remove(ictx, obj, obj->id, 0);
 }
 
+struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
+					     size_t size,
+					     enum iommufd_object_type type);
+
 #define __iommufd_object_alloc(ictx, ptr, type, obj)                           \
 	container_of(_iommufd_object_alloc(                                    \
 			     ictx,                                             \

include/linux/iommufd.h +0 ~~-10~~

index bf41b242b9f6..2d1bf2f97ee3 100644
@@ -190,9 +190,6 @@ static inline int iommufd_vfio_compat_set_no_iommu(struct iommufd_ctx *ictx)
 #endif /* CONFIG_IOMMUFD */
 
 #if IS_ENABLED(CONFIG_IOMMUFD_DRIVER_CORE)
-struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
-					     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,

@@ -201,13 +198,6 @@ int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
 				enum iommu_veventq_type type, void *event_data,
 				size_t data_len);
 #else /* !CONFIG_IOMMUFD_DRIVER_CORE */
-static inline struct iommufd_object *
-_iommufd_object_alloc(struct iommufd_ctx *ictx, 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 +0 ~~-33~~

index 922cd1fe7ec2..2fee399a148e 100644
@@ -3,39 +3,6 @@
  */
 #include "iommufd_private.h"
 
-struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
-					     size_t size,
-					     enum iommufd_object_type type)
-{
-	struct iommufd_object *obj;
-	int rc;
-
-	obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
-	if (!obj)
-		return ERR_PTR(-ENOMEM);
-	obj->type = type;
-	/* Starts out bias'd by 1 until it is removed from the xarray */
-	refcount_set(&obj->shortterm_users, 1);
-	refcount_set(&obj->users, 1);
-
-	/*
-	 * Reserve an ID in the xarray but do not publish the pointer yet since
-	 * the caller hasn't initialized it yet. Once the pointer is published
-	 * in the xarray and visible to other threads we can't reliably destroy
-	 * it anymore, so the caller must complete all errorable operations
-	 * before calling iommufd_object_finalize().
-	 */
-	rc = xa_alloc(&ictx->objects, &obj->id, XA_ZERO_ENTRY, xa_limit_31b,
-		      GFP_KERNEL_ACCOUNT);
-	if (rc)
-		goto out_free;
-	return obj;
-out_free:
-	kfree(obj);
-	return ERR_PTR(rc);
-}
-EXPORT_SYMBOL_NS_GPL(_iommufd_object_alloc, "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 +32 -0

index 347c56ef44d8..85ad2853da0b 100644
@@ -29,6 +29,38 @@ struct iommufd_object_ops {
 static const struct iommufd_object_ops iommufd_object_ops[];
 static struct miscdevice vfio_misc_dev;
 
+struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
+					     size_t size,
+					     enum iommufd_object_type type)
+{
+	struct iommufd_object *obj;
+	int rc;
+
+	obj = kzalloc(size, GFP_KERNEL_ACCOUNT);
+	if (!obj)
+		return ERR_PTR(-ENOMEM);
+	obj->type = type;
+	/* Starts out bias'd by 1 until it is removed from the xarray */
+	refcount_set(&obj->shortterm_users, 1);
+	refcount_set(&obj->users, 1);
+
+	/*
+	 * Reserve an ID in the xarray but do not publish the pointer yet since
+	 * the caller hasn't initialized it yet. Once the pointer is published
+	 * in the xarray and visible to other threads we can't reliably destroy
+	 * it anymore, so the caller must complete all errorable operations
+	 * before calling iommufd_object_finalize().
+	 */
+	rc = xa_alloc(&ictx->objects, &obj->id, XA_ZERO_ENTRY, xa_limit_31b,
+		      GFP_KERNEL_ACCOUNT);
+	if (rc)
+		goto out_free;
+	return obj;
+out_free:
+	kfree(obj);
+	return ERR_PTR(rc);
+}
+
 /*
  * Allow concurrent access to the object.
  *

Signature · 1 line

2.43.0

Tian, Kevin

maintainer Reviewed-by on v1 10/12

2025-06-12 08:24 UTC #39 lore raw

RE: [PATCH v1 10/12] iommufd: Move _iommufd_object_alloc out of driver.c

Quoted 15 lines

From: Nicolin Chen <[email protected]>
Sent: Tuesday, June 10, 2025 1:14 AM
Now, all driver structures will be allocated by the core, i.e. no longer a need of driver calling _iommufd_object_alloc. Thus, move it back.
Before:
   text	   data	    bss	    dec	    hex	filename
   3024	    180	      0	   3204	    c84	drivers/iommu/iommufd/driver.o
   9074	    610	     64	   9748	   2614	drivers/iommu/iommufd/main.o
After:
   text	   data	    bss	    dec	    hex	filename
   2665	    164	      0	   2829	    b0d	drivers/iommu/iommufd/driver.o
   9410	    618	     64	  10092	   276c	drivers/iommu/iommufd/main.o

Quoted 1 line

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

Reviewed-by Kevin Tian <[email protected]>

Jason Gunthorpe

maintainer Reviewed-by on v1 10/12

2025-06-13 13:46 UTC #40 lore raw

Re: [PATCH v1 10/12] iommufd: Move _iommufd_object_alloc out of driver.c

In reply to Nicolin Chen

Quoted 12 lines · Nicolin Chen

Now, all driver structures will be allocated by the core, i.e. no longer a need of driver calling _iommufd_object_alloc. Thus, move it back.

Before:

   text	   data	    bss	    dec	    hex	filename
   3024	    180	      0	   3204	    c84	drivers/iommu/iommufd/driver.o
   9074	    610	     64	   9748	   2614	drivers/iommu/iommufd/main.o
After:
   text	   data	    bss	    dec	    hex	filename
   2665	    164	      0	   2829	    b0d	drivers/iommu/iommufd/driver.o
   9410	    618	     64	  10092	   276c	drivers/iommu/iommufd/main.o

Quoted 7 lines

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

---
 drivers/iommu/iommufd/iommufd_private.h |  4 +++
 include/linux/iommufd.h                 | 10 --------
 drivers/iommu/iommufd/driver.c          | 33 -------------------------
 drivers/iommu/iommufd/main.c            | 32 ++++++++++++++++++++++++
 4 files changed, 36 insertions(+), 43 deletions(-)

Reviewed-by Jason Gunthorpe <[email protected]>

Jason

Nicolin Chen

author

2025-06-09 17:14 UTC #41 lore raw

PATCH 11/12 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.

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

Reviewed-by Kevin Tian <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/iommufd/iommufd_private.h | 17 +++++++++++++++++
 drivers/iommu/iommufd/main.c            | 24 ++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

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

index ec5b499d139c..884dbd67afc8 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,

@@ -246,6 +247,22 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
 #define iommufd_object_alloc(ictx, ptr, type) \
 	__iommufd_object_alloc(ictx, ptr, type, obj)
 
+struct iommufd_object *
+_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size,
+			   enum iommufd_object_type type);
+
+#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)
+
 /*
  * The IO Address Space (IOAS) pagetable is a virtual page table backed by the
  * io_pagetable object. It is a user controlled mapping of IOVA -> PFNs. The

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

index 85ad2853da0b..4880c085b159 100644
@@ -61,6 +61,23 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
 	return ERR_PTR(rc);
 }
 
+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;
+}
+
 /*
  * Allow concurrent access to the object.
  *

@@ -448,6 +465,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

Jason Gunthorpe

maintainer

2025-06-13 13:58 UTC #42 lore raw

Re: [PATCH v1 11/12] iommufd: Introduce iommufd_object_alloc_ucmd helper

drivers/iommu/iommufd/iommufd_private.h struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, · line 264

Quoted 2 lines · Nicolin Chen

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

+#define iommufd_object_alloc_ucmd(ucmd, ptr, type) \
+	__iommufd_object_alloc_ucmd(ucmd, ptr, type, obj)

Lets add a comment here and on the normal iommufd_object_alloc explaining that this function automatically calls finalize and abort, the non ucmd version requires the caller to do so.

drivers/iommu/iommufd/main.c struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, · line 70

Quoted 7 lines

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

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

WARN_ON? Something is coded wrong if we hit this right?

Jason

Nicolin Chen

author

2025-06-13 23:17 UTC #43 lore raw

Re: [PATCH v1 11/12] iommufd: Introduce iommufd_object_alloc_ucmd helper

Quoted 8 lines · Jason Gunthorpe

drivers/iommu/iommufd/iommufd_private.h struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, · line 264
Quoted 2 lines · Nicolin Chen
drivers/iommu/iommufd/iommufd_private.h +2 -0
+#define iommufd_object_alloc_ucmd(ucmd, ptr, type) \
+	__iommufd_object_alloc_ucmd(ucmd, ptr, type, obj)
Lets add a comment here and on the normal iommufd_object_alloc explaining that this function automatically calls finalize and abort, the non ucmd version requires the caller to do so.

Yes.

@@ -231,6 +231,11 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx,

        iommufd_object_remove(ictx, obj, obj->id, 0);

}

+/*
+ * Callers of these normal object allocators must call iommufd_object_finalize()
+ * to finalize the object, or call iommufd_object_abort_and_destroy() to revert
+ * the allocation.
+ */
struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,

                                             size_t size,
                                             enum iommufd_object_type type);

@@ -247,6 +252,10 @@ struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx,
#define iommufd_object_alloc(ictx, ptr, type) \
__iommufd_object_alloc(ictx, ptr, type, obj)

+/*
+ * Callers of these _ucmd allocators should not call iommufd_object_finalize()
+ * or iommufd_object_abort_and_destroy(), as the core automatically does that.
+ */
struct iommufd_object *
_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd, size_t size,

                           enum iommufd_object_type type);

Quoted 9 lines

drivers/iommu/iommufd/main.c struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, · line 70
Quoted 7 lines
drivers/iommu/iommufd/main.c +7 -0
+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)
WARN_ON? Something is coded wrong if we hit this right?

Yes.

@@ -67,7 +67,8 @@ struct iommufd_object *_iommufd_object_alloc_ucmd(struct iommufd_ucmd *ucmd,

 {
        struct iommufd_object *new_obj;

-       if (ucmd->new_obj)
+       /* Something is coded wrong if this is hit */
+       if (WARN_ON(ucmd->new_obj))
                return ERR_PTR(-EBUSY);

        new_obj = _iommufd_object_alloc(ucmd->ictx, size, type);

Thanks
Nicolin

Nicolin Chen

author

2025-06-09 17:14 UTC #44 lore raw

PATCH 12/12 iommufd: 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.

Reviewed-by Jason Gunthorpe <[email protected]>

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

---
 drivers/iommu/iommufd/eventq.c | 14 ++++----------
 drivers/iommu/iommufd/viommu.c | 24 ++++++------------------
 2 files changed, 10 insertions(+), 28 deletions(-)

drivers/iommu/iommufd/eventq.c +4 ~~-10~~

index e373b9eec7f5..fc4de63b0bce 100644
@@ -427,8 +427,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);

@@ -437,10 +437,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;

@@ -448,7 +446,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);

@@ -456,9 +453,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/viommu.c +6 ~~-18~~

index 63a92fb27ef4..742df3cb0ba5 100644
@@ -61,8 +61,8 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		goto out_put_hwpt;
 	}
 
-	viommu = (struct iommufd_viommu *)_iommufd_object_alloc(
-		ucmd->ictx, viommu_size, IOMMUFD_OBJ_VIOMMU);
+	viommu = (struct iommufd_viommu *)_iommufd_object_alloc_ucmd(
+		ucmd, viommu_size, IOMMUFD_OBJ_VIOMMU);
 	if (IS_ERR(viommu)) {
 		rc = PTR_ERR(viommu);
 		goto out_put_hwpt;

@@ -84,23 +84,17 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
 
 	rc = ops->viommu_init(viommu, hwpt_paging->common.domain);
 	if (rc)
-		goto out_abort;
+		goto out_put_hwpt;
 
 	/* It is a driver bug that viommu->ops isn't filled */
 	if (WARN_ON_ONCE(!viommu->ops)) {
 		rc = -EINVAL;
-		goto out_abort;
+		goto out_put_hwpt;
 	}
 
 	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:

@@ -148,7 +142,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;

@@ -163,18 +157,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

Nicolin Chen

author

2025-06-10 18:32 UTC #45 lore raw

In reply to Nicolin Chen

Quoted 7 lines · Nicolin Chen

The new HW Queue object will require more interactions with IOMMU drivers, with a few more for-driver APIs. This will complicate the driver-allocated structure design like the viommu_alloc op: since the core structure is not initialized during the driver allocation stage, a new for-driver API can't reference any member in the core vIOMMU structure.
Make a preparatory series doing:

So, Don pointed out that "Repare" is a typo here. All this series doing is to "Prepare", as IOMMUFD_OBJ_HW_QUEUE hasn't been merged yet.

Also the main goal of this series is to replace viommu_alloc with get_viommu_size + viommu_init. The patch that introduces the two new ops has the full details (though I should have some narrative clearly show the idea in the cover letter too):

--------------------------------------------------------------------------
So far, a vIOMMU object has been allocated by IOMMU driver and initialized with the driver-level structure, before it returns to the iommufd core for core-level structure initialization. It has been requiring iommufd core to expose some core structure/helpers in its driver.c file, which result in a size increase of this driver module.

    // my_viommu is successfully allocated
    my_viommu = iommufd_viommu_alloc(...);
    // This may crash if it reads viommu->ictx
    new = iommufd_new_viommu_helper(my_viommu->core ...);

Thus, this requires two new iommu ops, get_viommu_size and viommu_init, so iommufd core can communicate with drivers to replace the viommu_alloc op.

This also adds a VIOMMU_STRUCT_SIZE macro, for drivers to use, which would statically sanitize the driver structure.
--------------------------------------------------------------------------

Thanks.