[PATCH v2 15/19] iommu/arm-smmu-v3: Add a global static IDENTITY domain

[Jason Gunthorpe]

On Tue, Dec 05, 2023 at 10:21:18AM -0800, Nicolin Chen wrote:
> On Tue, Dec 05, 2023 at 01:42:19PM -0400, Jason Gunthorpe wrote:
> > On Tue, Dec 05, 2023 at 09:25:59AM -0800, Nicolin Chen wrote:
> > > On Tue, Dec 05, 2023 at 10:37:42AM -0400, Jason Gunthorpe wrote:
> > > > On Mon, Dec 04, 2023 at 08:28:23PM -0800, Nicolin Chen wrote:
> > > > 
> > > > > > +static int arm_smmu_attach_dev_ste(struct device *dev,
> > > > > > +				   struct arm_smmu_ste *ste)
> > > > > > +{
> > > > > > +	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> > > > > > +
> > > > > > +	if (arm_smmu_master_sva_enabled(master))
> > > > > > +		return -EBUSY;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Do not allow any ASID to be changed while are working on the STE,
> > > > > > +	 * otherwise we could miss invalidations.
> > > > > > +	 */
> > > > > > +	mutex_lock(&arm_smmu_asid_lock);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * The SMMU does not support enabling ATS with bypass/abort. When the
> > > > > > +	 * STE is in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests
> > > > > > +	 * and Translated transactions are denied as though ATS is disabled for
> > > > > > +	 * the stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and
> > > > > > +	 * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry).
> > > > > > +	 */
> > > > > > +	arm_smmu_detach_dev(master);
> > > > > > +
> > > > > > +	arm_smmu_install_ste_for_dev(master, ste);
> > > > > > +	mutex_unlock(&arm_smmu_asid_lock);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * This has to be done after removing the master from the
> > > > > > +	 * arm_smmu_domain->devices to avoid races updating the same context
> > > > > > +	 * descriptor from arm_smmu_share_asid().
> > > > > > +	 */
> > > > > > +	if (master->cd_table.cdtab)
> > > > > > +		arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, NULL);
> > > > >  
> > > > > This arm_smmu_write_ctx_desc was previously within the asid lock
> > > > > protection, yet now it's moved out of that?
> > > > 
> > > > Yes, arm_smmu_write_ctx_desc() updates a CD table entry and that does
> > > > not need ASID lock protection. The ASID lock exists because of the BTM
> > > > code rewriting STEs asyncronously.
> > > 
> > > I see. Thanks for elaborating. For this patch
> > 
> > Actually wait, that explanation is not right..
> > 
> > The BTM code is changing the ASID which is done with a CD update
> > 
> > The ordering is OK here because the BTM code iterates over the
> > &smmu_domain->devices list.
> > 
> > The arm_smmu_detach_dev() has removed the master from the devices list
> > under a lock so the BTM code won't see this.
> > 
> > Thus there is no race between the arm_smmu_share_asid() flow and this
> > code, indeed we've already removed the cdtable from the STE at this
> > point.
> 
> I see. Maybe worth mentioning this in the comments above or commit
> message?

It does have a comment:

 +	/*
 +	 * This has to be done after removing the master from the
 +	 * arm_smmu_domain->devices to avoid races updating the same context
 +	 * descriptor from arm_smmu_share_asid().
 +	 */
 
> Also, the arm_smmu_attach_dev_ste seems to be used by IDENTITY and
> BLOCK domains only. This turns its nature to be a cleanup function
> against a translate domain, while the naming sounds very generic.
> I cannot think of any better name than this one, yet maybe we can
> highlight this with a line of comments above the function?

I would not call it a cleanup, it installs a domain-less thing via a
raw STE

Jason