[RFC PATCH v1 0/8] Install domain onto multiple smmus

[RFC PATCH v1 0/8] Install domain onto multiple smmus

[Michael Shavit]

Hi all,

This series refactors the arm-smmu-v3 driver to support attaching
domains onto masters belonging to different smmu devices.

The main objective of this series is allow further refactorings of
arm-smmu-v3-sva. Specifically, we'd like to reach the state where:
1. A single SVA domain is allocated per MM/ASID
2. arm-smmu-v3-sva's set_dev_pasid implementation directly attaches that
   SVA domain to different masters, regardless of whether those masters
   belong to different smmus.

If armm-smmu-v3-sva is handed iommu_domains that have a 1:1 relationship
with an MM struct, then it won't have to share a CD with multiple
domains (or arm_smmu_mmu_notifiers). But to get there, the arm-smmu-v3
driver must first support domains installed on multiple SMMU devices.

This series depends on the CD table ownership refactor: https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@google.com/
as well as the VMID IDA patch: https://lore.kernel.org/all/169087904450.1290857.11726985177314533259.b4-ty@kernel.org/#r

This series is also available on gerrit: https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/24829/6

Thanks,
Michael Shavit


Michael Shavit (8):
  iommu/arm-smmu-v3: Add list of installed_smmus
  iommu/arm-smmu-v3: Perform invalidations over installed_smmus
  iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus
  iommu/arm-smmu-v3: check smmu compatibility on attach
  iommu/arm-smmu-v3: Add arm_smmu_device as a parameter to
    domain_finalise
  iommu/arm-smmu-v3: Free VMID when uninstalling domain from SMMU
  iommu/arm-smmu-v3: check for domain initialization using pgtbl_ops
  iommu/arm-smmu-v3: allow multi-SMMU domain installs.

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  62 +++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 348 +++++++++++++-----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  21 +-
 3 files changed, 320 insertions(+), 111 deletions(-)


base-commit: 6eaae198076080886b9e7d57f4ae06fa782f90ef
prerequisite-patch-id: f701e5ac2cce085366342edff287a35d1cb82b9c
prerequisite-patch-id: c8d21ff19c2c1dd18799a6b83f483add654d187e
prerequisite-patch-id: 6ebba95cb12a723645843b4bd1bc45c94779d853
prerequisite-patch-id: 3f767e1c37d2996323c4f6d2a2d1912ab75281f7
prerequisite-patch-id: 5a4109fa3e22e2399ad064951c2ca1aeba4a68f7
prerequisite-patch-id: c4b3bd34b8be7afebd3e44bc4ec218d74753ce77
prerequisite-patch-id: 6d89e53518d25ac983ac99786950ee1a558c271f
prerequisite-patch-id: 447219e565cadc34b03db05dad58d8e5c4b5a382
prerequisite-patch-id: 63adb2c3f97d4948d96a0d5960184f5ac814d7f7
prerequisite-patch-id: e71195fcf1aa56d8ef9d7403b9e4492c17b8fb84
prerequisite-patch-id: ba82add44850bf8fb271292020edb746aef93a65
-- 
2.42.0.rc1.204.g551eb34607-goog


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

[RFC PATCH v1 1/8] iommu/arm-smmu-v3: Add list of installed_smmus

[Michael Shavit]

Add a new arm_smmu_installed_smmu class to aggregate masters belonging
to the same SMMU that a domain is attached to.
Update usages of the domain->devices list to first iterate over this
parent installed_smmus list.

This allows functions that batch commands to an SMMU to first iterate
over the list of installed SMMUs before preparing the batched command
from the set of attached masters.

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

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  28 ++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 109 ++++++++++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  14 ++-
 3 files changed, 118 insertions(+), 33 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 238ede8368d10..a4e235b4f1c4b 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
@@ -48,21 +48,37 @@ 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_installed_smmu *installed_smmu;
 	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->installed_smmus_lock, flags);
+	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus, list) {
+		list_for_each_entry(master, &installed_smmu->devices, list) {
+			ret = arm_smmu_write_ctx_desc(master, ssid, cd);
+			if (ret) {
+				list_for_each_entry_from_reverse(
+					master, &installed_smmu->devices, list)
+					arm_smmu_write_ctx_desc(master, ssid,
+								NULL);
+				break;
+			}
+		}
 		if (ret) {
-			list_for_each_entry_from_reverse(master, &smmu_domain->devices, domain_head)
-				arm_smmu_write_ctx_desc(master, ssid, NULL);
+			list_for_each_entry_continue_reverse(
+				installed_smmu, &smmu_domain->installed_smmus,
+				list) {
+				list_for_each_entry(
+					master, &installed_smmu->devices, list)
+					arm_smmu_write_ctx_desc(master, ssid,
+								NULL);
+			}
 			break;
 		}
 	}
 
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
 	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 f17704c35858d..cb4bf0c7c3dd6 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1811,10 +1811,12 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
 			    unsigned long iova, size_t size)
 {
 	int i;
+	int ret;
 	unsigned long flags;
 	struct arm_smmu_cmdq_ent cmd;
 	struct arm_smmu_master *master;
 	struct arm_smmu_cmdq_batch cmds;
+	struct arm_smmu_installed_smmu *installed_smmu;
 
 	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_ATS))
 		return 0;
@@ -1838,21 +1840,26 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
 
 	arm_smmu_atc_inv_to_cmd(ssid, iova, size, &cmd);
 
-	cmds.num = 0;
-
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
-		if (!master->ats_enabled)
-			continue;
+	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
+	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus, list) {
+		cmds.num = 0;
+		list_for_each_entry(master, &installed_smmu->devices, list) {
+			if (!master->ats_enabled)
+				continue;
 
-		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);
+			for (i = 0; i < master->num_streams; i++) {
+				cmd.atc.sid = master->streams[i].id;
+				arm_smmu_cmdq_batch_add(installed_smmu->smmu,
+							&cmds, &cmd);
+			}
 		}
+		ret = arm_smmu_cmdq_batch_submit(installed_smmu->smmu, &cmds);
+		if (ret)
+			break;
 	}
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
 
-	return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
+	return ret;
 }
 
 /* IO_PGTABLE API */
@@ -2049,8 +2056,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->installed_smmus);
+	spin_lock_init(&smmu_domain->installed_smmus_lock);
 	INIT_LIST_HEAD(&smmu_domain->mmu_notifiers);
 
 	return &smmu_domain->domain;
@@ -2353,9 +2360,66 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 	pci_disable_pasid(pdev);
 }
 
-static void arm_smmu_detach_dev(struct arm_smmu_master *master)
+static void arm_smmu_installed_smmus_remove_device(
+	struct arm_smmu_domain *smmu_domain,
+	struct arm_smmu_master *master)
 {
+	struct arm_smmu_installed_smmu *installed_smmu;
+	struct arm_smmu_device *smmu;
 	unsigned long flags;
+
+	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
+	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus,
+			    list) {
+		smmu = installed_smmu->smmu;
+		if (smmu != master->smmu)
+			continue;
+		list_del(&master->list);
+		if (list_empty(&installed_smmu->devices)) {
+			list_del(&installed_smmu->list);
+			kfree(installed_smmu);
+		}
+		break;
+	}
+	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
+}
+
+static int
+arm_smmu_installed_smmus_add_device(struct arm_smmu_domain *smmu_domain,
+				    struct arm_smmu_master *master)
+{
+	struct arm_smmu_installed_smmu *installed_smmu;
+	struct arm_smmu_device *smmu = master->smmu;
+	bool list_entry_found = false;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
+	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus,
+			    list) {
+		if (installed_smmu->smmu == smmu) {
+			list_entry_found = true;
+			break;
+		}
+	}
+	if (!list_entry_found) {
+		installed_smmu = kzalloc(sizeof(*installed_smmu), GFP_KERNEL);
+		if (!installed_smmu) {
+			ret = -ENOMEM;
+			goto unlock;
+		}
+		INIT_LIST_HEAD(&installed_smmu->devices);
+		installed_smmu->smmu = smmu;
+		list_add(&installed_smmu->list, &smmu_domain->installed_smmus);
+	}
+	list_add(&master->list, &installed_smmu->devices);
+unlock:
+	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
+	return ret;
+}
+
+static void arm_smmu_detach_dev(struct arm_smmu_master *master)
+{
 	struct arm_smmu_domain *smmu_domain = master->domain;
 
 	if (!smmu_domain)
@@ -2363,9 +2427,7 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 
 	arm_smmu_disable_ats(master);
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_del(&master->domain_head);
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	arm_smmu_installed_smmus_remove_device(smmu_domain, master);
 
 	master->domain = NULL;
 	master->ats_enabled = false;
@@ -2385,7 +2447,6 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
 	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);
@@ -2435,9 +2496,11 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
 		master->ats_enabled = arm_smmu_ats_supported(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);
