[PATCH 0/2] Rename ops->domain_alloc_user() to domain_alloc_paging_flags()

[PATCH 0/2] Rename ops->domain_alloc_user() to domain_alloc_paging_flags()

[Jason Gunthorpe]

This tidies some of the naming that has become messy this cycle. This
series is on top of the iommufd and iommu tress as there are dependences
on the iommufd selftests files changes.

I would like to push this into this merge window so that the next cycle
can have the folowup got through the driver trees. Otherwise we will again
have conflicts and problems. I have written additional patches to
consolidate the drivers to use only domain_alloc_paging_flags() instead of
implementing both.

So I propose sending this as a late pull request in the second week.

Jason Gunthorpe (2):
  iommu: Add ops->domain_alloc_nested()
  iommu: Rename ops->domain_alloc_user() to domain_alloc_paging_flags()

 drivers/iommu/amd/iommu.c                   |  9 +++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  9 +++----
 drivers/iommu/intel/iommu.c                 | 15 +++---------
 drivers/iommu/intel/iommu.h                 |  6 +++--
 drivers/iommu/intel/nested.c                | 11 +++++++--
 drivers/iommu/iommu.c                       |  4 +--
 drivers/iommu/iommufd/hw_pagetable.c        | 16 ++++++------
 drivers/iommu/iommufd/selftest.c            | 15 +++++-------
 include/linux/iommu.h                       | 27 ++++++++++++---------
 9 files changed, 57 insertions(+), 55 deletions(-)


base-commit: 6143bfc47c18a1505b8f4cda83432c24c842a6ce
-- 
2.43.0

[PATCH 1/2] iommu: Add ops->domain_alloc_nested()

[Jason Gunthorpe]

It turns out all the drivers that are using this immediately call into
another function, so just make that function directly into the op. This
makes paging=NULL for domain_alloc_user and we can remove the argument in
the next patch.

The function mirrors the similar op in the viommu that allocates a nested
domain on top of the viommu's nesting parent. This version supports cases
where a viommu is not being used.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/intel/iommu.c          |  9 +++------
 drivers/iommu/intel/iommu.h          |  6 ++++--
 drivers/iommu/intel/nested.c         | 11 +++++++++--
 drivers/iommu/iommufd/hw_pagetable.c |  8 ++++----
 drivers/iommu/iommufd/selftest.c     |  7 ++++---
 include/linux/iommu.h                |  9 +++++----
 6 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 527f6f89d8a1f5..6f11a075114f7a 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3340,12 +3340,8 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 	struct iommu_domain *domain;
 	bool first_stage;
 
