[PATCH v1 0/4] iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent

[PATCH v1 0/4] iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent

[Nicolin Chen]

With a system having multiple SMMU physical instances, multiple vSMMUs can
be allocated for a VM that deivces behind different SMMUs are assigned to.
In such a use case, the IPA->PA mappings (i.e. the stage-2 I/O page table)
can be shared across the vSMMU instances.

With a shareable S2 parent domain, it is more natural to store a vmid per
vSMMU instance v.s. a shared S2 domain, since each physical SMMU instance
maintains its own vmid bitmap.

Have a pair of patches getting the functions ready for the vmid migration.
Decouple the vmid from S2 parent domains and move its allocation to vSMMU
instances. Note that a regular S2 domain (!nest_parent) has to retain the
s2_cfg and vmid for non-nesting use cases, if the SMMU HW doesn't support
stage 1. Then, an S2 invalidation has to be iterated for all the vmids in
the vSMMU list introduced in the S2 parent domain.

This is on Github:
https://github.com/nicolinc/iommufd/commits/smmuv3_vmid-v1

To test it with RMR:
https://github.com/nicolinc/iommufd/commits/smmuv3_vmid-v1-with-rmr
Pairing QEMU branch:
https://github.com/nicolinc/qemu/commits/wip/for_smmuv3_vmid-v1

Thanks
Nicolin

Nicolin Chen (4):
  iommu/arm-smmu-v3: Pass in vmid to arm_smmu_make_s2_domain_ste()
  iommu/arm-smmu-v3: Share arm_smmu_cmdq_issue_cmd_with_sync() with
    vsmmu
  iommu/arm-smmu-v3: Decouple vmid from S2 nest_parent domain
  iommu/arm-smmu-v3-iommufd: Allow a shared s2_parent to allocate vSMMU

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 14 ++++-
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 44 +++++++++++---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c  |  3 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 57 ++++++++++++++-----
 4 files changed, 93 insertions(+), 25 deletions(-)


base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
-- 
2.43.0

[PATCH v1 1/4] iommu/arm-smmu-v3: Pass in vmid to arm_smmu_make_s2_domain_ste()

[Nicolin Chen]

An stage-2 STE requires a vmid that has been so far allocated per domain,
so arm_smmu_make_s2_domain_ste() has been extracting the vmid from the S2
domain.

To share an S2 parent domain across vSMMUs in the same VM, a vmid will be
no longer allocated for nor stored in the S2 domain, but per vSMMU, which
means the arm_smmu_make_s2_domain_ste() can get a vmid either from an S2
domain (non nesting parent) or a vSMMU.

Allow to pass in vmid explicitly to arm_smmu_make_s2_domain_ste(), giving
its callers a chance to pick the vmid between a domain or a vSMMU.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h         | 2 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 6 ++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c    | 3 ++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c         | 6 +++---
 4 files changed, 10 insertions(+), 7 deletions(-)

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 bd9d7c85576a..e08c4ede4b2d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -887,7 +887,7 @@ struct arm_smmu_entry_writer_ops {
 void arm_smmu_make_abort_ste(struct arm_smmu_ste *target);
 void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
 				 struct arm_smmu_master *master,
-				 struct arm_smmu_domain *smmu_domain,
+				 struct arm_smmu_domain *smmu_domain, u16 vmid,
 				 bool ats_enabled);
 
 #if IS_ENABLED(CONFIG_KUNIT)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 5aa2e7af58b4..ff8b550159f2 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -34,8 +34,9 @@ static void arm_smmu_make_nested_cd_table_ste(
 	struct arm_smmu_ste *target, struct arm_smmu_master *master,
 	struct arm_smmu_nested_domain *nested_domain, bool ats_enabled)
 {
-	arm_smmu_make_s2_domain_ste(
-		target, master, nested_domain->vsmmu->s2_parent, ats_enabled);
+	arm_smmu_make_s2_domain_ste(target, master,
+				    nested_domain->vsmmu->s2_parent,
+				    nested_domain->vsmmu->vmid, ats_enabled);
 
 	target->data[0] = cpu_to_le64(STRTAB_STE_0_V |
 				      FIELD_PREP(STRTAB_STE_0_CFG,
@@ -76,6 +77,7 @@ static void arm_smmu_make_nested_domain_ste(
 	case STRTAB_STE_0_CFG_BYPASS:
 		arm_smmu_make_s2_domain_ste(target, master,
 					    nested_domain->vsmmu->s2_parent,
+					    nested_domain->vsmmu->vmid,
 					    ats_enabled);
 		break;
 	case STRTAB_STE_0_CFG_ABORT:
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
index d2671bfd3798..7fac5a112c5c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c
@@ -316,7 +316,8 @@ static void arm_smmu_test_make_s2_ste(struct arm_smmu_ste *ste,
 	io_pgtable.cfg.arm_lpae_s2_cfg.vtcr.sl = 3;
 	io_pgtable.cfg.arm_lpae_s2_cfg.vtcr.tsz = 4;
 
-	arm_smmu_make_s2_domain_ste(ste, &master, &smmu_domain, ats_enabled);
+	arm_smmu_make_s2_domain_ste(ste, &master, &smmu_domain,
+				    smmu_domain.s2_cfg.vmid, ats_enabled);
 }
 
 static void arm_smmu_v3_write_ste_test_s2_to_abort(struct kunit *test)
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 358072b4e293..310bb4109ec9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1656,10 +1656,9 @@ EXPORT_SYMBOL_IF_KUNIT(arm_smmu_make_cdtable_ste);
 
 void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
 				 struct arm_smmu_master *master,
-				 struct arm_smmu_domain *smmu_domain,
+				 struct arm_smmu_domain *smmu_domain, u16 vmid,
 				 bool ats_enabled)
 {
-	struct arm_smmu_s2_cfg *s2_cfg = &smmu_domain->s2_cfg;
 	const struct io_pgtable_cfg *pgtbl_cfg =
 		&io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops)->cfg;
 	typeof(&pgtbl_cfg->arm_lpae_s2_cfg.vtcr) vtcr =
@@ -1690,7 +1689,7 @@ void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
 		   FIELD_PREP(STRTAB_STE_2_VTCR_S2TG, vtcr->tg) |
 		   FIELD_PREP(STRTAB_STE_2_VTCR_S2PS, vtcr->ps);
 	target->data[2] = cpu_to_le64(
-		FIELD_PREP(STRTAB_STE_2_S2VMID, s2_cfg->vmid) |
+		FIELD_PREP(STRTAB_STE_2_S2VMID, vmid) |
 		FIELD_PREP(STRTAB_STE_2_VTCR, vtcr_val) |
 		STRTAB_STE_2_S2AA64 |
 #ifdef __BIG_ENDIAN
@@ -2969,6 +2968,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 	case ARM_SMMU_DOMAIN_S2:
 		arm_smmu_make_s2_domain_ste(&target, master, smmu_domain,
+					    smmu_domain->s2_cfg.vmid,
 					    state.ats_enabled);
 		arm_smmu_install_ste_for_dev(master, &target);
 		arm_smmu_clear_cd(master, IOMMU_NO_PASID);
-- 
2.43.0

[PATCH v1 2/4] iommu/arm-smmu-v3: Share arm_smmu_cmdq_issue_cmd_with_sync() with vsmmu

[Nicolin Chen]

Allow arm-smmu-v3-iommufd to call it for S2 cache invalidations.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 ++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

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 e08c4ede4b2d..3336d196062c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -984,6 +984,8 @@ void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master,
 int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
 				struct arm_smmu_cmdq *cmdq, u64 *cmds, int n,
 				bool sync);
+int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,
+				      struct arm_smmu_cmdq_ent *ent);
 
 #ifdef CONFIG_ARM_SMMU_V3_SVA
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
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 310bb4109ec9..0462eb1b2912 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -929,8 +929,8 @@ static int arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
 	return __arm_smmu_cmdq_issue_cmd(smmu, ent, false);
 }
 
-static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,
-					     struct arm_smmu_cmdq_ent *ent)
+int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,
+				      struct arm_smmu_cmdq_ent *ent)
 {
 	return __arm_smmu_cmdq_issue_cmd(smmu, ent, true);
 }
