[PATCH v5 0/6] Add PASID support to SMMUv3 unmanaged domains

[PATCH v5 0/6] Add PASID support to SMMUv3 unmanaged domains

[Michael Shavit]

This patch series implements the set_dev_pasid operation for DMA and UNMANAGED
iommu domains. The series depends on the CD table refactor patch series as a
pre-requisite.

This patch series is also available on gerrit with Jean's SMMU test
engine patches cherry-picked along with an additional set of tests for
this feature: https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/24770/6

Thanks,
Michael Shavit

Changes in v5:
- Renamed domain_head to list for consistency with other lists
- Renamed attached_domains to attached_ssids to avoid confusion. This is
  a list of master/ssid pairs the domain is attached to, not a list of
  other domains.
- Fix missing error value return in set_dev_pasid
- Fix issue where nr_attached_pasid_domains isn't updated when
  arm_smmu_write_ctx_desc fails
- Fix missing free of the attached_domain node
- Split off the CD table refactor to separate patch series: https://lore.kernel.org/all/20230802163328.2623773-1-mshavit@google.com/
- Link to v4: https://lore.kernel.org/all/20230621063825.268890-1-mshavit@google.com/
- New commit: Free attached pasid domains on release_device() call

Changes in v4:
- Fix build warning and error on patch 07. The error was introduced
  during a v1->v2 rebase and hidden by patch 09 which removed the
  offending line.
- Link to v3: https://lore.kernel.org/all/20230614154304.2860121-1-mshavit@google.com/

Changes in v3:
- Dropped the bulk of the SVA refactoring to re-work as a follow-up
  series.
- Reworded cover letter to omit dropped changes.
- Rebased on 6.4 tip
- Link to v2: https://lore.kernel.org/all/20230606120854.4170244-1-mshavit@google.com/

Changes in v2:
- Reworded cover letter and commits based on v1 feedback.
- Split and reworked `iommu/arm-smmu-v3: Move cdtable to arm_smmu_master`
- Added SVA clean-up and refactor.
- A few other small bug fixes and cosmetics.
- Link to v1: https://lore.kernel.org/all/20230510205054.2667898-1-mshavit@google.com/

Michael Shavit (6):
  iommu/arm-smmu-v3: Simplify arm_smmu_enable_ats
  iommu/arm-smmu-v3: Keep track of attached ssids
  iommu/arm-smmu-v3: Add helper for atc invalidation
  iommu/arm-smmu-v3: Implement set_dev_pasid
  iommu/arm-smmu-v3: Free pasid domains on iommu release
  iommu/arm-smmu-v3: Cleanup arm_smmu_domain_finalise

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  28 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 283 ++++++++++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  31 +-
 3 files changed, 267 insertions(+), 75 deletions(-)


base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
prerequisite-patch-id: c8d21ff19c2c1dd18799a6b83f483add654d187e
prerequisite-patch-id: bdeb88498393e7049fd22cfc24d2b7674e81bb85
prerequisite-patch-id: b84be729e187aa2d67a7f90ec396e2b878c76243
prerequisite-patch-id: 0ee7da8eaae46e2f8a0a791808f421025e148d79
prerequisite-patch-id: 2d80d99964059ecb31065ec4954130c817b9046c
prerequisite-patch-id: 49910462ec68b834c6af18ae9c58de25982e2752
prerequisite-patch-id: 1daf192523b0b7ed24a670b47ad07366aca6d26d
prerequisite-patch-id: 96101f0f4fd95cdb442cfe0881d80c3a61a93716
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH v5 1/6] iommu/arm-smmu-v3: Simplify arm_smmu_enable_ats

[Michael Shavit]

arm_smmu_enable_ats's call to inv_domain would trigger an invalidation
for all masters that a domain is attached to everytime it's attached to
another ATS-enabled master. It doesn't seem like those invalidations are
necessary, and it's easier to reason about arm_smmu_enable_ats if it
only issues invalidation commands for the current master.

Signed-off-by: Michael Shavit <mshavit@google.com>
---

(no changes since v2)

Changes in v2:
- Fix commit message wrapping

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 61de66d17a5d5..4df335424b266 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2305,7 +2305,7 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
 	pdev = to_pci_dev(master->dev);
 
 	atomic_inc(&smmu_domain->nr_ats_masters);
-	arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
+	arm_smmu_atc_inv_master(master);
 	if (pci_enable_ats(pdev, stu))
 		dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu);
 }
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids

[Michael Shavit]

The arm-smmu-v3 driver keeps track of all masters that a domain is
attached to so that it can re-write their STEs when the domain's ASID is
upated by SVA. This tracking is also used to invalidate ATCs on all
masters that a domain is attached to.

This change introduces a new data structures to track all the CD entries
that a domain is attached to. This change is a pre-requisite to allow
domain attachment on non 0 SSIDs.

Signed-off-by: Michael Shavit <mshavit@google.com>
---

Changes in v5:
- Renamed domain_head to list for consistency with other lists
- Renamed attached_domains to attached_ssids to avoid confusion. This is
  a list of master/ssid pairs the domain is attached to, not a list of
  other domains.

Changes in v4:
- Remove reference to the master's domain accidentally re-introduced
  during a rebase.  Make arm_smmu_atc_inv_domain static.

Changes in v2:
- Fix arm_smmu_atc_inv_cmd_set_ssid and other cosmetic changes

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 28 ++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 99 +++++++++++--------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 22 ++++-
 3 files changed, 98 insertions(+), 51 deletions(-)

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 e3992a0c16377..d80c39e7e2fb5 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
@@ -37,21 +37,35 @@ struct arm_smmu_bond {
 
 static DEFINE_MUTEX(sva_lock);
 
+/*
+ * When ssid is 0, update all the CD entries that this domain is attached to.
+ * When ssid is non-zero, write the CD into all the masters where this domain is
+ * the primary domain, with the provided SSID. This is used because SVA still
+ * piggybacks over the primary domain.
+ */
 static int arm_smmu_write_ctx_desc_devices(struct arm_smmu_domain *smmu_domain,
 					    int ssid,
 					    struct arm_smmu_ctx_desc *cd)
 {
+	struct arm_smmu_attached_domain *attached_domain;
 	struct arm_smmu_master *master;
 	unsigned long flags;
 	int ret;
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
-		ret = arm_smmu_write_ctx_desc(master, ssid, cd);
+	spin_lock_irqsave(&smmu_domain->attached_ssids_lock, flags);
+	list_for_each_entry(attached_domain, &smmu_domain->attached_ssids,
+			    list) {
+		master = attached_domain->master;
+		if (ssid && attached_domain->ssid == 0) {
+			ret = arm_smmu_write_ctx_desc(master, ssid, cd);
+		} else {
+			ret = arm_smmu_write_ctx_desc(
+				master, attached_domain->ssid, cd);
+		}
 		if (ret)
 			break;
 	}
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_unlock_irqrestore(&smmu_domain->attached_ssids_lock, flags);
 	return ret;
 }
 
@@ -222,7 +236,7 @@ static void arm_smmu_mm_invalidate_range(struct mmu_notifier *mn,
 	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_BTM))
 		arm_smmu_tlb_inv_range_asid(start, size, smmu_mn->cd->asid,
 					    PAGE_SIZE, false, smmu_domain);
