[PATCH v1 00/12] iommufd: Repare for IOMMUFD_OBJ_HW_QUEUE

[PATCH v1 00/12] iommufd: Repare for IOMMUFD_OBJ_HW_QUEUE

[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:
 - 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/cover.1747537752.git.nicolinc@nvidia.com/

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

-- 
2.43.0

[PATCH v1 01/12] iommufd: Apply obvious cosmetic fixes

[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

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 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(-)

diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
index 10c928a9a463..c115a51d9384 100644
--- a/drivers/iommu/iommufd/io_pagetable.h
+++ b/drivers/iommu/iommufd/io_pagetable.h
@@ -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);
 
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 9ccc83341f32..190ceb5dada3 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -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 {
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 34b6e6ca4bfa..498c9a768506 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -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;
 }
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 86244403b532..ed0dc539d490 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -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);
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 487779470261..8565a6f596b2 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -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;
diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
index 8a790e597e12..13d010f19ed1 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -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);
diff --git a/drivers/iommu/iommufd/iova_bitmap.c b/drivers/iommu/iommufd/iova_bitmap.c
index 39a86a4a1d3a..4514575818fc 100644
--- a/drivers/iommu/iommufd/iova_bitmap.c
+++ b/drivers/iommu/iommufd/iova_bitmap.c
@@ -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
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 3df468f64e7d..347c56ef44d8 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -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",
diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
index 3427749bc5ce..cbdde642d2af 100644
--- a/drivers/iommu/iommufd/pages.c
+++ b/drivers/iommu/iommufd/pages.c
@@ -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;
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 6bd0abf9a641..4d5dca8027b1 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -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;
 
-- 
2.43.0

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

[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 <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/iommufd_private.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 190ceb5dada3..4619285c09b7 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -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 */
-- 
2.43.0

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> 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 <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

[Jason Gunthorpe]

On Mon, Jun 09, 2025 at 10:13:25AM -0700, Nicolin Chen wrote:
> The core code can always get the ictx pointer via vdev->viommu->ictx, thus
> drop this unused one.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommufd/iommufd_private.h | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

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

[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 <nicolinc@nvidia.com>
---
 include/linux/iommufd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 498c9a768506..ac98e49e44fe 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -101,7 +101,7 @@ struct iommufd_viommu {
 	struct list_head veventqs;
 	struct rw_semaphore veventqs_rwsem;
 
-	unsigned int type;
+	enum iommu_viommu_type type;
 };
 
 /**
-- 
2.43.0

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> 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 <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

[Jason Gunthorpe]

On Mon, Jun 09, 2025 at 10:13:26AM -0700, Nicolin Chen wrote:
> 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 <nicolinc@nvidia.com>
> ---
>  include/linux/iommufd.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

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

[Nicolin Chen]

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

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/iommufd_private.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 4619285c09b7..32f0631368e1 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -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;
 
-- 
2.43.0

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, June 10, 2025 1:13 AM
> 
> Replace unsigned init, to make it clear. No functional changes.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

[Jason Gunthorpe]

On Mon, Jun 09, 2025 at 10:13:27AM -0700, Nicolin Chen wrote:
> Replace unsigned init, to make it clear. No functional changes.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommufd/iommufd_private.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

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

[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.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 include/linux/iommu.h   | 15 +++++++++++++++
 include/linux/iommufd.h |  6 ++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 156732807994..e42a28971182 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -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);
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index ac98e49e44fe..423e08963d90 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -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.
-- 
2.43.0

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> 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? 

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

[Nicolin Chen]

On Thu, Jun 12, 2025 at 08:12:29AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > 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

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, June 13, 2025 1:07 AM
> 
> On Thu, Jun 12, 2025 at 08:12:29AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > 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.
> 

Okay. Make sense.

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

[Jason Gunthorpe]

On Mon, Jun 09, 2025 at 10:13:28AM -0700, Nicolin Chen wrote:
> @@ -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

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

[Nicolin Chen]

On Fri, Jun 13, 2025 at 10:41:17AM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 09, 2025 at 10:13:28AM -0700, Nicolin Chen wrote:
> > @@ -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

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

[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 <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/selftest.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 4d5dca8027b1..b0de205a2303 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -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){
-- 
2.43.0

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> 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().
> 
> The mock_viommu_alloc() will be cleaned up once the transition is done.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

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

[Nicolin Chen]

On Thu, Jun 12, 2025 at 08:17:19AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > 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().
> > 
> > The mock_viommu_alloc() will be cleaned up once the transition is done.
> > 
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> 
> 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

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, June 13, 2025 1:12 AM
> 
> On Thu, Jun 12, 2025 at 08:17:19AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > 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().
> > >
> > > The mock_viommu_alloc() will be cleaned up once the transition is done.
> > >
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> >
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> >
> > 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..
> 

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

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

[Jason Gunthorpe]

On Mon, Jun 09, 2025 at 10:13:29AM -0700, Nicolin Chen wrote:
> 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

> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommufd/selftest.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
> index 4d5dca8027b1..b0de205a2303 100644
> --- a/drivers/iommu/iommufd/selftest.c
> +++ b/drivers/iommu/iommufd/selftest.c
> @@ -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;
> +}

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

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

[Nicolin Chen]

On Fri, Jun 13, 2025 at 10:45:22AM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 09, 2025 at 10:13:29AM -0700, Nicolin Chen wrote:
> > 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 <nicolinc@nvidia.com>
-------------------------------------------------------------------

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;
+       }

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

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

[Jason Gunthorpe]

On Fri, Jun 13, 2025 at 01:19:33PM -0700, Nicolin Chen wrote:
> > 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

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

[Nicolin Chen]

On Fri, Jun 13, 2025 at 08:23:18PM -0300, Jason Gunthorpe wrote:
> On Fri, Jun 13, 2025 at 01:19:33PM -0700, Nicolin Chen wrote:
> > > 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.

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.

> 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

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

[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 <nicolinc@nvidia.com>
---
 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(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index ea41d790463e..2357459099f4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -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,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index e4fd8d522af8..482a49f5c10c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -382,6 +382,62 @@ static const struct iommufd_viommu_ops arm_vsmmu_ops = {
 	.cache_invalidate = arm_vsmmu_cache_invalidate,
 };
 
+int arm_smmu_get_viommu_size(enum iommu_viommu_type viommu_type,
+			     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
+	 * defect is needed to determine if arm_vsmmu_cache_invalidate() needs
+	 * any change to remove this.
+	 */
+	if (WARN_ON(smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
+		return -EOPNOTSUPP;
+
+	/*
+	 * Must support some way to prevent the VM from bypassing the cache
+	 * because VFIO currently does not do any cache maintenance. canwbs
+	 * 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
+	 * things non-coherent using the memattr, but No-Snoop behavior is not
+	 * 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;
+
+	*viommu_size = VIOMMU_STRUCT_SIZE(struct arm_vsmmu, core);
+	return 0;
+}
+
+int arm_vsmmu_init(struct iommufd_viommu *viommu,
+		   struct iommu_domain *parent_domain)
+{
+	struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core);
+	struct arm_smmu_device *smmu =
+		container_of(viommu->iommu_dev, struct arm_smmu_device, iommu);
+	struct arm_smmu_domain *s2_parent = to_smmu_domain(parent_domain);
+
+	if (s2_parent->smmu != smmu)
+		return -EINVAL;
+
+	vsmmu->smmu = smmu;
+	vsmmu->s2_parent = s2_parent;
+	/* FIXME Move VMID allocation from the S2 domain allocation to here */
+	vsmmu->vmid = s2_parent->s2_cfg.vmid;
+
+	viommu->ops = &arm_vsmmu_ops;
+	return 0;
+}
+
 struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 				       struct iommu_domain *parent,
 				       struct iommufd_ctx *ictx,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 10cc6dc26b7b..eef1fbd68914 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -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 */
-- 
2.43.0

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, June 10, 2025 1:14 AM
> 
> +int arm_smmu_get_viommu_size(enum iommu_viommu_type
> viommu_type,
> +			     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
> +	 * defect is needed to determine if arm_vsmmu_cache_invalidate()
> needs
> +	 * any change to remove this.
> +	 */
> +	if (WARN_ON(smmu->options &
> ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * Must support some way to prevent the VM from bypassing the
> cache
> +	 * because VFIO currently does not do any cache maintenance.
> canwbs
> +	 * 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
> +	 * things non-coherent using the memattr, but No-Snoop behavior is
> not
> +	 * 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);

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

[Nicolin Chen]

On Thu, Jun 12, 2025 at 08:20:30AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Tuesday, June 10, 2025 1:14 AM
> > 
> > +int arm_smmu_get_viommu_size(enum iommu_viommu_type
> > viommu_type,
> > +			     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
> > +	 * defect is needed to determine if arm_vsmmu_cache_invalidate()
> > needs
> > +	 * any change to remove this.
> > +	 */
> > +	if (WARN_ON(smmu->options &
> > ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
> > +		return -EOPNOTSUPP;
> > +
> > +	/*
> > +	 * Must support some way to prevent the VM from bypassing the
> > cache
> > +	 * because VFIO currently does not do any cache maintenance.
> > canwbs
> > +	 * 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
> > +	 * things non-coherent using the memattr, but No-Snoop behavior is
> > not
> > +	 * 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.

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.

> 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

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, June 13, 2025 1:18 AM
> 
> On Thu, Jun 12, 2025 at 08:20:30AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Tuesday, June 10, 2025 1:14 AM
> > >
> > > +int arm_smmu_get_viommu_size(enum iommu_viommu_type
> > > viommu_type,
> > > +			     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
> > > +	 * defect is needed to determine if arm_vsmmu_cache_invalidate()
> > > needs
> > > +	 * any change to remove this.
> > > +	 */
> > > +	if (WARN_ON(smmu->options &
> > > ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	/*
> > > +	 * Must support some way to prevent the VM from bypassing the
> > > cache
> > > +	 * because VFIO currently does not do any cache maintenance.
> > > canwbs
> > > +	 * 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
> > > +	 * things non-coherent using the memattr, but No-Snoop behavior is
> > > not
> > > +	 * 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.
> 
> 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;
> 
> ===============================================================

Oh I see.

> 
> Otherwise, this following patch has to move the type check again.
> 
> > 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

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

[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.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/viommu.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
index 01df2b985f02..63a92fb27ef4 100644
--- a/drivers/iommu/iommufd/viommu.c
+++ b/drivers/iommu/iommufd/viommu.c
@@ -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)
-- 
2.43.0

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

[Baolu Lu]

On 6/10/25 01:13, Nicolin Chen wrote:
> 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.

> 
> Suggested-by: Jason Gunthorpe<jgg@nvidia.com>
> Signed-off-by: Nicolin Chen<nicolinc@nvidia.com>
> ---
>   drivers/iommu/iommufd/viommu.c | 30 +++++++++++++++++++++++++++---
>   1 file changed, 27 insertions(+), 3 deletions(-)
> 

<...>

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

Thanks,
baolu

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

[Nicolin Chen]

On Tue, Jun 10, 2025 at 01:55:05PM +0800, Baolu Lu wrote:
> On 6/10/25 01:13, Nicolin Chen wrote:
> > 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.

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

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, June 10, 2025 2:20 PM
> 
> On Tue, Jun 10, 2025 at 01:55:05PM +0800, Baolu Lu wrote:
> > On 6/10/25 01:13, Nicolin Chen wrote:
> > > 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.
> 
> 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 <kevin.tian@intel.com>

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

[Nicolin Chen]

On Thu, Jun 12, 2025 at 08:22:49AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Tuesday, June 10, 2025 2:20 PM
> > 
> > On Tue, Jun 10, 2025 at 01:55:05PM +0800, Baolu Lu wrote:
> > > On 6/10/25 01:13, Nicolin Chen wrote:
> > > > 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.
> > 
> > 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 <kevin.tian@intel.com>

Okay. And I can make the commit message clearer.

Thanks
Nicolin

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, June 10, 2025 1:14 AM
> 
> +	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;
> +	}
> +

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

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

[Nicolin Chen]

On Thu, Jun 12, 2025 at 08:27:12AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Tuesday, June 10, 2025 1:14 AM
> > 
> > +	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;
> > +	}
> > +
> 
> 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

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

[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 <nicolinc@nvidia.com>
---
 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(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 2357459099f4..3f5e3db7ae1c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -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
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e42a28971182..6389bb1915dc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -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;
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 423e08963d90..bf41b242b9f6 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -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
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 482a49f5c10c..2f79e9a58a5d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -438,59 +438,6 @@ int arm_vsmmu_init(struct iommufd_viommu *viommu,
 	return 0;
 }
 
-struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
-				       struct iommu_domain *parent,
-				       struct iommufd_ctx *ictx,
-				       unsigned int viommu_type)
-{
-	struct arm_smmu_device *smmu =
-		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);
-
-	if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
-		return ERR_PTR(-EOPNOTSUPP);
-
-	if (s2_parent->smmu != master->smmu)
-		return ERR_PTR(-EINVAL);
-
-	/*
-	 * FORCE_SYNC is not set with FEAT_NESTING. Some study of the exact HW
-	 * defect is needed to determine if arm_vsmmu_cache_invalidate() needs
-	 * any change to remove this.
-	 */
-	if (WARN_ON(smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
-		return ERR_PTR(-EOPNOTSUPP);
-
-	/*
-	 * Must support some way to prevent the VM from bypassing the cache
-	 * because VFIO currently does not do any cache maintenance. canwbs
-	 * 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
-	 * things non-coherent using the memattr, but No-Snoop behavior is not
-	 * effected.
-	 */
-	if (!arm_smmu_master_canwbs(master) &&
-	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
-		return ERR_PTR(-EOPNOTSUPP);
-
-	vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, core,
-				     &arm_vsmmu_ops);
-	if (IS_ERR(vsmmu))
-		return ERR_CAST(vsmmu);
-
-	vsmmu->smmu = smmu;
-	vsmmu->s2_parent = s2_parent;
-	/* FIXME Move VMID allocation from the S2 domain allocation to here */
-	vsmmu->vmid = s2_parent->s2_cfg.vmid;
-
-	return &vsmmu->core;
-}
-
 int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt)
 {
 	struct iommu_vevent_arm_smmuv3 vevt;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index eef1fbd68914..181d07bc1a9d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -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,
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index b0de205a2303..2f072f1b1dad 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -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,
-- 
2.43.0

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> 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 <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

[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 <nicolinc@nvidia.com>
---
 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(-)

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 32f0631368e1..ec5b499d139c 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -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,                                             \
diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index bf41b242b9f6..2d1bf2f97ee3 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -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)
 {
diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
index 922cd1fe7ec2..2fee399a148e 100644
--- a/drivers/iommu/iommufd/driver.c
+++ b/drivers/iommu/iommufd/driver.c
@@ -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)
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 347c56ef44d8..85ad2853da0b 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -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.
  *
-- 
2.43.0

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

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@nvidia.com>
> 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
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

[Jason Gunthorpe]

On Mon, Jun 09, 2025 at 10:13:33AM -0700, Nicolin Chen wrote:
> 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 <nicolinc@nvidia.com>
> ---
>  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 <jgg@nvidia.com>

Jason

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

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

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/iommufd_private.h | 17 +++++++++++++++++
 drivers/iommu/iommufd/main.c            | 24 ++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index ec5b499d139c..884dbd67afc8 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -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
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 85ad2853da0b..4880c085b159 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -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;
 }
 
-- 
2.43.0

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

[Jason Gunthorpe]

On Mon, Jun 09, 2025 at 10:13:34AM -0700, Nicolin Chen wrote:

> +#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.

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

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

[Nicolin Chen]

On Fri, Jun 13, 2025 at 10:58:17AM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 09, 2025 at 10:13:34AM -0700, Nicolin Chen wrote:
> 
> > +#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);

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

[PATCH v1 12/12] iommufd: Apply the new iommufd_object_alloc_ucmd helper

[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.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/eventq.c | 14 ++++----------
 drivers/iommu/iommufd/viommu.c | 24 ++++++------------------
 2 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/iommu/iommufd/eventq.c b/drivers/iommu/iommufd/eventq.c
index e373b9eec7f5..fc4de63b0bce 100644
--- a/drivers/iommu/iommufd/eventq.c
+++ b/drivers/iommu/iommufd/eventq.c
@@ -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;
 }
 
diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
index 63a92fb27ef4..742df3cb0ba5 100644
--- a/drivers/iommu/iommufd/viommu.c
+++ b/drivers/iommu/iommufd/viommu.c
@@ -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:
-- 
2.43.0

Re: [PATCH v1 00/12] iommufd: Repare for IOMMUFD_OBJ_HW_QUEUE

[Nicolin Chen]

On Mon, Jun 09, 2025 at 10:13:23AM -0700, Nicolin Chen wrote:
> 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.

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

Thanks.