+	ret = arm_smmu_installed_smmus_add_device(smmu_domain, master);
+	if (ret) {
+		master->domain = NULL;
+		return ret;
+	}
 
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		if (!master->cd_table.cdtab) {
@@ -2467,9 +2530,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return 0;
 
 out_list_del:
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_del(&master->domain_head);
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	arm_smmu_installed_smmus_remove_device(smmu_domain, master);
 
 	return 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 83d2790b701e7..a9202d2045537 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -690,12 +690,20 @@ struct arm_smmu_stream {
 	struct rb_node			node;
 };
 
+/* List of smmu devices that a domain is installed to */
+struct arm_smmu_installed_smmu {
+	struct list_head	list;
+	/* List of masters that the domain is attached to*/
+	struct list_head	devices;
+	struct arm_smmu_device	*smmu;
+};
+
 /* SMMU private data for each master */
 struct arm_smmu_master {
 	struct arm_smmu_device		*smmu;
 	struct device			*dev;
 	struct arm_smmu_domain		*domain;
-	struct list_head		domain_head;
+	struct list_head		list;
 	struct arm_smmu_stream		*streams;
 	/* Locked by the iommu core using the group mutex */
 	struct arm_smmu_ctx_desc_cfg	cd_table;
@@ -731,8 +739,8 @@ struct arm_smmu_domain {
 
 	struct iommu_domain		domain;
 
-	struct list_head		devices;
-	spinlock_t			devices_lock;
+	struct list_head		installed_smmus;
+	spinlock_t			installed_smmus_lock;
 
 	struct list_head		mmu_notifiers;
 };
-- 
2.42.0.rc1.204.g551eb34607-goog


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

[RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Michael Shavit]

Prepare and batch invalidation commands for each SMMU that a domain is
installed onto.
Move SVA's check against the smmu's ARM_SMMU_FEAT_BTM bit into
arm_smmu_tlb_inv_range_asid so that it can be checked against each
installed SMMU.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
It's not obvious to me whether skipping the tlb_inv_range_asid when
ARM_SMMU_FEAT_BTM is somehow specific to SVA? Is moving the check into
arm_smmu_tlb_inv_range_asid still valid if that function were called
outside of SVA?

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  11 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 103 +++++++++++++-----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   2 +-
 3 files changed, 80 insertions(+), 36 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 a4e235b4f1c4b..58def59c36004 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
@@ -128,7 +128,7 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
 	arm_smmu_write_ctx_desc_devices(smmu_domain, 0, cd);
 
 	/* Invalidate TLB entries previously associated with that context */
-	arm_smmu_tlb_inv_asid(smmu, asid);
+	arm_smmu_tlb_inv_asid(smmu_domain, asid);
 
 	xa_erase(&arm_smmu_asid_xa, asid);
 	return NULL;
@@ -246,9 +246,8 @@ static void arm_smmu_mm_invalidate_range(struct mmu_notifier *mn,
 	 */
 	size = end - start;
 
-	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_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);
 }
 
@@ -269,7 +268,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_tlb_inv_asid(smmu_domain, smmu_mn->cd->asid);
 	arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
 
 	smmu_mn->cleared = true;
@@ -357,7 +356,7 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 	 * new TLB entry can have been formed.
 	 */
 	if (!smmu_mn->cleared) {
-		arm_smmu_tlb_inv_asid(smmu_domain->smmu, cd->asid);
+		arm_smmu_tlb_inv_asid(smmu_domain, cd->asid);
 		arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
 	}
 
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 cb4bf0c7c3dd6..447af74dbe280 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -960,15 +960,24 @@ static int arm_smmu_page_response(struct device *dev,
 }
 
 /* Context descriptor manipulation functions */
-void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid)
+void arm_smmu_tlb_inv_asid(struct arm_smmu_domain *smmu_domain, u16 asid)
 {
+	struct arm_smmu_installed_smmu *installed_smmu;
+	struct arm_smmu_device *smmu;
 	struct arm_smmu_cmdq_ent cmd = {
-		.opcode	= smmu->features & ARM_SMMU_FEAT_E2H ?
-			CMDQ_OP_TLBI_EL2_ASID : CMDQ_OP_TLBI_NH_ASID,
 		.tlbi.asid = asid,
 	};
+	unsigned long flags;
 
-	arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
+	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus,
+			    list) {
+		smmu = installed_smmu->smmu;
+		cmd.opcode	= smmu->features & ARM_SMMU_FEAT_E2H ?
+			CMDQ_OP_TLBI_EL2_ASID : CMDQ_OP_TLBI_NH_ASID;
+		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+	}
+	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
 }
 
 static void arm_smmu_sync_cd(struct arm_smmu_master *master,
@@ -1818,9 +1827,6 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
 	struct arm_smmu_cmdq_batch cmds;
 	struct arm_smmu_installed_smmu *installed_smmu;
 
-	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_ATS))
-		return 0;
-
 	/*
 	 * Ensure that we've completed prior invalidation of the main TLBs
 	 * before we read 'nr_ats_masters' in case of a concurrent call to
@@ -1862,12 +1868,29 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
 	return ret;
 }
 
+static void arm_smmu_tlb_inv_vmid(struct arm_smmu_domain *smmu_domain)
+{
+	struct arm_smmu_installed_smmu *installed_smmu;
+	struct arm_smmu_device *smmu;
+	struct arm_smmu_cmdq_ent cmd = {
+		.opcode = CMDQ_OP_TLBI_S12_VMALL,
+		.tlbi.vmid = smmu_domain->s2_cfg.vmid,
+	};
+	unsigned long flags;
+
+	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
+	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus,
+			    list) {
+		smmu = installed_smmu->smmu;
+		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+	}
+	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
+}
+
 /* IO_PGTABLE API */
 static void arm_smmu_tlb_inv_context(void *cookie)
 {
 	struct arm_smmu_domain *smmu_domain = cookie;
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
-	struct arm_smmu_cmdq_ent cmd;
 
 	/*
 	 * NOTE: when io-pgtable is in non-strict mode, we may get here with
@@ -1877,11 +1900,9 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 	 * careful, 007.
 	 */
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		arm_smmu_tlb_inv_asid(smmu, smmu_domain->cd.asid);
+		arm_smmu_tlb_inv_asid(smmu_domain, smmu_domain->cd.asid);
 	} else {
-		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
-		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
-		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+		arm_smmu_tlb_inv_vmid(smmu_domain);
 	}
 	arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
 }
@@ -1889,9 +1910,9 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 				     unsigned long iova, size_t size,
 				     size_t granule,
-				     struct arm_smmu_domain *smmu_domain)
+				     struct arm_smmu_domain *smmu_domain,
+				     struct arm_smmu_device *smmu)
 {
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	unsigned long end = iova + size, num_pages = 0, tg = 0;
 	size_t inv_range = granule;
 	struct arm_smmu_cmdq_batch cmds;
@@ -1956,21 +1977,32 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
 					  size_t granule, bool leaf,
 					  struct arm_smmu_domain *smmu_domain)
 {
+	struct arm_smmu_installed_smmu *installed_smmu;
+	struct arm_smmu_device *smmu;
 	struct arm_smmu_cmdq_ent cmd = {
 		.tlbi = {
 			.leaf	= leaf,
 		},
 	};
+	unsigned long flags;
 
-	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		cmd.opcode	= smmu_domain->smmu->features & ARM_SMMU_FEAT_E2H ?
-				  CMDQ_OP_TLBI_EL2_VA : CMDQ_OP_TLBI_NH_VA;
-		cmd.tlbi.asid	= smmu_domain->cd.asid;
-	} else {
-		cmd.opcode	= CMDQ_OP_TLBI_S2_IPA;
-		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
+	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
+	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus,
+			    list) {
+		smmu = installed_smmu->smmu;
+		if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
+			cmd.opcode = smmu->features & ARM_SMMU_FEAT_E2H ?
+					     CMDQ_OP_TLBI_EL2_VA :
+					     CMDQ_OP_TLBI_NH_VA;
+			cmd.tlbi.asid = smmu_domain->cd.asid;
+		} else {
+			cmd.opcode = CMDQ_OP_TLBI_S2_IPA;
+			cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid;
+		}
+		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain,
+					 smmu);
 	}
-	__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain);
+	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
 
 	/*
 	 * Unfortunately, this can't be leaf-only since we may have
@@ -1983,16 +2015,30 @@ 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)
 {
+
+	struct arm_smmu_installed_smmu *installed_smmu;
+	struct arm_smmu_device *smmu;
+	unsigned long flags;
 	struct arm_smmu_cmdq_ent cmd = {
-		.opcode	= smmu_domain->smmu->features & ARM_SMMU_FEAT_E2H ?
-			  CMDQ_OP_TLBI_EL2_VA : CMDQ_OP_TLBI_NH_VA,
 		.tlbi = {
 			.asid	= asid,
 			.leaf	= leaf,
 		},
 	};
-
-	__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain);
+	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
+	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus,
+			    list) {
+		smmu = installed_smmu->smmu;
+		if (smmu->features & ARM_SMMU_FEAT_BTM)
+			continue;
+		cmd.opcode = smmu->features &
+					     ARM_SMMU_FEAT_E2H ?
+				     CMDQ_OP_TLBI_EL2_VA :
+				     CMDQ_OP_TLBI_NH_VA;
+		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain,
+					 smmu);
+	}
+	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
 }
 
 static void arm_smmu_tlb_inv_page_nosync(struct iommu_iotlb_gather *gather,
@@ -2564,8 +2610,7 @@ static void arm_smmu_flush_iotlb_all(struct iommu_domain *domain)
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
-	if (smmu_domain->smmu)
-		arm_smmu_tlb_inv_context(smmu_domain);
+	arm_smmu_tlb_inv_context(smmu_domain);
 }
 
 static void arm_smmu_iotlb_sync(struct iommu_domain *domain,
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 a9202d2045537..2ab23139c796e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -756,7 +756,7 @@ extern struct arm_smmu_ctx_desc quiet_cd;
 
 int arm_smmu_write_ctx_desc(struct arm_smmu_master *smmu_master, int ssid,
 			    struct arm_smmu_ctx_desc *cd);
-void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid);
+void arm_smmu_tlb_inv_asid(struct arm_smmu_domain *smmu_domain, u16 asid);
 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);
-- 
2.42.0.rc1.204.g551eb34607-goog


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

[RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Michael Shavit]

Pick an ASID that is within the supported range of all SMMUs that the
domain is installed to.

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

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 23 +++++++++++++++----
 1 file changed, 19 insertions(+), 4 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 58def59c36004..ab941e394cae5 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
@@ -82,6 +82,20 @@ static int arm_smmu_write_ctx_desc_devices(struct arm_smmu_domain *smmu_domain,
 	return ret;
 }
 
+static u32 arm_smmu_domain_max_asid_bits(struct arm_smmu_domain *smmu_domain)
+{
+	struct arm_smmu_installed_smmu *installed_smmu;
+	unsigned long flags;
+	u32 asid_bits = 16;
+
+	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
+	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus,
+			    list)
+		asid_bits = min(asid_bits, installed_smmu->smmu->asid_bits);
+	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
+	return asid_bits;
+}
+
 /*
  * Check if the CPU ASID is available on the SMMU side. If a private context
  * descriptor is using it, try to replace it.
@@ -92,7 +106,6 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
 	int ret;
 	u32 new_asid;
 	struct arm_smmu_ctx_desc *cd;
-	struct arm_smmu_device *smmu;
 	struct arm_smmu_domain *smmu_domain;
 
 	cd = xa_load(&arm_smmu_asid_xa, asid);
@@ -108,10 +121,12 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
 	}
 
 	smmu_domain = container_of(cd, struct arm_smmu_domain, cd);
-	smmu = smmu_domain->smmu;
 
-	ret = xa_alloc(&arm_smmu_asid_xa, &new_asid, cd,
-		       XA_LIMIT(1, (1 << smmu->asid_bits) - 1), GFP_KERNEL);
+	ret = xa_alloc(
+		&arm_smmu_asid_xa, &new_asid, cd,
+		XA_LIMIT(1,
+			 (1 << arm_smmu_domain_max_asid_bits(smmu_domain)) - 1),
+		GFP_KERNEL);
 	if (ret)
 		return ERR_PTR(-ENOSPC);
 	/*
-- 
2.42.0.rc1.204.g551eb34607-goog


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

[RFC PATCH v1 4/8] iommu/arm-smmu-v3: check smmu compatibility on attach

[Michael Shavit]

Record the domain's pgtbl_cfg when it's being prepared so that it can
later be compared to the features an smmu supports.
Verify a domain's compatibility with the smmu when it's being attached
to a master belong to a different smmu device.

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

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 103 ++++++++++++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   2 +
 2 files changed, 86 insertions(+), 19 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 447af74dbe280..c0943cf3c09ca 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2195,17 +2195,48 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_domain *smmu_domain,
 	return 0;
 }
 
+static int arm_smmu_prepare_pgtbl_cfg(struct arm_smmu_device *smmu,
+				      enum arm_smmu_domain_stage stage,
+				      struct io_pgtable_cfg *pgtbl_cfg)
+{
+	unsigned long ias, oas;
+
+	switch (stage) {
+	case ARM_SMMU_DOMAIN_S1:
+		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
+		ias = min_t(unsigned long, ias, VA_BITS);
+		oas = smmu->ias;
+		break;
+	case ARM_SMMU_DOMAIN_NESTED:
+	case ARM_SMMU_DOMAIN_S2:
+		ias = smmu->ias;
+		oas = smmu->oas;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	*pgtbl_cfg = (struct io_pgtable_cfg) {
+		.pgsize_bitmap	= smmu->pgsize_bitmap,
+		.ias		= ias,
+		.oas		= oas,
+		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
+		.tlb		= &arm_smmu_flush_ops,
+		.iommu_dev	= smmu->dev,
+	};
+	return 0;
+}
+
 static int arm_smmu_domain_finalise(struct iommu_domain *domain)
 {
 	int ret;
-	unsigned long ias, oas;
 	enum io_pgtable_fmt fmt;
-	struct io_pgtable_cfg pgtbl_cfg;
 	struct io_pgtable_ops *pgtbl_ops;
 	int (*finalise_stage_fn)(struct arm_smmu_domain *,
 				 struct io_pgtable_cfg *);
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct io_pgtable_cfg *pgtbl_cfg = &smmu_domain->pgtbl_cfg;
 
 	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
 		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
@@ -2220,16 +2251,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
 
 	switch (smmu_domain->stage) {
 	case ARM_SMMU_DOMAIN_S1:
-		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
-		ias = min_t(unsigned long, ias, VA_BITS);
-		oas = smmu->ias;
 		fmt = ARM_64_LPAE_S1;
 		finalise_stage_fn = arm_smmu_domain_finalise_s1;
 		break;
 	case ARM_SMMU_DOMAIN_NESTED:
 	case ARM_SMMU_DOMAIN_S2:
-		ias = smmu->ias;
-		oas = smmu->oas;
 		fmt = ARM_64_LPAE_S2;
 		finalise_stage_fn = arm_smmu_domain_finalise_s2;
 		break;
@@ -2237,24 +2263,19 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
 		return -EINVAL;
 	}
 
-	pgtbl_cfg = (struct io_pgtable_cfg) {
-		.pgsize_bitmap	= smmu->pgsize_bitmap,
-		.ias		= ias,
-		.oas		= oas,
-		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
-		.tlb		= &arm_smmu_flush_ops,
-		.iommu_dev	= smmu->dev,
-	};
+	ret = arm_smmu_prepare_pgtbl_cfg(smmu, smmu_domain->stage, pgtbl_cfg);
+	if (ret)
+		return ret;
 
-	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
+	pgtbl_ops = alloc_io_pgtable_ops(fmt, pgtbl_cfg, smmu_domain);
 	if (!pgtbl_ops)
 		return -ENOMEM;
 
-	domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
-	domain->geometry.aperture_end = (1UL << pgtbl_cfg.ias) - 1;
+	domain->pgsize_bitmap = pgtbl_cfg->pgsize_bitmap;
+	domain->geometry.aperture_end = (1UL << pgtbl_cfg->ias) - 1;
 	domain->geometry.force_aperture = true;
 
-	ret = finalise_stage_fn(smmu_domain, &pgtbl_cfg);
+	ret = finalise_stage_fn(smmu_domain, pgtbl_cfg);
 	if (ret < 0) {
 		free_io_pgtable_ops(pgtbl_ops);
 		return ret;
@@ -2406,6 +2427,46 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 	pci_disable_pasid(pdev);
 }
 
+static int
+arm_smmu_verify_domain_compatible(struct arm_smmu_device *smmu,
+				  struct arm_smmu_domain *smmu_domain)
+{
+	struct io_pgtable_cfg pgtbl_cfg;
+	int ret;
+
+	if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY)
+		return 0;
+
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2) {
+		if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
+			return -EINVAL;
+		if (smmu_domain->s2_cfg.vmid >> smmu->vmid_bits)
+			return -EINVAL;
+	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
+		if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
+			return -EINVAL;
+		if (smmu_domain->cd.asid >> smmu->asid_bits)
+			return -EINVAL;
+	}
+
+	ret = arm_smmu_prepare_pgtbl_cfg(smmu, smmu_domain->stage, &pgtbl_cfg);
+	if (ret)
+		return ret;
+
+	if (smmu_domain->pgtbl_cfg.ias > pgtbl_cfg.ias ||
+	    smmu_domain->pgtbl_cfg.oas > pgtbl_cfg.oas ||
+	    /*
+	     * The supported pgsize_bitmap must be a superset of the domain's
+	     * pgsize_bitmap.
+	     */
+	    (smmu_domain->pgtbl_cfg.pgsize_bitmap ^ pgtbl_cfg.pgsize_bitmap) &
+		    smmu_domain->pgtbl_cfg.pgsize_bitmap ||
+	    smmu_domain->pgtbl_cfg.coherent_walk != pgtbl_cfg.coherent_walk)
+		return -EINVAL;
+
+	return 0;
+}
+
 static void arm_smmu_installed_smmus_remove_device(
 	struct arm_smmu_domain *smmu_domain,
 	struct arm_smmu_master *master)