-	arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, start, size);
+	arm_smmu_atc_inv_domain_ssid(smmu_domain, mm->pasid, start, size);
 }
 
 static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
@@ -243,7 +257,7 @@ 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);
 
 	arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid);
-	arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
+	arm_smmu_atc_inv_domain_ssid(smmu_domain, mm->pasid, 0, 0);
 
 	smmu_mn->cleared = true;
 	mutex_unlock(&sva_lock);
@@ -333,7 +347,7 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 	 */
 	if (!smmu_mn->cleared) {
 		arm_smmu_tlb_inv_asid(smmu_domain->smmu, cd->asid);
-		arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
+		arm_smmu_atc_inv_domain_ssid(smmu_domain, mm->pasid, 0, 0);
 	}
 
 	/* Frees smmu_mn */
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 4df335424b266..6e614ad12fb48 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1282,7 +1282,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
 	};
 
 	if (master) {
-		smmu_domain = master->domain;
+		smmu_domain = master->non_pasid_domain.domain;
 		smmu = master->smmu;
 	}
 
@@ -1725,7 +1725,14 @@ static irqreturn_t arm_smmu_combined_irq_handler(int irq, void *dev)
 }
 
 static void
-arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size,
+arm_smmu_atc_inv_cmd_set_ssid(int ssid, struct arm_smmu_cmdq_ent *cmd)
+{
+	cmd->substream_valid = !!ssid;
+	cmd->atc.ssid = ssid;
+}
+
+static void
+arm_smmu_atc_inv_to_cmd(unsigned long iova, size_t size,
 			struct arm_smmu_cmdq_ent *cmd)
 {
 	size_t log2_span;
@@ -1750,8 +1757,8 @@ arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size,
 	 */
 	*cmd = (struct arm_smmu_cmdq_ent) {
 		.opcode			= CMDQ_OP_ATC_INV,
-		.substream_valid	= !!ssid,
-		.atc.ssid		= ssid,
+		.substream_valid	= false,
+		.atc.ssid		= 0,
 	};
 
 	if (!size) {
@@ -1797,8 +1804,7 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
 	struct arm_smmu_cmdq_ent cmd;
 	struct arm_smmu_cmdq_batch cmds;
 
-	arm_smmu_atc_inv_to_cmd(0, 0, 0, &cmd);
-
+	arm_smmu_atc_inv_to_cmd(0, 0, &cmd);
 	cmds.num = 0;
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.atc.sid = master->streams[i].id;
@@ -1808,13 +1814,19 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
 	return arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
 }
 
-int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
-			    unsigned long iova, size_t size)
+/*
+ * If ssid is non-zero, issue atc invalidations with the given ssid instead of
+ * the one the domain is attached to. This is used by SVA since it's pasid
+ * attachments aren't recorded in smmu_domain yet.
+ */
+int arm_smmu_atc_inv_domain_ssid(struct arm_smmu_domain *smmu_domain, int ssid,
+				 unsigned long iova, size_t size)
 {
 	int i;
 	unsigned long flags;
 	struct arm_smmu_cmdq_ent cmd;
 	struct arm_smmu_master *master;
+	struct arm_smmu_attached_domain *attached_domain;
 	struct arm_smmu_cmdq_batch cmds;
 
 	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_ATS))
@@ -1837,25 +1849,37 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
 	if (!atomic_read(&smmu_domain->nr_ats_masters))
 		return 0;
 
-	arm_smmu_atc_inv_to_cmd(ssid, iova, size, &cmd);
+	arm_smmu_atc_inv_to_cmd(iova, size, &cmd);
 
 	cmds.num = 0;
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
+	spin_lock_irqsave(&smmu_domain->attached_ssids_lock, flags);
+	list_for_each_entry(attached_domain, &smmu_domain->attached_ssids,
+			    list) {
+		master = attached_domain->master;
 		if (!master->ats_enabled)
 			continue;
+		if (ssid != 0)
+			arm_smmu_atc_inv_cmd_set_ssid(ssid, &cmd);
+		else
+			arm_smmu_atc_inv_cmd_set_ssid(attached_domain->ssid, &cmd);
 
 		for (i = 0; i < master->num_streams; i++) {
 			cmd.atc.sid = master->streams[i].id;
 			arm_smmu_cmdq_batch_add(smmu_domain->smmu, &cmds, &cmd);
 		}
 	}
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_unlock_irqrestore(&smmu_domain->attached_ssids_lock, flags);
 
 	return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
 }
 
+static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
+				   unsigned long iova, size_t size)
+{
+	return arm_smmu_atc_inv_domain_ssid(smmu_domain, 0, iova, size);
+}
+
 /* IO_PGTABLE API */
 static void arm_smmu_tlb_inv_context(void *cookie)
 {
@@ -1877,7 +1901,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
 		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
 	}
-	arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
+	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
 }
 
 static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
@@ -1970,7 +1994,7 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
 	 * Unfortunately, this can't be leaf-only since we may have
 	 * zapped an entire table.
 	 */
-	arm_smmu_atc_inv_domain(smmu_domain, 0, iova, size);
+	arm_smmu_atc_inv_domain(smmu_domain, iova, size);
 }
 
 void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
@@ -2050,8 +2074,8 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 		return NULL;
 
 	mutex_init(&smmu_domain->init_mutex);
-	INIT_LIST_HEAD(&smmu_domain->devices);
-	spin_lock_init(&smmu_domain->devices_lock);
+	INIT_LIST_HEAD(&smmu_domain->attached_ssids);
+	spin_lock_init(&smmu_domain->attached_ssids_lock);
 	INIT_LIST_HEAD(&smmu_domain->mmu_notifiers);
 
 	return &smmu_domain->domain;