-- 
2.43.0

[PATCH v1 3/4] iommu/arm-smmu-v3: Decouple vmid from S2 nest_parent domain

[Nicolin Chen]

An S2 nest_parent domain can be shared across vSMMUs in the same VM, since
the S2 domain is basically the IPA mappings for the entire RAM of the VM.

Meanwhile, each vSMMU can have its own VMID, so the VMID allocation should
be done per vSMMU instance v.s. per S2 nest_parent domain.

However, an S2 domain can be also allocated when a physical SMMU instance
doesn't support S1. So, the structure has to retain the s2_cfg and vmid.

Allocate a vmid for a vSMMU instance in arm_vsmmu_alloc() and add a proper
arm_vsmmu_destroy() to clean it up.

Add a per-domain "vsmmus" list pairing with a spinlock, maintaining a list
on the S2 parent domain, to iterate S2 invalidations over the vmids across
the vSMMU instances created for the same VM.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 10 +++-
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 35 ++++++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 47 +++++++++++++++----
 3 files changed, 79 insertions(+), 13 deletions(-)

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 3336d196062c..1f6696bc4f6c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -849,8 +849,12 @@ struct arm_smmu_domain {
 
 	enum arm_smmu_domain_stage	stage;
 	union {
-		struct arm_smmu_ctx_desc	cd;
-		struct arm_smmu_s2_cfg		s2_cfg;
+		struct arm_smmu_ctx_desc cd;	/* S1 */
+		struct arm_smmu_s2_cfg s2_cfg;	/* S2 && !nest_parent */
+		struct {			/* S2 && nest_parent */
+			struct list_head list;
+			spinlock_t lock;
+		} vsmmus;
 	};
 
 	struct iommu_domain		domain;
@@ -1049,6 +1053,8 @@ struct arm_vsmmu {
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_domain *s2_parent;
 	u16 vmid;
+
+	struct list_head vsmmus_elm; /* arm_smmu_domain::vsmmus::list */
 };
 
 #if IS_ENABLED(CONFIG_ARM_SMMU_V3_IOMMUFD)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index ff8b550159f2..2c5a9d0abed5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -30,6 +30,23 @@ void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type)
 	return info;
 }
 
+static void arm_vsmmu_destroy(struct iommufd_viommu *viommu)
+{
+	struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core);
+	struct arm_smmu_device *smmu = vsmmu->smmu;
+	struct arm_smmu_cmdq_ent cmd = {
+		.opcode = CMDQ_OP_TLBI_S12_VMALL,
+		.tlbi.vmid = vsmmu->vmid,
+	};
+	unsigned long flags;
+
+	spin_lock_irqsave(&vsmmu->s2_parent->vsmmus.lock, flags);
+	list_del(&vsmmu->vsmmus_elm);
+	spin_unlock_irqrestore(&vsmmu->s2_parent->vsmmus.lock, flags);
+	arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+	ida_free(&smmu->vmid_map, vsmmu->vmid);
+}
+
 static void arm_smmu_make_nested_cd_table_ste(
 	struct arm_smmu_ste *target, struct arm_smmu_master *master,
 	struct arm_smmu_nested_domain *nested_domain, bool ats_enabled)
@@ -337,6 +354,7 @@ static int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
 }
 
 static const struct iommufd_viommu_ops arm_vsmmu_ops = {
+	.destroy = arm_vsmmu_destroy,
 	.alloc_domain_nested = arm_vsmmu_alloc_domain_nested,
 	.cache_invalidate = arm_vsmmu_cache_invalidate,
 };
@@ -351,6 +369,8 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 	struct arm_smmu_domain *s2_parent = to_smmu_domain(parent);
 	struct arm_vsmmu *vsmmu;
+	unsigned long flags;
+	int vmid;
 
 	if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
 		return ERR_PTR(-EOPNOTSUPP);
@@ -381,15 +401,24 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
 		return ERR_PTR(-EOPNOTSUPP);
 
+	vmid = ida_alloc_range(&smmu->vmid_map, 1, (1 << smmu->vmid_bits) - 1,
+			       GFP_KERNEL);
+	if (vmid < 0)
+		return ERR_PTR(vmid);
+
 	vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, core,
 				     &arm_vsmmu_ops);
-	if (IS_ERR(vsmmu))
+	if (IS_ERR(vsmmu)) {
+		ida_free(&smmu->vmid_map, vmid);
 		return ERR_CAST(vsmmu);
+	}
 
 	vsmmu->smmu = smmu;
+	vsmmu->vmid = (u16)vmid;
 	vsmmu->s2_parent = s2_parent;
-	/* FIXME Move VMID allocation from the S2 domain allocation to here */
-	vsmmu->vmid = s2_parent->s2_cfg.vmid;
+	spin_lock_irqsave(&s2_parent->vsmmus.lock, flags);
+	list_add_tail(&vsmmu->vsmmus_elm, &s2_parent->vsmmus.list);
+	spin_unlock_irqrestore(&s2_parent->vsmmus.lock, flags);
 
 	return &vsmmu->core;
 }
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 0462eb1b2912..addc6308742b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2249,10 +2249,22 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 	 */
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		arm_smmu_tlb_inv_asid(smmu, smmu_domain->cd.asid);
-	} else {
+	} else if (!smmu_domain->nest_parent) {
 		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
 		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
 		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+	} else {
+		struct arm_vsmmu *vsmmu, *next;
+		unsigned long flags;
+
+		cmd.opcode = CMDQ_OP_TLBI_S12_VMALL;
+		spin_lock_irqsave(&smmu_domain->vsmmus.lock, flags);
+		list_for_each_entry_safe(vsmmu, next, &smmu_domain->vsmmus.list,
+					 vsmmus_elm) {
+			cmd.tlbi.vmid = vsmmu->vmid;
+			arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+		}
+		spin_unlock_irqrestore(&smmu_domain->vsmmus.lock, flags);
 	}
 	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
 }
@@ -2342,19 +2354,33 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
 		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 {
+		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
+					 smmu_domain);
+	} else if (!smmu_domain->nest_parent) {
 		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);
+		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
+					 smmu_domain);
+	} else {
+		struct arm_vsmmu *vsmmu, *next;
+		unsigned long flags;
 
-	if (smmu_domain->nest_parent) {
 		/*
 		 * When the S2 domain changes all the nested S1 ASIDs have to be
 		 * flushed too.
 		 */
 		cmd.opcode = CMDQ_OP_TLBI_NH_ALL;
 		arm_smmu_cmdq_issue_cmd_with_sync(smmu_domain->smmu, &cmd);
+
+		cmd.opcode = CMDQ_OP_TLBI_S2_IPA;
+		spin_lock_irqsave(&smmu_domain->vsmmus.lock, flags);
+		list_for_each_entry_safe(vsmmu, next, &smmu_domain->vsmmus.list,
+					 vsmmus_elm) {
+			cmd.tlbi.vmid = vsmmu->vmid;
+			__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
+						 smmu_domain);
+		}
+		spin_unlock_irqrestore(&smmu_domain->vsmmus.lock, flags);
 	}
 
 	/*
@@ -2477,7 +2503,7 @@ static void arm_smmu_domain_free_paging(struct iommu_domain *domain)
 		mutex_lock(&arm_smmu_asid_lock);
 		xa_erase(&arm_smmu_asid_xa, smmu_domain->cd.asid);
 		mutex_unlock(&arm_smmu_asid_lock);
-	} else {
+	} else if (!smmu_domain->nest_parent) {
 		struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
 		if (cfg->vmid)
 			ida_free(&smmu->vmid_map, cfg->vmid);
@@ -2506,7 +2532,10 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_device *smmu,
 				       struct arm_smmu_domain *smmu_domain)
 {
 	int vmid;
-	struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
+
+	/* nest_parent stores vmid in vSMMU instead of a shared S2 domain */
+	if (smmu_domain->nest_parent)
+		return 0;
 
 	/* Reserve VMID 0 for stage-2 bypass STEs */
 	vmid = ida_alloc_range(&smmu->vmid_map, 1, (1 << smmu->vmid_bits) - 1,
@@ -2514,7 +2543,7 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_device *smmu,
 	if (vmid < 0)
 		return vmid;
 
-	cfg->vmid	= (u16)vmid;
+	smmu_domain->s2_cfg.vmid = (u16)vmid;
 	return 0;
 }
 
@@ -3233,6 +3262,8 @@ arm_smmu_domain_alloc_paging_flags(struct device *dev, u32 flags,
 		}
 		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
 		smmu_domain->nest_parent = true;
+		INIT_LIST_HEAD(&smmu_domain->vsmmus.list);
+		spin_lock_init(&smmu_domain->vsmmus.lock);
 		break;
 	case IOMMU_HWPT_ALLOC_DIRTY_TRACKING:
 	case IOMMU_HWPT_ALLOC_DIRTY_TRACKING | IOMMU_HWPT_ALLOC_PASID:
-- 
2.43.0

[PATCH v1 4/4] iommu/arm-smmu-v3-iommufd: Allow a shared s2_parent to allocate vSMMU

[Nicolin Chen]

Now, vmids are stored in vSMMU objects. So all vSMMUs assigned to the same
VM can share a s2_parent domain. This means a vIOMMU allocation per device
behind one SMMU can be given with a s2_parent domain that's allocated per
another device behind another SMMU, i.e. s2_parent->smmu != master->smmu.

Remove the validation line to allow this use case.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 2c5a9d0abed5..9bfa5fa5bafa 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -378,9 +378,6 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
 	if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
 		return ERR_PTR(-EOPNOTSUPP);
 