-	/* Must be NESTING domain */
-	if (parent) {
-		if (!nested_supported(iommu) || flags)
-			return ERR_PTR(-EOPNOTSUPP);
-		return intel_nested_domain_alloc(parent, user_data);
-	}
+	if (parent)
+		return ERR_PTR(-EOPNOTSUPP);
 
 	if (flags &
 	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING
@@ -4475,6 +4471,7 @@ const struct iommu_ops intel_iommu_ops = {
 	.domain_alloc_user	= intel_iommu_domain_alloc_user,
 	.domain_alloc_sva	= intel_svm_domain_alloc,
 	.domain_alloc_paging	= intel_iommu_domain_alloc_paging,
+	.domain_alloc_nested	= intel_iommu_domain_alloc_nested,
 	.probe_device		= intel_iommu_probe_device,
 	.release_device		= intel_iommu_release_device,
 	.get_resv_regions	= intel_iommu_get_resv_regions,
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 2cca094c259dc1..6ea7bbe26b19d5 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1265,8 +1265,10 @@ int __domain_setup_first_level(struct intel_iommu *iommu,
 int dmar_ir_support(void);
 
 void iommu_flush_write_buffer(struct intel_iommu *iommu);
-struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
-					       const struct iommu_user_data *user_data);
+struct iommu_domain *
+intel_iommu_domain_alloc_nested(struct device *dev, struct iommu_domain *parent,
+				u32 flags,
+				const struct iommu_user_data *user_data);
 struct device *device_rbtree_find(struct intel_iommu *iommu, u16 rid);
 
 enum cache_tag_type {
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index 42c4533a6ea21d..aba92c00b42740 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -186,14 +186,21 @@ static const struct iommu_domain_ops intel_nested_domain_ops = {
 	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
 };
 
-struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
-					       const struct iommu_user_data *user_data)
+struct iommu_domain *
+intel_iommu_domain_alloc_nested(struct device *dev, struct iommu_domain *parent,
+				u32 flags,
+				const struct iommu_user_data *user_data)
 {
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *s2_domain = to_dmar_domain(parent);
+	struct intel_iommu *iommu = info->iommu;
 	struct iommu_hwpt_vtd_s1 vtd;
 	struct dmar_domain *domain;
 	int ret;
 
+	if (!nested_supported(iommu) || flags)
+		return ERR_PTR(-EOPNOTSUPP);
+
 	/* Must be nested domain */
 	if (user_data->type != IOMMU_HWPT_DATA_VTD_S1)
 		return ERR_PTR(-EOPNOTSUPP);
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 9236e8ca9aa864..ec3c64a8c79633 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -227,7 +227,7 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
 	int rc;
 
 	if ((flags & ~IOMMU_HWPT_FAULT_ID_VALID) ||
-	    !user_data->len || !ops->domain_alloc_user)
+	    !user_data->len || !ops->domain_alloc_nested)
 		return ERR_PTR(-EOPNOTSUPP);
 	if (parent->auto_domain || !parent->nest_parent ||
 	    parent->common.domain->owner != ops)
@@ -242,9 +242,9 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx,
 	refcount_inc(&parent->common.obj.users);
 	hwpt_nested->parent = parent;
 
-	hwpt->domain = ops->domain_alloc_user(idev->dev,
-					      flags & ~IOMMU_HWPT_FAULT_ID_VALID,
-					      parent->common.domain, user_data);
+	hwpt->domain = ops->domain_alloc_nested(
+		idev->dev, parent->common.domain,
+		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/selftest.c b/drivers/iommu/iommufd/selftest.c
index 2f9de177dffc79..c58083c3660aee 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -356,8 +356,8 @@ __mock_domain_alloc_nested(const struct iommu_user_data *user_data)
 }
 
 static struct iommu_domain *
-mock_domain_alloc_nested(struct iommu_domain *parent, u32 flags,
-			 const struct iommu_user_data *user_data)
+mock_domain_alloc_nested(struct device *dev, struct iommu_domain *parent,
+			 u32 flags, const struct iommu_user_data *user_data)
 {
 	struct mock_iommu_domain_nested *mock_nested;
 	struct mock_iommu_domain *mock_parent;
@@ -391,7 +391,7 @@ mock_domain_alloc_user(struct device *dev, u32 flags,
 	struct iommu_domain *domain;
 
 	if (parent)
-		return mock_domain_alloc_nested(parent, flags, user_data);
+		return ERR_PTR(-EOPNOTSUPP);
 
 	if (user_data)
 		return ERR_PTR(-EOPNOTSUPP);
@@ -719,6 +719,7 @@ static const struct iommu_ops mock_ops = {
 	.hw_info = mock_domain_hw_info,
 	.domain_alloc_paging = mock_domain_alloc_paging,
 	.domain_alloc_user = mock_domain_alloc_user,
+	.domain_alloc_nested = mock_domain_alloc_nested,
 	.capable = mock_domain_capable,
 	.device_group = generic_device_group,
 	.probe_device = mock_probe_device,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d6aaaec3caf462..0472cc1245192d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -559,15 +559,13 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
  *                the caller iommu_domain_alloc() returns.
  * @domain_alloc_user: Allocate an iommu domain corresponding to the input
  *                     parameters as defined in include/uapi/linux/iommufd.h.
- *                     Upon success, if the @user_data is valid and the @parent
- *                     points to a kernel-managed domain, the new domain must be
- *                     IOMMU_DOMAIN_NESTED type; otherwise, the @parent must be
- *                     NULL while the @user_data can be optionally provided, the
+ *                     The @user_data can be optionally provided, the
  *                     new domain must support __IOMMU_DOMAIN_PAGING.
  *                     Upon failure, ERR_PTR must be returned.
  * @domain_alloc_paging: Allocate an iommu_domain that can be used for
  *                       UNMANAGED, DMA, and DMA_FQ domain types.
  * @domain_alloc_sva: Allocate an iommu_domain for Shared Virtual Addressing.
+ * @domain_alloc_nested: Allocate an iommu_domain for nested translation.
  * @probe_device: Add device to iommu driver handling
  * @release_device: Remove device from iommu driver handling
  * @probe_finalize: Do final setup work after the device is added to an IOMMU
@@ -622,6 +620,9 @@ struct iommu_ops {
 	struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
 	struct iommu_domain *(*domain_alloc_sva)(struct device *dev,
 						 struct mm_struct *mm);
+	struct iommu_domain *(*domain_alloc_nested)(
+		struct device *dev, struct iommu_domain *parent, u32 flags,
+		const struct iommu_user_data *user_data);
 
 	struct iommu_device *(*probe_device)(struct device *dev);
 	void (*release_device)(struct device *dev);
-- 
2.43.0

[PATCH 2/2] iommu: Rename ops->domain_alloc_user() to domain_alloc_paging_flags()

[Jason Gunthorpe]

Now that the main domain allocating path is calling this function it
doesn't make sense to leave it named _user. Change the name to
alloc_paging_flags() to mirror the new iommu_paging_domain_alloc_flags()
function.

A driver should implement only one of ops->domain_alloc_paging() or
ops->domain_alloc_paging_user(). The former is a simpler interface with
less boiler plate that the majority of drivers use. The latter is for
drivers with a greater feature set (PASID, multiple page table support,
advanced iommufd support, nesting, etc). Additional patches will be needed
to achieve this.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c                   |  9 ++++-----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  9 ++++-----
 drivers/iommu/intel/iommu.c                 | 10 +++-------
 drivers/iommu/iommu.c                       |  4 ++--
 drivers/iommu/iommufd/hw_pagetable.c        |  8 ++++----
 drivers/iommu/iommufd/selftest.c            | 10 +++-------
 include/linux/iommu.h                       | 20 ++++++++++++--------
 7 files changed, 32 insertions(+), 38 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 5ce8e6504ba7ee..3f691e1fd22ce4 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2407,9 +2407,8 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type)
 }
 
 static struct iommu_domain *
-amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
-			    struct iommu_domain *parent,
-			    const struct iommu_user_data *user_data)
+amd_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
+				    const struct iommu_user_data *user_data)
 
 {
 	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
@@ -2420,7 +2419,7 @@ amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
 	if (dev)
 		iommu = get_amd_iommu_from_dev(dev);
 
-	if ((flags & ~supported_flags) || parent || user_data)
+	if ((flags & ~supported_flags) || user_data)
 		return ERR_PTR(-EOPNOTSUPP);
 
 	/* Allocate domain with v2 page table if IOMMU supports PASID. */
@@ -2884,7 +2883,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.release_domain = &release_domain,
 	.identity_domain = &identity_domain.domain,
 	.domain_alloc = amd_iommu_domain_alloc,
-	.domain_alloc_user = amd_iommu_domain_alloc_user,
+	.domain_alloc_paging_flags = amd_iommu_domain_alloc_paging_flags,
 	.domain_alloc_sva = amd_iommu_domain_alloc_sva,
 	.probe_device = amd_iommu_probe_device,
 	.release_device = amd_iommu_release_device,
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 04630dbfedd92a..e4ebd9e12ad468 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3132,9 +3132,8 @@ static struct iommu_domain arm_smmu_blocked_domain = {
 };
 
 static struct iommu_domain *
-arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
-			   struct iommu_domain *parent,
-			   const struct iommu_user_data *user_data)
+arm_smmu_domain_alloc_paging_flags(struct device *dev, u32 flags,
+				   const struct iommu_user_data *user_data)
 {
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 	const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
@@ -3145,7 +3144,7 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
 
 	if (flags & ~PAGING_FLAGS)
 		return ERR_PTR(-EOPNOTSUPP);
-	if (parent || user_data)
+	if (user_data)
 		return ERR_PTR(-EOPNOTSUPP);
 
 	if (flags & IOMMU_HWPT_ALLOC_PASID)
@@ -3546,7 +3545,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.hw_info		= arm_smmu_hw_info,
 	.domain_alloc_paging    = arm_smmu_domain_alloc_paging,
 	.domain_alloc_sva       = arm_smmu_sva_domain_alloc,
-	.domain_alloc_user	= arm_smmu_domain_alloc_user,
+	.domain_alloc_paging_flags = arm_smmu_domain_alloc_paging_flags,
 	.probe_device		= arm_smmu_probe_device,
 	.release_device		= arm_smmu_release_device,
 	.device_group		= arm_smmu_device_group,
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 6f11a075114f7a..7d0acb74d5a543 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3328,9 +3328,8 @@ static struct dmar_domain *paging_domain_alloc(struct device *dev, bool first_st
 }
 
 static struct iommu_domain *
-intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
-			      struct iommu_domain *parent,
-			      const struct iommu_user_data *user_data)
+intel_iommu_domain_alloc_paging_flags(struct device *dev, u32 flags,
+				      const struct iommu_user_data *user_data)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	bool dirty_tracking = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
@@ -3340,9 +3339,6 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 	struct iommu_domain *domain;
 	bool first_stage;
 
-	if (parent)
-		return ERR_PTR(-EOPNOTSUPP);
-
 	if (flags &
 	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING
 	       | IOMMU_HWPT_FAULT_ID_VALID)))