@@ -2289,12 +2313,12 @@ static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
 	return dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev));
 }
 
-static void arm_smmu_enable_ats(struct arm_smmu_master *master)
+static void arm_smmu_enable_ats(struct arm_smmu_master *master,
+				struct arm_smmu_domain *smmu_domain)
 {
 	size_t stu;
 	struct pci_dev *pdev;
 	struct arm_smmu_device *smmu = master->smmu;
-	struct arm_smmu_domain *smmu_domain = master->domain;
 
 	/* Don't enable ATS at the endpoint if it's not enabled in the STE */
 	if (!master->ats_enabled)
@@ -2310,10 +2334,9 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
 		dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu);
 }
 
-static void arm_smmu_disable_ats(struct arm_smmu_master *master)
+static void arm_smmu_disable_ats(struct arm_smmu_master *master,
+				 struct arm_smmu_domain *smmu_domain)
 {
-	struct arm_smmu_domain *smmu_domain = master->domain;
-
 	if (!master->ats_enabled)
 		return;
 
@@ -2377,19 +2400,19 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 {
 	unsigned long flags;
-	struct arm_smmu_domain *smmu_domain = master->domain;
+	struct arm_smmu_domain *smmu_domain = master->non_pasid_domain.domain;
 
 	if (!smmu_domain)
 		return;
 
-	arm_smmu_disable_ats(master);
+	arm_smmu_disable_ats(master, smmu_domain);
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_del(&master->domain_head);
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_lock_irqsave(&smmu_domain->attached_ssids_lock, flags);
+	list_del(&master->non_pasid_domain.list);
+	spin_unlock_irqrestore(&smmu_domain->attached_ssids_lock, flags);
 
-	master->domain = NULL;
 	master->ats_enabled = false;
+	master->non_pasid_domain.domain = NULL;
 	arm_smmu_install_ste_for_dev(master);
 }
 
@@ -2434,8 +2457,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	if (ret)
 		return ret;
 
-	master->domain = smmu_domain;
-
 	/*
 	 * The SMMU does not support enabling ATS with bypass. When the STE is
 	 * in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests and
@@ -2449,26 +2470,26 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		if (!master->cd_table.cdtab) {
 			ret = arm_smmu_alloc_cd_tables(master);
-			if (ret) {
-				master->domain = NULL;
+			if (ret)
 				return ret;
-			}
 		}
 
 		ret = arm_smmu_write_ctx_desc(master, 0, &smmu_domain->cd);
-		if (ret) {
-			master->domain = NULL;
+		if (ret)
 			return ret;
-		}
 	}
 
+	master->non_pasid_domain.master = master;
+	master->non_pasid_domain.domain = smmu_domain;
+	master->non_pasid_domain.ssid = 0;
 	arm_smmu_install_ste_for_dev(master);
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_add(&master->domain_head, &smmu_domain->devices);
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_lock_irqsave(&smmu_domain->attached_ssids_lock, flags);
+	list_add(&master->non_pasid_domain.list,
+		 &smmu_domain->attached_ssids);
+	spin_unlock_irqrestore(&smmu_domain->attached_ssids_lock, flags);
 
-	arm_smmu_enable_ats(master);
+	arm_smmu_enable_ats(master, smmu_domain);
 	return 0;
 }
 
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 e76452e735a04..66a492cafe2e8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -689,11 +689,19 @@ struct arm_smmu_stream {
 	struct rb_node			node;
 };
 
+/* List of {masters, ssid} that a domain is attached to */
+struct arm_smmu_attached_domain {
+	struct list_head	list;
+	struct arm_smmu_domain  *domain;
+	struct arm_smmu_master  *master;
+	int			ssid;
+};
+
 /* SMMU private data for each master */
 struct arm_smmu_master {
 	struct arm_smmu_device		*smmu;
 	struct device			*dev;
-	struct arm_smmu_domain		*domain;
+	struct arm_smmu_attached_domain	non_pasid_domain;
 	struct list_head		domain_head;
 	struct arm_smmu_stream		*streams;
 	/* Locked by the iommu core using the group mutex */
@@ -730,8 +738,12 @@ struct arm_smmu_domain {
 
 	struct iommu_domain		domain;
 
-	struct list_head		devices;
-	spinlock_t			devices_lock;
+	/*
+	 * List of arm_smmu_attached_domain nodes used to track all the
+	 * {master, ssid} pairs that this domain is attached to.
+	 */
+	struct list_head		attached_ssids;
+	spinlock_t			attached_ssids_lock;
 
 	struct list_head		mmu_notifiers;
 };
@@ -752,8 +764,8 @@ void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
 				 size_t granule, bool leaf,
 				 struct arm_smmu_domain *smmu_domain);
 bool arm_smmu_free_asid(struct arm_smmu_ctx_desc *cd);
-int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
-			    unsigned long iova, size_t size);
+int arm_smmu_atc_inv_domain_ssid(struct arm_smmu_domain *smmu_domain, int ssid,
+				 unsigned long iova, size_t size);
 
 #ifdef CONFIG_ARM_SMMU_V3_SVA
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH v5 3/6] iommu/arm-smmu-v3: Add helper for atc invalidation

[Michael Shavit]

This will be used to invalidate ATC entries made on an SSID for a master
when detaching a domain with pasid.

Signed-off-by: Michael Shavit <mshavit@google.com>
---

(no changes since v2)

Changes in v2:
- Make use of arm_smmu_atc_inv_cmd_set_ssid

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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 6e614ad12fb48..e0565c644ffdb 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1798,13 +1798,15 @@ arm_smmu_atc_inv_to_cmd(unsigned long iova, size_t size,
 	cmd->atc.size	= log2_span;
 }
 
-static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
+static int arm_smmu_atc_inv_master_ssid(struct arm_smmu_master *master,
+					int ssid)
 {
 	int i;
 	struct arm_smmu_cmdq_ent cmd;
 	struct arm_smmu_cmdq_batch cmds;
 
 	arm_smmu_atc_inv_to_cmd(0, 0, &cmd);
+	arm_smmu_atc_inv_cmd_set_ssid(ssid, &cmd);
 	cmds.num = 0;
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.atc.sid = master->streams[i].id;
@@ -1814,6 +1816,11 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
 	return arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
 }
 
