Re: [PATCH v5 07/27] iommu/arm-smmu-v3: Move the CD generation for S1 domains into a function

From: Jason Gunthorpe <jgg@nvidia.com>
To: Michael Shavit <mshavit@google.com>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>, Eric Auger <eric.auger@redhat.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Moritz Fischer <mdf@kernel.org>,
	Nicolin Chen <nicolinc@nvidia.com>,
	patches@lists.linux.dev,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH v5 07/27] iommu/arm-smmu-v3: Move the CD generation for S1 domains into a function
Date: Mon, 18 Mar 2024 15:11:15 -0300	[thread overview]
Message-ID: <20240318181115.GG5825@nvidia.com> (raw)
In-Reply-To: <CAKHBV25_FzGHY8zEsXtR=vsTi3k2F8Pm4nY9wYRKudJmnk2LWw@mail.gmail.com>

On Wed, Mar 13, 2024 at 08:13:12PM +0800, Michael Shavit wrote:
> > 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 bb9bb6fd7914ce..6acc65f6d00a71 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
> > @@ -54,6 +54,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)
> > +{
> > +       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);
> > +       }
> > +       spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> > +}
> > +
> >  /*
> >   * Check if the CPU ASID is available on the SMMU side. If a private context
> >   * descriptor is using it, try to replace it.
> > @@ -97,7 +120,7 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
> >          * be some overlap between use of both ASIDs, until we invalidate the
> >          * TLB.
> >          */
> > -       arm_smmu_update_ctx_desc_devices(smmu_domain, IOMMU_NO_PASID, cd);
> > +       arm_smmu_update_s1_domain_cd_entry(smmu_domain);
> 
> Not to be too nitpicky, but is calling this
> arm_smmu_update_s1_domain_cd_entry the right choice here?  Yes the RID
> domain has type "ARM_SMMU_DOMAIN_S1", but CDs are also stage 1
> translations.

A later patch will inline this function into
arm_smmu_realloc_s1_domain_asid(), so I wouldn't fuss too much over
the name..

But the name is trying to convey the two important details:
 1) The domain is an ARM_SMMU_DOMAIN_S1
 2) We are changing the CD Table Entry *not* the STE

One of the things that was really confusing about this code was
exactly what the smm_domain *was*, it is actually always a
ARM_SMMU_DOMAIN_S1.

Thanks,
Jason

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