-	if (s2_parent->smmu != master->smmu)
-		return ERR_PTR(-EINVAL);
-
 	/*
 	 * FORCE_SYNC is not set with FEAT_NESTING. Some study of the exact HW
 	 * defect is needed to determine if arm_vsmmu_cache_invalidate() needs
-- 
2.43.0

RE: [PATCH v1 1/4] iommu/arm-smmu-v3: Pass in vmid to arm_smmu_make_s2_domain_ste()

[Shameerali Kolothum Thodi]

Hi Nicolin,

Thanks for sending out this series. This might as well help me to rework my pinned KVM
VMID series here,
https://lore.kernel.org/linux-iommu/20240208151837.35068-1-shameerali.kolothum.thodi@huawei.com/


> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, March 5, 2025 5:04 AM
> To: will@kernel.org; robin.murphy@arm.com; jgg@nvidia.com
> Cc: joro@8bytes.org; linux-arm-kernel@lists.infradead.org;
> iommu@lists.linux.dev; linux-kernel@vger.kernel.org; Shameerali Kolothum
> Thodi <shameerali.kolothum.thodi@huawei.com>
> Subject: [PATCH v1 1/4] iommu/arm-smmu-v3: Pass in vmid to
> arm_smmu_make_s2_domain_ste()
> 
> An stage-2 STE requires a vmid that has been so far allocated per domain,
> so arm_smmu_make_s2_domain_ste() has been extracting the vmid from
> the S2
> domain.
> 
> To share an S2 parent domain across vSMMUs in the same VM, a vmid will
> be
> no longer allocated for nor stored in the S2 domain, but per vSMMU, which
> means the arm_smmu_make_s2_domain_ste() can get a vmid either from
> an S2
> domain (non nesting parent) or a vSMMU.
> 
> Allow to pass in vmid explicitly to arm_smmu_make_s2_domain_ste(),
> giving
> its callers a chance to pick the vmid between a domain or a vSMMU.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h         | 2 +-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 6 ++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c    | 3 ++-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c         | 6 +++---
>  4 files changed, 10 insertions(+), 7 deletions(-)
> 
> 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 bd9d7c85576a..e08c4ede4b2d 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -887,7 +887,7 @@ struct arm_smmu_entry_writer_ops {
>  void arm_smmu_make_abort_ste(struct arm_smmu_ste *target);
>  void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
>  				 struct arm_smmu_master *master,
> -				 struct arm_smmu_domain *smmu_domain,
> +				 struct arm_smmu_domain *smmu_domain,
> u16 vmid,
>  				 bool ats_enabled);

Now that vmid is an input, do we need some kind of validation here as
at least vmid = 0 is reserved I guess for bypass STEs.

Thanks,
Shameer

RE: [PATCH v1 4/4] iommu/arm-smmu-v3-iommufd: Allow a shared s2_parent to allocate vSMMU

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, March 5, 2025 5:04 AM
> To: will@kernel.org; robin.murphy@arm.com; jgg@nvidia.com
> Cc: joro@8bytes.org; linux-arm-kernel@lists.infradead.org;
> iommu@lists.linux.dev; linux-kernel@vger.kernel.org; Shameerali Kolothum
> Thodi <shameerali.kolothum.thodi@huawei.com>
> Subject: [PATCH v1 4/4] iommu/arm-smmu-v3-iommufd: Allow a shared
> s2_parent to allocate vSMMU
> 
> Now, vmids are stored in vSMMU objects. So all vSMMUs assigned to the
> same
> VM can share a s2_parent domain. This means a vIOMMU allocation per
> device
> behind one SMMU can be given with a s2_parent domain that's allocated
> per
> another device behind another SMMU, i.e. s2_parent->smmu != master-
> >smmu.
> 
> Remove the validation line to allow this use case.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> index 2c5a9d0abed5..9bfa5fa5bafa 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> @@ -378,9 +378,6 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct
> device *dev,
>  	if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
>  		return ERR_PTR(-EOPNOTSUPP);
> 
> -	if (s2_parent->smmu != master->smmu)
> -		return ERR_PTR(-EINVAL);
> -

Not sure we can just relax this like this. What if the two physical SMMUs are different in
functionality/features? Do we need some kind of sanity check here?

Thanks,
Shameer

Re: [PATCH v1 0/4] iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent

[Jason Gunthorpe]

On Tue, Mar 04, 2025 at 09:03:59PM -0800, Nicolin Chen wrote:

> Have a pair of patches getting the functions ready for the vmid migration.
> Decouple the vmid from S2 parent domains and move its allocation to vSMMU
> instances. Note that a regular S2 domain (!nest_parent) has to retain the
> s2_cfg and vmid for non-nesting use cases, if the SMMU HW doesn't support
> stage 1. Then, an S2 invalidation has to be iterated for all the vmids in
> the vSMMU list introduced in the S2 parent domain.

I was planning to also fix the S2 to be able to attach to multiple
IOMMU instances at the same time as getting VMID to the viommu.. It
doesn't quite make sense to me that viommu would allow multi-attach
but the normal cases wouldn't. Did you find a shortcut?

The main issue here was to do this without degrading the invalidation
workflow and harming SVA and DMA API performance..

Jason

Re: [PATCH v1 1/4] iommu/arm-smmu-v3: Pass in vmid to arm_smmu_make_s2_domain_ste()

[Jason Gunthorpe]

On Tue, Mar 04, 2025 at 09:04:00PM -0800, Nicolin Chen wrote:
> An stage-2 STE requires a vmid that has been so far allocated per domain,
> so arm_smmu_make_s2_domain_ste() has been extracting the vmid from the S2
> domain.
> 
> To share an S2 parent domain across vSMMUs in the same VM, a vmid will be
> no longer allocated for nor stored in the S2 domain, but per vSMMU, which
> means the arm_smmu_make_s2_domain_ste() can get a vmid either from an S2
> domain (non nesting parent) or a vSMMU.
> 
> Allow to pass in vmid explicitly to arm_smmu_make_s2_domain_ste(), giving
> its callers a chance to pick the vmid between a domain or a vSMMU.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h         | 2 +-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 6 ++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c    | 3 ++-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c         | 6 +++---
>  4 files changed, 10 insertions(+), 7 deletions(-)

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

Jason

Re: [PATCH v1 4/4] iommu/arm-smmu-v3-iommufd: Allow a shared s2_parent to allocate vSMMU

[Jason Gunthorpe]

On Wed, Mar 05, 2025 at 09:01:40AM +0000, Shameerali Kolothum Thodi wrote:

> >  	if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
> >  		return ERR_PTR(-EOPNOTSUPP);
> > 
> > -	if (s2_parent->smmu != master->smmu)
> > -		return ERR_PTR(-EINVAL);
> > -
> 
> Not sure we can just relax this like this. What if the two physical SMMUs are different in
> functionality/features? Do we need some kind of sanity check here?

Yes, a function to check if a domain's iopgtbl config is compatible
with the instance is required.

Jason

Re: [PATCH v1 3/4] iommu/arm-smmu-v3: Decouple vmid from S2 nest_parent domain

[Jason Gunthorpe]

On Tue, Mar 04, 2025 at 09:04:02PM -0800, Nicolin Chen wrote:
> @@ -2249,10 +2249,22 @@ static void arm_smmu_tlb_inv_context(void *cookie)
>  	 */
>  	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>  		arm_smmu_tlb_inv_asid(smmu, smmu_domain->cd.asid);
> -	} else {
> +	} else if (!smmu_domain->nest_parent) {
>  		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
>  		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
>  		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> +	} else {
> +		struct arm_vsmmu *vsmmu, *next;
> +		unsigned long flags;
> +
> +		cmd.opcode = CMDQ_OP_TLBI_S12_VMALL;
> +		spin_lock_irqsave(&smmu_domain->vsmmus.lock, flags);
> +		list_for_each_entry_safe(vsmmu, next, &smmu_domain->vsmmus.list,
> +					 vsmmus_elm) {
> +			cmd.tlbi.vmid = vsmmu->vmid;
> +			arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> +		}
> +		spin_unlock_irqrestore(&smmu_domain->vsmmus.lock, flags);
>  	}

I see.. So this just makes a 3rd classification of invalidation
protocol that uses a spinlock and linked list

>  	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);

This is no good, arm_smmu_atc_inv_domain() is invalidating against the
instance that created the domain.

IMHO if you do this you should set domain->iommu = NULL to indicate
that the iommu is non-valid in this mode to catch issues.

Jason

Re: [PATCH v1 1/4] iommu/arm-smmu-v3: Pass in vmid to arm_smmu_make_s2_domain_ste()

[Nicolin Chen]

On Wed, Mar 05, 2025 at 08:50:17AM +0000, Shameerali Kolothum Thodi 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 bd9d7c85576a..e08c4ede4b2d 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -887,7 +887,7 @@ struct arm_smmu_entry_writer_ops {
> >  void arm_smmu_make_abort_ste(struct arm_smmu_ste *target);
> >  void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
> >  				 struct arm_smmu_master *master,
> > -				 struct arm_smmu_domain *smmu_domain,
> > +				 struct arm_smmu_domain *smmu_domain,
> > u16 vmid,
> >  				 bool ats_enabled);
> 
> Now that vmid is an input, do we need some kind of validation here as
> at least vmid = 0 is reserved I guess for bypass STEs.

Perhaps it should do a WARN_ON_ONCE(!vmid), as it doesn't make
sense for a caller to make an S2-bypass STE with this function.

Thanks
Nicolin

Re: [PATCH v1 4/4] iommu/arm-smmu-v3-iommufd: Allow a shared s2_parent to allocate vSMMU

[Nicolin Chen]

On Wed, Mar 05, 2025 at 12:57:43PM -0400, Jason Gunthorpe wrote:
> On Wed, Mar 05, 2025 at 09:01:40AM +0000, Shameerali Kolothum Thodi wrote:
> 
> > >  	if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
> > >  		return ERR_PTR(-EOPNOTSUPP);
> > > 
> > > -	if (s2_parent->smmu != master->smmu)
> > > -		return ERR_PTR(-EINVAL);
> > > -
> > 
> > Not sure we can just relax this like this. What if the two physical SMMUs are different in
> > functionality/features? Do we need some kind of sanity check here?
> 
> Yes, a function to check if a domain's iopgtbl config is compatible
> with the instance is required.

Yea. Will rework.

Thanks!
Nicolin

Re: [PATCH v1 0/4] iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent

[Nicolin Chen]

