[PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Yi Liu]

This splits the preparation works of the iommu and the Intel iommu driver
out from the iommufd pasid attach/replace series. [1]

To support domain replacement, the definition of the set_dev_pasid op
needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
should be extended as well to suit the new definition.

pasid attach/replace is mandatory on Intel VT-d given the PASID table
locates in the physical address space hence must be managed by the kernel,
both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD
which allow configuring the PASID/CD table either in host physical address
space or nested on top of an GPA address space. This series only extends
the Intel iommu driver as the minimal requirement.

This series first prepares the Intel iommu set_dev_pasid op for the new
definition, adds the missing set_dev_pasid support for nested domain, and
in the end enhances the definition of set_dev_pasid op. The ARM and AMD
set_dev_pasid callbacks is extended to fail if the caller tries to do domain
replacement to meet the new definition of set_dev_pasid op.

This series is on top of Baolu's paging domain alloc refactor, where his
code can be found at [2].

[1] https://lore.kernel.org/linux-iommu/20240412081516.31168-1-yi.l.liu@intel.com/
[2] https://github.com/LuBaolu/intel-iommu/commits/vtd-paging-domain-refactor-v1

Regards,
	Yi Liu

Lu Baolu (1):
  iommu/vt-d: Add set_dev_pasid callback for nested domain

Yi Liu (5):
  iommu: Pass old domain to set_dev_pasid op
  iommu/vt-d: Move intel_drain_pasid_prq() into
    intel_pasid_tear_down_entry()
  iommu/vt-d: Make helpers support modifying present pasid entry
  iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain
    replacement
  iommu: Make set_dev_pasid op support domain replacement

 drivers/iommu/amd/amd_iommu.h                 |   3 +-
 drivers/iommu/amd/pasid.c                     |   6 +-
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |   6 +-
 drivers/iommu/intel/debugfs.c                 |   2 +
 drivers/iommu/intel/iommu.c                   | 117 ++++++++++++------
 drivers/iommu/intel/iommu.h                   |   3 +
 drivers/iommu/intel/nested.c                  |   1 +
 drivers/iommu/intel/pasid.c                   |  46 +++----
 drivers/iommu/intel/pasid.h                   |   5 +-
 drivers/iommu/intel/svm.c                     |   9 +-
 drivers/iommu/iommu.c                         |   3 +-
 include/linux/iommu.h                         |   5 +-
 12 files changed, 132 insertions(+), 74 deletions(-)

-- 
2.34.1

[PATCH 1/6] iommu: Pass old domain to set_dev_pasid op

[Yi Liu]

To support domain replacement for pasid, the underlying iommu driver needs
to know the old domain hence be able to clean up the existing attachment.
It would be much convenient for iommu layer to pass down the old domain.
Otherwise, iommu drivers would need to track domain for pasids by themselves,
this would duplicate code among the iommu drivers. Or iommu drivers would
rely group->pasid_array to get domain, which may not always the correct
one.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/amd/amd_iommu.h                   | 3 ++-
 drivers/iommu/amd/pasid.c                       | 3 ++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 ++-
 drivers/iommu/intel/iommu.c                     | 6 ++++--
 drivers/iommu/intel/svm.c                       | 3 ++-
 drivers/iommu/iommu.c                           | 3 ++-
 include/linux/iommu.h                           | 2 +-
 7 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 2fde1302a584..31141c6bdf54 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -51,7 +51,8 @@ struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev,
 						struct mm_struct *mm);
 void amd_iommu_domain_free(struct iommu_domain *dom);
 int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
-			    struct device *dev, ioasid_t pasid);
+			    struct device *dev, ioasid_t pasid,
+			    struct iommu_domain *old);
 void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 				struct iommu_domain *domain);
 
diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
index a68215f2b3e1..77bf5f5f947a 100644
--- a/drivers/iommu/amd/pasid.c
+++ b/drivers/iommu/amd/pasid.c
@@ -100,7 +100,8 @@ static const struct mmu_notifier_ops sva_mn = {
 };
 
 int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
-			    struct device *dev, ioasid_t pasid)
+			    struct device *dev, ioasid_t pasid,
+			    struct iommu_domain *old)
 {
 	struct pdom_dev_data *pdom_dev_data;
 	struct protection_domain *sva_pdom = to_pdomain(domain);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index e490ffb38015..c058949749cb 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -631,7 +631,8 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 }
 
 static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
-				      struct device *dev, ioasid_t id)
+				      struct device *dev, ioasid_t id,
+				      struct iommu_domain *old)
 {
 	int ret = 0;
 	struct mm_struct *mm = domain->mm;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 129e5b685762..288c929b3d15 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4065,7 +4065,8 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 }
 
 static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid)
+				     struct device *dev, ioasid_t pasid,
+				     struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -4352,7 +4353,8 @@ static int identity_domain_attach_dev(struct iommu_domain *domain,
 }
 
 static int identity_domain_set_dev_pasid(struct iommu_domain *domain,
-					 struct device *dev, ioasid_t pasid)
+					 struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index ef12e95e400a..a5845ca94867 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -194,7 +194,8 @@ static const struct mmu_notifier_ops intel_mmuops = {
 };
 
 static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
-				   struct device *dev, ioasid_t pasid)
+				   struct device *dev, ioasid_t pasid,
+				   struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8212fed27e18..b3a1dabed2dd 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3274,7 +3274,8 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 	int ret;
 
 	for_each_group_device(group, device) {
-		ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
+		ret = domain->ops->set_dev_pasid(domain, device->dev,
+						 pasid, NULL);
 		if (ret)
 			goto err_revert;
 	}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index bff6e7e81fa3..a33f53aab61b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -633,7 +633,7 @@ struct iommu_ops {
 struct iommu_domain_ops {
 	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
 	int (*set_dev_pasid)(struct iommu_domain *domain, struct device *dev,
-			     ioasid_t pasid);
+			     ioasid_t pasid, struct iommu_domain *old);
 
 	int (*map_pages)(struct iommu_domain *domain, unsigned long iova,
 			 phys_addr_t paddr, size_t pgsize, size_t pgcount,
-- 
2.34.1

[PATCH 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Yi Liu]

Draining PRQ is needed before repurposing a PASID. It makes sense to invoke
it in the intel_pasid_tear_down_entry().

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c | 5 ++---
 drivers/iommu/intel/pasid.c | 9 ++++++++-
 drivers/iommu/intel/pasid.h | 5 ++---
 drivers/iommu/intel/svm.c   | 6 +++++-
 4 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 288c929b3d15..dd3de95c7122 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3241,7 +3241,7 @@ void device_block_translation(struct device *dev)
 	if (!dev_is_real_dma_subdevice(dev)) {
 		if (sm_supported(iommu))
 			intel_pasid_tear_down_entry(iommu, dev,
-						    IOMMU_NO_PASID, false);
+						    IOMMU_NO_PASID, false, false);
 		else
 			domain_context_clear(info);
 	}
@@ -4060,8 +4060,7 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
 	kfree(dev_pasid);
 out_tear_down:
-	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
-	intel_drain_pasid_prq(dev, pasid);
+	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
 }
 
 static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 573e1b8e3cfb..b18eebb479de 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -233,8 +233,12 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
 		qi_flush_dev_iotlb_pasid(iommu, sid, pfsid, pasid, qdep, 0, 64 - VTD_PAGE_SHIFT);
 }
 
+/*
+ * Not all PASID entry destroy requires PRQ drain as it can be handled in
+ * the remove_dev_pasid path. Caller should be clear on it.
+ */
 void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