+static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
+{
+	return arm_smmu_atc_inv_master_ssid(master, 0);
+}
+
 /*
  * If ssid is non-zero, issue atc invalidations with the given ssid instead of
  * the one the domain is attached to. This is used by SVA since it's pasid
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH v5 4/6] iommu/arm-smmu-v3: Implement set_dev_pasid

[Michael Shavit]

This change enables the use of the iommu_attach_dev_pasid API for
UNMANAGED domains. The primary use-case is to allow in-kernel users of
the iommu API to manage domains with PASID. This change also allows for
future support of pasid in the DMA api.

Signed-off-by: Michael Shavit <mshavit@google.com>
---

Changes in v5:
- Fix missing error value return in set_dev_pasid
- Fix issue where nr_attached_pasid_domains isn't updated when
  arm_smmu_write_ctx_desc fails
- Fix missing free of the attached_domain node
- Split off the CD table refactor to separate patch series: https://lore.kernel.org/all/20230802163328.2623773-1-mshavit@google.com/
- Link to v4: https://lore.kernel.org/all/20230621063825.268890-1-mshavit@google.com/
- Remove districting change where a NULL master is passed to
  arm_smmu_prepare_domain_for_smmu

Changes in v4:
- Fix build warning and error on patch 07. The error was introduced
  during a v1->v2 rebase and hidden by patch 09 which removed the
  offending line.
- Link to v3: https://lore.kernel.org/all/20230614154304.2860121-1-mshavit@google.com/

Changes in v3:
- Dropped the bulk of the SVA refactoring to re-work as a follow-up
  series.
- Reworded cover letter to omit dropped changes.
- Rebased on 6.4 tip
- Link to v2: https://lore.kernel.org/all/20230606120854.4170244-1-mshavit@google.com/

Changes in v2:
- Reworded cover letter and commits based on v1 feedback.
- Split and reworked `iommu/arm-smmu-v3: Move cdtable to arm_smmu_master`
- Added SVA clean-up and refactor.
- A few other small bug fixes and cosmetics.
- Link to v1: https://lore.kernel.org/all/20230510205054.2667898-1-mshavit@google.com/
- Add missing atc invalidation when detaching with pasid

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 156 ++++++++++++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   1 +
 2 files changed, 141 insertions(+), 16 deletions(-)

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 e0565c644ffdb..7b296458dafec 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2388,6 +2388,11 @@ static int arm_smmu_enable_pasid(struct arm_smmu_master *master)
 	return 0;
 }
 
+static bool arm_smmu_master_has_pasid_domains(struct arm_smmu_master *master)
+{
+	return master->nr_attached_pasid_domains > 0;
+}
+
 static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 {
 	struct pci_dev *pdev;
@@ -2423,6 +2428,25 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 	arm_smmu_install_ste_for_dev(master);
 }
 
+static int arm_smmu_prepare_domain_for_smmu(struct arm_smmu_device *smmu,
+					    struct arm_smmu_domain *smmu_domain,
+					    struct arm_smmu_master *master)
+{
+	int ret = 0;
+
+	mutex_lock(&smmu_domain->init_mutex);
+	if (!smmu_domain->smmu) {
+		smmu_domain->smmu = smmu;
+		ret = arm_smmu_domain_finalise(&smmu_domain->domain, master);
+		if (ret)
+			smmu_domain->smmu = NULL;
+	} else if (smmu_domain->smmu != smmu)
+		ret = -EINVAL;
+
+	mutex_unlock(&smmu_domain->init_mutex);
+	return ret;
+}
+
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
 	int ret = 0;
@@ -2438,6 +2462,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	master = dev_iommu_priv_get(dev);
 	smmu = master->smmu;
 
+	ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain, master);
+	if (ret)
+		return ret;
+
 	/*
 	 * Checking that SVA is disabled ensures that this device isn't bound to
 	 * any mm, and can be safely detached from its old domain. Bonds cannot
@@ -2448,21 +2476,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		return -EBUSY;
 	}
 
-	arm_smmu_detach_dev(master);
-
-	mutex_lock(&smmu_domain->init_mutex);
-
-	if (!smmu_domain->smmu) {
-		smmu_domain->smmu = smmu;
-		ret = arm_smmu_domain_finalise(domain, master);
-		if (ret)
-			smmu_domain->smmu = NULL;
-	} else if (smmu_domain->smmu != smmu)
-		ret = -EINVAL;
+	/*
+	 * Attaching a bypass or stage 2 domain would break any domains attached
+	 * with pasid. Attaching an S1 domain should be feasible but requires
+	 * more complicated logic to handle.
+	 */
+	if (arm_smmu_master_has_pasid_domains(master)) {
+		dev_err(dev, "cannot attach - domain attached with pasid\n");
+		return -EBUSY;
+	}
 
-	mutex_unlock(&smmu_domain->init_mutex);
-	if (ret)
-		return ret;
+	arm_smmu_detach_dev(master);
 
 	/*
 	 * The SMMU does not support enabling ATS with bypass. When the STE is
@@ -2500,6 +2524,72 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return 0;
 }
 
+static int arm_smmu_set_dev_pasid(struct iommu_domain *domain,
+				  struct device *dev, ioasid_t pasid)
+{
+	int ret = 0;
+	unsigned long flags;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct arm_smmu_device *smmu;
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct arm_smmu_attached_domain *attached_domain;
+	struct arm_smmu_master *master;
+
+	if (!fwspec)
+		return -ENOENT;
+
+	master = dev_iommu_priv_get(dev);
+	smmu = master->smmu;
+
+	ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain, master);
+	if (ret)
+		return ret;
+
+	if (pasid == 0) {
+		dev_err(dev, "pasid 0 is reserved for the device's primary domain\n");
+		return -ENODEV;
+	}
+
+	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) {
+		dev_err(dev, "set_dev_pasid only supports stage 1 domains\n");
+		return -EINVAL;
+	}
+
+	if (!master->cd_table.cdtab)
+		return -EBUSY;
+
+	attached_domain = kzalloc(sizeof(*attached_domain), GFP_KERNEL);
+	if (!attached_domain)
+		return -ENOMEM;
+
+	attached_domain->master = master;
+	attached_domain->domain = smmu_domain;
+	attached_domain->ssid = pasid;
+
+	/*
+	 * arm_smmu_share_asid may update the cd's asid value and write the
+	 * ctx_desc for every attached_domains in the list. There's a potential
+	 * race here regardless of whether we first write the ctx_desc or
+	 * first insert into the domain's list. Grabbing the asic_lock prevents
+	 * SVA from changing the cd's ASID while the cd is being attached.
+	 */
+	mutex_lock(&arm_smmu_asid_lock);
+	ret = arm_smmu_write_ctx_desc(master, pasid, &smmu_domain->cd);
+	if (ret) {
+		mutex_unlock(&arm_smmu_asid_lock);
+		kfree(attached_domain);
+		return ret;
+	}
+
+	spin_lock_irqsave(&smmu_domain->attached_ssids_lock, flags);
+	list_add(&attached_domain->list, &smmu_domain->attached_ssids);
+	spin_unlock_irqrestore(&smmu_domain->attached_ssids_lock, flags);
+	mutex_unlock(&arm_smmu_asid_lock);
+
+	master->nr_attached_pasid_domains += 1;
+	return 0;
+}
+
 static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			      int prot, gfp_t gfp, size_t *mapped)