On Wed, Mar 05, 2025 at 12:54:52PM -0400, Jason Gunthorpe wrote:
> On Tue, Mar 04, 2025 at 09:03:59PM -0800, Nicolin Chen wrote:
> 
> > Have a pair of patches getting the functions ready for the vmid migration.
> > Decouple the vmid from S2 parent domains and move its allocation to vSMMU
> > instances. Note that a regular S2 domain (!nest_parent) has to retain the
> > s2_cfg and vmid for non-nesting use cases, if the SMMU HW doesn't support
> > stage 1. Then, an S2 invalidation has to be iterated for all the vmids in
> > the vSMMU list introduced in the S2 parent domain.
> 
> I was planning to also fix the S2 to be able to attach to multiple
> IOMMU instances at the same time as getting VMID to the viommu.. It
> doesn't quite make sense to me that viommu would allow multi-attach
> but the normal cases wouldn't. Did you find a shortcut?

Hmm, not quite following the question. You mean vIOMMU attaching to
multiple S2 domains?

> The main issue here was to do this without degrading the invalidation
> workflow and harming SVA and DMA API performance..

I see.. The ATC_INV and TLBI_NH_ALL will be increased for the shared
S2, even if the S2 change is about a device behind one physical SMMU.

Thanks
Nicolin

Re: [PATCH v1 0/4] iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent

[Jason Gunthorpe]

On Wed, Mar 05, 2025 at 10:23:36AM -0800, Nicolin Chen wrote:
> On Wed, Mar 05, 2025 at 12:54:52PM -0400, Jason Gunthorpe wrote:
> > On Tue, Mar 04, 2025 at 09:03:59PM -0800, Nicolin Chen wrote:
> > 
> > > Have a pair of patches getting the functions ready for the vmid migration.
> > > Decouple the vmid from S2 parent domains and move its allocation to vSMMU
> > > instances. Note that a regular S2 domain (!nest_parent) has to retain the
> > > s2_cfg and vmid for non-nesting use cases, if the SMMU HW doesn't support
> > > stage 1. Then, an S2 invalidation has to be iterated for all the vmids in
> > > the vSMMU list introduced in the S2 parent domain.
> > 
> > I was planning to also fix the S2 to be able to attach to multiple
> > IOMMU instances at the same time as getting VMID to the viommu.. It
> > doesn't quite make sense to me that viommu would allow multi-attach
> > but the normal cases wouldn't. Did you find a shortcut?
> 
> Hmm, not quite following the question. You mean vIOMMU attaching to
> multiple S2 domains?

I mean a normal S2 domain attaching to multiple devices on multiple
instances.

Jason

Re: [PATCH v1 3/4] iommu/arm-smmu-v3: Decouple vmid from S2 nest_parent domain

[Nicolin Chen]

On Wed, Mar 05, 2025 at 01:01:57PM -0400, Jason Gunthorpe wrote:
> On Tue, Mar 04, 2025 at 09:04:02PM -0800, Nicolin Chen wrote:
> > @@ -2249,10 +2249,22 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> >  	 */
> >  	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> >  		arm_smmu_tlb_inv_asid(smmu, smmu_domain->cd.asid);
> > -	} else {
> > +	} else if (!smmu_domain->nest_parent) {
> >  		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
> >  		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
> >  		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> > +	} else {
> > +		struct arm_vsmmu *vsmmu, *next;
> > +		unsigned long flags;
> > +
> > +		cmd.opcode = CMDQ_OP_TLBI_S12_VMALL;
> > +		spin_lock_irqsave(&smmu_domain->vsmmus.lock, flags);
> > +		list_for_each_entry_safe(vsmmu, next, &smmu_domain->vsmmus.list,
> > +					 vsmmus_elm) {
> > +			cmd.tlbi.vmid = vsmmu->vmid;
> > +			arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);

Just noticed that here should be vsmmu->smmu.

> > +		}
> > +		spin_unlock_irqrestore(&smmu_domain->vsmmus.lock, flags);
> >  	}
> 
> I see.. So this just makes a 3rd classification of invalidation
> protocol that uses a spinlock and linked list
>
> >  	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
> 
> This is no good, arm_smmu_atc_inv_domain() is invalidating against the
> instance that created the domain.

Oh right... we might need an arm_smmu_atc_inv_all() that takes
an smmu pointer. This might have some performance downgrade as
you worried about though.

> IMHO if you do this you should set domain->iommu = NULL to indicate
> that the iommu is non-valid in this mode to catch issues.

You mean smmu_domain->smmu pointer right?

Thanks
Nicolin

Re: [PATCH v1 0/4] iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent

[Nicolin Chen]

On Wed, Mar 05, 2025 at 02:31:51PM -0400, Jason Gunthorpe wrote:
> On Wed, Mar 05, 2025 at 10:23:36AM -0800, Nicolin Chen wrote:
> > On Wed, Mar 05, 2025 at 12:54:52PM -0400, Jason Gunthorpe wrote:
> > > On Tue, Mar 04, 2025 at 09:03:59PM -0800, Nicolin Chen wrote:
> > > 
> > > > Have a pair of patches getting the functions ready for the vmid migration.
> > > > Decouple the vmid from S2 parent domains and move its allocation to vSMMU
> > > > instances. Note that a regular S2 domain (!nest_parent) has to retain the
> > > > s2_cfg and vmid for non-nesting use cases, if the SMMU HW doesn't support
> > > > stage 1. Then, an S2 invalidation has to be iterated for all the vmids in
> > > > the vSMMU list introduced in the S2 parent domain.
> > > 
> > > I was planning to also fix the S2 to be able to attach to multiple
> > > IOMMU instances at the same time as getting VMID to the viommu.. It
> > > doesn't quite make sense to me that viommu would allow multi-attach
> > > but the normal cases wouldn't. Did you find a shortcut?
> > 
> > Hmm, not quite following the question. You mean vIOMMU attaching to
> > multiple S2 domains?
> 
> I mean a normal S2 domain attaching to multiple devices on multiple
> instances.

Oh, I haven't thought about a !nest_parent S2 case.

A nest_parent case will not allow devices to attach the S2 but
always to a proxy nested S1 as we discussed previously. So, I
think the implementation could be very different?

Thanks
Nicolin

Re: [PATCH v1 0/4] iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent

[Jason Gunthorpe]

On Wed, Mar 05, 2025 at 10:51:38AM -0800, Nicolin Chen wrote:

> > I mean a normal S2 domain attaching to multiple devices on multiple
> > instances.
> 
> Oh, I haven't thought about a !nest_parent S2 case.
> 
> A nest_parent case will not allow devices to attach the S2 but
> always to a proxy nested S1 as we discussed previously. So, I
> think the implementation could be very different?

It could, and that is what you show here

But also, it could be the same implementation.

Jason

Re: [PATCH v1 0/4] iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent

[Nicolin Chen]

On Wed, Mar 05, 2025 at 03:29:50PM -0400, Jason Gunthorpe wrote:
> On Wed, Mar 05, 2025 at 10:51:38AM -0800, Nicolin Chen wrote:
> 
> > > I mean a normal S2 domain attaching to multiple devices on multiple
> > > instances.
> > 
> > Oh, I haven't thought about a !nest_parent S2 case.
> > 
> > A nest_parent case will not allow devices to attach the S2 but
> > always to a proxy nested S1 as we discussed previously. So, I
> > think the implementation could be very different?
> 
> It could, and that is what you show here
> 
> But also, it could be the same implementation.