@@ -2449,6 +2510,10 @@ arm_smmu_installed_smmus_add_device(struct arm_smmu_domain *smmu_domain,
 		}
 	}
 	if (!list_entry_found) {
+		ret = arm_smmu_verify_domain_compatible(smmu, smmu_domain);
+		if (ret)
+			goto unlock;
+
 		installed_smmu = kzalloc(sizeof(*installed_smmu), GFP_KERNEL);
 		if (!installed_smmu) {
 			ret = -ENOMEM;
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 2ab23139c796e..143b287be2f8b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -9,6 +9,7 @@
 #define _ARM_SMMU_V3_H
 
 #include <linux/bitfield.h>
+#include <linux/io-pgtable.h>
 #include <linux/iommu.h>
 #include <linux/kernel.h>
 #include <linux/mmzone.h>
@@ -729,6 +730,7 @@ struct arm_smmu_domain {
 	struct mutex			init_mutex; /* Protects smmu pointer */
 
 	struct io_pgtable_ops		*pgtbl_ops;
+	struct io_pgtable_cfg		pgtbl_cfg;
 	atomic_t			nr_ats_masters;
 
 	enum arm_smmu_domain_stage	stage;
-- 
2.42.0.rc1.204.g551eb34607-goog


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

[RFC PATCH v1 5/8] iommu/arm-smmu-v3: Add arm_smmu_device as a parameter to domain_finalise

[Michael Shavit]

Remove the usage of arm_smmu_domain->smmu in arm_smmu_domain_finalise to
prepare for the commit where a domain can be attached to multiple
masters.

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

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 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 c0943cf3c09ca..208fec5fba462 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2132,11 +2132,11 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 }
 
 static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
+				       struct arm_smmu_device *smmu,
 				       struct io_pgtable_cfg *pgtbl_cfg)
 {
 	int ret;
 	u32 asid;
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_ctx_desc *cd = &smmu_domain->cd;
 	typeof(&pgtbl_cfg->arm_lpae_s1_cfg.tcr) tcr = &pgtbl_cfg->arm_lpae_s1_cfg.tcr;
 
@@ -2169,10 +2169,10 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 }
 
 static int arm_smmu_domain_finalise_s2(struct arm_smmu_domain *smmu_domain,
+				       struct arm_smmu_device *smmu,
 				       struct io_pgtable_cfg *pgtbl_cfg)
 {
 	int vmid;
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
 	typeof(&pgtbl_cfg->arm_lpae_s2_cfg.vtcr) vtcr;
 
@@ -2227,15 +2227,16 @@ static int arm_smmu_prepare_pgtbl_cfg(struct arm_smmu_device *smmu,
 	return 0;
 }
 
-static int arm_smmu_domain_finalise(struct iommu_domain *domain)
+static int arm_smmu_domain_finalise(struct iommu_domain *domain,
+				    struct arm_smmu_device *smmu)
 {
 	int ret;
 	enum io_pgtable_fmt fmt;
 	struct io_pgtable_ops *pgtbl_ops;
 	int (*finalise_stage_fn)(struct arm_smmu_domain *,
+				 struct arm_smmu_device *,
 				 struct io_pgtable_cfg *);
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct io_pgtable_cfg *pgtbl_cfg = &smmu_domain->pgtbl_cfg;
 
 	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
@@ -2275,7 +2276,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, pgtbl_cfg);
+	ret = finalise_stage_fn(smmu_domain, smmu, pgtbl_cfg);
 	if (ret < 0) {
 		free_io_pgtable_ops(pgtbl_ops);
 		return ret;
@@ -2585,7 +2586,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	if (!smmu_domain->smmu) {
 		smmu_domain->smmu = smmu;
-		ret = arm_smmu_domain_finalise(domain);
+		ret = arm_smmu_domain_finalise(domain, smmu);
 		if (ret)
 			smmu_domain->smmu = NULL;
 	} else if (smmu_domain->smmu != smmu)
-- 
2.42.0.rc1.204.g551eb34607-goog


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

[RFC PATCH v1 6/8] iommu/arm-smmu-v3: Free VMID when uninstalling domain from SMMU

[Michael Shavit]

This will allow installing a domain onto multiple smmu devices.

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

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 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 208fec5fba462..7f88b2b19cbe5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2112,7 +2112,6 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 static void arm_smmu_domain_free(struct iommu_domain *domain)
 {
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
 
 	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
 
@@ -2122,10 +2121,6 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 		mutex_lock(&arm_smmu_asid_lock);
 		arm_smmu_free_asid(&smmu_domain->cd);
 		mutex_unlock(&arm_smmu_asid_lock);
-	} else {
-		struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
-		if (cfg->vmid)
-			ida_free(&smmu->vmid_map, cfg->vmid);
 	}
 
 	kfree(smmu_domain);
@@ -2484,6 +2479,14 @@ static void arm_smmu_installed_smmus_remove_device(
 			continue;
 		list_del(&master->list);
 		if (list_empty(&installed_smmu->devices)) {
+			if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2) {
+				struct arm_smmu_s2_cfg *cfg =
+					&smmu_domain->s2_cfg;
+
+				if (cfg->vmid)
+					ida_free(&smmu->vmid_map,
+						 cfg->vmid);
+			}
 			list_del(&installed_smmu->list);
 			kfree(installed_smmu);
 		}
-- 
2.42.0.rc1.204.g551eb34607-goog


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

[RFC PATCH v1 7/8] iommu/arm-smmu-v3: check for domain initialization using pgtbl_ops

[Michael Shavit]

In order to remove smmu_domain->smmu in the next commit

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

 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 7f88b2b19cbe5..c9f89f4f47721 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2918,7 +2918,7 @@ static int arm_smmu_enable_nesting(struct iommu_domain *domain)
 	int ret = 0;
 
 	mutex_lock(&smmu_domain->init_mutex);
-	if (smmu_domain->smmu)
+	if (smmu_domain->pgtbl_ops)
 		ret = -EPERM;
 	else
 		smmu_domain->stage = ARM_SMMU_DOMAIN_NESTED;
-- 
2.42.0.rc1.204.g551eb34607-goog


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

[RFC PATCH v1 8/8] iommu/arm-smmu-v3: allow multi-SMMU domain installs

[Michael Shavit]

Remove the arm_smmu_domain->smmu handle now that a domain may be
attached to devices with different upstream SMMUs.

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

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++-------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +--
 2 files changed, 3 insertions(+), 9 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 c9f89f4f47721..08594612a81a8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2587,13 +2587,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	mutex_lock(&smmu_domain->init_mutex);
 
-	if (!smmu_domain->smmu) {
-		smmu_domain->smmu = smmu;
-		ret = arm_smmu_domain_finalise(domain, smmu);
-		if (ret)
-			smmu_domain->smmu = NULL;
-	} else if (smmu_domain->smmu != smmu)
-		ret = -EINVAL;
+	if (!smmu_domain->pgtbl_ops)
+		ret = arm_smmu_domain_finalise(&smmu_domain->domain, smmu);
 
 	mutex_unlock(&smmu_domain->init_mutex);
 	if (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 143b287be2f8b..342fa6ef837ab 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -726,8 +726,7 @@ enum arm_smmu_domain_stage {
 };
 
 struct arm_smmu_domain {
-	struct arm_smmu_device		*smmu;
-	struct mutex			init_mutex; /* Protects smmu pointer */
+	struct mutex			init_mutex; /* Protects pgtbl_ops pointer */
 
 	struct io_pgtable_ops		*pgtbl_ops;
 	struct io_pgtable_cfg		pgtbl_cfg;
-- 
2.42.0.rc1.204.g551eb34607-goog


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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 02:16:25AM +0800, Michael Shavit wrote:
> Pick an ASID that is within the supported range of all SMMUs that the
> domain is installed to.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---

This seems like a pretty niche scenario, maybe we should just keep a
global for the max ASID?

Otherwise we need a code to change the ASID, even for non-SVA domains,
when the domain is installed in different devices if the current ASID
is over the instance max..

Ideally domain ASID would be selected at domain allocation time.

Jason

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

Re: [RFC PATCH v1 6/8] iommu/arm-smmu-v3: Free VMID when uninstalling domain from SMMU

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 02:16:28AM +0800, Michael Shavit wrote:
> This will allow installing a domain onto multiple smmu devices.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)

The VMID is basically a different ASID, eg ASID is the cahage tag for
a S1 and VMID is the cache tag for S2.

It is strange that it has a different lifecycle. It seems like the
issue is that the VMID IDA is per smmu while the ASID xarray is
global?

So, I'd suggest consistency. Either the domain ASID/VMID is global and
assigned at allocation time.

Or it is local to the instance, assigned at bind time, and stored in
the domain's attached device list. Some logic is needed to optimize
this - probably keep the instance list sorted by instance.

Having per-instance cache-tags avoids the limit problem a prior patch
was struggling with, at the cost of some more complexity in other
places.

Jason

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

Re: [RFC PATCH v1 1/8] iommu/arm-smmu-v3: Add list of installed_smmus

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 02:16:23AM +0800, Michael Shavit wrote:
> 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 83d2790b701e7..a9202d2045537 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -690,12 +690,20 @@ struct arm_smmu_stream {
>  	struct rb_node			node;
>  };
>  
> +/* List of smmu devices that a domain is installed to */
> +struct arm_smmu_installed_smmu {
> +	struct list_head	list;

'installed_smmus_item'

> +	/* List of masters that the domain is attached to*/
> +	struct list_head	devices;
> +	struct arm_smmu_device	*smmu;
> +};

Ah, if you have this struct you could just put

  u32 cache_tag; // ASID or VMID depending on domain type

And make both allocators local to the instance.

AFAIK the only specific thing that needs a global ASID is the SVA BTM
stuff and it already has code to handle collisions, it can just run
that code per-instance.

>  /* SMMU private data for each master */
>  struct arm_smmu_master {
>  	struct arm_smmu_device		*smmu;
>  	struct device			*dev;
>  	struct arm_smmu_domain		*domain;
> -	struct list_head		domain_head;
> +	struct list_head		list;

What is list for now?

Jason

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

Re: [RFC PATCH v1 4/8] iommu/arm-smmu-v3: check smmu compatibility on attach

[Robin Murphy]

On 2023-08-17 19:16, Michael Shavit wrote:
> Record the domain's pgtbl_cfg when it's being prepared so that it can
> later be compared to the features an smmu supports.

What's wrong with retrieving the existing config from the 
io_pgtable_ops, same as all the other io-pgtable code does?

Thanks,
Robin.

> Verify a domain's compatibility with the smmu when it's being attached
> to a master belong to a different smmu device.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 103 ++++++++++++++++----
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   2 +
>   2 files changed, 86 insertions(+), 19 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 447af74dbe280..c0943cf3c09ca 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2195,17 +2195,48 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_domain *smmu_domain,
>   	return 0;
>   }
>   
> +static int arm_smmu_prepare_pgtbl_cfg(struct arm_smmu_device *smmu,
> +				      enum arm_smmu_domain_stage stage,
> +				      struct io_pgtable_cfg *pgtbl_cfg)
> +{
> +	unsigned long ias, oas;
> +
> +	switch (stage) {
> +	case ARM_SMMU_DOMAIN_S1:
> +		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> +		ias = min_t(unsigned long, ias, VA_BITS);
> +		oas = smmu->ias;
> +		break;
> +	case ARM_SMMU_DOMAIN_NESTED:
> +	case ARM_SMMU_DOMAIN_S2:
> +		ias = smmu->ias;
> +		oas = smmu->oas;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	*pgtbl_cfg = (struct io_pgtable_cfg) {
> +		.pgsize_bitmap	= smmu->pgsize_bitmap,
> +		.ias		= ias,
> +		.oas		= oas,
> +		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
> +		.tlb		= &arm_smmu_flush_ops,
> +		.iommu_dev	= smmu->dev,
> +	};
> +	return 0;
> +}
> +
>   static int arm_smmu_domain_finalise(struct iommu_domain *domain)
>   {
>   	int ret;
> -	unsigned long ias, oas;
>   	enum io_pgtable_fmt fmt;
> -	struct io_pgtable_cfg pgtbl_cfg;
>   	struct io_pgtable_ops *pgtbl_ops;
>   	int (*finalise_stage_fn)(struct arm_smmu_domain *,
>   				 struct io_pgtable_cfg *);
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
> +	struct io_pgtable_cfg *pgtbl_cfg = &smmu_domain->pgtbl_cfg;
>   
>   	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
>   		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> @@ -2220,16 +2251,11 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
>   
>   	switch (smmu_domain->stage) {
>   	case ARM_SMMU_DOMAIN_S1:
> -		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> -		ias = min_t(unsigned long, ias, VA_BITS);
> -		oas = smmu->ias;
>   		fmt = ARM_64_LPAE_S1;
>   		finalise_stage_fn = arm_smmu_domain_finalise_s1;
>   		break;
>   	case ARM_SMMU_DOMAIN_NESTED:
>   	case ARM_SMMU_DOMAIN_S2:
> -		ias = smmu->ias;
> -		oas = smmu->oas;
>   		fmt = ARM_64_LPAE_S2;
>   		finalise_stage_fn = arm_smmu_domain_finalise_s2;
>   		break;
> @@ -2237,24 +2263,19 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain)
>   		return -EINVAL;
>   	}
>   
> -	pgtbl_cfg = (struct io_pgtable_cfg) {
> -		.pgsize_bitmap	= smmu->pgsize_bitmap,
> -		.ias		= ias,
> -		.oas		= oas,
> -		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
> -		.tlb		= &arm_smmu_flush_ops,
> -		.iommu_dev	= smmu->dev,
> -	};
> +	ret = arm_smmu_prepare_pgtbl_cfg(smmu, smmu_domain->stage, pgtbl_cfg);
> +	if (ret)
> +		return ret;
>   
> -	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
> +	pgtbl_ops = alloc_io_pgtable_ops(fmt, pgtbl_cfg, smmu_domain);
>   	if (!pgtbl_ops)
>   		return -ENOMEM;
>   
> -	domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> -	domain->geometry.aperture_end = (1UL << pgtbl_cfg.ias) - 1;
> +	domain->pgsize_bitmap = pgtbl_cfg->pgsize_bitmap;
> +	domain->geometry.aperture_end = (1UL << pgtbl_cfg->ias) - 1;
>   	domain->geometry.force_aperture = true;
>   
> -	ret = finalise_stage_fn(smmu_domain, &pgtbl_cfg);
> +	ret = finalise_stage_fn(smmu_domain, pgtbl_cfg);
>   	if (ret < 0) {
>   		free_io_pgtable_ops(pgtbl_ops);
>   		return ret;
> @@ -2406,6 +2427,46 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>   	pci_disable_pasid(pdev);
>   }
>   
> +static int
> +arm_smmu_verify_domain_compatible(struct arm_smmu_device *smmu,
> +				  struct arm_smmu_domain *smmu_domain)
> +{
> +	struct io_pgtable_cfg pgtbl_cfg;
> +	int ret;
> +
> +	if (smmu_domain->domain.type == IOMMU_DOMAIN_IDENTITY)
> +		return 0;
> +
> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2) {
> +		if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
> +			return -EINVAL;
> +		if (smmu_domain->s2_cfg.vmid >> smmu->vmid_bits)
> +			return -EINVAL;
> +	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> +		if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
> +			return -EINVAL;
> +		if (smmu_domain->cd.asid >> smmu->asid_bits)
> +			return -EINVAL;
> +	}
> +
> +	ret = arm_smmu_prepare_pgtbl_cfg(smmu, smmu_domain->stage, &pgtbl_cfg);
> +	if (ret)
> +		return ret;
> +
> +	if (smmu_domain->pgtbl_cfg.ias > pgtbl_cfg.ias ||
> +	    smmu_domain->pgtbl_cfg.oas > pgtbl_cfg.oas ||
> +	    /*
> +	     * The supported pgsize_bitmap must be a superset of the domain's
> +	     * pgsize_bitmap.
> +	     */
> +	    (smmu_domain->pgtbl_cfg.pgsize_bitmap ^ pgtbl_cfg.pgsize_bitmap) &
> +		    smmu_domain->pgtbl_cfg.pgsize_bitmap ||
> +	    smmu_domain->pgtbl_cfg.coherent_walk != pgtbl_cfg.coherent_walk)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>   static void arm_smmu_installed_smmus_remove_device(
>   	struct arm_smmu_domain *smmu_domain,
>   	struct arm_smmu_master *master)
> @@ -2449,6 +2510,10 @@ arm_smmu_installed_smmus_add_device(struct arm_smmu_domain *smmu_domain,
>   		}
>   	}
>   	if (!list_entry_found) {
> +		ret = arm_smmu_verify_domain_compatible(smmu, smmu_domain);
> +		if (ret)
> +			goto unlock;
> +
>   		installed_smmu = kzalloc(sizeof(*installed_smmu), GFP_KERNEL);
>   		if (!installed_smmu) {
>   			ret = -ENOMEM;
> 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 2ab23139c796e..143b287be2f8b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -9,6 +9,7 @@
>   #define _ARM_SMMU_V3_H
>   
>   #include <linux/bitfield.h>
> +#include <linux/io-pgtable.h>
>   #include <linux/iommu.h>
>   #include <linux/kernel.h>
>   #include <linux/mmzone.h>
> @@ -729,6 +730,7 @@ struct arm_smmu_domain {
>   	struct mutex			init_mutex; /* Protects smmu pointer */
>   
>   	struct io_pgtable_ops		*pgtbl_ops;
> +	struct io_pgtable_cfg		pgtbl_cfg;
>   	atomic_t			nr_ats_masters;
>   
>   	enum arm_smmu_domain_stage	stage;

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 02:16:24AM +0800, Michael Shavit wrote:
> Prepare and batch invalidation commands for each SMMU that a domain is
> installed onto.
> Move SVA's check against the smmu's ARM_SMMU_FEAT_BTM bit into
> arm_smmu_tlb_inv_range_asid so that it can be checked against each
> installed SMMU.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> It's not obvious to me whether skipping the tlb_inv_range_asid when
> ARM_SMMU_FEAT_BTM is somehow specific to SVA? Is moving the check into
> arm_smmu_tlb_inv_range_asid still valid if that function were called
> outside of SVA?

Logically it should be linked to SVA, and specifically to the mmu
notifier callback. The mmu notifier callback is done whenever the CPU
did an invalidation and BTM means the SMMU tracks exactly those
automatically. Thus we don't need to duplicated it. Indeed, we should
probably not even register a mmu notifier on BTM capable devices.

It is certainly wrong to skip invalidations generated for any other
reason.

From what I can tell SVA domains should have their CD table entry
programmed with "ASET=0" and normal paging domains should be
programmed with "ASET=1". This causes only the SVA domains to listen
to the BTM invalidations.

Jason

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

Re: [RFC PATCH v1 1/8] iommu/arm-smmu-v3: Add list of installed_smmus

[Robin Murphy]

On 2023-08-17 19:16, Michael Shavit wrote:
> Add a new arm_smmu_installed_smmu class to aggregate masters belonging
> to the same SMMU that a domain is attached to.
> Update usages of the domain->devices list to first iterate over this
> parent installed_smmus list.
> 
> This allows functions that batch commands to an SMMU to first iterate
> over the list of installed SMMUs before preparing the batched command
> from the set of attached masters.

I get the feeling that any purpose this serves could be achieved far 
more simply by just keeping the lists sorted by SMMU instance. Then we 
just iterate the list as normal, do per-device stuff for each device, 
and do per-instance stuff each time we see an instance that's different 
from the last one. That should represent about 3 or 4 extra lines of 
code in the places that actually do per-instance stuff, compared to 
blowing up simple per-device iterations like the comically unreadable 
first hunk below...

Thanks,
Robin.

> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
>   .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  28 ++++-
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 109 ++++++++++++++----
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  14 ++-
>   3 files changed, 118 insertions(+), 33 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 238ede8368d10..a4e235b4f1c4b 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
> @@ -48,21 +48,37 @@ 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_installed_smmu *installed_smmu;
>   	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->installed_smmus_lock, flags);
> +	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus, list) {
> +		list_for_each_entry(master, &installed_smmu->devices, list) {
> +			ret = arm_smmu_write_ctx_desc(master, ssid, cd);
> +			if (ret) {
> +				list_for_each_entry_from_reverse(
> +					master, &installed_smmu->devices, list)
> +					arm_smmu_write_ctx_desc(master, ssid,
> +								NULL);
> +				break;
> +			}
> +		}
>   		if (ret) {
> -			list_for_each_entry_from_reverse(master, &smmu_domain->devices, domain_head)
> -				arm_smmu_write_ctx_desc(master, ssid, NULL);
> +			list_for_each_entry_continue_reverse(
> +				installed_smmu, &smmu_domain->installed_smmus,
> +				list) {
> +				list_for_each_entry(
> +					master, &installed_smmu->devices, list)
> +					arm_smmu_write_ctx_desc(master, ssid,
> +								NULL);
> +			}
>   			break;
>   		}
>   	}
>   
> -	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> +	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
>   	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 f17704c35858d..cb4bf0c7c3dd6 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1811,10 +1811,12 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
>   			    unsigned long iova, size_t size)
>   {
>   	int i;
> +	int ret;
>   	unsigned long flags;
>   	struct arm_smmu_cmdq_ent cmd;
>   	struct arm_smmu_master *master;
>   	struct arm_smmu_cmdq_batch cmds;
> +	struct arm_smmu_installed_smmu *installed_smmu;
>   
>   	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_ATS))
>   		return 0;
> @@ -1838,21 +1840,26 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
>   
>   	arm_smmu_atc_inv_to_cmd(ssid, iova, size, &cmd);
>   
> -	cmds.num = 0;
> -
> -	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> -		if (!master->ats_enabled)
> -			continue;
> +	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
> +	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus, list) {
> +		cmds.num = 0;
> +		list_for_each_entry(master, &installed_smmu->devices, list) {
> +			if (!master->ats_enabled)
> +				continue;
>   
> -		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);
> +			for (i = 0; i < master->num_streams; i++) {
> +				cmd.atc.sid = master->streams[i].id;
> +				arm_smmu_cmdq_batch_add(installed_smmu->smmu,
> +							&cmds, &cmd);
> +			}
>   		}
> +		ret = arm_smmu_cmdq_batch_submit(installed_smmu->smmu, &cmds);
> +		if (ret)
> +			break;
>   	}
> -	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> +	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
>   
> -	return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
> +	return ret;
>   }
>   
>   /* IO_PGTABLE API */
> @@ -2049,8 +2056,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->installed_smmus);
> +	spin_lock_init(&smmu_domain->installed_smmus_lock);
>   	INIT_LIST_HEAD(&smmu_domain->mmu_notifiers);
>   
>   	return &smmu_domain->domain;
> @@ -2353,9 +2360,66 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>   	pci_disable_pasid(pdev);
>   }
>   
> -static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> +static void arm_smmu_installed_smmus_remove_device(
> +	struct arm_smmu_domain *smmu_domain,
> +	struct arm_smmu_master *master)
>   {
> +	struct arm_smmu_installed_smmu *installed_smmu;
> +	struct arm_smmu_device *smmu;
>   	unsigned long flags;
> +
> +	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
> +	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus,
> +			    list) {
> +		smmu = installed_smmu->smmu;
> +		if (smmu != master->smmu)
> +			continue;
> +		list_del(&master->list);
> +		if (list_empty(&installed_smmu->devices)) {
> +			list_del(&installed_smmu->list);
> +			kfree(installed_smmu);
> +		}
> +		break;
> +	}
> +	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
> +}
> +
> +static int
> +arm_smmu_installed_smmus_add_device(struct arm_smmu_domain *smmu_domain,
> +				    struct arm_smmu_master *master)
> +{
> +	struct arm_smmu_installed_smmu *installed_smmu;
> +	struct arm_smmu_device *smmu = master->smmu;
> +	bool list_entry_found = false;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&smmu_domain->installed_smmus_lock, flags);
> +	list_for_each_entry(installed_smmu, &smmu_domain->installed_smmus,
> +			    list) {
> +		if (installed_smmu->smmu == smmu) {
> +			list_entry_found = true;
> +			break;
> +		}
> +	}
> +	if (!list_entry_found) {
> +		installed_smmu = kzalloc(sizeof(*installed_smmu), GFP_KERNEL);
> +		if (!installed_smmu) {
> +			ret = -ENOMEM;
> +			goto unlock;
> +		}
> +		INIT_LIST_HEAD(&installed_smmu->devices);
> +		installed_smmu->smmu = smmu;
> +		list_add(&installed_smmu->list, &smmu_domain->installed_smmus);
> +	}
> +	list_add(&master->list, &installed_smmu->devices);
> +unlock:
> +	spin_unlock_irqrestore(&smmu_domain->installed_smmus_lock, flags);
> +	return ret;
> +}
> +
> +static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> +{
>   	struct arm_smmu_domain *smmu_domain = master->domain;
>   
>   	if (!smmu_domain)
> @@ -2363,9 +2427,7 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>   
>   	arm_smmu_disable_ats(master);
>   
> -	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -	list_del(&master->domain_head);
> -	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> +	arm_smmu_installed_smmus_remove_device(smmu_domain, master);
>   
>   	master->domain = NULL;
>   	master->ats_enabled = false;
> @@ -2385,7 +2447,6 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>   static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   {
>   	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);
> @@ -2435,9 +2496,11 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   	if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
>   		master->ats_enabled = arm_smmu_ats_supported(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);
> +	ret = arm_smmu_installed_smmus_add_device(smmu_domain, master);
> +	if (ret) {
> +		master->domain = NULL;
> +		return ret;
> +	}
>   
>   	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>   		if (!master->cd_table.cdtab) {
> @@ -2467,9 +2530,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   	return 0;
>   
>   out_list_del:
> -	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -	list_del(&master->domain_head);
> -	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> +	arm_smmu_installed_smmus_remove_device(smmu_domain, master);
>   
>   	return 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 83d2790b701e7..a9202d2045537 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -690,12 +690,20 @@ struct arm_smmu_stream {
>   	struct rb_node			node;
>   };
>   
> +/* List of smmu devices that a domain is installed to */
> +struct arm_smmu_installed_smmu {
> +	struct list_head	list;
> +	/* List of masters that the domain is attached to*/
> +	struct list_head	devices;
> +	struct arm_smmu_device	*smmu;
> +};
> +
>   /* SMMU private data for each master */
>   struct arm_smmu_master {
>   	struct arm_smmu_device		*smmu;
>   	struct device			*dev;
>   	struct arm_smmu_domain		*domain;
> -	struct list_head		domain_head;
> +	struct list_head		list;
>   	struct arm_smmu_stream		*streams;
>   	/* Locked by the iommu core using the group mutex */
>   	struct arm_smmu_ctx_desc_cfg	cd_table;
> @@ -731,8 +739,8 @@ struct arm_smmu_domain {
>   
>   	struct iommu_domain		domain;
>   
> -	struct list_head		devices;
> -	spinlock_t			devices_lock;
> +	struct list_head		installed_smmus;
> +	spinlock_t			installed_smmus_lock;
>   
>   	struct list_head		mmu_notifiers;
>   };

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Robin Murphy]

On 2023-08-17 20:20, Jason Gunthorpe wrote:
> On Fri, Aug 18, 2023 at 02:16:24AM +0800, Michael Shavit wrote:
>> Prepare and batch invalidation commands for each SMMU that a domain is
>> installed onto.
>> Move SVA's check against the smmu's ARM_SMMU_FEAT_BTM bit into
>> arm_smmu_tlb_inv_range_asid so that it can be checked against each
>> installed SMMU.
>>
>> Signed-off-by: Michael Shavit <mshavit@google.com>
>> ---
>> It's not obvious to me whether skipping the tlb_inv_range_asid when
>> ARM_SMMU_FEAT_BTM is somehow specific to SVA? Is moving the check into
>> arm_smmu_tlb_inv_range_asid still valid if that function were called
>> outside of SVA?
> 
> Logically it should be linked to SVA, and specifically to the mmu
> notifier callback. The mmu notifier callback is done whenever the CPU
> did an invalidation and BTM means the SMMU tracks exactly those
> automatically. Thus we don't need to duplicated it. Indeed, we should
> probably not even register a mmu notifier on BTM capable devices.

Almost - broadcast invalidates from the CPU only apply to SMMU TLBs; we 
still need the notifier for the sake of issuing ATC invalidate commands 
to endpoints.

> It is certainly wrong to skip invalidations generated for any other
> reason.
> 
>  From what I can tell SVA domains should have their CD table entry
> programmed with "ASET=0" and normal paging domains should be
> programmed with "ASET=1". This causes only the SVA domains to listen
> to the BTM invalidations.

Correct.

Thanks,
Robin.

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

Re: [RFC PATCH v1 4/8] iommu/arm-smmu-v3: check smmu compatibility on attach

[Michael Shavit]

On Fri, Aug 18, 2023 at 3:17 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2023-08-17 19:16, Michael Shavit wrote:
> > Record the domain's pgtbl_cfg when it's being prepared so that it can
> > later be compared to the features an smmu supports.
>
> What's wrong with retrieving the existing config from the
> io_pgtable_ops, same as all the other io-pgtable code does?
>

I didn't think accessing the io_pgtable struct outside of io-pgtable
code would be ok, but now that you mention it I do see usages of
io_pgtable_ops_to_pgtable in iommu drivers.

Will change.

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Michael Shavit]

On Fri, Aug 18, 2023 at 3:41 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2023-08-17 20:20, Jason Gunthorpe wrote:
> > It is certainly wrong to skip invalidations generated for any other
> > reason.
> >
> >  From what I can tell SVA domains should have their CD table entry
> > programmed with "ASET=0" and normal paging domains should be
> > programmed with "ASET=1". This causes only the SVA domains to listen
> > to the BTM invalidations.
>
> Correct.
>
> Thanks,
> Robin.

Would it be fair to rename arm_smmu_tlb_inv_asid (or move into
arm-smmu-v3-sva) to make it explicit that it shouldn't be used outside
of SVA then? Or add a parameter such as skip_btm_capable_devices.

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

Re: [RFC PATCH v1 1/8] iommu/arm-smmu-v3: Add list of installed_smmus

[Michael Shavit]

On Fri, Aug 18, 2023 at 3:34 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2023-08-17 19:16, Michael Shavit wrote:
> > Add a new arm_smmu_installed_smmu class to aggregate masters belonging
> > to the same SMMU that a domain is attached to.
> > Update usages of the domain->devices list to first iterate over this
> > parent installed_smmus list.
> >
> > This allows functions that batch commands to an SMMU to first iterate
> > over the list of installed SMMUs before preparing the batched command
> > from the set of attached masters.
>
> I get the feeling that any purpose this serves could be achieved far
> more simply by just keeping the lists sorted by SMMU instance.

I initially tried just that, but then you need more sophisticated code
when iterating over the list to correctly batch operations by SMMU,
which is the more common operation.
That first hunk is very unfortunate though... Let me revive that
version and send it out so we can better compare.

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Jason Gunthorpe]

