Re: [PATCH v7 04/14] iommu/arm-smmu-v3: Make changing domains be hitless for ATS

[Jason Gunthorpe <jgg@nvidia.com>]

On Sat, May 11, 2024 at 02:56:41PM -0700, Nicolin Chen wrote:
> >  static void arm_smmu_v3_write_ste_test_s2_to_abort(struct kunit *test)
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 532fe17f28bfe5..01e970f6ee4363 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> 
> > -static void arm_smmu_disable_ats(struct arm_smmu_master *master,
> > -				 struct arm_smmu_domain *smmu_domain)
> > -{
> > -	if (!master->ats_enabled)
> > -		return;
> > -
> > -	pci_disable_ats(to_pci_dev(master->dev));
> > -	/*
> > -	 * Ensure ATS is disabled at the endpoint before we issue the
> > -	 * ATC invalidation via the SMMU.
> > -	 */
> > -	wmb();
> > -	arm_smmu_atc_inv_master(master);
> > -	atomic_dec(&smmu_domain->nr_ats_masters);
> > -}
> > -
> 
> Maybe we could keep the function for symmetry, since prepare()
> still calls pci_disable_ats() + wmb()?

I removed it mainly because it is not symmetrical. On the disable
side the work enable does is split up between prepare and commit.

> > +/*
> > + * Prepare to attach a domain to a master. If disable_ats is not set this will
> > + * turn on ATS if supported. smmu_domain can be NULL if the domain being
> 
> This function doesn't turn on ATS actually, but commit() does.

Lets write a nicer comment:

/*
 * Start the sequence to attach a domain to a master. The sequence contains three
 * steps:
 *  arm_smmu_attach_prepare()
 *  arm_smmu_install_ste_for_dev()
 *  arm_smmu_attach_commit()
 *
 * If prepare succeeds then the sequence must be completed. The STE installed
 * must set the STE.EATS field according to state.ats_enabled.
 *
 * ATS is automatically enabled if the underlying device supports it.
 * disable_ats can inhibit this to support STEs like bypass that don't allow
 * ATS.
 *
 * The change of the EATS in the STE and the PCI ATS config space is managed by
 * this sequence to be in the right order such that if PCI ATS is enabled then
 * STE.ETAS is enabled.
 *
 * new_domain can be NULL if the domain being attached does not have a page
 * table and does not require invalidation tracking, and does not support ATS.
 */

> Should it be more accurate "If disable_ats is set this will turn
> off ATS if enabled"?

After looking a bit I removed disable_ats. We can now tell that ATS is
not possible because smmu_domain = NULL.

> > + * attached does not have a page table and does not require invalidation
> > + * tracking.
> > + */
> > +static int arm_smmu_attach_prepare(struct arm_smmu_master *master,
> > +				   struct iommu_domain *domain,
> > +				   struct attach_state *state)
> > +{
> > +	struct arm_smmu_domain *smmu_domain =
> > +		to_smmu_domain_devices(domain);
> 
> This could fit into a single line.

Done

> > -static int arm_smmu_attach_dev_ste(struct device *dev,
> > -				   struct arm_smmu_ste *ste)
> > +static int arm_smmu_attach_dev_ste(struct iommu_domain *domain,
> > +				   struct device *dev, struct arm_smmu_ste *ste)
> 
> How about arm_smmu_domain_attach_dev_ste?

It is reflecting the ops name 'iommu_domain_ops attach_dev'

> >  void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
> > -			       struct arm_smmu_master *master);
> > +			       struct arm_smmu_master *master,
> > +			       bool ats_enabled);
> >  void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
> >  				 struct arm_smmu_master *master,
> > -				 struct arm_smmu_domain *smmu_domain);
> > +				 struct arm_smmu_domain *smmu_domain,
> > +				 bool ats_enabled);
> 
> We seem to pass state.want_ats all the time. So maybe just name
> it "bool want_ats", given that "ats_enabled" is only true after
> commit() that always comes after these two functions?

Lets rename want_ats to ats_enabled to have a consistent name.

Jason