-				 u32 pasid, bool fault_ignore)
+				 u32 pasid, bool fault_ignore, bool drain_prq)
 {
 	struct pasid_entry *pte;
 	u16 did, pgtt;
@@ -264,6 +268,9 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 	/* Device IOTLB doesn't need to be flushed in caching mode. */
 	if (!cap_caching_mode(iommu->cap))
 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
+
+	if (drain_prq)
+		intel_drain_pasid_prq(dev, pasid);
 }
 
 /*
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index da9978fef7ac..8b77b0d21c6e 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -313,9 +313,8 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 				   struct device *dev, u32 pasid);
 int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev,
 			     u32 pasid, struct dmar_domain *domain);
-void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
-				 struct device *dev, u32 pasid,
-				 bool fault_ignore);
+void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
+				 u32 pasid, bool fault_ignore, bool drain_prq);
 void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
 					  struct device *dev, u32 pasid);
 int intel_pasid_setup_sm_context(struct device *dev);
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index a5845ca94867..679e094d9f52 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -175,8 +175,12 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	spin_lock_irqsave(&domain->lock, flags);
 	list_for_each_entry(dev_pasid, &domain->dev_pasids, link_domain) {
 		info = dev_iommu_priv_get(dev_pasid->dev);
+		/*
+		 * PRQ drain would happen in the remove_dev_pasid() path,
+		 * no need to do it here.
+		 */
 		intel_pasid_tear_down_entry(info->iommu, dev_pasid->dev,
-					    dev_pasid->pasid, true);
+					    dev_pasid->pasid, true, false);
 	}
 	spin_unlock_irqrestore(&domain->lock, flags);
 
-- 
2.34.1

Re: [PATCH 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Baolu Lu]

On 2024/6/28 16:55, Yi Liu wrote:
> Draining PRQ is needed before repurposing a PASID. It makes sense to invoke
> it in the intel_pasid_tear_down_entry().

Can you please elaborate on the value of this merge?

Draining the PRQ is necessary when PRI is enabled on the device, and a
page table is about to be removed from the PASID. This might occur in
conjunction with tearing down a PASID entry, but it seems they are two
distinct actions.

Best regards,
baolu

Re: [PATCH 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Yi Liu]

On 2024/6/28 17:42, Baolu Lu wrote:
> On 2024/6/28 16:55, Yi Liu wrote:
>> Draining PRQ is needed before repurposing a PASID. It makes sense to invoke
>> it in the intel_pasid_tear_down_entry().
> 
> Can you please elaborate on the value of this merge?
> 

The major reason is that the next patch would have multiple places that
need to destroy pasid entry and do prq drain. Wrap them would make life
easier I suppose.

> Draining the PRQ is necessary when PRI is enabled on the device, and a
> page table is about to be removed from the PASID. This might occur in
> conjunction with tearing down a PASID entry, but it seems they are two
> distinct actions.

Seems like mostly they have conjunction, while there is indeed one
exception in the intel_mm_release(). Given the above reason, do you have
any suggestion for it?

-- 
Regards,
Yi Liu

RE: [PATCH 2/6] iommu/vt-d: Move intel_drain_pasid_prq() into intel_pasid_tear_down_entry()

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, June 28, 2024 6:52 PM
> 
> On 2024/6/28 17:42, Baolu Lu wrote:
> > On 2024/6/28 16:55, Yi Liu wrote:
> >> Draining PRQ is needed before repurposing a PASID. It makes sense to
> invoke
> >> it in the intel_pasid_tear_down_entry().
> >
> > Can you please elaborate on the value of this merge?
> >
> 
> The major reason is that the next patch would have multiple places that
> need to destroy pasid entry and do prq drain. Wrap them would make life
> easier I suppose.
> 
> > Draining the PRQ is necessary when PRI is enabled on the device, and a
> > page table is about to be removed from the PASID. This might occur in
> > conjunction with tearing down a PASID entry, but it seems they are two
> > distinct actions.
> 
> Seems like mostly they have conjunction, while there is indeed one
> exception in the intel_mm_release(). Given the above reason, do you have
> any suggestion for it?
> 

If we really have such need please don't add more boolean fields.

Let's extend the existing one into a flag instead, for better readability.

[PATCH 3/6] iommu/vt-d: Make helpers support modifying present pasid entry

[Yi Liu]

To handle domain replacement, set_dev_pasid op needs to modify a present
pasid entry. One way is sharing the most logics of remove_dev_pasid() in
the beginning of set_dev_pasid() to remove the old config. But this means
the set_dev_pasid path needs to rollback to the old config if it fails to
set up the new pasid entry. This needs to invoke the set_dev_pasid op of
the old domain. It breaks the iommu layering a bit. Another way is
implementing the set_dev_pasid() without rollback to old hardware config.
This can be achieved by implementing it in the order of preparing the
dev_pasid info for the new domain, modify the pasid entry, then undo the
dev_pasid info of the old domain, and if failed, undo the dev_pasid info
of the new domain. This would keep the old domain unchanged.

Following the second way, needs to make the pasid entry set up helpers
support modifying present pasid entry.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/pasid.c | 37 ++++++++++++-------------------------
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index b18eebb479de..5d3a12b081a2 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 		return -EINVAL;
 	}
 
+	/* Clear the old configuration if it already exists */
+	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
+
 	spin_lock(&iommu->lock);
 	pte = intel_pasid_get_entry(dev, pasid);
 	if (!pte) {
@@ -321,13 +324,6 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 		return -ENODEV;
 	}
 
-	if (pasid_pte_is_present(pte)) {
-		spin_unlock(&iommu->lock);
-		return -EBUSY;
-	}
-
-	pasid_clear_entry(pte);
-
 	/* Setup the first level page table pointer: */
 	pasid_set_flptr(pte, (u64)__pa(pgd));
 
@@ -378,6 +374,9 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 	pgd_val = virt_to_phys(pgd);
 	did = domain_id_iommu(domain, iommu);
 
+	/* Clear the old configuration if it already exists */
+	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
+
 	spin_lock(&iommu->lock);
 	pte = intel_pasid_get_entry(dev, pasid);
 	if (!pte) {
@@ -385,12 +384,6 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 		return -ENODEV;
 	}
 
-	if (pasid_pte_is_present(pte)) {
-		spin_unlock(&iommu->lock);
-		return -EBUSY;
-	}
-
-	pasid_clear_entry(pte);
 	pasid_set_domain_id(pte, did);
 	pasid_set_slptr(pte, pgd_val);
 	pasid_set_address_width(pte, domain->agaw);
@@ -488,6 +481,9 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 	u16 did = FLPT_DEFAULT_DID;
 	struct pasid_entry *pte;
 
+	/* Clear the old configuration if it already exists */
+	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
+
 	spin_lock(&iommu->lock);
 	pte = intel_pasid_get_entry(dev, pasid);
 	if (!pte) {
@@ -495,12 +491,6 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 		return -ENODEV;
 	}
 
-	if (pasid_pte_is_present(pte)) {
-		spin_unlock(&iommu->lock);
-		return -EBUSY;
-	}
-
-	pasid_clear_entry(pte);
 	pasid_set_domain_id(pte, did);
 	pasid_set_address_width(pte, iommu->agaw);
 	pasid_set_translation_type(pte, PASID_ENTRY_PGTT_PT);
@@ -606,18 +596,15 @@ int intel_pasid_setup_nested(struct intel_iommu *iommu, struct device *dev,
 		return -EINVAL;
 	}
 
+	/* Clear the old configuration if it already exists */
+	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
+
 	spin_lock(&iommu->lock);
 	pte = intel_pasid_get_entry(dev, pasid);
 	if (!pte) {
 		spin_unlock(&iommu->lock);
 		return -ENODEV;
 	}
-	if (pasid_pte_is_present(pte)) {
-		spin_unlock(&iommu->lock);
-		return -EBUSY;
-	}
-
-	pasid_clear_entry(pte);
 
 	if (s1_cfg->addr_width == ADDR_WIDTH_5LEVEL)
 		pasid_set_flpm(pte, 1);
-- 
2.34.1

Re: [PATCH 3/6] iommu/vt-d: Make helpers support modifying present pasid entry

[Baolu Lu]