OK.

Obviously I underestimated this part of work.

Implementation aside, this at lease needs some assessment of any
invalidation impact. I should do some extra tracing.

Thanks
Nicolin

Re: [PATCH v1 1/4] iommu/arm-smmu-v3: Pass in vmid to arm_smmu_make_s2_domain_ste()

[Pranjal Shrivastava]

On Tue, Mar 04, 2025 at 09:04:00PM -0800, Nicolin Chen wrote:
> An stage-2 STE requires a vmid that has been so far allocated per domain,
> so arm_smmu_make_s2_domain_ste() has been extracting the vmid from the S2
> domain.
> 
> To share an S2 parent domain across vSMMUs in the same VM, a vmid will be
> no longer allocated for nor stored in the S2 domain, but per vSMMU, which
> means the arm_smmu_make_s2_domain_ste() can get a vmid either from an S2
> domain (non nesting parent) or a vSMMU.
> 
> Allow to pass in vmid explicitly to arm_smmu_make_s2_domain_ste(), giving
> its callers a chance to pick the vmid between a domain or a vSMMU.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Pranjal Shrivastava <praan@google.com>

> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h         | 2 +-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 6 ++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-test.c    | 3 ++-
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c         | 6 +++---
>  4 files changed, 10 insertions(+), 7 deletions(-)
> 

Thanks

Re: [PATCH v1 1/4] iommu/arm-smmu-v3: Pass in vmid to arm_smmu_make_s2_domain_ste()

[Pranjal Shrivastava]

On Wed, Mar 05, 2025 at 09:44:30AM -0800, Nicolin Chen wrote:
> On Wed, Mar 05, 2025 at 08:50:17AM +0000, Shameerali Kolothum Thodi 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 bd9d7c85576a..e08c4ede4b2d 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > @@ -887,7 +887,7 @@ struct arm_smmu_entry_writer_ops {
> > >  void arm_smmu_make_abort_ste(struct arm_smmu_ste *target);
> > >  void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
> > >  				 struct arm_smmu_master *master,
> > > -				 struct arm_smmu_domain *smmu_domain,
> > > +				 struct arm_smmu_domain *smmu_domain,
> > > u16 vmid,
> > >  				 bool ats_enabled);
> > 
> > Now that vmid is an input, do we need some kind of validation here as
> > at least vmid = 0 is reserved I guess for bypass STEs.
> 
> Perhaps it should do a WARN_ON_ONCE(!vmid), as it doesn't make
> sense for a caller to make an S2-bypass STE with this function.
> 

+1, a warning should suffice as the caller isn't expected to call this
with vmid = 0. For the s2 bypass case, we already set vmid field to 0 in
make_cdtable_ste for clients who *really* want the vmid = 0.

> Thanks
> Nicolin
> 

Thanks
Praan

Re: [PATCH v1 2/4] iommu/arm-smmu-v3: Share arm_smmu_cmdq_issue_cmd_with_sync() with vsmmu

[Pranjal Shrivastava]

On Tue, Mar 04, 2025 at 09:04:01PM -0800, Nicolin Chen wrote:
> Allow arm-smmu-v3-iommufd to call it for S2 cache invalidations.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Pranjal Shrivastava <praan@google.com>
Thanks
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 ++
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> 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 e08c4ede4b2d..3336d196062c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -984,6 +984,8 @@ void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master,
>  int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
>  				struct arm_smmu_cmdq *cmdq, u64 *cmds, int n,
>  				bool sync);
> +int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,
> +				      struct arm_smmu_cmdq_ent *ent);
>  
>  #ifdef CONFIG_ARM_SMMU_V3_SVA
>  bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
> 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 310bb4109ec9..0462eb1b2912 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -929,8 +929,8 @@ static int arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
>  	return __arm_smmu_cmdq_issue_cmd(smmu, ent, false);
>  }
>  
> -static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,
> -					     struct arm_smmu_cmdq_ent *ent)
> +int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,
> +				      struct arm_smmu_cmdq_ent *ent)
>  {
>  	return __arm_smmu_cmdq_issue_cmd(smmu, ent, true);
>  }
> -- 
> 2.43.0
> 
> 

Re: [PATCH v1 3/4] iommu/arm-smmu-v3: Decouple vmid from S2 nest_parent domain

[Pranjal Shrivastava]

On Tue, Mar 04, 2025 at 09:04:02PM -0800, Nicolin Chen wrote:
> An S2 nest_parent domain can be shared across vSMMUs in the same VM, since
> the S2 domain is basically the IPA mappings for the entire RAM of the VM.
> 
> Meanwhile, each vSMMU can have its own VMID, so the VMID allocation should
> be done per vSMMU instance v.s. per S2 nest_parent domain.
> 
> However, an S2 domain can be also allocated when a physical SMMU instance
> doesn't support S1. So, the structure has to retain the s2_cfg and vmid.
> 
> Allocate a vmid for a vSMMU instance in arm_vsmmu_alloc() and add a proper
> arm_vsmmu_destroy() to clean it up.
> 
> Add a per-domain "vsmmus" list pairing with a spinlock, maintaining a list
> on the S2 parent domain, to iterate S2 invalidations over the vmids across
> the vSMMU instances created for the same VM.
> 
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 10 +++-
>  .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     | 35 ++++++++++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 47 +++++++++++++++----
>  3 files changed, 79 insertions(+), 13 deletions(-)
> 
> 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 3336d196062c..1f6696bc4f6c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -849,8 +849,12 @@ struct arm_smmu_domain {
>  
>  	enum arm_smmu_domain_stage	stage;
>  	union {
> -		struct arm_smmu_ctx_desc	cd;
> -		struct arm_smmu_s2_cfg		s2_cfg;
> +		struct arm_smmu_ctx_desc cd;	/* S1 */
> +		struct arm_smmu_s2_cfg s2_cfg;	/* S2 && !nest_parent */
> +		struct {			/* S2 && nest_parent */
> +			struct list_head list;
> +			spinlock_t lock;
> +		} vsmmus;
>  	};
>  
>  	struct iommu_domain		domain;
> @@ -1049,6 +1053,8 @@ struct arm_vsmmu {
>  	struct arm_smmu_device *smmu;
>  	struct arm_smmu_domain *s2_parent;
>  	u16 vmid;
> +
> +	struct list_head vsmmus_elm; /* arm_smmu_domain::vsmmus::list */
>  };
>  
>  #if IS_ENABLED(CONFIG_ARM_SMMU_V3_IOMMUFD)
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> index ff8b550159f2..2c5a9d0abed5 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> @@ -30,6 +30,23 @@ void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type)
>  	return info;
>  }
>  
> +static void arm_vsmmu_destroy(struct iommufd_viommu *viommu)
> +{
> +	struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core);
> +	struct arm_smmu_device *smmu = vsmmu->smmu;
> +	struct arm_smmu_cmdq_ent cmd = {
> +		.opcode = CMDQ_OP_TLBI_S12_VMALL,
> +		.tlbi.vmid = vsmmu->vmid,
> +	};
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vsmmu->s2_parent->vsmmus.lock, flags);
> +	list_del(&vsmmu->vsmmus_elm);
> +	spin_unlock_irqrestore(&vsmmu->s2_parent->vsmmus.lock, flags);
> +	arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> +	ida_free(&smmu->vmid_map, vsmmu->vmid);
> +}
> +
>  static void arm_smmu_make_nested_cd_table_ste(
>  	struct arm_smmu_ste *target, struct arm_smmu_master *master,
>  	struct arm_smmu_nested_domain *nested_domain, bool ats_enabled)
> @@ -337,6 +354,7 @@ static int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
>  }
>  
>  static const struct iommufd_viommu_ops arm_vsmmu_ops = {
> +	.destroy = arm_vsmmu_destroy,
>  	.alloc_domain_nested = arm_vsmmu_alloc_domain_nested,
>  	.cache_invalidate = arm_vsmmu_cache_invalidate,
>  };
> @@ -351,6 +369,8 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
>  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>  	struct arm_smmu_domain *s2_parent = to_smmu_domain(parent);
>  	struct arm_vsmmu *vsmmu;
> +	unsigned long flags;
> +	int vmid;
>  
>  	if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
>  		return ERR_PTR(-EOPNOTSUPP);
> @@ -381,15 +401,24 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
>  	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
>  		return ERR_PTR(-EOPNOTSUPP);
>  
> +	vmid = ida_alloc_range(&smmu->vmid_map, 1, (1 << smmu->vmid_bits) - 1,
> +			       GFP_KERNEL);
> +	if (vmid < 0)
> +		return ERR_PTR(vmid);
> +

