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