On Fri, Aug 18, 2023 at 11:44:55AM +0800, Michael Shavit wrote:
> On Fri, Aug 18, 2023 at 3:41 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > On 2023-08-17 20:20, Jason Gunthorpe wrote:
> > > It is certainly wrong to skip invalidations generated for any other
> > > reason.
> > >
> > >  From what I can tell SVA domains should have their CD table entry
> > > programmed with "ASET=0" and normal paging domains should be
> > > programmed with "ASET=1". This causes only the SVA domains to listen
> > > to the BTM invalidations.
> >
> > Correct.
> >
> > Thanks,
> > Robin.
> 
> Would it be fair to rename arm_smmu_tlb_inv_asid (or move into
> arm-smmu-v3-sva) to make it explicit that it shouldn't be used outside
> of SVA then? Or add a parameter such as skip_btm_capable_devices.

???

arm_smmu_tlb_inv_asid() is generally used in many places and has
nothing to do with BTM..

Did you mean arm_smmu_tlb_inv_range_asid ?

Broadly, invalidation is not SVA specific..

Notice that arm_smmu_tlb_inv_range_asid() already duplicates
arm_smmu_tlb_inv_range_domain().

IMHO I would split the ATC step out of arm_smmu_mm_invalidate_range(),
get rid of arm_smmu_tlb_inv_range_domain(), and have the mmu notifier
just do as it already does:

	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_BTM))
		arm_smmu_tlb_inv_range_domain_no_atc(start, size, smmu_mn->cd->asid,
					    PAGE_SIZE, false, smmu_domain);
	arm_smmu_atc_inv_domain(smmu_domain, start, size);