On 2024/6/28 16:55, Yi Liu wrote:
> To handle domain replacement, set_dev_pasid op needs to modify a present
> pasid entry. One way is sharing the most logics of remove_dev_pasid() in
> the beginning of set_dev_pasid() to remove the old config. But this means
> the set_dev_pasid path needs to rollback to the old config if it fails to
> set up the new pasid entry. This needs to invoke the set_dev_pasid op of
> the old domain. It breaks the iommu layering a bit. Another way is
> implementing the set_dev_pasid() without rollback to old hardware config.
> This can be achieved by implementing it in the order of preparing the
> dev_pasid info for the new domain, modify the pasid entry, then undo the
> dev_pasid info of the old domain, and if failed, undo the dev_pasid info
> of the new domain. This would keep the old domain unchanged.
> 
> Following the second way, needs to make the pasid entry set up helpers
> support modifying present pasid entry.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/intel/pasid.c | 37 ++++++++++++-------------------------
>   1 file changed, 12 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index b18eebb479de..5d3a12b081a2 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
>   		return -EINVAL;
>   	}
>   
> +	/* Clear the old configuration if it already exists */
> +	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
> +
>   	spin_lock(&iommu->lock);
>   	pte = intel_pasid_get_entry(dev, pasid);
>   	if (!pte) {
> @@ -321,13 +324,6 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
>   		return -ENODEV;
>   	}
>   
> -	if (pasid_pte_is_present(pte)) {
> -		spin_unlock(&iommu->lock);
> -		return -EBUSY;
> -	}
> -
> -	pasid_clear_entry(pte);
> -
>   	/* Setup the first level page table pointer: */
>   	pasid_set_flptr(pte, (u64)__pa(pgd));

The above changes the previous assumption that when a new page table is
about to be set up on a PASID, there should be no existing one still in
place.

Is this a requirement for the replace functionality?

Best regards,
baolu

Re: [PATCH 3/6] iommu/vt-d: Make helpers support modifying present pasid entry

[Yi Liu]

On 2024/6/28 17:52, Baolu Lu wrote:
> On 2024/6/28 16:55, Yi Liu wrote:
>> To handle domain replacement, set_dev_pasid op needs to modify a present
>> pasid entry. One way is sharing the most logics of remove_dev_pasid() in
>> the beginning of set_dev_pasid() to remove the old config. But this means
>> the set_dev_pasid path needs to rollback to the old config if it fails to
>> set up the new pasid entry. This needs to invoke the set_dev_pasid op of
>> the old domain. It breaks the iommu layering a bit. Another way is
>> implementing the set_dev_pasid() without rollback to old hardware config.
>> This can be achieved by implementing it in the order of preparing the
>> dev_pasid info for the new domain, modify the pasid entry, then undo the
>> dev_pasid info of the old domain, and if failed, undo the dev_pasid info
>> of the new domain. This would keep the old domain unchanged.
>>
>> Following the second way, needs to make the pasid entry set up helpers
>> support modifying present pasid entry.
>>
>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 37 ++++++++++++-------------------------
>>   1 file changed, 12 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index b18eebb479de..5d3a12b081a2 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu 
>> *iommu,
>>           return -EINVAL;
>>       }
>> +    /* Clear the old configuration if it already exists */
>> +    intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
>> +

[1]

>>       spin_lock(&iommu->lock);
>>       pte = intel_pasid_get_entry(dev, pasid);
>>       if (!pte) {
>> @@ -321,13 +324,6 @@ int intel_pasid_setup_first_level(struct intel_iommu 
>> *iommu,
>>           return -ENODEV;
>>       }
>> -    if (pasid_pte_is_present(pte)) {
>> -        spin_unlock(&iommu->lock);
>> -        return -EBUSY;
>> -    }
>> -
>> -    pasid_clear_entry(pte);
>> -
>>       /* Setup the first level page table pointer: */
>>       pasid_set_flptr(pte, (u64)__pa(pgd));
> 
> The above changes the previous assumption that when a new page table is
> about to be set up on a PASID, there should be no existing one still in
> place.

actually, this does not break the assumption. In [1]. it already clears the
pasid entry.

> Is this a requirement for the replace functionality?

Replace would surely need to modify a present pasid entry. But this patch
only moves adds the pasid entry tear down and prq drain into the helpers.

-- 
Regards,
Yi Liu

RE: [PATCH 3/6] iommu/vt-d: Make helpers support modifying present pasid entry

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, June 28, 2024 4:56 PM
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index b18eebb479de..5d3a12b081a2 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu
> *iommu,
>  		return -EINVAL;
>  	}
> 
> +	/* Clear the old configuration if it already exists */
> +	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
> +
>  	spin_lock(&iommu->lock);
>  	pte = intel_pasid_get_entry(dev, pasid);
>  	if (!pte) {

with this change there will be two invocations on
intel_pasid_tear_down_entry() in the call stack of RID attach:

  intel_iommu_attach_device()
    device_block_translation()
      intel_pasid_tear_down_entry()
    dmar_domain_attach_device()
      domain_setup_first_level()
        intel_pasid_tear_down_entry()

it's not being a real problem as intel_pasid_tear_down_entry()
exits early if the pasid entry is non-present, but it will likely cause
confusion when reading the code.

What about moving it into intel_iommu_set_dev_pasid() to
better show the purpose?

Re: [PATCH 3/6] iommu/vt-d: Make helpers support modifying present pasid entry

[Yi Liu]

On 2024/7/15 15:53, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Friday, June 28, 2024 4:56 PM
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index b18eebb479de..5d3a12b081a2 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -314,6 +314,9 @@ int intel_pasid_setup_first_level(struct intel_iommu
>> *iommu,
>>   		return -EINVAL;
>>   	}
>>
>> +	/* Clear the old configuration if it already exists */
>> +	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
>> +
>>   	spin_lock(&iommu->lock);
>>   	pte = intel_pasid_get_entry(dev, pasid);
>>   	if (!pte) {
> 
> with this change there will be two invocations on
> intel_pasid_tear_down_entry() in the call stack of RID attach:
> 
>    intel_iommu_attach_device()
>      device_block_translation()
>        intel_pasid_tear_down_entry()
>      dmar_domain_attach_device()
>        domain_setup_first_level()
>          intel_pasid_tear_down_entry()
> 
> it's not being a real problem as intel_pasid_tear_down_entry()
> exits early if the pasid entry is non-present, but it will likely cause
> confusion when reading the code.
> 
> What about moving it into intel_iommu_set_dev_pasid() to
> better show the purpose?

I see. will do.

-- 
Regards,
Yi Liu

[PATCH 4/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Yi Liu]

set_dev_pasid op is going to support domain replacement and keep the old
hardware config if it fails. Make the Intel iommu driver be prepared for
it.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/debugfs.c |  2 +
 drivers/iommu/intel/iommu.c   | 94 +++++++++++++++++++++++------------
 2 files changed, 64 insertions(+), 32 deletions(-)

diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
index affbf4a1558d..8deb6bc262c5 100644
--- a/drivers/iommu/intel/debugfs.c
+++ b/drivers/iommu/intel/debugfs.c
@@ -806,5 +806,7 @@ void intel_iommu_debugfs_create_dev_pasid(struct dev_pasid_info *dev_pasid)
 /* Remove the device pasid debugfs directory. */
 void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info *dev_pasid)
 {
+	if (!dev_pasid->debugfs_dentry)
+		return;
 	debugfs_remove_recursive(dev_pasid->debugfs_dentry);
 }
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index dd3de95c7122..8ef6c06f7e73 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4031,8 +4031,8 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
 	return 0;
 }
 
-static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-					 struct iommu_domain *domain)
+static void domain_undo_dev_pasid(struct iommu_domain *domain,
+				  struct device *dev, ioasid_t pasid)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dev_pasid_info *curr, *dev_pasid = NULL;
@@ -4040,9 +4040,6 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	struct dmar_domain *dmar_domain;
 	unsigned long flags;
 
-	if (domain->type == IOMMU_DOMAIN_IDENTITY)
-		goto out_tear_down;
-
 	dmar_domain = to_dmar_domain(domain);
 	spin_lock_irqsave(&dmar_domain->lock, flags);
 	list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
@@ -4059,13 +4056,23 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 	domain_detach_iommu(dmar_domain, iommu);
 	intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
 	kfree(dev_pasid);
-out_tear_down:
+}
+
+static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *domain)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+
 	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
+	if (domain->type == IOMMU_DOMAIN_IDENTITY)
+		return;
+	domain_undo_dev_pasid(domain, dev, pasid);
 }
 
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid,
-				     struct iommu_domain *old)
+static struct dev_pasid_info *
+domain_prepare_dev_pasid(struct iommu_domain *domain,
+			 struct device *dev, ioasid_t pasid)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -4074,22 +4081,13 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	unsigned long flags;
 	int ret;
 
