Re: [PATCH 14/27] iommu/arm-smmu-v3: Make changing domains be hitless for ATS

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Oct 26, 2023 at 03:00:11PM +0800, Michael Shavit wrote:

> Huh. Can this be checked by calling iommu_get_domain_for_dev() and
> comparing against the input iommu_domain parameter, or is
> iommu_get_domain_for_dev() cleared in this scenario?

Yes, that should work, but I dislike the frailty. 

Hmm. When I first wrote this the whole picture wasn't finished, now I
think we can just take the optimization out. If the same master has
two master_domains in the list for a moment it is not actually a
problem. The commit stage will remove only one.

So, like this:

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 15d1688d7f7034..31ce450448a7f0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2490,6 +2490,10 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
 	struct arm_smmu_master_domain *master_domain;
 	unsigned long flags;
 
+	/* NULL means the old domain is IDENTITY/BLOCKED which we don't track */
+	if (!smmu_domain)
+                return;
+
 	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
 	master_domain = arm_smmu_find_master_domain(smmu_domain, master);
 	if (master_domain) {
@@ -2502,8 +2506,7 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
 }
 
 struct attach_state {
-	bool existing_master_domain : 1;
-	bool want_ats : 1;
+	bool want_ats;
 };
 
 /*
@@ -2514,7 +2517,6 @@ static int arm_smmu_attach_prepare(struct arm_smmu_master *master,
 				   struct arm_smmu_domain *smmu_domain,
 				   struct attach_state *state)
 {
-	struct arm_smmu_master_domain *cur_master_domain;
 	struct arm_smmu_master_domain *master_domain;
 	unsigned long flags;
 
@@ -2541,17 +2543,14 @@ static int arm_smmu_attach_prepare(struct arm_smmu_master *master,
 	 * It is tempting to make this list only track masters that are using
 	 * ATS, but arm_smmu_share_asid() also uses this to change the ASID of a
 	 * domain, unrelated to ATS.
+	 *
+	 * Notice if we are re-attaching the same domain then the list will have
+	 * two identical entries and commit will remove only one of them.
 	 */
 	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	cur_master_domain = arm_smmu_find_master_domain(smmu_domain, master);
-	if (cur_master_domain) {
-		kfree(master_domain);
-		state->existing_master_domain = true;
-	} else {
-		if (state->want_ats)
-			atomic_inc(&smmu_domain->nr_ats_masters);
-		list_add(&master_domain->devices_elm, &smmu_domain->devices);
-	}
+	if (state->want_ats)
+		atomic_inc(&smmu_domain->nr_ats_masters);
+	list_add(&master_domain->devices_elm, &smmu_domain->devices);
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 	return 0;
 }
@@ -2581,13 +2580,9 @@ static void arm_smmu_attach_commit(struct arm_smmu_master *master,
 		arm_smmu_atc_inv_master(master);
 	}
 
-	if (!state->existing_master_domain) {
-		struct arm_smmu_domain *old_smmu_domain = to_smmu_domain_safe(
-			iommu_get_domain_for_dev(master->dev));
-
-		if (old_smmu_domain)
-			arm_smmu_remove_master_domain(master, old_smmu_domain);
-	}
+	arm_smmu_remove_master_domain(
+		master,
+		to_smmu_domain_safe(iommu_get_domain_for_dev(master->dev)));
 }
 
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)