And make arm_smmu_tlb_inv_range_domain() just call
   arm_smmu_tlb_inv_range_domain_no_atc();
   arm_smmu_atc_inv_domain();

Jason

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Michael Shavit]

On Fri, Aug 18, 2023 at 9:51 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Fri, Aug 18, 2023 at 11:44:55AM +0800, Michael Shavit wrote:
> > On Fri, Aug 18, 2023 at 3:41 AM Robin Murphy <robin.murphy@arm.com> wrote:
> > >
> > > On 2023-08-17 20:20, Jason Gunthorpe wrote:
> > > > It is certainly wrong to skip invalidations generated for any other
> > > > reason.
> > > >
> > > >  From what I can tell SVA domains should have their CD table entry
> > > > programmed with "ASET=0" and normal paging domains should be
> > > > programmed with "ASET=1". This causes only the SVA domains to listen
> > > > to the BTM invalidations.
> > >
> > > Correct.
> > >
> > > Thanks,
> > > Robin.
> >
> > Would it be fair to rename arm_smmu_tlb_inv_asid (or move into
> > arm-smmu-v3-sva) to make it explicit that it shouldn't be used outside
> > of SVA then? Or add a parameter such as skip_btm_capable_devices.
>
> ???
>
> arm_smmu_tlb_inv_asid() is generally used in many places and has
> nothing to do with BTM..
>
> Did you mean arm_smmu_tlb_inv_range_asid ?

Whoops yes that's what I meant.


>
> Broadly, invalidation is not SVA specific..
>
> Notice that arm_smmu_tlb_inv_range_asid() already duplicates
> arm_smmu_tlb_inv_range_domain().
>
> IMHO I would split the ATC step out of arm_smmu_mm_invalidate_range(),
> get rid of arm_smmu_tlb_inv_range_domain(), and have the mmu notifier
> just do as it already does:
>
>         if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_BTM))
>                 arm_smmu_tlb_inv_range_domain_no_atc(start, size, smmu_mn->cd->asid,
>                                             PAGE_SIZE, false, smmu_domain);
>         arm_smmu_atc_inv_domain(smmu_domain, start, size);
>
> And make arm_smmu_tlb_inv_range_domain() just call
>    arm_smmu_tlb_inv_range_domain_no_atc();
>    arm_smmu_atc_inv_domain();

That's a nice clean-up but doesn't really solve the problem faced by this patch.

This patch series eliminates the smmu_domain->smmu handle, replacing
it for a list of SMMUs. So SVA can no longer optimize the
arm_smmu_tlb_inv_range_asid call away by checking whether the SMMU BTM
feature is enabled since there's now a list of SMMUs with possibly
heterogeneous support for the feature. Since there's now a loop over a
series of SMMUs inside arm_smmu_tlb_inv_range_asid, it makes sense to
move the check into that loop. This technically works because only SVA
is calling arm_smmu_tlb_inv_range_asid but can (IMO) risk introducing
bugs in the future since it's not obvious from the function name.

The suggestion was then to introduce a parameter to
arm_smmu_tlb_inv_range_asid (or arm_smmu_tlb_inv_range_domain_no_atc)
to make this behavior explicit in the API.

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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Michael Shavit]

On Fri, Aug 18, 2023 at 2:38 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Fri, Aug 18, 2023 at 02:16:25AM +0800, Michael Shavit wrote:
> > Pick an ASID that is within the supported range of all SMMUs that the
> > domain is installed to.
> >
> > Signed-off-by: Michael Shavit <mshavit@google.com>
> > ---
>
> This seems like a pretty niche scenario, maybe we should just keep a
> global for the max ASID?
>
> Otherwise we need a code to change the ASID, even for non-SVA domains,
> when the domain is installed in different devices if the current ASID
> is over the instance max..

This RFC took the other easy way out for this problem by rejecting
attaching a domain if its currently assigned ASID/VMID
is out of range when attaching to a new SMMU. But I'm not sure
which of the two options is the right trade-off.
Especially if we move VMID to a global allocator (which I plan to add
for v2), setting a global maximum for VMID of 256 sounds small.

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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Jason Gunthorpe]

On Mon, Aug 21, 2023 at 05:31:23PM +0800, Michael Shavit wrote:
> On Fri, Aug 18, 2023 at 2:38 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Fri, Aug 18, 2023 at 02:16:25AM +0800, Michael Shavit wrote:
> > > Pick an ASID that is within the supported range of all SMMUs that the
> > > domain is installed to.
> > >
> > > Signed-off-by: Michael Shavit <mshavit@google.com>
> > > ---
> >
> > This seems like a pretty niche scenario, maybe we should just keep a
> > global for the max ASID?
> >
> > Otherwise we need a code to change the ASID, even for non-SVA domains,
> > when the domain is installed in different devices if the current ASID
> > is over the instance max..
> 
> This RFC took the other easy way out for this problem by rejecting
> attaching a domain if its currently assigned ASID/VMID
> is out of range when attaching to a new SMMU. But I'm not sure
> which of the two options is the right trade-off.
> Especially if we move VMID to a global allocator (which I plan to add
> for v2), setting a global maximum for VMID of 256 sounds small.

IMHO the simplest and best thing is to make both vmid and asid as
local allocators. Then alot of these problems disappear

Jason

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Jason Gunthorpe]

On Mon, Aug 21, 2023 at 04:33:36PM +0800, Michael Shavit wrote:
> > Notice that arm_smmu_tlb_inv_range_asid() already duplicates
> > arm_smmu_tlb_inv_range_domain().
> >
> > IMHO I would split the ATC step out of arm_smmu_mm_invalidate_range(),
> > get rid of arm_smmu_tlb_inv_range_domain(), and have the mmu notifier
> > just do as it already does:
> >
> >         if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_BTM))
> >                 arm_smmu_tlb_inv_range_domain_no_atc(start, size, smmu_mn->cd->asid,
> >                                             PAGE_SIZE, false, smmu_domain);
> >         arm_smmu_atc_inv_domain(smmu_domain, start, size);
> >
> > And make arm_smmu_tlb_inv_range_domain() just call
> >    arm_smmu_tlb_inv_range_domain_no_atc();
> >    arm_smmu_atc_inv_domain();
> 
> That's a nice clean-up but doesn't really solve the problem faced by this patch.
>
> This patch series eliminates the smmu_domain->smmu handle, replacing
> it for a list of SMMUs. So SVA can no longer optimize the
> arm_smmu_tlb_inv_range_asid call away by checking whether the SMMU BTM
> feature is enabled since there's now a list of SMMUs with possibly
> heterogeneous support for the feature. 

You could also go in the direction of making a SVA BTM and SV non-BTM
domain type and then you know what to do immediately in the notifier.

> Since there's now a loop over a series of SMMUs inside
> arm_smmu_tlb_inv_range_asid, it makes sense to move the check into
> that loop. This technically works because only SVA is calling
> arm_smmu_tlb_inv_range_asid but can (IMO) risk introducing bugs in
> the future since it's not obvious from the function name.

Well, I would remove the duplication and add an argument if you intend
to share the function that loops

Jason

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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Michael Shavit]

On Mon, Aug 21, 2023 at 7:54 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, Aug 21, 2023 at 05:31:23PM +0800, Michael Shavit wrote:
> > On Fri, Aug 18, 2023 at 2:38 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > On Fri, Aug 18, 2023 at 02:16:25AM +0800, Michael Shavit wrote:
> > > > Pick an ASID that is within the supported range of all SMMUs that the
> > > > domain is installed to.
> > > >
> > > > Signed-off-by: Michael Shavit <mshavit@google.com>
> > > > ---
> > >
> > > This seems like a pretty niche scenario, maybe we should just keep a
> > > global for the max ASID?
> > >
> > > Otherwise we need a code to change the ASID, even for non-SVA domains,
> > > when the domain is installed in different devices if the current ASID
> > > is over the instance max..
> >
> > This RFC took the other easy way out for this problem by rejecting
> > attaching a domain if its currently assigned ASID/VMID
> > is out of range when attaching to a new SMMU. But I'm not sure
> > which of the two options is the right trade-off.
> > Especially if we move VMID to a global allocator (which I plan to add
> > for v2), setting a global maximum for VMID of 256 sounds small.
>
> IMHO the simplest and best thing is to make both vmid and asid as
> local allocators. Then alot of these problems disappear

Well that does sound like the most flexible, but IMO quite a lot more
complicated.

