iommu/arm-smmu-v3: Allocate vmid per vsmmu instead of s2_parent

-----Original Message-----
From: Nicolin Chen <[email protected]>
Sent: Wednesday, March 5, 2025 5:04 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Shameerali Kolothum Thodi <[email protected]>
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 <[email protected]>

---
 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(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0 -0

index bd9d7c85576a..e08c4ede4b2d 100644

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0 -0

index bd9d7c85576a..e08c4ede4b2d 100644
@@ -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,

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_entry_writer_ops { · line 891

Quoted 5 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -1

 				 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.

Quoted 8 lines · Shameerali Kolothum Thodi

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0 -0

index bd9d7c85576a..e08c4ede4b2d 100644
@@ -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,

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_entry_writer_ops { · line 891

Quoted 5 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -1

 				 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

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 <[email protected]>

---
 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(-)

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 <[email protected]>

---
 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(-)

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

Signed-off-by Nicolin Chen <[email protected]>

---
 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(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -0

index e08c4ede4b2d..3336d196062c 100644
@@ -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);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -2

index 310bb4109ec9..0462eb1b2912 100644
@@ -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);
 }

Signature · 1 line

2.43.0

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -1

@@ -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;

Quoted 11 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -1

@@ -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;

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_tlb_inv_context(void *cookie) · line 2265

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -0

+		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);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_tlb_inv_context(void *cookie) · line 2268

Quoted 3 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -0

+		}
+		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

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_tlb_inv_context(void *cookie) · line 2269

Quoted 1 line

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0

 	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.

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 <[email protected]>

---
 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(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +8 -2

index 3336d196062c..1f6696bc4f6c 100644
@@ -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)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +20 -0

index ff8b550159f2..2c5a9d0abed5 100644
@@ -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);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +7 -3

 	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;
 }

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -1

index 0462eb1b2912..addc6308742b 100644
@@ -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);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5 -1

+		}
+		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;

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +11 -1

 		/*
 		 * 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);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5 -1

+
+	/* 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;
 }
 

2.43.0

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, · line 408

Quoted 9 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +5 -0

@@ -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?

Quoted 11 lines · Pranjal Shrivastava

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, · line 408

Quoted 9 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +5 -0

@@ -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

Quoted 3 lines

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.

Quoted 2 lines

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

Maybe so

Quoted 13 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5 -1

@@ -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;
+

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_tlb_inv_context(void *cookie) · line 2265

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +6 -0

+		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?

Quoted 18 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -3

@@ -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;
 

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size, · line 2351

Quoted 1 line

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -1

-	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).

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_domain_finalise_s2(struct arm_smmu_device *smmu, · line 2509

Quoted 5 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -1

@@ -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)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_domain_finalise_s2(struct arm_smmu_device *smmu, · line 2538

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -0

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

-----Original Message-----
From: Nicolin Chen <[email protected]>
Sent: Wednesday, March 5, 2025 5:04 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Shameerali Kolothum Thodi <[email protected]>
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-

Quoted 1 line

smmu.

Remove the validation line to allow this use case.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +0 -0

index 2c5a9d0abed5..9bfa5fa5bafa 100644

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, · line 383

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +0 -3

 	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?

Quoted 9 lines · Shameerali Kolothum Thodi

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev, · line 383

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +0 -3

 	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.

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.

Quoted 6 lines · Nicolin Chen

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..

Quoted 9 lines · Jason Gunthorpe

Quoted 6 lines · Nicolin Chen

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.

Quoted 4 lines

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?

Quoted 15 lines · Nicolin Chen

Quoted 13 lines · Jason Gunthorpe

Quoted 6 lines · Nicolin Chen

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?

Quoted 2 lines

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.

Quoted 2 lines

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?

Quoted 5 lines · Nicolin Chen

Quoted 2 lines

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

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

Quoted 3 lines

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.