-	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
-		return -EOPNOTSUPP;
-
-	if (domain->dirty_ops)
-		return -EINVAL;
-
-	if (context_copied(iommu, info->bus, info->devfn))
-		return -EBUSY;
-
 	ret = prepare_domain_attach_device(domain, dev);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
 	if (!dev_pasid)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	ret = domain_attach_iommu(dmar_domain, iommu);
 	if (ret)
@@ -4099,6 +4097,43 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (ret)
 		goto out_detach_iommu;
 
+	dev_pasid->dev = dev;
+	dev_pasid->pasid = pasid;
+	spin_lock_irqsave(&dmar_domain->lock, flags);
+	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
+	spin_unlock_irqrestore(&dmar_domain->lock, flags);
+
+	return dev_pasid;
+out_detach_iommu:
+	domain_detach_iommu(dmar_domain, iommu);
+out_free:
+	kfree(dev_pasid);
+	return ERR_PTR(ret);
+}
+
+static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+				     struct device *dev, ioasid_t pasid,
+				     struct iommu_domain *old)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+	struct dev_pasid_info *dev_pasid;
+	int ret;
+
+	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
+		return -EOPNOTSUPP;
+
+	if (domain->dirty_ops)
+		return -EINVAL;
+
+	if (context_copied(iommu, info->bus, info->devfn))
+		return -EBUSY;
+
+	dev_pasid = domain_prepare_dev_pasid(domain, dev, pasid);
+	if (IS_ERR(dev_pasid))
+		return PTR_ERR(dev_pasid);
+
 	if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid);
@@ -4106,24 +4141,19 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
 						     dev, pasid);
 	if (ret)
-		goto out_unassign_tag;
+		goto out_undo_dev_pasid;
 
-	dev_pasid->dev = dev;
-	dev_pasid->pasid = pasid;
-	spin_lock_irqsave(&dmar_domain->lock, flags);
-	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
-	spin_unlock_irqrestore(&dmar_domain->lock, flags);
+	/* Undo the association between the old domain and pasid of a device */
+	if (old)
+		domain_undo_dev_pasid(old, dev, pasid);
 
 	if (domain->type & __IOMMU_DOMAIN_PAGING)
 		intel_iommu_debugfs_create_dev_pasid(dev_pasid);
 
 	return 0;
-out_unassign_tag:
-	cache_tag_unassign_domain(dmar_domain, dev, pasid);
-out_detach_iommu:
-	domain_detach_iommu(dmar_domain, iommu);
-out_free:
-	kfree(dev_pasid);
+
+out_undo_dev_pasid:
+	domain_undo_dev_pasid(domain, dev, pasid);
 	return ret;
 }
 
-- 
2.34.1

RE: [PATCH 4/6] iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain replacement

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, June 28, 2024 4:56 PM
> 
> @@ -806,5 +806,7 @@ void intel_iommu_debugfs_create_dev_pasid(struct
> dev_pasid_info *dev_pasid)
>  /* Remove the device pasid debugfs directory. */
>  void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info
> *dev_pasid)
>  {
> +	if (!dev_pasid->debugfs_dentry)
> +		return;
>  	debugfs_remove_recursive(dev_pasid->debugfs_dentry);

debugfs_remove_recursive() already includes a check on null entry.

> -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t
> pasid,
> -					 struct iommu_domain *domain)
> +static void domain_undo_dev_pasid(struct iommu_domain *domain,
> +				  struct device *dev, ioasid_t pasid)

'domain_remove_dev_pasid'

> +static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t
> pasid,
> +					 struct iommu_domain *domain)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct intel_iommu *iommu = info->iommu;
> +
>  	intel_pasid_tear_down_entry(iommu, dev, pasid, false, true);
> +	if (domain->type == IOMMU_DOMAIN_IDENTITY)
> +		return;
> +	domain_undo_dev_pasid(domain, dev, pasid);

this reverts the order by tearing down the pasid entry in the beginning.
but I cannot think of a problem here.

> 
> -	dev_pasid->dev = dev;
> -	dev_pasid->pasid = pasid;
> -	spin_lock_irqsave(&dmar_domain->lock, flags);
> -	list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids);
> -	spin_unlock_irqrestore(&dmar_domain->lock, flags);
> +	/* Undo the association between the old domain and pasid of a
> device */
> +	if (old)
> +		domain_undo_dev_pasid(old, dev, pasid);

the comment is useless

[PATCH 5/6] iommu/vt-d: Add set_dev_pasid callback for nested domain

[Yi Liu]

From: Lu Baolu <baolu.lu@linux.intel.com>

Extend intel_iommu_set_dev_pasid() to set a nested type domain to a PASID
of a device.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Co-developed-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c  | 22 +++++++++++++++++-----
 drivers/iommu/intel/iommu.h  |  3 +++
 drivers/iommu/intel/nested.c |  1 +
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 8ef6c06f7e73..b7051d9460cd 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -284,6 +284,11 @@ static int __init intel_iommu_setup(char *str)
 }
 __setup("intel_iommu=", intel_iommu_setup);
 
+static int domain_type_is_nested(struct dmar_domain *domain)
+{
+	return domain->domain.type == IOMMU_DOMAIN_NESTED;
+}
+
 static int domain_pfn_supported(struct dmar_domain *domain, unsigned long pfn)
 {
 	int addr_width = agaw_to_width(domain->agaw) - VTD_PAGE_SHIFT;
@@ -4081,7 +4086,12 @@ domain_prepare_dev_pasid(struct iommu_domain *domain,
 	unsigned long flags;
 	int ret;
 
-	ret = prepare_domain_attach_device(domain, dev);
+	/* Nested type domain should prepare its parent domain */
+	if (domain_type_is_nested(dmar_domain))
+		ret = prepare_domain_attach_device(
+				&dmar_domain->s2_domain->domain, dev);
+	else
+		ret = prepare_domain_attach_device(domain, dev);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -4111,9 +4121,9 @@ domain_prepare_dev_pasid(struct iommu_domain *domain,
 	return ERR_PTR(ret);
 }
 
-static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
-				     struct device *dev, ioasid_t pasid,
-				     struct iommu_domain *old)
+int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+			      struct device *dev, ioasid_t pasid,
+			      struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
@@ -4134,7 +4144,9 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (IS_ERR(dev_pasid))
 		return PTR_ERR(dev_pasid);
 
-	if (dmar_domain->use_first_level)
+	if (domain_type_is_nested(dmar_domain))
+		ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain);
+	else if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid);
 	else
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index eaf015b4353b..63ef3a8a8832 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1104,6 +1104,9 @@ void device_block_translation(struct device *dev);
 int prepare_domain_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 void domain_update_iommu_cap(struct dmar_domain *domain);
+int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+			      struct device *dev, ioasid_t pasid,
+			      struct iommu_domain *old);
 
 int dmar_ir_support(void);
 
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index 16a2bcf5cfeb..179868d34a4b 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -133,6 +133,7 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain,
 
 static const struct iommu_domain_ops intel_nested_domain_ops = {
 	.attach_dev		= intel_nested_attach_dev,
+	.set_dev_pasid		= intel_iommu_set_dev_pasid,
 	.free			= intel_nested_domain_free,
 	.cache_invalidate_user	= intel_nested_cache_invalidate_user,
 };
-- 
2.34.1

[PATCH 6/6] iommu: Make set_dev_pasid op support domain replacement

[Yi Liu]

The iommu core is going to support domain replacement for pasid, it needs
to make the set_dev_pasid op support replacing domain and keep the old
domain config in the failure case.

Currently only the Intel iommu driver supports the latest set_dev_pasid
op definition. ARM and AMD iommu driver do not support domain replacement
for pasid yet, both drivers would fail the set_dev_pasid op to keep the
old config if the input @old is non-NULL.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/amd/pasid.c                       | 3 +++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++
 include/linux/iommu.h                           | 3 ++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
index 77bf5f5f947a..30e27bda3fac 100644
--- a/drivers/iommu/amd/pasid.c
+++ b/drivers/iommu/amd/pasid.c
@@ -109,6 +109,9 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
 	unsigned long flags;
 	int ret = -EINVAL;
 