I'll post a v2 RFC that removes the `iommu/arm-smmu-v3: Add list of
installed_smmus` patch and uses a flat master list in smmu_domain as
suggested by Robin, for comparison with the v1. But at a glance using a
local allocator would require:
1. Keeping that patch so we can track the asid/vmid for a domain on a
per smmu instance
2. Keeping a map in the smmu struct so that arm_smmu_share_asid can
find any arm_smmu_installed_smmu that need to have their asid updated
(on a loop over every smmu the domain in arm_smmu_mmu_notifier_get is
attached to, which just at a glance looks headache inducing because of
sva's piggybacking on the rid domain.)

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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Jason Gunthorpe]

On Mon, Aug 21, 2023 at 09:38:40PM +0800, Michael Shavit wrote:
> On Mon, Aug 21, 2023 at 7:54 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Mon, Aug 21, 2023 at 05:31:23PM +0800, Michael Shavit wrote:
> > > On Fri, Aug 18, 2023 at 2:38 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > >
> > > > On Fri, Aug 18, 2023 at 02:16:25AM +0800, Michael Shavit wrote:
> > > > > Pick an ASID that is within the supported range of all SMMUs that the
> > > > > domain is installed to.
> > > > >
> > > > > Signed-off-by: Michael Shavit <mshavit@google.com>
> > > > > ---
> > > >
> > > > This seems like a pretty niche scenario, maybe we should just keep a
> > > > global for the max ASID?
> > > >
> > > > Otherwise we need a code to change the ASID, even for non-SVA domains,
> > > > when the domain is installed in different devices if the current ASID
> > > > is over the instance max..
> > >
> > > This RFC took the other easy way out for this problem by rejecting
> > > attaching a domain if its currently assigned ASID/VMID
> > > is out of range when attaching to a new SMMU. But I'm not sure
> > > which of the two options is the right trade-off.
> > > Especially if we move VMID to a global allocator (which I plan to add
> > > for v2), setting a global maximum for VMID of 256 sounds small.
> >
> > IMHO the simplest and best thing is to make both vmid and asid as
> > local allocators. Then alot of these problems disappear
> 
> Well that does sound like the most flexible, but IMO quite a lot more
> complicated.
> 
> I'll post a v2 RFC that removes the `iommu/arm-smmu-v3: Add list of
> installed_smmus` patch and uses a flat master list in smmu_domain as
> suggested by Robin, for comparison with the v1. But at a glance using a
> local allocator would require:

> 1. Keeping that patch so we can track the asid/vmid for a domain on a
> per smmu instance

You'd have to store the cache tag in the per-master struct on that
list and take it out of the domain struct.

Ie the list of attached masters contains the per-master cache tag
instead of a global cache tag.

The only place you need the cache tag is when iterating over the list
of masters, so it is OK.

If the list of masters is sorted by smmu then the first master of each
smmu can be used to perform the cache tag invalidation, then the rest
of the list is the ATC invalidation.

The looping code will be a bit ugly.

> 2. Keeping a map in the smmu struct so that arm_smmu_share_asid can
> find any arm_smmu_installed_smmu that need to have their asid
> updated

Yes, the global xarray moves into the smmu

> (on a loop over every smmu the domain in arm_smmu_mmu_notifier_get is
> attached to, which just at a glance looks headache inducing because of
> sva's piggybacking on the rid domain.)

Not every smmu, just the one you are *currently* attaching to. We
don't care if the *other* smmu's have different ASIDs, maybe they are
not using BTM, or won't use SVA.

We care that *our* smmu has the right ASID when we go to attach the
domain.

So the logic is not really any different, you already know the smmu
because it is attaching, and you do the same locking xarray stuff as
was done globally, just against this local smmu.

Jason

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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Michael Shavit]

On Mon, Aug 21, 2023 at 9:50 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, Aug 21, 2023 at 09:38:40PM +0800, Michael Shavit wrote:
> > On Mon, Aug 21, 2023 at 7:54 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > On Mon, Aug 21, 2023 at 05:31:23PM +0800, Michael Shavit wrote:
> > > > On Fri, Aug 18, 2023 at 2:38 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > > >
> > > > > On Fri, Aug 18, 2023 at 02:16:25AM +0800, Michael Shavit wrote:
> > > > > > Pick an ASID that is within the supported range of all SMMUs that the
> > > > > > domain is installed to.
> > > > > >
> > > > > > Signed-off-by: Michael Shavit <mshavit@google.com>
> > > > > > ---
> > > > >
> > > > > This seems like a pretty niche scenario, maybe we should just keep a
> > > > > global for the max ASID?
> > > > >
> > > > > Otherwise we need a code to change the ASID, even for non-SVA domains,
> > > > > when the domain is installed in different devices if the current ASID
> > > > > is over the instance max..
> > > >
> > > > This RFC took the other easy way out for this problem by rejecting
> > > > attaching a domain if its currently assigned ASID/VMID
> > > > is out of range when attaching to a new SMMU. But I'm not sure
> > > > which of the two options is the right trade-off.
> > > > Especially if we move VMID to a global allocator (which I plan to add
> > > > for v2), setting a global maximum for VMID of 256 sounds small.
> > >
> > > IMHO the simplest and best thing is to make both vmid and asid as
> > > local allocators. Then alot of these problems disappear
> >
> > Well that does sound like the most flexible, but IMO quite a lot more
> > complicated.
> >
> > I'll post a v2 RFC that removes the `iommu/arm-smmu-v3: Add list of
> > installed_smmus` patch and uses a flat master list in smmu_domain as
> > suggested by Robin, for comparison with the v1. But at a glance using a
> > local allocator would require:
>
> > 1. Keeping that patch so we can track the asid/vmid for a domain on a
> > per smmu instance
>
> You'd have to store the cache tag in the per-master struct on that
> list and take it out of the domain struct.
>
> Ie the list of attached masters contains the per-master cache tag
> instead of a global cache tag.
>
> The only place you need the cache tag is when iterating over the list
> of masters, so it is OK.
>
> If the list of masters is sorted by smmu then the first master of each
> smmu can be used to perform the cache tag invalidation, then the rest
> of the list is the ATC invalidation.
>
> The looping code will be a bit ugly.

I suppose that could work.... but I'm worried it's gonna be messy,
especially if we think about how the PASID feature would interact.
With PASID, there could be multiple domains attached to a master. So
we won't be able to store a single cache tag/asid for the currently
attached domain on the arm_smmu_master. Still doable however; as it
could move into the struct mapping a domain to each PASID/master pair,
with your loop still using the first entry in the list (until it meets
an entry belonging to a different SMMU) for the invalidation.

> > 2. Keeping a map in the smmu struct so that arm_smmu_share_asid can
> > find any arm_smmu_installed_smmu that need to have their asid
> > updated
>
> Yes, the global xarray moves into the smmu
>
> > (on a loop over every smmu the domain in arm_smmu_mmu_notifier_get is
> > attached to, which just at a glance looks headache inducing because of
> > sva's piggybacking on the rid domain.)
>
> Not every smmu, just the one you are *currently* attaching to. We
> don't care if the *other* smmu's have different ASIDs, maybe they are
> not using BTM, or won't use SVA.

I mean because the domain in arm_smmu_mmu_notifier_get is the RID
domain (not the SVA domain, same issue we discussed in previous
thread) , which can be attached to multiple SMMUs.

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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Jason Gunthorpe]

On Mon, Aug 21, 2023 at 10:16:54PM +0800, Michael Shavit wrote:
> On Mon, Aug 21, 2023 at 9:50 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Mon, Aug 21, 2023 at 09:38:40PM +0800, Michael Shavit wrote:
> > > On Mon, Aug 21, 2023 at 7:54 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > >
> > > > On Mon, Aug 21, 2023 at 05:31:23PM +0800, Michael Shavit wrote:
> > > > > On Fri, Aug 18, 2023 at 2:38 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 18, 2023 at 02:16:25AM +0800, Michael Shavit wrote:
> > > > > > > Pick an ASID that is within the supported range of all SMMUs that the
> > > > > > > domain is installed to.
> > > > > > >
> > > > > > > Signed-off-by: Michael Shavit <mshavit@google.com>
> > > > > > > ---
> > > > > >
> > > > > > This seems like a pretty niche scenario, maybe we should just keep a
> > > > > > global for the max ASID?
> > > > > >
> > > > > > Otherwise we need a code to change the ASID, even for non-SVA domains,
> > > > > > when the domain is installed in different devices if the current ASID
> > > > > > is over the instance max..
> > > > >
> > > > > This RFC took the other easy way out for this problem by rejecting
> > > > > attaching a domain if its currently assigned ASID/VMID
> > > > > is out of range when attaching to a new SMMU. But I'm not sure
> > > > > which of the two options is the right trade-off.
> > > > > Especially if we move VMID to a global allocator (which I plan to add
> > > > > for v2), setting a global maximum for VMID of 256 sounds small.
> > > >
> > > > IMHO the simplest and best thing is to make both vmid and asid as
> > > > local allocators. Then alot of these problems disappear
> > >
> > > Well that does sound like the most flexible, but IMO quite a lot more
> > > complicated.
> > >
> > > I'll post a v2 RFC that removes the `iommu/arm-smmu-v3: Add list of
> > > installed_smmus` patch and uses a flat master list in smmu_domain as
> > > suggested by Robin, for comparison with the v1. But at a glance using a
> > > local allocator would require:
> >
> > > 1. Keeping that patch so we can track the asid/vmid for a domain on a
> > > per smmu instance
> >
> > You'd have to store the cache tag in the per-master struct on that
> > list and take it out of the domain struct.
> >
> > Ie the list of attached masters contains the per-master cache tag
> > instead of a global cache tag.
> >
> > The only place you need the cache tag is when iterating over the list
> > of masters, so it is OK.
> >
> > If the list of masters is sorted by smmu then the first master of each
> > smmu can be used to perform the cache tag invalidation, then the rest
> > of the list is the ATC invalidation.
> >
> > The looping code will be a bit ugly.
> 
> I suppose that could work.... but I'm worried it's gonna be messy,
> especially if we think about how the PASID feature would interact.
> With PASID, there could be multiple domains attached to a master. So
> we won't be able to store a single cache tag/asid for the currently
> attached domain on the arm_smmu_master. 

I wasn't suggesting to store it in the arm_smmu_master, I was
suggesting to store it in the same place you store the per-master
PASID.

eg I expect that on attach the domain will allocate new memory to
store the pasid/cache tag/master/domain and thread that memory on a
list of attached masters.

> > > (on a loop over every smmu the domain in arm_smmu_mmu_notifier_get is
> > > attached to, which just at a glance looks headache inducing because of
> > > sva's piggybacking on the rid domain.)
> >
> > Not every smmu, just the one you are *currently* attaching to. We
> > don't care if the *other* smmu's have different ASIDs, maybe they are
> > not using BTM, or won't use SVA.
> 
> I mean because the domain in arm_smmu_mmu_notifier_get is the RID
> domain (not the SVA domain, same issue we discussed in previous
> thread) , which can be attached to multiple SMMUs.

Oh that is totally nonsensical. I expect you will need to fix that
sooner than later. Once the CD table is moved and there is a proper
way to track the PASID it should not be needed. It shouldn't fall into
the decision making about where to put the ASID xarray.

Jason


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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Michael Shavit]

On Mon, Aug 21, 2023 at 10:26 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, Aug 21, 2023 at 10:16:54PM +0800, Michael Shavit wrote:
> > On Mon, Aug 21, 2023 at 9:50 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > On Mon, Aug 21, 2023 at 09:38:40PM +0800, Michael Shavit wrote:
> > > > On Mon, Aug 21, 2023 at 7:54 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > > >
> > > > > On Mon, Aug 21, 2023 at 05:31:23PM +0800, Michael Shavit wrote:
> > > > > > On Fri, Aug 18, 2023 at 2:38 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > > > > >
> > > > > > > On Fri, Aug 18, 2023 at 02:16:25AM +0800, Michael Shavit wrote:
> > > > > > > > Pick an ASID that is within the supported range of all SMMUs that the
> > > > > > > > domain is installed to.
> > > > > > > >
> > > > > > > > Signed-off-by: Michael Shavit <mshavit@google.com>
> > > > > > > > ---
> > > > > > >
> > > > > > > This seems like a pretty niche scenario, maybe we should just keep a
> > > > > > > global for the max ASID?
> > > > > > >
> > > > > > > Otherwise we need a code to change the ASID, even for non-SVA domains,
> > > > > > > when the domain is installed in different devices if the current ASID
> > > > > > > is over the instance max..
> > > > > >
> > > > > > This RFC took the other easy way out for this problem by rejecting
> > > > > > attaching a domain if its currently assigned ASID/VMID
> > > > > > is out of range when attaching to a new SMMU. But I'm not sure
> > > > > > which of the two options is the right trade-off.
> > > > > > Especially if we move VMID to a global allocator (which I plan to add
> > > > > > for v2), setting a global maximum for VMID of 256 sounds small.
> > > > >
> > > > > IMHO the simplest and best thing is to make both vmid and asid as
> > > > > local allocators. Then alot of these problems disappear
> > > >
> > > > Well that does sound like the most flexible, but IMO quite a lot more
> > > > complicated.
> > > >
> > > > I'll post a v2 RFC that removes the `iommu/arm-smmu-v3: Add list of
> > > > installed_smmus` patch and uses a flat master list in smmu_domain as
> > > > suggested by Robin, for comparison with the v1. But at a glance using a
> > > > local allocator would require:
> > >
> > > > 1. Keeping that patch so we can track the asid/vmid for a domain on a
> > > > per smmu instance
> > >
> > > You'd have to store the cache tag in the per-master struct on that
> > > list and take it out of the domain struct.
> > >
> > > Ie the list of attached masters contains the per-master cache tag
> > > instead of a global cache tag.
> > >
> > > The only place you need the cache tag is when iterating over the list
> > > of masters, so it is OK.
> > >
> > > If the list of masters is sorted by smmu then the first master of each
> > > smmu can be used to perform the cache tag invalidation, then the rest
> > > of the list is the ATC invalidation.
> > >
> > > The looping code will be a bit ugly.
> >
> > I suppose that could work.... but I'm worried it's gonna be messy,
> > especially if we think about how the PASID feature would interact.
> > With PASID, there could be multiple domains attached to a master. So
> > we won't be able to store a single cache tag/asid for the currently
> > attached domain on the arm_smmu_master.
>
> I wasn't suggesting to store it in the arm_smmu_master, I was
> suggesting to store it in the same place you store the per-master
> PASID.
>
> eg I expect that on attach the domain will allocate new memory to
> store the pasid/cache tag/master/domain and thread that memory on a
> list of attached masters.

Gotcha.

> > > > (on a loop over every smmu the domain in arm_smmu_mmu_notifier_get is
> > > > attached to, which just at a glance looks headache inducing because of
> > > > sva's piggybacking on the rid domain.)
> > >
> > > Not every smmu, just the one you are *currently* attaching to. We
> > > don't care if the *other* smmu's have different ASIDs, maybe they are
> > > not using BTM, or won't use SVA.
> >
> > I mean because the domain in arm_smmu_mmu_notifier_get is the RID
> > domain (not the SVA domain, same issue we discussed in previous
> > thread) , which can be attached to multiple SMMUs.
>
> Oh that is totally nonsensical. I expect you will need to fix that
> sooner than later. Once the CD table is moved and there is a proper
> way to track the PASID it should not be needed. It shouldn't fall into
> the decision making about where to put the ASID xarray.

Right I got a bit of a chicken and egg problem with all these series.

Can we keep the simpler solutions where ASID/VMID across SMMUs has
non-optimal constraints and re-consider this after all the other
changes land (this series, set_dev_pasid series, fixing sva)?

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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Jason Gunthorpe]

On Mon, Aug 21, 2023 at 10:39:14PM +0800, Michael Shavit wrote:
> > > > > (on a loop over every smmu the domain in arm_smmu_mmu_notifier_get is
> > > > > attached to, which just at a glance looks headache inducing because of
> > > > > sva's piggybacking on the rid domain.)
> > > >
> > > > Not every smmu, just the one you are *currently* attaching to. We
> > > > don't care if the *other* smmu's have different ASIDs, maybe they are
> > > > not using BTM, or won't use SVA.
> > >
> > > I mean because the domain in arm_smmu_mmu_notifier_get is the RID
> > > domain (not the SVA domain, same issue we discussed in previous
> > > thread) , which can be attached to multiple SMMUs.
> >
> > Oh that is totally nonsensical. I expect you will need to fix that
> > sooner than later. Once the CD table is moved and there is a proper
> > way to track the PASID it should not be needed. It shouldn't fall into
> > the decision making about where to put the ASID xarray.
> 
> Right I got a bit of a chicken and egg problem with all these series.

Yes, I'm not surprised to hear this

Still, it would nice to move forward without going in a weird
direction too much.

Once the CD table is moved to the master what do you think is blocking
fixing up the SVA stuff to not rely on a RID domain?

Jason

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Michael Shavit]

On Mon, Aug 21, 2023 at 7:58 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> > Since there's now a loop over a series of SMMUs inside
> > arm_smmu_tlb_inv_range_asid, it makes sense to move the check into
> > that loop. This technically works because only SVA is calling
> > arm_smmu_tlb_inv_range_asid but can (IMO) risk introducing bugs in
> > the future since it's not obvious from the function name.
>
> Well, I would remove the duplication and add an argument if you intend
> to share the function that loops

What do you think about this as a final stage:
Once the set_dev_pasid and sva refactor lands, SVA could call a common
arm_smmu_inv_range_domain implementation which would:
1. Skip the TLB invalidation on a per-smmu basis if it detects that
the domain type is SVA, or based on a passed-in parameter that is only
set True by SVA.
2. Issue ATC invalidations with SSIDs found in the arm_smmu_domain.
This common function would be used for all use-cases: invalidations of
domains attached on RIDs, on PASIDs (SVA and non SVA).

Then we have two options for the intermediate stage with this series:
1. Non-SVA code uses arm_smmu_inv_range_domain which calls
arm_smmu_tlb_inv_range_domain(_no_atc) and arm_smmu_atc_range_domain,
SVA code individually calls those two functions.
arm_smmu_tlb_inv_range_domain(_no_atc) accepts a parameter to skip the
invalidation if BTM feature is set.
2. Same as option 1, but SVA also calls arm_smmu_inv_range_domain.
arm_smmu_inv_range_domain accepts both a parameter to skip TLB inv
when BTM is set, as well as an SSID for the atc invalidation. SSID
would be 0 in non-sva callers, and mm->pasid for SVA.

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Michael Shavit]

On Tue, Aug 22, 2023 at 4:17 PM Michael Shavit <mshavit@google.com> wrote:
>
> On Mon, Aug 21, 2023 at 7:58 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > > Since there's now a loop over a series of SMMUs inside
> > > arm_smmu_tlb_inv_range_asid, it makes sense to move the check into
> > > that loop. This technically works because only SVA is calling
> > > arm_smmu_tlb_inv_range_asid but can (IMO) risk introducing bugs in
> > > the future since it's not obvious from the function name.
> >
> > Well, I would remove the duplication and add an argument if you intend
> > to share the function that loops
>
> What do you think about this as a final stage:
> Once the set_dev_pasid and sva refactor lands, SVA could call a common
> arm_smmu_inv_range_domain implementation which would:
> 1. Skip the TLB invalidation on a per-smmu basis if it detects that
> the domain type is SVA, or based on a passed-in parameter that is only
> set True by SVA.
> 2. Issue ATC invalidations with SSIDs found in the arm_smmu_domain.
> This common function would be used for all use-cases: invalidations of
> domains attached on RIDs, on PASIDs (SVA and non SVA).
>
> Then we have two options for the intermediate stage with this series:
> 1. Non-SVA code uses arm_smmu_inv_range_domain which calls
> arm_smmu_tlb_inv_range_domain(_no_atc) and arm_smmu_atc_range_domain,
> SVA code individually calls those two functions.
> arm_smmu_tlb_inv_range_domain(_no_atc) accepts a parameter to skip the
> invalidation if BTM feature is set.
> 2. Same as option 1, but SVA also calls arm_smmu_inv_range_domain.
> arm_smmu_inv_range_domain accepts both a parameter to skip TLB inv
> when BTM is set, as well as an SSID for the atc invalidation. SSID
> would be 0 in non-sva callers, and mm->pasid for SVA.

(Something I sneaked in there is renaming operations that invalidate
both TLBs and ATC to remove the tlb/atc in-fix, but if people aren't
keen on such renames then yeah I suppose we'd need awkward names like
arm_smmu_tlb_inv_range_domain_no_atc)

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

Re: [RFC PATCH v1 3/8] iommu/arm-smmu-v3-sva: Allocate new ASID from installed_smmus

[Michael Shavit]

On Mon, Aug 21, 2023 at 10:56 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Mon, Aug 21, 2023 at 10:39:14PM +0800, Michael Shavit wrote:
> > > > > > (on a loop over every smmu the domain in arm_smmu_mmu_notifier_get is
> > > > > > attached to, which just at a glance looks headache inducing because of
> > > > > > sva's piggybacking on the rid domain.)
> > > > >
> > > > > Not every smmu, just the one you are *currently* attaching to. We
> > > > > don't care if the *other* smmu's have different ASIDs, maybe they are
> > > > > not using BTM, or won't use SVA.
> > > >
> > > > I mean because the domain in arm_smmu_mmu_notifier_get is the RID
> > > > domain (not the SVA domain, same issue we discussed in previous
> > > > thread) , which can be attached to multiple SMMUs.
> > >
> > > Oh that is totally nonsensical. I expect you will need to fix that
> > > sooner than later. Once the CD table is moved and there is a proper
> > > way to track the PASID it should not be needed. It shouldn't fall into
> > > the decision making about where to put the ASID xarray.
> >
> > Right I got a bit of a chicken and egg problem with all these series.
>
> Yes, I'm not surprised to hear this
>
> Still, it would nice to move forward without going in a weird
> direction too much.
>
> Once the CD table is moved to the master what do you think is blocking
> fixing up the SVA stuff to not rely on a RID domain?

These aren't necessarily strict dependencies, but ideally I'd like to:
1. Natively support PASID attachments in the smmu domain (patch(es)
from the set_dev_pasid series)
2. Support attaching a domain to multiple SMMUs (this series)
3. SVA framework support for allocating a single SVA domain per MM
struct (Tina's series)

SVA can then directly attach an sva domain to a master in its
set_dev_pasid call, without having to do any sort of sharing of
smmu_notifiers or CDs across domains. The SVA domain allocated would
directly be attached to the master.

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Michael Shavit]

On Tue, Aug 22, 2023 at 4:17 PM Michael Shavit <mshavit@google.com> wrote:
>
> On Mon, Aug 21, 2023 at 7:58 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > > Since there's now a loop over a series of SMMUs inside
> > > arm_smmu_tlb_inv_range_asid, it makes sense to move the check into
> > > that loop. This technically works because only SVA is calling
> > > arm_smmu_tlb_inv_range_asid but can (IMO) risk introducing bugs in
> > > the future since it's not obvious from the function name.
> >
> > Well, I would remove the duplication and add an argument if you intend
> > to share the function that loops
>
> What do you think about this as a final stage:
> Once the set_dev_pasid and sva refactor lands, SVA could call a common
> arm_smmu_inv_range_domain implementation which would:
> 1. Skip the TLB invalidation on a per-smmu basis if it detects that
> the domain type is SVA, or based on a passed-in parameter that is only
> set True by SVA.
> 2. Issue ATC invalidations with SSIDs found in the arm_smmu_domain.
> This common function would be used for all use-cases: invalidations of
> domains attached on RIDs, on PASIDs (SVA and non SVA).
>
> Then we have two options for the intermediate stage with this series:
> 1. Non-SVA code uses arm_smmu_inv_range_domain which calls
> arm_smmu_tlb_inv_range_domain(_no_atc) and arm_smmu_atc_range_domain,
> SVA code individually calls those two functions.
> arm_smmu_tlb_inv_range_domain(_no_atc) accepts a parameter to skip the
> invalidation if BTM feature is set.
> 2. Same as option 1, but SVA also calls arm_smmu_inv_range_domain.
> arm_smmu_inv_range_domain accepts both a parameter to skip TLB inv
> when BTM is set, as well as an SSID for the atc invalidation. SSID
> would be 0 in non-sva callers, and mm->pasid for SVA.

Ugh, ok, there's a reason arm_smmu_tlb_inv_range_asid duplicates
arm_smmu_tlb_inv_range_domain!
The invalidation isn't performed with the smmu_domain's ASID... but
with the CD's ASID (because the domain is the RID domain.... again).

I think we better leave this out of scope for this series as well.
Let's keep the redundancy for now, and add a parameter to make things
explicit. We can clean things up when the above is fixed.

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

Re: [RFC PATCH v1 2/8] iommu/arm-smmu-v3: Perform invalidations over installed_smmus

[Jason Gunthorpe]

On Tue, Aug 22, 2023 at 04:17:31PM +0800, Michael Shavit wrote:
> On Mon, Aug 21, 2023 at 7:58 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > > Since there's now a loop over a series of SMMUs inside
> > > arm_smmu_tlb_inv_range_asid, it makes sense to move the check into
> > > that loop. This technically works because only SVA is calling
> > > arm_smmu_tlb_inv_range_asid but can (IMO) risk introducing bugs in
> > > the future since it's not obvious from the function name.
> >
> > Well, I would remove the duplication and add an argument if you intend
> > to share the function that loops
> 
> What do you think about this as a final stage:
> Once the set_dev_pasid and sva refactor lands, SVA could call a common
> arm_smmu_inv_range_domain implementation which would:
> 1. Skip the TLB invalidation on a per-smmu basis if it detects that
> the domain type is SVA, or based on a passed-in parameter that is only
> set True by SVA.
> 2. Issue ATC invalidations with SSIDs found in the arm_smmu_domain.
> This common function would be used for all use-cases: invalidations of
> domains attached on RIDs, on PASIDs (SVA and non SVA).

That seems like a good place to aim for

Jason

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