Probably a basic question, I hope we'll have one vSMMU per VM? Even
if that's not the case then the VMM should take care of invalidating
contexts of all associated vSMMUs anyway? (Just thinking if we should
allocate a VMID per VM or per vSMMU)

Nit: Does it makes sense to create a helper like `arm_smmu_vmid_alloc`
and call it here and finalise_s2?

>  	vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, core,
>  				     &arm_vsmmu_ops);
> -	if (IS_ERR(vsmmu))
> +	if (IS_ERR(vsmmu)) {
> +		ida_free(&smmu->vmid_map, vmid);
>  		return ERR_CAST(vsmmu);
> +	}
>  
>  	vsmmu->smmu = smmu;
> +	vsmmu->vmid = (u16)vmid;
>  	vsmmu->s2_parent = s2_parent;
> -	/* FIXME Move VMID allocation from the S2 domain allocation to here */
> -	vsmmu->vmid = s2_parent->s2_cfg.vmid;
> +	spin_lock_irqsave(&s2_parent->vsmmus.lock, flags);
> +	list_add_tail(&vsmmu->vsmmus_elm, &s2_parent->vsmmus.list);
> +	spin_unlock_irqrestore(&s2_parent->vsmmus.lock, flags);
>  
>  	return &vsmmu->core;
>  }
> 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 0462eb1b2912..addc6308742b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2249,10 +2249,22 @@ static void arm_smmu_tlb_inv_context(void *cookie)
>  	 */
>  	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>  		arm_smmu_tlb_inv_asid(smmu, smmu_domain->cd.asid);
> -	} else {
> +	} else if (!smmu_domain->nest_parent) {
>  		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
>  		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
>  		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> +	} else {
> +		struct arm_vsmmu *vsmmu, *next;
> +		unsigned long flags;
> +
> +		cmd.opcode = CMDQ_OP_TLBI_S12_VMALL;
> +		spin_lock_irqsave(&smmu_domain->vsmmus.lock, flags);
> +		list_for_each_entry_safe(vsmmu, next, &smmu_domain->vsmmus.list,
> +					 vsmmus_elm) {
> +			cmd.tlbi.vmid = vsmmu->vmid;
> +			arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);

Shouldn't this be vsmmu->smmu?

> +		}
> +		spin_unlock_irqrestore(&smmu_domain->vsmmus.lock, flags);
>  	}
>  	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
>  }
> @@ -2342,19 +2354,33 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
>  		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 {
> +		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
> +					 smmu_domain);
> +	} else if (!smmu_domain->nest_parent) {
>  		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);
> +		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
> +					 smmu_domain);
> +	} else {
> +		struct arm_vsmmu *vsmmu, *next;
> +		unsigned long flags;
>  
> -	if (smmu_domain->nest_parent) {

Minor Nit: IMO, an explicit like this clarifies it better. I think we 
can keep this add gotos for the __arm_smmu_tlb_inv_range calls above?
(Like the arm_smmu_domain_finalise_s2 changes below).

>  		/*
>  		 * When the S2 domain changes all the nested S1 ASIDs have to be
>  		 * flushed too.
>  		 */
>  		cmd.opcode = CMDQ_OP_TLBI_NH_ALL;
>  		arm_smmu_cmdq_issue_cmd_with_sync(smmu_domain->smmu, &cmd);
> +
> +		cmd.opcode = CMDQ_OP_TLBI_S2_IPA;
> +		spin_lock_irqsave(&smmu_domain->vsmmus.lock, flags);
> +		list_for_each_entry_safe(vsmmu, next, &smmu_domain->vsmmus.list,
> +					 vsmmus_elm) {
> +			cmd.tlbi.vmid = vsmmu->vmid;
> +			__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
> +						 smmu_domain);
> +		}
> +		spin_unlock_irqrestore(&smmu_domain->vsmmus.lock, flags);
>  	}
>  
>  	/*
> @@ -2477,7 +2503,7 @@ static void arm_smmu_domain_free_paging(struct iommu_domain *domain)
>  		mutex_lock(&arm_smmu_asid_lock);
>  		xa_erase(&arm_smmu_asid_xa, smmu_domain->cd.asid);
>  		mutex_unlock(&arm_smmu_asid_lock);
> -	} else {
> +	} else if (!smmu_domain->nest_parent) {
>  		struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
>  		if (cfg->vmid)
>  			ida_free(&smmu->vmid_map, cfg->vmid);
> @@ -2506,7 +2532,10 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_device *smmu,
>  				       struct arm_smmu_domain *smmu_domain)
>  {
>  	int vmid;
> -	struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;

Is this really required? I see we're still doing the same thing for
the nest_parent == false case.. we'll anyway return without doing much
if (smmu_domain->nest_parent)

> +
> +	/* nest_parent stores vmid in vSMMU instead of a shared S2 domain */
> +	if (smmu_domain->nest_parent)
> +		return 0;
>  
>  	/* Reserve VMID 0 for stage-2 bypass STEs */
>  	vmid = ida_alloc_range(&smmu->vmid_map, 1, (1 << smmu->vmid_bits) - 1,
> @@ -2514,7 +2543,7 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_device *smmu,
>  	if (vmid < 0)
>  		return vmid;
>  
> -	cfg->vmid	= (u16)vmid;
> +	smmu_domain->s2_cfg.vmid = (u16)vmid;
>  	return 0;
>  }
>  
> @@ -3233,6 +3262,8 @@ arm_smmu_domain_alloc_paging_flags(struct device *dev, u32 flags,
>  		}
>  		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
>  		smmu_domain->nest_parent = true;
> +		INIT_LIST_HEAD(&smmu_domain->vsmmus.list);
> +		spin_lock_init(&smmu_domain->vsmmus.lock);
>  		break;
>  	case IOMMU_HWPT_ALLOC_DIRTY_TRACKING:
>  	case IOMMU_HWPT_ALLOC_DIRTY_TRACKING | IOMMU_HWPT_ALLOC_PASID:
> -- 