+	if (old)
+		return -EOPNOTSUPP;
+
 	/* PASID zero is used for requests from the I/O device without PASID */
 	if (!is_pasid_valid(dev_data, pasid))
 		return ret;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index c058949749cb..a1e411c71efa 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -637,6 +637,9 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 	int ret = 0;
 	struct mm_struct *mm = domain->mm;
 
+	if (old)
+		return -EOPNOTSUPP;
+
 	if (mm_get_enqcmd_pasid(mm) != id)
 		return -EINVAL;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a33f53aab61b..3259f77ff2e3 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -607,7 +607,8 @@ struct iommu_ops {
  * * EBUSY	- device is attached to a domain and cannot be changed
  * * ENODEV	- device specific errors, not able to be attached
  * * <others>	- treated as ENODEV by the caller. Use is discouraged
- * @set_dev_pasid: set an iommu domain to a pasid of device
+ * @set_dev_pasid: set or replace an iommu domain to a pasid of device. The pasid of
+ *                 the device should be left in the old config in error case.
  * @map_pages: map a physically contiguous set of pages of the same size to
  *             an iommu domain.
  * @unmap_pages: unmap a number of pages of the same size from an iommu domain
-- 
2.34.1

RE: [PATCH 6/6] iommu: Make set_dev_pasid op support domain replacement

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, June 28, 2024 4:56 PM
> 
> The iommu core is going to support domain replacement for pasid, it needs
> to make the set_dev_pasid op support replacing domain and keep the old
> domain config in the failure case.
> 
> Currently only the Intel iommu driver supports the latest set_dev_pasid
> op definition. ARM and AMD iommu driver do not support domain
> replacement
> for pasid yet, both drivers would fail the set_dev_pasid op to keep the
> old config if the input @old is non-NULL.

why splitting this from patch01?

> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/amd/pasid.c                       | 3 +++
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++
>  include/linux/iommu.h                           | 3 ++-
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
> index 77bf5f5f947a..30e27bda3fac 100644
> --- a/drivers/iommu/amd/pasid.c
> +++ b/drivers/iommu/amd/pasid.c
> @@ -109,6 +109,9 @@ int iommu_sva_set_dev_pasid(struct iommu_domain
> *domain,
>  	unsigned long flags;
>  	int ret = -EINVAL;
> 
> +	if (old)
> +		return -EOPNOTSUPP;
> +
>  	/* PASID zero is used for requests from the I/O device without PASID
> */
>  	if (!is_pasid_valid(dev_data, pasid))
>  		return ret;
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index c058949749cb..a1e411c71efa 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -637,6 +637,9 @@ static int arm_smmu_sva_set_dev_pasid(struct
> iommu_domain *domain,
>  	int ret = 0;
>  	struct mm_struct *mm = domain->mm;
> 
> +	if (old)
> +		return -EOPNOTSUPP;
> +
>  	if (mm_get_enqcmd_pasid(mm) != id)
>  		return -EINVAL;
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a33f53aab61b..3259f77ff2e3 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -607,7 +607,8 @@ struct iommu_ops {
>   * * EBUSY	- device is attached to a domain and cannot be changed
>   * * ENODEV	- device specific errors, not able to be attached
>   * * <others>	- treated as ENODEV by the caller. Use is discouraged
> - * @set_dev_pasid: set an iommu domain to a pasid of device
> + * @set_dev_pasid: set or replace an iommu domain to a pasid of device.
> The pasid of
> + *                 the device should be left in the old config in error case.
>   * @map_pages: map a physically contiguous set of pages of the same size
> to
>   *             an iommu domain.
>   * @unmap_pages: unmap a number of pages of the same size from an
> iommu domain
> --
> 2.34.1

Re: [PATCH 6/6] iommu: Make set_dev_pasid op support domain replacement

[Yi Liu]

On 2024/7/15 16:02, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Friday, June 28, 2024 4:56 PM
>>
>> The iommu core is going to support domain replacement for pasid, it needs
>> to make the set_dev_pasid op support replacing domain and keep the old
>> domain config in the failure case.
>>
>> Currently only the Intel iommu driver supports the latest set_dev_pasid
>> op definition. ARM and AMD iommu driver do not support domain
>> replacement
>> for pasid yet, both drivers would fail the set_dev_pasid op to keep the
>> old config if the input @old is non-NULL.
> 
> why splitting this from patch01?

The major reason is we already have multiple set_dev_pasid callbacks. It's
better to make the existing callbacks suit the new definition before
claiming it. Although this is not quite true for ARM and AMD since their
set_dev_pasid() callback would fail replacement attempts. But it looks
like there is no pasid domain replacement usage on AMD and ARM due to arch
difference.

>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/amd/pasid.c                       | 3 +++
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +++
>>   include/linux/iommu.h                           | 3 ++-
>>   3 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
>> index 77bf5f5f947a..30e27bda3fac 100644
>> --- a/drivers/iommu/amd/pasid.c
>> +++ b/drivers/iommu/amd/pasid.c
>> @@ -109,6 +109,9 @@ int iommu_sva_set_dev_pasid(struct iommu_domain
>> *domain,
>>   	unsigned long flags;
>>   	int ret = -EINVAL;
>>
>> +	if (old)
>> +		return -EOPNOTSUPP;
>> +
>>   	/* PASID zero is used for requests from the I/O device without PASID
>> */
>>   	if (!is_pasid_valid(dev_data, pasid))
>>   		return ret;
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> index c058949749cb..a1e411c71efa 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
>> @@ -637,6 +637,9 @@ static int arm_smmu_sva_set_dev_pasid(struct
>> iommu_domain *domain,
>>   	int ret = 0;
>>   	struct mm_struct *mm = domain->mm;
>>
>> +	if (old)
>> +		return -EOPNOTSUPP;
>> +
>>   	if (mm_get_enqcmd_pasid(mm) != id)
>>   		return -EINVAL;
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index a33f53aab61b..3259f77ff2e3 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -607,7 +607,8 @@ struct iommu_ops {
>>    * * EBUSY	- device is attached to a domain and cannot be changed
>>    * * ENODEV	- device specific errors, not able to be attached
>>    * * <others>	- treated as ENODEV by the caller. Use is discouraged
>> - * @set_dev_pasid: set an iommu domain to a pasid of device
>> + * @set_dev_pasid: set or replace an iommu domain to a pasid of device.
>> The pasid of
>> + *                 the device should be left in the old config in error case.
>>    * @map_pages: map a physically contiguous set of pages of the same size
>> to
>>    *             an iommu domain.
>>    * @unmap_pages: unmap a number of pages of the same size from an
>> iommu domain
>> --
>> 2.34.1
> 

-- 
Regards,
Yi Liu

RE: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Friday, June 28, 2024 4:56 PM
> 
> This splits the preparation works of the iommu and the Intel iommu driver
> out from the iommufd pasid attach/replace series. [1]
> 
> To support domain replacement, the definition of the set_dev_pasid op
> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
> should be extended as well to suit the new definition.
> 
> pasid attach/replace is mandatory on Intel VT-d given the PASID table
> locates in the physical address space hence must be managed by the kernel,
> both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD
> which allow configuring the PASID/CD table either in host physical address
> space or nested on top of an GPA address space. This series only extends
> the Intel iommu driver as the minimal requirement.

Looks above is only within VFIO/IOMMUFD context (copied from the old
series). But this series is all in IOMMU and pasid attach is certainly not
optional for SVA on all platforms. this needs to be revised.

> 
> This series first prepares the Intel iommu set_dev_pasid op for the new
> definition, adds the missing set_dev_pasid support for nested domain, and
> in the end enhances the definition of set_dev_pasid op. The ARM and AMD
> set_dev_pasid callbacks is extended to fail if the caller tries to do domain
> replacement to meet the new definition of set_dev_pasid op.
> 
> This series is on top of Baolu's paging domain alloc refactor, where his
> code can be found at [2].
> 
> [1] https://lore.kernel.org/linux-iommu/20240412081516.31168-1-
> yi.l.liu@intel.com/
> [2] https://github.com/LuBaolu/intel-iommu/commits/vtd-paging-domain-
> refactor-v1
> 
> Regards,
> 	Yi Liu
> 
> Lu Baolu (1):
>   iommu/vt-d: Add set_dev_pasid callback for nested domain
> 
> Yi Liu (5):
>   iommu: Pass old domain to set_dev_pasid op
>   iommu/vt-d: Move intel_drain_pasid_prq() into
>     intel_pasid_tear_down_entry()
>   iommu/vt-d: Make helpers support modifying present pasid entry
>   iommu/vt-d: Make intel_iommu_set_dev_pasid() to handle domain
>     replacement
>   iommu: Make set_dev_pasid op support domain replacement
> 
>  drivers/iommu/amd/amd_iommu.h                 |   3 +-
>  drivers/iommu/amd/pasid.c                     |   6 +-
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |   6 +-
>  drivers/iommu/intel/debugfs.c                 |   2 +
>  drivers/iommu/intel/iommu.c                   | 117 ++++++++++++------
>  drivers/iommu/intel/iommu.h                   |   3 +
>  drivers/iommu/intel/nested.c                  |   1 +
>  drivers/iommu/intel/pasid.c                   |  46 +++----
>  drivers/iommu/intel/pasid.h                   |   5 +-
>  drivers/iommu/intel/svm.c                     |   9 +-
>  drivers/iommu/iommu.c                         |   3 +-
>  include/linux/iommu.h                         |   5 +-
>  12 files changed, 132 insertions(+), 74 deletions(-)
> 
> --
> 2.34.1

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Jason Gunthorpe]

On Wed, Jul 10, 2024 at 08:24:16AM +0000, Tian, Kevin wrote:
> > From: Liu, Yi L <yi.l.liu@intel.com>
> > Sent: Friday, June 28, 2024 4:56 PM
> > 
> > This splits the preparation works of the iommu and the Intel iommu driver
> > out from the iommufd pasid attach/replace series. [1]
> > 
> > To support domain replacement, the definition of the set_dev_pasid op
> > needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
> > should be extended as well to suit the new definition.
> > 
> > pasid attach/replace is mandatory on Intel VT-d given the PASID table
> > locates in the physical address space hence must be managed by the kernel,
> > both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD
> > which allow configuring the PASID/CD table either in host physical address
> > space or nested on top of an GPA address space. This series only extends
> > the Intel iommu driver as the minimal requirement.
> 
> Looks above is only within VFIO/IOMMUFD context (copied from the old
> series). But this series is all in IOMMU and pasid attach is certainly not
> optional for SVA on all platforms. this needs to be revised.

I feel like we should explicitly block replace on AMD by checking if
old_domain is !NULL and failing.

Then the description is sort of like

Replace is useful for iommufd/VFIO to provide perfect HW emulation in
case the VM is expecting to be able to change a PASID on the fly. As
AMD will only support PASID in VM's using nested translation where we
don't use the set_dev_pasid API leave it disabled for now.

Jason

RE: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, July 12, 2024 2:41 AM
> 
> On Wed, Jul 10, 2024 at 08:24:16AM +0000, Tian, Kevin wrote:
> > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > Sent: Friday, June 28, 2024 4:56 PM
> > >
> > > This splits the preparation works of the iommu and the Intel iommu
> driver
> > > out from the iommufd pasid attach/replace series. [1]
> > >
> > > To support domain replacement, the definition of the set_dev_pasid op
> > > needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
> > > should be extended as well to suit the new definition.
> > >
> > > pasid attach/replace is mandatory on Intel VT-d given the PASID table
> > > locates in the physical address space hence must be managed by the
> kernel,
> > > both for supporting vSVA and coming SIOV. But it's optional on
> ARM/AMD
> > > which allow configuring the PASID/CD table either in host physical
> address
> > > space or nested on top of an GPA address space. This series only extends
> > > the Intel iommu driver as the minimal requirement.
> >
> > Looks above is only within VFIO/IOMMUFD context (copied from the old
> > series). But this series is all in IOMMU and pasid attach is certainly not
> > optional for SVA on all platforms. this needs to be revised.
> 
> I feel like we should explicitly block replace on AMD by checking if
> old_domain is !NULL and failing.

this is what patch06 does.

> 
> Then the description is sort of like
> 
> Replace is useful for iommufd/VFIO to provide perfect HW emulation in
> case the VM is expecting to be able to change a PASID on the fly. As
> AMD will only support PASID in VM's using nested translation where we
> don't use the set_dev_pasid API leave it disabled for now.
> 

yes that's clearer.

btw I don't remember whether we have discussed the rationale behind
the different driver semantics between RID and PASID. Currently RID
replace is same as RID attach, with the driver simply blocking the old
translation from the start and then no rollback upon failure when
switching to the new domain (expecting the iommu core to recover),
while for PASID replace we expect the driver to implement the hitless
switch.

Is it because there is no need of perfect HW emulation for RID or just
to be cleaned up later?

This difference at least starts bringing some puzzle to me when
reading the related code in the intel-iommu driver.

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Jason Gunthorpe]

On Mon, Jul 15, 2024 at 08:16:40AM +0000, Tian, Kevin wrote:

> > Then the description is sort of like
> > 
> > Replace is useful for iommufd/VFIO to provide perfect HW emulation in
> > case the VM is expecting to be able to change a PASID on the fly. As
> > AMD will only support PASID in VM's using nested translation where we
> > don't use the set_dev_pasid API leave it disabled for now.
> > 
> 
> yes that's clearer.
> 
> btw I don't remember whether we have discussed the rationale behind
> the different driver semantics between RID and PASID. Currently RID
> replace is same as RID attach, with the driver simply blocking the old
> translation from the start and then no rollback upon failure when
> switching to the new domain (expecting the iommu core to recover),
> while for PASID replace we expect the driver to implement the hitless
> switch.
> 
> Is it because there is no need of perfect HW emulation for RID or just
> to be cleaned up later?

Yeah, too much legacy it would be hard to go in and do something about
this for RID across every driver.

PASID can be a bit more well defined from the start since we only have
three drivers and two of them support replace.

An ideal driver should be like ARM and support hitless replace
whenever possible in all paths so there is no confusion within the
driver.

Jason

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Yi Liu]

On 2024/7/12 02:41, Jason Gunthorpe wrote:
> On Wed, Jul 10, 2024 at 08:24:16AM +0000, Tian, Kevin wrote:
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Friday, June 28, 2024 4:56 PM
>>>
>>> This splits the preparation works of the iommu and the Intel iommu driver
>>> out from the iommufd pasid attach/replace series. [1]
>>>
>>> To support domain replacement, the definition of the set_dev_pasid op
>>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
>>> should be extended as well to suit the new definition.
>>>
>>> pasid attach/replace is mandatory on Intel VT-d given the PASID table
>>> locates in the physical address space hence must be managed by the kernel,
>>> both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD
>>> which allow configuring the PASID/CD table either in host physical address
>>> space or nested on top of an GPA address space. This series only extends
>>> the Intel iommu driver as the minimal requirement.
>>
>> Looks above is only within VFIO/IOMMUFD context (copied from the old
>> series). But this series is all in IOMMU and pasid attach is certainly not
>> optional for SVA on all platforms. this needs to be revised.
> 
> I feel like we should explicitly block replace on AMD by checking if
> old_domain is !NULL and failing.

yes, patch 6 of this series has made it fail in AMD's set_dev_pasid
callback.

> Then the description is sort of like
> 
> Replace is useful for iommufd/VFIO to provide perfect HW emulation in
> case the VM is expecting to be able to change a PASID on the fly. As
> AMD will only support PASID in VM's using nested translation where we
> don't use the set_dev_pasid API leave it disabled for now.

Does it apply to SMMUv3 as well? IIRC. ARM SMMUv3 also has the CD table
(a.k.a PASID table) within the guest. And I think this is the major reason
for your above statement. right?

-- 
Regards,
Yi Liu

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Jason Gunthorpe]

On Mon, Jul 15, 2024 at 04:23:07PM +0800, Yi Liu wrote:
> > Then the description is sort of like
> > 
> > Replace is useful for iommufd/VFIO to provide perfect HW emulation in
> > case the VM is expecting to be able to change a PASID on the fly. As
> > AMD will only support PASID in VM's using nested translation where we
> > don't use the set_dev_pasid API leave it disabled for now.
> 
> Does it apply to SMMUv3 as well? IIRC. ARM SMMUv3 also has the CD table
> (a.k.a PASID table) within the guest. And I think this is the major reason
> for your above statement. right?

It does, but it also supports replace so no need to explain why we
don't enable it :)

Jason

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Yi Liu]

On 2024/7/15 20:19, Jason Gunthorpe wrote:
> On Mon, Jul 15, 2024 at 04:23:07PM +0800, Yi Liu wrote:
>>> Then the description is sort of like
>>>
>>> Replace is useful for iommufd/VFIO to provide perfect HW emulation in
>>> case the VM is expecting to be able to change a PASID on the fly. As
>>> AMD will only support PASID in VM's using nested translation where we
>>> don't use the set_dev_pasid API leave it disabled for now.
>>
>> Does it apply to SMMUv3 as well? IIRC. ARM SMMUv3 also has the CD table
>> (a.k.a PASID table) within the guest. And I think this is the major reason
>> for your above statement. right?
> 
> It does, but it also supports replace so no need to explain why we
> don't enable it :)

got it. :)

-- 
Regards,
Yi Liu

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Jason Gunthorpe]

