On Thu, Aug 10, 2023 at 03:40:52PM +0100, Will Deacon wrote:

On Thu, Aug 10, 2023 at 05:15:50PM +0800, Michael Shavit wrote:

quoted

On Wed, Aug 9, 2023 at 9:50 PM Will Deacon [off-list ref] wrote:

quoted

quoted

-     ret = arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, cd);
-     if (ret)
+     ret = arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, cd);
+     if (ret) {
+             arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, NULL);

Why is it safe to drop the lock between these two calls?

Hmmm this is a tricky question.
Tracing through the SVA flow, it seems like there's a scenario where
multiple masters (with the same upstream SMMU device) can be attached
to the same primary/non-sva domain, in which case calling
iommu_attach_device_pasid on one device will write the CD entry for
both masters. This is still the case even with this patch series, and
changing this behavior will be the subject of a separate follow-up.
This is weird, especially since the second master need not even have
the sva_enabled bit set. This also means that the list of attached
masters can indeed change between these two calls if that second
master (not the one used on the iommu_attach_device_pasid call leading
to this code) is detached/attached at the same time. It's hard for me
to reason about whether this is safe or not, since this is already
weird behavior...

I really think the writing of the context descriptors should look atomic;
dropping the lock half way through a failed update and then coming back
to NULL them out definitely isn't correct. So I think you've probably pushed
the locking too far down the stack.

Urk, the issue is that progressive refactorings have left this kind of
wrong. 

Basically we always have a singular master we are supposed to be
installing the SVA domain into a PASID for, we just need to load the
CD table entry into that master's existing CD table.

Actually, I don't think this even works as nothing on the PASID path
adds to the list that arm_smmu_write_ctx_desc_devices() iterates over ??

Then the remaining two calls:

arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
        arm_smmu_write_ctx_desc_devices(smmu_domain, 0, cd);
	
	This is OK only if the sketchy assumption that the CD
	we extracted for a conflicting ASID is not asigned to a PASID.

static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
        arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, &quiet_cd);

	This doesn't work because we didn't add the master to the list
	during __arm_smmu_sva_bind and this path is expressly working
	on the PASID binds, not the RID binds.

This is all far too complicated. We really need to get this series:

https://lore.kernel.org/linux-iommu/20230808074944.7825-1-tina.zhang@intel.com/ (local)

And rip out all this crazy code in the drivers trying to de-duplicate
the SVA domains. The core code will do it, the driver can assume it
has exactly one SVA domain per mm and do sane and simple things. :(

Maybe for now we just accept that quiet_cd doesn't work, it is a minor
issue and your next series fixes it, right?

Anyhow, something like this will fix what Will pointed to, probably as
an additional prep patch:

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 e3992a0c163779..8e751ba91e810a 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

@@ -297,12 +297,6 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
 		goto err_free_cd;
 	}
 
-	ret = arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, cd);
-	if (ret) {
-		arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, NULL);
-		goto err_put_notifier;
-	}
-
 	list_add(&smmu_mn->list, &smmu_domain->mmu_notifiers);
 	return smmu_mn;
 

@@ -325,8 +319,6 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 
 	list_del(&smmu_mn->list);
 
-	arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, NULL);
-
 	/*
 	 * If we went through clear(), we've already invalidated, and no
 	 * new TLB entry can have been formed.

@@ -342,7 +334,7 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 }
 
 static struct iommu_sva *
-__arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
+__arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, ioasid_t ssid)
 {
 	int ret;
 	struct arm_smmu_bond *bond;

@@ -375,9 +367,15 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 		goto err_free_bond;
 	}
 
+	ret = arm_smmu_write_ctx_desc(master, ssid, bond->smmu_mn->cd);
+	if (ret)
+		goto err_put_notifier;
+
 	list_add(&bond->list, &master->bonds);
 	return &bond->sva;
 
+err_put_notifier:
+	arm_smmu_mmu_notifier_put(bond->smmu_mn);
 err_free_bond:
 	kfree(bond);
 	return ERR_PTR(ret);

@@ -548,6 +546,7 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 
 	if (!WARN_ON(!bond) && refcount_dec_and_test(&bond->refs)) {
 		list_del(&bond->list);
+		arm_smmu_write_ctx_desc(master, id, NULL);
 		arm_smmu_mmu_notifier_put(bond->smmu_mn);
 		kfree(bond);
 	}

@@ -562,7 +561,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 	struct mm_struct *mm = domain->mm;
 
 	mutex_lock(&sva_lock);
-	handle = __arm_smmu_sva_bind(dev, mm);
+	handle = __arm_smmu_sva_bind(dev, mm, id);
 	if (IS_ERR(handle))
 		ret = PTR_ERR(handle);
 	mutex_unlock(&sva_lock);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel