Re: [PATCH v7 3/9] iommu/arm-smmu-v3: Move the CD generation for S1 domains into a function

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Apr 19, 2024 at 09:10:59PM +0000, Mostafa Saleh wrote:
> Hi Jason,
> 
> On Tue, Apr 16, 2024 at 04:28:14PM -0300, Jason Gunthorpe wrote:
> > Introduce arm_smmu_make_s1_cd() to build the CD from the paging S1 domain,
> > and reorganize all the places programming S1 domain CD table entries to
> > call it.
> > 
> > Split arm_smmu_update_s1_domain_cd_entry() from
> > arm_smmu_update_ctx_desc_devices() so that the S1 path has its own call
> > chain separate from the unrelated SVA path.
> > 
> > arm_smmu_update_s1_domain_cd_entry() only works on S1 domains
> > attached to RIDs and refreshes all their CDs.
> > 
> > Remove the forced clear of the CD during S1 domain attach,
> > arm_smmu_write_cd_entry() will do this automatically if necessary.
> > 
> > Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Reviewed-by: Michael Shavit <mshavit@google.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 25 +++++++-
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 60 +++++++++++++------
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  9 +++
> >  3 files changed, 76 insertions(+), 18 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 41b44baef15e80..d159f60480935e 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
> > @@ -53,6 +53,29 @@ static void arm_smmu_update_ctx_desc_devices(struct arm_smmu_domain *smmu_domain
> >  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> >  }
> >  
> > +static void
> > +arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain)
> 
> nit: shouldn’t that be arm_smmu_update_sva_domain_cd_entry?

No, that actually was my same confusion too when I was first looking
at this. The logic updates a *S1* domain's CD, it doesn't touch a SVA
CD or a SVA domain.

It actually has nothing to do with SVA, this is part of BTM support to
change the ASID in already programmed S1 domains.

> > +{
> > +	struct arm_smmu_master *master;
> > +	struct arm_smmu_cd target_cd;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> > +	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> > +		struct arm_smmu_cd *cdptr;
> > +
> > +		/* S1 domains only support RID attachment right now */
> > +		cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID);
> > +		if (WARN_ON(!cdptr))
> > +			continue;
> > +
> > +		arm_smmu_make_s1_cd(&target_cd, master, smmu_domain);
> > +		arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr,
> > +					&target_cd);
> 
> Case ARM_SMMU_DOMAIN_S1 has the some code:
>   arm_smmu_get_cd_pter => arm_smmu_make_s1_cd => arm_smmu_write_cd_entry
> I’d prefer if that was abstracted with the SMMUv3 driver and it provides a higher
> level API rather than exposing these low-level functions in the header file.
> But no strong opinion.

It is only slightly the same now, and it will keep getting more
different as the patches progress. For instance "Make
arm_smmu_alloc_cd_ptr()" makes them call different alloc functions.

Later on this code will handle a SSID too.

I don't think of those functions as a lower level API, ptr/make/write
is the API design. We have different versions of each of those
functions. The call site needs to string together the right sequence
of three operations for its specific context.

At the end this is an atomic context working on S1 domains with SSID -
there isn't another case exactly like this.

> > +void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,
> > +			 struct arm_smmu_master *master,
> > +			 struct arm_smmu_domain *smmu_domain)
> > +{
> > +	struct arm_smmu_ctx_desc *cd = &smmu_domain->cd;
> > +
> > +	memset(target, 0, sizeof(*target));
> > +
> > +	target->data[0] = cpu_to_le64(
> > +		cd->tcr |
> > +#ifdef __BIG_ENDIAN
> > +		CTXDESC_CD_0_ENDI |
> > +#endif
> > +		CTXDESC_CD_0_V |
> > +		CTXDESC_CD_0_AA64 |
> > +		(master->stall_enabled ? CTXDESC_CD_0_S : 0) |
> > +		CTXDESC_CD_0_R |
> > +		CTXDESC_CD_0_A |
> > +		CTXDESC_CD_0_ASET |
> > +		FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid)
> > +		);
> > +
> > +	target->data[1] = cpu_to_le64(cd->ttbr & CTXDESC_CD_1_TTB0_MASK);
> > +	target->data[3] = cpu_to_le64(cd->mair);
> > +}
> > +
> 
> IMO, patches to handle CD = NULL and quiet CD should be introduced first so it is
> easier to follow as now there is duplicate code in arm_smmu_write_ctx_desc() which
> is dead and makes it a little harder to review, but if reordered,
> arm_smmu_write_ctx_desc() can be removed in this patch so we can see how code moved.

arm_smmu_write_ctx_desc() can't be removed until all of S1, clear, SVA
and quiet_cd are converted. No matter what order you pick there will
be some weirdness.

The duplicate code "(1) and (2)" is also still being used for the SVA
domains, it is not unused until patch "Move the CD generation for SVA
into a function".

The only dead code here is the ASID change. So I'll brung this hunk forward:

--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1328,14 +1328,11 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
         *
         * (1) Install primary CD, for normal DMA traffic (SSID = IOMMU_NO_PASID = 0).
         * (2) Install a secondary CD, for SID+SSID traffic.
-        * (3) Update ASID of a CD. Atomically write the first 64 bits of the
-        *     CD, then invalidate the old entry and mappings.
         * (4) Quiesce the context without clearing the valid bit. Disable
         *     translation, and ignore any translation fault.
         * (5) Remove a secondary CD.
         */
        u64 val;
-       bool cd_live;
        struct arm_smmu_cd target;
        struct arm_smmu_cd *cdptr = &target;
        struct arm_smmu_cd *cd_table_entry;
@@ -1351,7 +1348,6 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
 
        target = *cd_table_entry;
        val = le64_to_cpu(cdptr->data[0]);
-       cd_live = !!(val & CTXDESC_CD_0_V);
 
        if (!cd) { /* (5) */
                val = 0;
@@ -1359,13 +1355,6 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
                if (!(smmu->features & ARM_SMMU_FEAT_STALL_FORCE))
                        val &= ~(CTXDESC_CD_0_S | CTXDESC_CD_0_R);
                val |= CTXDESC_CD_0_TCR_EPD0;
-       } else if (cd_live) { /* (3) */
-               val &= ~CTXDESC_CD_0_ASID;
-               val |= FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid);
-               /*
-                * Until CD+TLB invalidation, both ASIDs may be used for tagging
-                * this substream's traffic
-                */
        } else { /* (1) and (2) */
                cdptr->data[1] = cpu_to_le64(cd->ttbr & CTXDESC_CD_1_TTB0_MASK);
                cdptr->data[2] = 0;

> Otherwise:
> Reviewed-by: Mostafa Saleh <smostafa@google.com>

Thanks,
Jason

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