Thanks,
Praan

> 2.43.0
> 
> 

Re: [PATCH v1 3/4] iommu/arm-smmu-v3: Decouple vmid from S2 nest_parent domain

[Jason Gunthorpe]

On Mon, Apr 07, 2025 at 10:51:24AM +0000, Pranjal Shrivastava wrote:
> > @@ -381,15 +401,24 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
> >  	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
> >  		return ERR_PTR(-EOPNOTSUPP);
> >  
> > +	vmid = ida_alloc_range(&smmu->vmid_map, 1, (1 << smmu->vmid_bits) - 1,
> > +			       GFP_KERNEL);
> > +	if (vmid < 0)
> > +		return ERR_PTR(vmid);
> > +
> 
> Probably a basic question, I hope we'll have one vSMMU per VM? 

A VIOMMU is tied to the physical SMMU, it cannot be shared across
physical SMMU, so this is the right sort of way to get the ID

> Even if that's not the case then the VMM should take care of
> invalidating contexts of all associated vSMMUs anyway? (Just
> thinking if we should allocate a VMID per VM or per vSMMU)

If the VMM wants to present a single vSMMU to the VM then the VMM
needs to replicate invalidations as required to all the physical
VIOMMU objects. This will prevent using the HW accelerated
invalidation paths, so I expect that the VMM will have one vSMM per
physical.

> Nit: Does it makes sense to create a helper like `arm_smmu_vmid_alloc`
> and call it here and finalise_s2?

Maybe so

Jason

Re: [PATCH v1 3/4] iommu/arm-smmu-v3: Decouple vmid from S2 nest_parent domain

[Pranjal Shrivastava]

On Mon, Apr 07, 2025 at 01:52:20PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 07, 2025 at 10:51:24AM +0000, Pranjal Shrivastava wrote:
> > > @@ -381,15 +401,24 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
> > >  	    !(smmu->features & ARM_SMMU_FEAT_S2FWB))
> > >  		return ERR_PTR(-EOPNOTSUPP);
> > >  
> > > +	vmid = ida_alloc_range(&smmu->vmid_map, 1, (1 << smmu->vmid_bits) - 1,
> > > +			       GFP_KERNEL);
> > > +	if (vmid < 0)
> > > +		return ERR_PTR(vmid);
> > > +
> > 
> > Probably a basic question, I hope we'll have one vSMMU per VM? 
> 
> A VIOMMU is tied to the physical SMMU, it cannot be shared across
> physical SMMU, so this is the right sort of way to get the ID
> 
> > Even if that's not the case then the VMM should take care of
> > invalidating contexts of all associated vSMMUs anyway? (Just
> > thinking if we should allocate a VMID per VM or per vSMMU)
> 
> If the VMM wants to present a single vSMMU to the VM then the VMM
> needs to replicate invalidations as required to all the physical
> VIOMMU objects. This will prevent using the HW accelerated
> invalidation paths, so I expect that the VMM will have one vSMM per
> physical.
> 

Makes sense. Thanks!

> > Nit: Does it makes sense to create a helper like `arm_smmu_vmid_alloc`
> > and call it here and finalise_s2?
> 
> Maybe so
> 

I recently saw Shameer's patch [1] using a different vmid allocation
scheme, so I guess it's okay if we don't share this function..


Thanks,
Praan

[1] https://lore.kernel.org/linux-arm-kernel/20250319173202.78988-5-shameerali.kolothum.thodi@huawei.com/

Re: [PATCH v1 3/4] iommu/arm-smmu-v3: Decouple vmid from S2 nest_parent domain

[Nicolin Chen]

On Mon, Apr 07, 2025 at 10:51:24AM +0000, Pranjal Shrivastava wrote:
> On Tue, Mar 04, 2025 at 09:04:02PM -0800, Nicolin Chen wrote:
> > @@ -2249,10 +2249,22 @@ static void arm_smmu_tlb_inv_context(void *cookie)
> >  	 */
> >  	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> >  		arm_smmu_tlb_inv_asid(smmu, smmu_domain->cd.asid);
> > -	} else {
> > +	} else if (!smmu_domain->nest_parent) {
> >  		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
> >  		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
> >  		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> > +	} else {
> > +		struct arm_vsmmu *vsmmu, *next;
> > +		unsigned long flags;
> > +
> > +		cmd.opcode = CMDQ_OP_TLBI_S12_VMALL;
> > +		spin_lock_irqsave(&smmu_domain->vsmmus.lock, flags);
> > +		list_for_each_entry_safe(vsmmu, next, &smmu_domain->vsmmus.list,
> > +					 vsmmus_elm) {
> > +			cmd.tlbi.vmid = vsmmu->vmid;
> > +			arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> 
> Shouldn't this be vsmmu->smmu?

Yes. I had fixed that locally after I sent this..

> > @@ -2342,19 +2354,33 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
> >  		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 {
> > +		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
> > +					 smmu_domain);
> > +	} else if (!smmu_domain->nest_parent) {
> >  		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);
> > +		__arm_smmu_tlb_inv_range(&cmd, iova, size, granule,
> > +					 smmu_domain);
> > +	} else {
> > +		struct arm_vsmmu *vsmmu, *next;
> > +		unsigned long flags;
> >  
> > -	if (smmu_domain->nest_parent) {
> 
> Minor Nit: IMO, an explicit like this clarifies it better. I think we 
> can keep this add gotos for the __arm_smmu_tlb_inv_range calls above?
> (Like the arm_smmu_domain_finalise_s2 changes below).

I've reworked this part. It looks like this now:
	 if (smmu_domain->nest_parent) {
		 return arm_smmu_s2_parent_tlb_inv_range(smmu_domain, iova, size,
							 granule, leaf);
	 }

	 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;
	 }
	 __arm_smmu_tlb_inv_range(smmu_domain->smmu, &cmd, iova, size, granule,
				  &smmu_domain->domain);

> > @@ -2506,7 +2532,10 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_device *smmu,
> >  				       struct arm_smmu_domain *smmu_domain)
> >  {
> >  	int vmid;
> > -	struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
> 
> Is this really required? I see we're still doing the same thing for
> the nest_parent == false case.. we'll anyway return without doing much
> if (smmu_domain->nest_parent)

It's clearer and safer to reference S2_cfg after the "if" below.

> > +
> > +	/* nest_parent stores vmid in vSMMU instead of a shared S2 domain */
> > +	if (smmu_domain->nest_parent)
> > +		return 0;

Thanks
Nicolin