On Fri, Jun 28, 2024 at 01:55:32AM -0700, Yi Liu wrote:
> This splits the preparation works of the iommu and the Intel iommu driver
> out from the iommufd pasid attach/replace series. [1]
> 
> To support domain replacement, the definition of the set_dev_pasid op
> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
> should be extended as well to suit the new definition.
> 
> pasid attach/replace is mandatory on Intel VT-d given the PASID table
> locates in the physical address space hence must be managed by the kernel,
> both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD
> which allow configuring the PASID/CD table either in host physical address
> space or nested on top of an GPA address space. This series only extends
> the Intel iommu driver as the minimal requirement.

Sicne this will be pushed to the next cyle that will have my ARM code
the smmuv3 will need to be updated too. It is already prepped to
support replace, just add this please:


diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index ead83d67421f10..44434978a218ae 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -350,7 +351,8 @@ void arm_smmu_sva_notifier_synchronize(void)
 }
 
 static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
-				      struct device *dev, ioasid_t id)
+				      struct device *dev, ioasid_t id,
+				      struct iommu_domain *old_domain)
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
@@ -367,7 +369,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 	 */
 	arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->asid,
 			     smmu_domain->btm_invalidation);
-	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target);
+	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old_domain);
 
 	mmput(domain->mm);
 	return ret;
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 238968b1709936..140aac5cd4ef57 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2943,7 +2943,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 }
 
 static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