@@ -2738,6 +2828,15 @@ static void arm_smmu_release_device(struct device *dev)
 
 	if (WARN_ON(arm_smmu_master_sva_enabled(master)))
 		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
+	if (WARN_ON(master->nr_attached_pasid_domains != 0)) {
+		/*
+		 * TODO: Do we need to handle this case?
+		 * This requires a mechanism to obtain all the pasid domains
+		 * that this master is attached to so that we can clean up the
+		 * domain's attached_domain list.
+		 */
+	}
+
 	arm_smmu_detach_dev(master);
 	arm_smmu_disable_pasid(master);
 	arm_smmu_remove_master(master);
@@ -2874,12 +2973,36 @@ static int arm_smmu_def_domain_type(struct device *dev)
 static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 {
 	struct iommu_domain *domain;
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_domain *smmu_domain;
+	struct arm_smmu_attached_domain *attached_domain;
+	unsigned long flags;
 
-	domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
+	if (!master || pasid == 0)
+		return;
+
+	domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
 	if (WARN_ON(IS_ERR(domain)) || !domain)
 		return;
+	if (domain->type == IOMMU_DOMAIN_SVA)
+		return arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
 
-	arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
+	smmu_domain = to_smmu_domain(domain);
+	mutex_lock(&arm_smmu_asid_lock);
+	spin_lock_irqsave(&smmu_domain->attached_ssids_lock, flags);
+	list_for_each_entry(attached_domain, &smmu_domain->attached_ssids, list) {
+		if (attached_domain->master != master ||
+		    attached_domain->ssid != pasid)
+			continue;
+		list_del(&attached_domain->list);
+		master->nr_attached_pasid_domains -= 1;
+		kfree(attached_domain);
+		break;
+	}
+	spin_unlock_irqrestore(&smmu_domain->attached_ssids_lock, flags);
+	arm_smmu_write_ctx_desc(master, pasid, NULL);
+	arm_smmu_atc_inv_master_ssid(master, pasid);
+	mutex_unlock(&arm_smmu_asid_lock);
 }
 
 static struct iommu_ops arm_smmu_ops = {
@@ -2899,6 +3022,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.owner			= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev		= arm_smmu_attach_dev,
+		.set_dev_pasid		= arm_smmu_set_dev_pasid,
 		.map_pages		= arm_smmu_map_pages,
 		.unmap_pages		= arm_smmu_unmap_pages,
 		.flush_iotlb_all	= arm_smmu_flush_iotlb_all,
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 66a492cafe2e8..433f58bd99dd2 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -713,6 +713,7 @@ struct arm_smmu_master {
 	bool				iopf_enabled;
 	struct list_head		bonds;
 	unsigned int			ssid_bits;
+	unsigned int			nr_attached_pasid_domains;
 };
 
 /* SMMU private data for an IOMMU domain */
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH v5 5/6] iommu/arm-smmu-v3: Free pasid domains on iommu release

[Michael Shavit]

The iommu core doesn't guarantee that pasid domains will be detached
before the device is released.

Track the list of domains that a master is attached to with PASID, so
that they can be freed when the iommu is released.

Signed-off-by: Michael Shavit <mshavit@google.com>
---

Changes in v5:
- New commit: Free attached pasid domains on release_device() call

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 +++++++++++++++------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 10 +++++++++-
 2 files changed, 25 insertions(+), 7 deletions(-)

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 7b296458dafec..5fd6c4d4f0ae4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2587,6 +2587,9 @@ static int arm_smmu_set_dev_pasid(struct iommu_domain *domain,
 	mutex_unlock(&arm_smmu_asid_lock);
 
 	master->nr_attached_pasid_domains += 1;
+	list_add(&attached_domain->list_in_master,
+		 &master->attached_domains);
+
 	return 0;
 }
 
@@ -2786,6 +2789,7 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 	master->dev = dev;
 	master->smmu = smmu;
 	INIT_LIST_HEAD(&master->bonds);
+	INIT_LIST_HEAD(&master->attached_domains);
 	dev_iommu_priv_set(dev, master);
 
 	ret = arm_smmu_insert_master(smmu, master);
@@ -2825,16 +2829,21 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 static void arm_smmu_release_device(struct device *dev)
 {
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_attached_domain *attached_domain;
+	struct arm_smmu_domain *smmu_domain;
+	unsigned long flags;
 
 	if (WARN_ON(arm_smmu_master_sva_enabled(master)))
 		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
 	if (WARN_ON(master->nr_attached_pasid_domains != 0)) {
-		/*
-		 * TODO: Do we need to handle this case?
-		 * This requires a mechanism to obtain all the pasid domains
-		 * that this master is attached to so that we can clean up the
-		 * domain's attached_domain list.
-		 */
+		list_for_each_entry(attached_domain, &master->attached_domains, list_in_master) {
+			smmu_domain = attached_domain->domain;
+			spin_lock_irqsave(&smmu_domain->attached_ssids_lock, flags);
+			list_del(&attached_domain->list);
+			list_del(&attached_domain->list_in_master);
+			kfree(&attached_domain->list_in_master);
+			spin_unlock_irqrestore(&smmu_domain->attached_ssids_lock, flags);
+		}
 	}
 
 	arm_smmu_detach_dev(master);
@@ -2995,6 +3004,7 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 		    attached_domain->ssid != pasid)
 			continue;
 		list_del(&attached_domain->list);
+		list_del(&attached_domain->list_in_master);
 		master->nr_attached_pasid_domains -= 1;
 		kfree(attached_domain);
 		break;
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 433f58bd99dd2..efa428629f4d9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -689,9 +689,15 @@ struct arm_smmu_stream {
 	struct rb_node			node;
 };
 