@@ -4468,7 +4464,7 @@ const struct iommu_ops intel_iommu_ops = {
 	.identity_domain	= &identity_domain,
 	.capable		= intel_iommu_capable,
 	.hw_info		= intel_iommu_hw_info,
-	.domain_alloc_user	= intel_iommu_domain_alloc_user,
+	.domain_alloc_paging_flags = intel_iommu_domain_alloc_paging_flags,
 	.domain_alloc_sva	= intel_svm_domain_alloc,
 	.domain_alloc_paging	= intel_iommu_domain_alloc_paging,
 	.domain_alloc_nested	= intel_iommu_domain_alloc_nested,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7618e9c65d3fa8..9bc0c74cca3c7e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1987,8 +1987,8 @@ __iommu_paging_domain_alloc_flags(struct device *dev, unsigned int type,
 
 	if (ops->domain_alloc_paging && !flags)
 		domain = ops->domain_alloc_paging(dev);
-	else if (ops->domain_alloc_user)
-		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
+	else if (ops->domain_alloc_paging_flags)
+		domain = ops->domain_alloc_paging_flags(dev, flags, NULL);
 	else if (ops->domain_alloc && !flags)
 		domain = ops->domain_alloc(IOMMU_DOMAIN_UNMANAGED);
 	else
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index ec3c64a8c79633..ce03c380465154 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -119,7 +119,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 
 	lockdep_assert_held(&ioas->mutex);
 
-	if ((flags || user_data) && !ops->domain_alloc_user)
+	if ((flags || user_data) && !ops->domain_alloc_paging_flags)
 		return ERR_PTR(-EOPNOTSUPP);
 	if (flags & ~valid_flags)
 		return ERR_PTR(-EOPNOTSUPP);
@@ -139,9 +139,9 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 	hwpt_paging->ioas = ioas;
 	hwpt_paging->nest_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
 
-	if (ops->domain_alloc_user) {
-		hwpt->domain = ops->domain_alloc_user(idev->dev, flags, NULL,
-						      user_data);
+	if (ops->domain_alloc_paging_flags) {
+		hwpt->domain = ops->domain_alloc_paging_flags(idev->dev, flags,
+							      user_data);
 		if (IS_ERR(hwpt->domain)) {
 			rc = PTR_ERR(hwpt->domain);
 			hwpt->domain = NULL;
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index c58083c3660aee..a0de6d6d4e689c 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -379,9 +379,8 @@ mock_domain_alloc_nested(struct device *dev, struct iommu_domain *parent,
 }
 
 static struct iommu_domain *
-mock_domain_alloc_user(struct device *dev, u32 flags,
-		       struct iommu_domain *parent,
-		       const struct iommu_user_data *user_data)
+mock_domain_alloc_paging_flags(struct device *dev, u32 flags,
+			       const struct iommu_user_data *user_data)
 {
 	bool has_dirty_flag = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
 	const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
@@ -390,9 +389,6 @@ mock_domain_alloc_user(struct device *dev, u32 flags,
 			    MOCK_FLAGS_DEVICE_NO_DIRTY;
 	struct iommu_domain *domain;
 
-	if (parent)
-		return ERR_PTR(-EOPNOTSUPP);
-
 	if (user_data)
 		return ERR_PTR(-EOPNOTSUPP);
 	if ((flags & ~PAGING_FLAGS) || (has_dirty_flag && no_dirty_ops))
@@ -718,7 +714,7 @@ static const struct iommu_ops mock_ops = {
 	.pgsize_bitmap = MOCK_IO_PAGE_SIZE,
 	.hw_info = mock_domain_hw_info,
 	.domain_alloc_paging = mock_domain_alloc_paging,
-	.domain_alloc_user = mock_domain_alloc_user,
+	.domain_alloc_paging_flags = mock_domain_alloc_paging_flags,
 	.domain_alloc_nested = mock_domain_alloc_nested,
 	.capable = mock_domain_capable,
 	.device_group = generic_device_group,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0472cc1245192d..1e3308e899969d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -557,13 +557,17 @@ iommu_copy_struct_from_full_user_array(void *kdst, size_t kdst_entry_size,
  * @domain_alloc: allocate and return an iommu domain if success. Otherwise
  *                NULL is returned. The domain is not fully initialized until
  *                the caller iommu_domain_alloc() returns.
- * @domain_alloc_user: Allocate an iommu domain corresponding to the input
- *                     parameters as defined in include/uapi/linux/iommufd.h.
- *                     The @user_data can be optionally provided, the
- *                     new domain must support __IOMMU_DOMAIN_PAGING.
- *                     Upon failure, ERR_PTR must be returned.
+ * @domain_alloc_paging_flags: Allocate an iommu domain corresponding to the
+ *                     input parameters as defined in
+ *                     include/uapi/linux/iommufd.h. The @user_data can be
+ *                     optionally provided, the new domain must support
+ *                     __IOMMU_DOMAIN_PAGING. Upon failure, ERR_PTR must be
+ *                     returned.
  * @domain_alloc_paging: Allocate an iommu_domain that can be used for
- *                       UNMANAGED, DMA, and DMA_FQ domain types.
+ *                       UNMANAGED, DMA, and DMA_FQ domain types. This is the
+ *                       same as invoking domain_alloc_paging_flags() with
+ *                       @flags=0, @user_data=NULL. A driver should implement
+ *                       only one of the two ops.
  * @domain_alloc_sva: Allocate an iommu_domain for Shared Virtual Addressing.
  * @domain_alloc_nested: Allocate an iommu_domain for nested translation.
  * @probe_device: Add device to iommu driver handling
@@ -614,8 +618,8 @@ struct iommu_ops {
 
 	/* Domain allocation and freeing by the iommu driver */
 	struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
-	struct iommu_domain *(*domain_alloc_user)(
-		struct device *dev, u32 flags, struct iommu_domain *parent,
+	struct iommu_domain *(*domain_alloc_paging_flags)(
+		struct device *dev, u32 flags,
 		const struct iommu_user_data *user_data);
 	struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
 	struct iommu_domain *(*domain_alloc_sva)(struct device *dev,
-- 
2.43.0

Re: [PATCH 1/2] iommu: Add ops->domain_alloc_nested()

[Baolu Lu]

On 11/15/24 03:55, Jason Gunthorpe wrote:
> It turns out all the drivers that are using this immediately call into
> another function, so just make that function directly into the op. This
> makes paging=NULL for domain_alloc_user and we can remove the argument in
> the next patch.
> 
> The function mirrors the similar op in the viommu that allocates a nested
> domain on top of the viommu's nesting parent. This version supports cases
> where a viommu is not being used.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> ---
>   drivers/iommu/intel/iommu.c          |  9 +++------
>   drivers/iommu/intel/iommu.h          |  6 ++++--
>   drivers/iommu/intel/nested.c         | 11 +++++++++--
>   drivers/iommu/iommufd/hw_pagetable.c |  8 ++++----
>   drivers/iommu/iommufd/selftest.c     |  7 ++++---
>   include/linux/iommu.h                |  9 +++++----
>   6 files changed, 29 insertions(+), 21 deletions(-)
>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

with below minor suggestion.

[...]

> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
> index 42c4533a6ea21d..aba92c00b42740 100644
> --- a/drivers/iommu/intel/nested.c
> +++ b/drivers/iommu/intel/nested.c
> @@ -186,14 +186,21 @@ static const struct iommu_domain_ops intel_nested_domain_ops = {
>   	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
>   };
>   
> -struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
> -					       const struct iommu_user_data *user_data)
> +struct iommu_domain *
> +intel_iommu_domain_alloc_nested(struct device *dev, struct iommu_domain *parent,
> +				u32 flags,
> +				const struct iommu_user_data *user_data)
>   {
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
>   	struct dmar_domain *s2_domain = to_dmar_domain(parent);
> +	struct intel_iommu *iommu = info->iommu;
>   	struct iommu_hwpt_vtd_s1 vtd;
>   	struct dmar_domain *domain;
>   	int ret;
>   
> +	if (!nested_supported(iommu) || flags)
> +		return ERR_PTR(-EOPNOTSUPP);

How about making it like

         if (!nested_supported(iommu) || (flags & 
~IOMMU_HWPT_FAULT_ID_VALID))
                 return ERR_PTR(-EOPNOTSUPP);

         if ((flags & IOMMU_HWPT_FAULT_ID_VALID) && !info->pri_supported)
                 return ERR_PTR(-EOPNOTSUPP);

?

--
baolu

Re: [PATCH 2/2] iommu: Rename ops->domain_alloc_user() to domain_alloc_paging_flags()

[Baolu Lu]

On 11/15/24 03:55, Jason Gunthorpe wrote:
> Now that the main domain allocating path is calling this function it
> doesn't make sense to leave it named _user. Change the name to
> alloc_paging_flags() to mirror the new iommu_paging_domain_alloc_flags()
> function.
> 
> A driver should implement only one of ops->domain_alloc_paging() or
> ops->domain_alloc_paging_user(). The former is a simpler interface with

s/user/flags/

I think domain_alloc_paging and domain_alloc_paging_flags will
eventually be merged into a single domain_alloc_paging with flag
support.

> less boiler plate that the majority of drivers use. The latter is for
> drivers with a greater feature set (PASID, multiple page table support,
> advanced iommufd support, nesting, etc). Additional patches will be needed
> to achieve this.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Re: [PATCH 2/2] iommu: Rename ops->domain_alloc_user() to domain_alloc_paging_flags()

[Jason Gunthorpe]

On Fri, Nov 15, 2024 at 11:22:50AM +0800, Baolu Lu wrote:
> On 11/15/24 03:55, Jason Gunthorpe wrote:
> > Now that the main domain allocating path is calling this function it
> > doesn't make sense to leave it named _user. Change the name to
> > alloc_paging_flags() to mirror the new iommu_paging_domain_alloc_flags()
> > function.
> > 
> > A driver should implement only one of ops->domain_alloc_paging() or
> > ops->domain_alloc_paging_user(). The former is a simpler interface with
> 
> s/user/flags/
> 
> I think domain_alloc_paging and domain_alloc_paging_flags will
> eventually be merged into a single domain_alloc_paging with flag
> support.

I have patches to do that to arm, amd and intel. Alot of drivers will
just stay with domain_alloc_paging() because they don't need anything
more complex and there is no reason to spread around boilerplate

Jason

Re: [PATCH 1/2] iommu: Add ops->domain_alloc_nested()

[Jason Gunthorpe]

On Fri, Nov 15, 2024 at 11:19:03AM +0800, Baolu Lu wrote:

> > diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
> > index 42c4533a6ea21d..aba92c00b42740 100644
> > --- a/drivers/iommu/intel/nested.c
> > +++ b/drivers/iommu/intel/nested.c
> > @@ -186,14 +186,21 @@ static const struct iommu_domain_ops intel_nested_domain_ops = {
> >   	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
> >   };
> > -struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
> > -					       const struct iommu_user_data *user_data)
> > +struct iommu_domain *
> > +intel_iommu_domain_alloc_nested(struct device *dev, struct iommu_domain *parent,
> > +				u32 flags,
> > +				const struct iommu_user_data *user_data)
> >   {
> > +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> >   	struct dmar_domain *s2_domain = to_dmar_domain(parent);
> > +	struct intel_iommu *iommu = info->iommu;
> >   	struct iommu_hwpt_vtd_s1 vtd;
> >   	struct dmar_domain *domain;
> >   	int ret;
> > +	if (!nested_supported(iommu) || flags)
> > +		return ERR_PTR(-EOPNOTSUPP);
> 
> How about making it like
> 
>         if (!nested_supported(iommu) || (flags &
> ~IOMMU_HWPT_FAULT_ID_VALID))
>                 return ERR_PTR(-EOPNOTSUPP);
> 
>         if ((flags & IOMMU_HWPT_FAULT_ID_VALID) && !info->pri_supported)
>                 return ERR_PTR(-EOPNOTSUPP);

I think that is possibly a good idea for a followup, but right now it
was left like this:

+       hwpt->domain = ops->domain_alloc_nested(
+               idev->dev, parent->common.domain,
+               flags & ~IOMMU_HWPT_FAULT_ID_VALID, user_data);

So you'd also want to remove the FAUlT_ID_VALID masking there to move
it to the driver.

I'm also wondering if we should put the pri_supported concept into
core code flags and have less boilerplate in drivers.

Thanks,
Jason

Re: [PATCH 0/2] Rename ops->domain_alloc_user() to domain_alloc_paging_flags()

[Jason Gunthorpe]

On Thu, Nov 14, 2024 at 03:55:29PM -0400, Jason Gunthorpe wrote:
> This tidies some of the naming that has become messy this cycle. This
> series is on top of the iommufd and iommu tress as there are dependences
> on the iommufd selftests files changes.
> 
> I would like to push this into this merge window so that the next cycle
> can have the folowup got through the driver trees. Otherwise we will again
> have conflicts and problems. I have written additional patches to
> consolidate the drivers to use only domain_alloc_paging_flags() instead of
> implementing both.
> 
> So I propose sending this as a late pull request in the second week.
> 
> Jason Gunthorpe (2):
>   iommu: Add ops->domain_alloc_nested()
>   iommu: Rename ops->domain_alloc_user() to domain_alloc_paging_flags()

I'm going to stick this in linux-next the next few days and hope to
send it next week if there are no comments

Thanks,
Jason