-				      struct device *dev, ioasid_t id)
+				      struct device *dev, ioasid_t id,
+				      struct iommu_domain *old_domain)
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
@@ -2969,7 +2970,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
 	 */
 	arm_smmu_make_s1_cd(&target_cd, master, smmu_domain);
 	return arm_smmu_set_pasid(master, to_smmu_domain(domain), id,
-				  &target_cd);
+				  &target_cd, old_domain);
 }
 
 static void arm_smmu_update_ste(struct arm_smmu_master *master,
@@ -2999,7 +3000,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master,
 
 int arm_smmu_set_pasid(struct arm_smmu_master *master,
 		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
-		       struct arm_smmu_cd *cd)
+		       struct arm_smmu_cd *cd, struct iommu_domain *old_domain)
 {
 	struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev);
 	struct arm_smmu_attach_state state = {
@@ -3009,6 +3010,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
 		 * already attached, no need to set old_domain.
 		 */
 		.ssid = pasid,
+		.old_domain = old_domain,
 	};
 	struct arm_smmu_cd *cdptr;
 	int ret;
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 bcf9ea9d929f5f..447a3cdf1c4e1c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -828,7 +828,7 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
 
 int arm_smmu_set_pasid(struct arm_smmu_master *master,
 		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
-		       struct arm_smmu_cd *cd);
+		       struct arm_smmu_cd *cd, struct iommu_domain *old_domain);
 
 int arm_smmu_domain_alloc_id(struct arm_smmu_device *smmu,
 			     struct arm_smmu_domain *smmu_domain);

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Yi Liu]

On 2024/7/12 02:37, Jason Gunthorpe wrote:
> On Fri, Jun 28, 2024 at 01:55:32AM -0700, Yi Liu wrote:
>> This splits the preparation works of the iommu and the Intel iommu driver
>> out from the iommufd pasid attach/replace series. [1]
>>
>> To support domain replacement, the definition of the set_dev_pasid op
>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
>> should be extended as well to suit the new definition.
>>
>> pasid attach/replace is mandatory on Intel VT-d given the PASID table
>> locates in the physical address space hence must be managed by the kernel,
>> both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD
>> which allow configuring the PASID/CD table either in host physical address
>> space or nested on top of an GPA address space. This series only extends
>> the Intel iommu driver as the minimal requirement.
> 
> Sicne this will be pushed to the next cyle that will have my ARM code
> the smmuv3 will need to be updated too. It is already prepped to
> support replace, just add this please:

thanks. So your related series has made the internal helpers to support
domain replacement in set_dev_pasid path. The below diff just passes the
old_domain across the helpers. Is it? Just to double confirm with you. :)

> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index ead83d67421f10..44434978a218ae 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -350,7 +351,8 @@ void arm_smmu_sva_notifier_synchronize(void)
>   }
>   
>   static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
> -				      struct device *dev, ioasid_t id)
> +				      struct device *dev, ioasid_t id,
> +				      struct iommu_domain *old_domain)
>   {
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> @@ -367,7 +369,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
>   	 */
>   	arm_smmu_make_sva_cd(&target, master, domain->mm, smmu_domain->asid,
>   			     smmu_domain->btm_invalidation);
> -	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target);
> +	ret = arm_smmu_set_pasid(master, smmu_domain, id, &target, old_domain);
>   
>   	mmput(domain->mm);
>   	return ret;
> 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 238968b1709936..140aac5cd4ef57 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2943,7 +2943,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   }
>   
>   static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
> -				      struct device *dev, ioasid_t id)
> +				      struct device *dev, ioasid_t id,
> +				      struct iommu_domain *old_domain)
>   {
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> @@ -2969,7 +2970,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
>   	 */
>   	arm_smmu_make_s1_cd(&target_cd, master, smmu_domain);
>   	return arm_smmu_set_pasid(master, to_smmu_domain(domain), id,
> -				  &target_cd);
> +				  &target_cd, old_domain);
>   }
>   
>   static void arm_smmu_update_ste(struct arm_smmu_master *master,
> @@ -2999,7 +3000,7 @@ static void arm_smmu_update_ste(struct arm_smmu_master *master,
>   
>   int arm_smmu_set_pasid(struct arm_smmu_master *master,
>   		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
> -		       struct arm_smmu_cd *cd)
> +		       struct arm_smmu_cd *cd, struct iommu_domain *old_domain)
>   {
>   	struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev);
>   	struct arm_smmu_attach_state state = {
> @@ -3009,6 +3010,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
>   		 * already attached, no need to set old_domain.
>   		 */
>   		.ssid = pasid,
> +		.old_domain = old_domain,
>   	};
>   	struct arm_smmu_cd *cdptr;
>   	int ret;
> 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 bcf9ea9d929f5f..447a3cdf1c4e1c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -828,7 +828,7 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
>   
>   int arm_smmu_set_pasid(struct arm_smmu_master *master,
>   		       struct arm_smmu_domain *smmu_domain, ioasid_t pasid,
> -		       struct arm_smmu_cd *cd);
> +		       struct arm_smmu_cd *cd, struct iommu_domain *old_domain);
>   
>   int arm_smmu_domain_alloc_id(struct arm_smmu_device *smmu,
>   			     struct arm_smmu_domain *smmu_domain);

-- 
Regards,
Yi Liu

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Jason Gunthorpe]