-/* List of {masters, ssid} that a domain is attached to */
+/*
+ * List of {masters, ssid} that a domain is attached to, and conversely of
+ * domains that a master is attached to.
+ */
 struct arm_smmu_attached_domain {
+	/* List node arm_smmu_domain*/
 	struct list_head	list;
+	/* List node in arm_smmu_master*/
+	struct list_head	list_in_master;
 	struct arm_smmu_domain  *domain;
 	struct arm_smmu_master  *master;
 	int			ssid;
@@ -714,6 +720,8 @@ struct arm_smmu_master {
 	struct list_head		bonds;
 	unsigned int			ssid_bits;
 	unsigned int			nr_attached_pasid_domains;
+	/* Locked by the iommu core using the group mutex */
+	struct list_head		attached_domains;
 };
 
 /* SMMU private data for an IOMMU domain */
-- 
2.41.0.585.gd2178a4bd4-goog

[PATCH v5 6/6] iommu/arm-smmu-v3: Cleanup arm_smmu_domain_finalise

[Michael Shavit]

Remove unused master parameter now that the CD table is allocated
elsewhere.

Signed-off-by: Michael Shavit <mshavit@google.com>
---

(no changes since v1)
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

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 5fd6c4d4f0ae4..db8fd4b3591b5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2129,7 +2129,6 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 }
 
 static int arm_smmu_domain_finalise_cd(struct arm_smmu_domain *smmu_domain,
-				       struct arm_smmu_master *master,
 				       struct io_pgtable_cfg *pgtbl_cfg)
 {
 	int ret;
@@ -2167,7 +2166,6 @@ static int arm_smmu_domain_finalise_cd(struct arm_smmu_domain *smmu_domain,
 }
 
 static int arm_smmu_domain_finalise_s2(struct arm_smmu_domain *smmu_domain,
-				       struct arm_smmu_master *master,
 				       struct io_pgtable_cfg *pgtbl_cfg)
 {
 	int vmid;
@@ -2192,8 +2190,7 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_domain *smmu_domain,
 	return 0;
 }
 
-static int arm_smmu_domain_finalise(struct iommu_domain *domain,
-				    struct arm_smmu_master *master)
+static int arm_smmu_domain_finalise(struct iommu_domain *domain)
 {
 	int ret;
 	unsigned long ias, oas;
@@ -2201,7 +2198,6 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
 	struct io_pgtable_cfg pgtbl_cfg;
 	struct io_pgtable_ops *pgtbl_ops;
 	int (*finalise_stage_fn)(struct arm_smmu_domain *,
-				 struct arm_smmu_master *,
 				 struct io_pgtable_cfg *);
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
@@ -2253,7 +2249,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
 	domain->geometry.aperture_end = (1UL << pgtbl_cfg.ias) - 1;
 	domain->geometry.force_aperture = true;
 
-	ret = finalise_stage_fn(smmu_domain, master, &pgtbl_cfg);
+	ret = finalise_stage_fn(smmu_domain, &pgtbl_cfg);
 	if (ret < 0) {
 		free_io_pgtable_ops(pgtbl_ops);
 		return ret;
@@ -2429,15 +2425,14 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 }
 
 static int arm_smmu_prepare_domain_for_smmu(struct arm_smmu_device *smmu,
-					    struct arm_smmu_domain *smmu_domain,
-					    struct arm_smmu_master *master)
+					    struct arm_smmu_domain *smmu_domain)
 {
 	int ret = 0;
 
 	mutex_lock(&smmu_domain->init_mutex);
 	if (!smmu_domain->smmu) {
 		smmu_domain->smmu = smmu;
-		ret = arm_smmu_domain_finalise(&smmu_domain->domain, master);
+		ret = arm_smmu_domain_finalise(&smmu_domain->domain);
 		if (ret)
 			smmu_domain->smmu = NULL;
 	} else if (smmu_domain->smmu != smmu)
@@ -2462,7 +2457,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	master = dev_iommu_priv_get(dev);
 	smmu = master->smmu;
 
-	ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain, master);
+	ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain);
 	if (ret)
 		return ret;
 
@@ -2541,7 +2536,7 @@ static int arm_smmu_set_dev_pasid(struct iommu_domain *domain,
 	master = dev_iommu_priv_get(dev);
 	smmu = master->smmu;
 
-	ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain, master);
+	ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain);
 	if (ret)
 		return ret;
 
-- 
2.41.0.585.gd2178a4bd4-goog

Re: [PATCH v5 6/6] iommu/arm-smmu-v3: Cleanup arm_smmu_domain_finalise

[Michael Shavit]

On Thu, Aug 3, 2023 at 6:14 PM Michael Shavit <mshavit@google.com> wrote:
>
> Remove unused master parameter now that the CD table is allocated
> elsewhere.

This almost certainly belongs in the CD table patch series. I'd like
to move it there if this commit looks OK.

Re: [PATCH v5 5/6] iommu/arm-smmu-v3: Free pasid domains on iommu release

[Michael Shavit]

On Thu, Aug 3, 2023 at 6:14 PM Michael Shavit <mshavit@google.com> wrote:
>
> The iommu core doesn't guarantee that pasid domains will be detached
> before the device is released.

I'm not certain whether this is possible or not. Is this change really
necessary?

Re: [PATCH v5 6/6] iommu/arm-smmu-v3: Cleanup arm_smmu_domain_finalise

[Jason Gunthorpe]

On Thu, Aug 03, 2023 at 06:12:26PM +0800, Michael Shavit wrote:
> Remove unused master parameter now that the CD table is allocated
> elsewhere.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
> (no changes since v1)
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)

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

Agree with moving the commit

Jason

Re: [PATCH v5 5/6] iommu/arm-smmu-v3: Free pasid domains on iommu release

[Jason Gunthorpe]

On Thu, Aug 03, 2023 at 06:17:15PM +0800, Michael Shavit wrote:
> On Thu, Aug 3, 2023 at 6:14 PM Michael Shavit <mshavit@google.com> wrote:
> >
> > The iommu core doesn't guarantee that pasid domains will be detached
> > before the device is released.
> 
> I'm not certain whether this is possible or not. Is this change really
> necessary?

I think we should rely on the core code to detach all PASIDs prior to
calling release, drivers should not do this. I suppose that means we
have a missing bit in the core code.

FWIW, I'd like to get to a point where the core code can also
automatically attach an identity domain so that the driver's release
functions don't have to open code that either...

Thus I wouldn't do this patch..

Jason

Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids

[Jason Gunthorpe]

On Thu, Aug 03, 2023 at 06:12:22PM +0800, Michael Shavit wrote:
> The arm-smmu-v3 driver keeps track of all masters that a domain is
> attached to so that it can re-write their STEs when the domain's ASID is
> upated by SVA.

Wah?

A domain's ASID shouldn't change, why does it change for SVA? Doesn't
SVA use CDTE's only? Why would it ever change a STE? I'm confused what
you are trying to explain here.

> +/*
> + * When ssid is 0, update all the CD entries that this domain is attached to.
> + * When ssid is non-zero, write the CD into all the masters where this domain is
> + * the primary domain, with the provided SSID. This is used because SVA still
> + * piggybacks over the primary domain.
> + */

What is a "primary domain"? Why can't we fix SVA first so it doesn't
have this weird "piggybacks" or:

> +/*
> + * If ssid is non-zero, issue atc invalidations with the given ssid instead of
> + * the one the domain is attached to. This is used by SVA since it's pasid
> + * attachments aren't recorded in smmu_domain yet.
> + */

fails to be recorded?

This patch is not making sense to me, the goal in the commit message
seems logical, but why is tracking CD entries introducing this concept
of a primary domain and doing special stuff for SSID=0?

> 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 e76452e735a04..66a492cafe2e8 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -689,11 +689,19 @@ struct arm_smmu_stream {
>  	struct rb_node			node;
>  };
>  
> +/* List of {masters, ssid} that a domain is attached to */
> +struct arm_smmu_attached_domain {
> +	struct list_head	list;

I like to call this something like

 attached_ssids_item

To help understand where the list head is and that this is a list
element.

> +	struct arm_smmu_domain  *domain;
> +	struct arm_smmu_master  *master;
> +	int			ssid;
> +};
> +
>  /* SMMU private data for each master */
>  struct arm_smmu_master {
>  	struct arm_smmu_device		*smmu;
>  	struct device			*dev;
> -	struct arm_smmu_domain		*domain;
> +	struct arm_smmu_attached_domain	non_pasid_domain;

Probably should consistently use the word ssid not pasid in driver
internals. 

The smmu spec talks about the substream ID being optional and the
behavior is controleld by the STE.S1DSS (Default substream)

So maybe non_subtream_domain ?

> @@ -730,8 +738,12 @@ struct arm_smmu_domain {
>  
>  	struct iommu_domain		domain;
>  
> -	struct list_head		devices;
> -	spinlock_t			devices_lock;
> +	/*
> +	 * List of arm_smmu_attached_domain nodes used to track all the
> +	 * {master, ssid} pairs that this domain is attached to.
> +	 */
> +	struct list_head		attached_ssids;
> +	spinlock_t			attached_ssids_lock;

So ssid = 0 means that the list entry == master->non_pasid_domain ?

It would be clearer to just test that directly if that is what needs
to be determined.

At least these struct changes seem aligned with the commit message :)

Jason

Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids

[Michael Shavit]

On Thu, Aug 3, 2023 at 11:42 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Thu, Aug 03, 2023 at 06:12:22PM +0800, Michael Shavit wrote:
> > The arm-smmu-v3 driver keeps track of all masters that a domain is
> > attached to so that it can re-write their STEs when the domain's ASID is
> > upated by SVA.
>
> Wah?
>
> A domain's ASID shouldn't change, why does it change for SVA? Doesn't
> SVA use CDTE's only? Why would it ever change a STE? I'm confused what
> you are trying to explain here.

Urh right, I mixed up CD entry and STE here. Before this patch, SVA
keeps tracks of all the masters attached to a CD domain, and updates
the CD entry 0 in their CD table. Now that a CD domain can be attached
on non-zero SSIDs, SVA can't simply update slot 0 in the table; it
must know which slot(s) this domain is attached to.

>
> What is a "primary domain"? Why can't we fix SVA first so it doesn't
> have this weird "piggybacks" or:
>
...
>
> This patch is not making sense to me, the goal in the commit message
> seems logical, but why is tracking CD entries introducing this concept
> of a primary domain and doing special stuff for SSID=0?

I'd argue this patch isn't introducing anything the driver isn't
already doing. It's just making the status-quo explicit since it has
to handle it. I do have a patch series in the works to properly fix
SVA, but it's growing quite large and I was trying to get this feature
out first. At a high level, the subsequent series:
1. Nests the list of attached masters in a list of SMMUs the domain is
installed in. Updates SMMU-level operations (such as invalidations) to
iterate over said list.
2. Checks the compatibility of a domain when attaching to a new SMMU
instead of outright rejecting, to allow attaching a domain to multiple
SMMUs.
3. Thus allowing SVA to alloc a single domain for the MM struct (which
the series maps from multiple SVA domains internally, pending support
at the iommu core layer)
4. And allowing SVA to use the same set_dev_pasid implementation used
here on that domain.

Now having said that, it might be possible to get rid of piggybacking
sooner if we create an MM per master instead of per "primary-domain",
but I'm not too sure about performance implications. AFAICT, the only
downside might be for invalidate_range commands that could no longer
be sent as a batched command to the SMMU (since each mmu notifier
would be called independently).

Re: [PATCH v5 2/6] iommu/arm-smmu-v3: Keep track of attached ssids

[Jason Gunthorpe]

On Fri, Aug 04, 2023 at 12:32:08AM +0800, Michael Shavit wrote:
> On Thu, Aug 3, 2023 at 11:42 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Thu, Aug 03, 2023 at 06:12:22PM +0800, Michael Shavit wrote:
> > > The arm-smmu-v3 driver keeps track of all masters that a domain is
> > > attached to so that it can re-write their STEs when the domain's ASID is
> > > upated by SVA.
> >
> > Wah?
> >
> > A domain's ASID shouldn't change, why does it change for SVA? Doesn't
> > SVA use CDTE's only? Why would it ever change a STE? I'm confused what
> > you are trying to explain here.
> 
> Urh right, I mixed up CD entry and STE here. Before this patch, SVA
> keeps tracks of all the masters attached to a CD domain, and updates
> the CD entry 0 in their CD table. 

Because it assumes that if a domain is returned from the ASID lookup
it is a RID domain.

> Now that a CD domain can be attached on non-zero SSIDs, SVA can't
> simply update slot 0 in the table; it must know which slot(s) this
> domain is attached to.

Yes, so why are you passing in 0 as the ssid argument to
arm_smmu_write_ctx_desc_devices() for the ASID change?

I think your commit message is trying to say:

The SMMUv3 driver keeps track of all the iommu_domains that are
assigned to an ASID in an xarray. The SVA code needs to re-use the
same ASID as the CPU's ASID (presumably only for BTM mode?) so it has
a mechanism to reclaim an already used ASID from an existing domain.

This is currently hardwired with the assumption that a domain using an
ASID is only on SSID 0.

Add a list to the struct arm_smmu_domain recording each master and
SSID that the domain is attached to and update it when any domain is
attached/detached.

Make arm_smmu_write_ctx_desc_devices() follow this list when storing
the CD table entries for the domain.

Remove 'ssid' as an argument to arm_smmu_write_ctx_desc_devices()
since it is always found in the attached_ssids.

> > What is a "primary domain"? Why can't we fix SVA first so it doesn't
> > have this weird "piggybacks" or:
> >
> ...
> >
> > This patch is not making sense to me, the goal in the commit message
> > seems logical, but why is tracking CD entries introducing this concept
> > of a primary domain and doing special stuff for SSID=0?
> 
> I'd argue this patch isn't introducing anything the driver isn't
> already doing.

So this I don't understand:

+               if (ssid && attached_domain->ssid == 0) {
+                       ret = arm_smmu_write_ctx_desc(master, ssid, cd);
+               } else {
+                       ret = arm_smmu_write_ctx_desc(
+                               master, attached_domain->ssid, cd);
+               }

Fix this patch so attached_domain->ssid is never wrong?

Remove ssid as an input to the function.

(I'd ultimately expect to remove CD too)

> it. I do have a patch series in the works to properly fix SVA, but
> it's growing quite large and I was trying to get this feature
> out first. At a high level, the subsequent series:
> 1. Nests the list of attached masters in a list of SMMUs the domain is
> installed in. Updates SMMU-level operations (such as invalidations) to
> iterate over said list.
> 2. Checks the compatibility of a domain when attaching to a new SMMU
> instead of outright rejecting, to allow attaching a domain to multiple
> SMMUs.
> 3. Thus allowing SVA to alloc a single domain for the MM struct (which
> the series maps from multiple SVA domains internally, pending support
> at the iommu core layer)

This should not be hard for the core code

> 4. And allowing SVA to use the same set_dev_pasid implementation used
> here on that domain.

This list all makes alot of sense to me

> Now having said that, it might be possible to get rid of piggybacking
> sooner if we create an MM per master instead of per "primary-domain",
> but I'm not too sure about performance implications. AFAICT, the only
> downside might be for invalidate_range commands that could no longer
> be sent as a batched command to the SMMU (since each mmu notifier
> would be called independently).

I'm not sure this series leaves things in a better state than before,
now we have two parallel domain attachment paths for PASID :(

Jason

Re: [PATCH v5 4/6] iommu/arm-smmu-v3: Implement set_dev_pasid

[Jason Gunthorpe]

On Thu, Aug 03, 2023 at 06:12:24PM +0800, Michael Shavit wrote:

> @@ -2448,21 +2476,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  		return -EBUSY;
>  	}
>  
> -	arm_smmu_detach_dev(master);
> -
> -	mutex_lock(&smmu_domain->init_mutex);
> -
> -	if (!smmu_domain->smmu) {
> -		smmu_domain->smmu = smmu;
> -		ret = arm_smmu_domain_finalise(domain, master);
> -		if (ret)
> -			smmu_domain->smmu = NULL;
> -	} else if (smmu_domain->smmu != smmu)
> -		ret = -EINVAL;
> +	/*
> +	 * Attaching a bypass or stage 2 domain would break any domains attached
> +	 * with pasid. Attaching an S1 domain should be feasible but requires
> +	 * more complicated logic to handle.
> +	 */
> +	if (arm_smmu_master_has_pasid_domains(master)) {
> +		dev_err(dev, "cannot attach - domain attached with pasid\n");
> +		return -EBUSY;
> +	}

Basically don't you just call set_dev_pased(pasid = 0) and it will do
that logic?

> +static int arm_smmu_set_dev_pasid(struct iommu_domain *domain,
> +				  struct device *dev, ioasid_t pasid)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +	struct arm_smmu_device *smmu;
> +	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +	struct arm_smmu_attached_domain *attached_domain;
> +	struct arm_smmu_master *master;
> +
> +	if (!fwspec)
> +		return -ENOENT;

fwspec drivers always have fwspecs, they don't need to check this.

> +
> +	master = dev_iommu_priv_get(dev);
> +	smmu = master->smmu;
> +
> +	ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain, master);
> +	if (ret)
> +		return ret;
> +
> +	if (pasid == 0) {
> +		dev_err(dev, "pasid 0 is reserved for the device's primary domain\n");
> +		return -ENODEV;
> +	}
> +
> +	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) {
> +		dev_err(dev, "set_dev_pasid only supports stage 1 domains\n");

don't print messages, iommufd can trigger these paths from userspace.

> @@ -2738,6 +2828,15 @@ static void arm_smmu_release_device(struct device *dev)
>  
>  	if (WARN_ON(arm_smmu_master_sva_enabled(master)))
>  		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> +	if (WARN_ON(master->nr_attached_pasid_domains != 0)) {
> +		/*
> +		 * TODO: Do we need to handle this case?
> +		 * This requires a mechanism to obtain all the pasid domains
> +		 * that this master is attached to so that we can clean up the
> +		 * domain's attached_domain list.
> +		 */
> +	}

Unnecessary, the core code should handle this, add a patch that does this:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5b5cf74edc7e53..57cac1ffba1cfe 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -438,9 +438,16 @@ static void iommu_deinit_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	struct iommu_domain *{pasid_domain;
+	unsigned long pasid;
 
 	lockdep_assert_held(&group->mutex);
 
+	xa_for_each(&group->pasid_array, pasid, pasid_domain) {
+		__iommu_remove_group_pasid(pasid);
+		xa_erase(&group->pasid_array, pasid);
+	}
+
 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
 
 	/*


> @@ -2874,12 +2973,36 @@ static int arm_smmu_def_domain_type(struct device *dev)
>  static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>  {
>  	struct iommu_domain *domain;
> +	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> +	struct arm_smmu_domain *smmu_domain;
> +	struct arm_smmu_attached_domain *attached_domain;
> +	unsigned long flags;
>  
> -	domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
> +	if (!master || pasid == 0)
> +		return;
> +
> +	domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
>  	if (WARN_ON(IS_ERR(domain)) || !domain)
>  		return;

I feel like we have made a mistake here to not pass in the current
domain from the core code, it already knows what the domain was..

> +	if (domain->type == IOMMU_DOMAIN_SVA)
> +		return arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);

Yuk.

Jason