On Mon, Jul 15, 2024 at 04:11:43PM +0800, Yi Liu wrote:
> On 2024/7/12 02:37, Jason Gunthorpe wrote:
> > On Fri, Jun 28, 2024 at 01:55:32AM -0700, Yi Liu wrote:
> > > This splits the preparation works of the iommu and the Intel iommu driver
> > > out from the iommufd pasid attach/replace series. [1]
> > > 
> > > To support domain replacement, the definition of the set_dev_pasid op
> > > needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
> > > should be extended as well to suit the new definition.
> > > 
> > > pasid attach/replace is mandatory on Intel VT-d given the PASID table
> > > locates in the physical address space hence must be managed by the kernel,
> > > both for supporting vSVA and coming SIOV. But it's optional on ARM/AMD
> > > which allow configuring the PASID/CD table either in host physical address
> > > space or nested on top of an GPA address space. This series only extends
> > > the Intel iommu driver as the minimal requirement.
> > 
> > Sicne this will be pushed to the next cyle that will have my ARM code
> > the smmuv3 will need to be updated too. It is already prepped to
> > support replace, just add this please:
> 
> thanks. So your related series has made the internal helpers to support
> domain replacement in set_dev_pasid path. The below diff just passes the
> old_domain across the helpers. Is it? Just to double confirm with you. :)

Yes

Jason

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Vasant Hegde]

Hi All,

On 6/28/2024 2:25 PM, Yi Liu wrote:
> This splits the preparation works of the iommu and the Intel iommu driver
> out from the iommufd pasid attach/replace series. [1]
> 
> To support domain replacement, the definition of the set_dev_pasid op
> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
> should be extended as well to suit the new definition.

IIUC this will remove PASID from old SVA domain and attaches to new SVA domain.
(basically attaching same dev/PASID to different process). Is that the correct?

So the expectation is replace existing PASID from PASID table only if old_domain
is passed. Otherwise sev_dev_pasid() should throw an error right?

-Vasant

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Yi Liu]

On 2024/8/16 01:49, Vasant Hegde wrote:
> Hi All,
> 
> On 6/28/2024 2:25 PM, Yi Liu wrote:
>> This splits the preparation works of the iommu and the Intel iommu driver
>> out from the iommufd pasid attach/replace series. [1]
>>
>> To support domain replacement, the definition of the set_dev_pasid op
>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
>> should be extended as well to suit the new definition.
> 
> IIUC this will remove PASID from old SVA domain and attaches to new SVA domain.
> (basically attaching same dev/PASID to different process). Is that the correct?

In brief, yes. But it's not only for SVA domain. Remember that SIOVr1
extends the usage of PASID. At least on Intel side, a PASID may be
attached to paging domains.

> So the expectation is replace existing PASID from PASID table only if old_domain
> is passed. Otherwise sev_dev_pasid() should throw an error right?
> 

yes. If no old_domain passed in, then it is just a normal attachment. As
you are working on AMD iommu, it would be great if you can have a patch to
make the AMD set_dev_pasid() op suit this expectation. Then it can be
incorporated in this series. :)

-- 
Regards,
Yi Liu

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Baolu Lu]

On 8/16/24 9:19 AM, Yi Liu wrote:
> On 2024/8/16 01:49, Vasant Hegde wrote:
>> Hi All,
>>
>> On 6/28/2024 2:25 PM, Yi Liu wrote:
>>> This splits the preparation works of the iommu and the Intel iommu 
>>> driver
>>> out from the iommufd pasid attach/replace series. [1]
>>>
>>> To support domain replacement, the definition of the set_dev_pasid op
>>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
>>> should be extended as well to suit the new definition.
>>
>> IIUC this will remove PASID from old SVA domain and attaches to new 
>> SVA domain.
>> (basically attaching same dev/PASID to different process). Is that the 
>> correct?
> 
> In brief, yes. But it's not only for SVA domain. Remember that SIOVr1
> extends the usage of PASID. At least on Intel side, a PASID may be
> attached to paging domains.

You are correct.

The idxd driver attaches a paging domain to a non-zero PASID for kernel
DMA with PASID. From an architectural perspective, other architectures,
like ARM, AMD, and RISC-V, also support this. Therefore, attaching a
paging domain to a PASID is not Intel-specific but a generic feature.

Thanks,
baolu

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Vasant Hegde]

Baolu,


On 8/16/2024 8:19 AM, Baolu Lu wrote:
> On 8/16/24 9:19 AM, Yi Liu wrote:
>> On 2024/8/16 01:49, Vasant Hegde wrote:
>>> Hi All,
>>>
>>> On 6/28/2024 2:25 PM, Yi Liu wrote:
>>>> This splits the preparation works of the iommu and the Intel iommu driver
>>>> out from the iommufd pasid attach/replace series. [1]
>>>>
>>>> To support domain replacement, the definition of the set_dev_pasid op
>>>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
>>>> should be extended as well to suit the new definition.
>>>
>>> IIUC this will remove PASID from old SVA domain and attaches to new SVA domain.
>>> (basically attaching same dev/PASID to different process). Is that the correct?
>>
>> In brief, yes. But it's not only for SVA domain. Remember that SIOVr1
>> extends the usage of PASID. At least on Intel side, a PASID may be
>> attached to paging domains.
> 
> You are correct.

Thanks.

> 
> The idxd driver attaches a paging domain to a non-zero PASID for kernel
> DMA with PASID. From an architectural perspective, other architectures,
> like ARM, AMD, and RISC-V, also support this. Therefore, attaching a
> paging domain to a PASID is not Intel-specific but a generic feature.

Right. We can enhance AMD driver to support attaching PASID to paging/UNMANAGED
domains. It needs some more rework/cleanup before we do that.

-Vasant

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Baolu Lu]

On 8/16/24 9:19 AM, Yi Liu wrote:
>> So the expectation is replace existing PASID from PASID table only if 
>> old_domain
>> is passed. Otherwise sev_dev_pasid() should throw an error right?
>>
> 
> yes. If no old_domain passed in, then it is just a normal attachment. As
> you are working on AMD iommu, it would be great if you can have a patch to
> make the AMD set_dev_pasid() op suit this expectation. Then it can be
> incorporated in this series. 🙂

Perhaps this has been discussed before, but I can't recall. Out of
curiosity, why not introduce a specific replace_dev_pasid callback?

Thanks,
baolu

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Yi Liu]

On 2024/8/16 10:52, Baolu Lu wrote:
> On 8/16/24 9:19 AM, Yi Liu wrote:
>>> So the expectation is replace existing PASID from PASID table only if 
>>> old_domain
>>> is passed. Otherwise sev_dev_pasid() should throw an error right?
>>>
>>
>> yes. If no old_domain passed in, then it is just a normal attachment. As
>> you are working on AMD iommu, it would be great if you can have a patch to
>> make the AMD set_dev_pasid() op suit this expectation. Then it can be
>> incorporated in this series. 🙂
> 
> Perhaps this has been discussed before, but I can't recall. Out of
> curiosity, why not introduce a specific replace_dev_pasid callback?

you have a good memory. It was discussed in the below link. I suppose it
reached agreement at that time.

https://lore.kernel.org/linux-iommu/20240416174749.GI3637727@nvidia.com/

-- 
Regards,
Yi Liu

Re: [PATCH 0/6] Make set_dev_pasid op supportting domain replacement

[Vasant Hegde]

Yi,


On 8/16/2024 6:49 AM, Yi Liu wrote:
> On 2024/8/16 01:49, Vasant Hegde wrote:
>> Hi All,
>>
>> On 6/28/2024 2:25 PM, Yi Liu wrote:
>>> This splits the preparation works of the iommu and the Intel iommu driver
>>> out from the iommufd pasid attach/replace series. [1]
>>>
>>> To support domain replacement, the definition of the set_dev_pasid op
>>> needs to be enhanced. Meanwhile, the existing set_dev_pasid callbacks
>>> should be extended as well to suit the new definition.
>>
>> IIUC this will remove PASID from old SVA domain and attaches to new SVA domain.
>> (basically attaching same dev/PASID to different process). Is that the correct?
> 
> In brief, yes. But it's not only for SVA domain. Remember that SIOVr1
> extends the usage of PASID. At least on Intel side, a PASID may be
> attached to paging domains.

Right. I missed SIOV case.

> 
>> So the expectation is replace existing PASID from PASID table only if old_domain
>> is passed. Otherwise sev_dev_pasid() should throw an error right?
>>
> 
> yes. If no old_domain passed in, then it is just a normal attachment. As
> you are working on AMD iommu, it would be great if you can have a patch to
> make the AMD set_dev_pasid() op suit this expectation. Then it can be
> incorporated in this series. :)'

Sure. It should be simple. I will try to get the patch next week.

-Vasant