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

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Jun 19, 2024 at 06:20:56PM +0800, Michael Shavit wrote:
> > +/*
> > + * 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.
> 
> This comment is out of date since disable_ats was removed between v7 and v8.
> A nit, but "automatically" is also a little imprecise IMO (almost
> sounds like the device is automatically enabling it). How about:
> 
> + * ATS is enabled after the STE is installed if the new domain and
> underlying device
> + * supports it. On the other hand, ATS is disabled before installing
> the STE if it doesn't
> + * support ATS like bypass domains.
> 
> Or something else if that's too redundant with the next paragraph :) .
> 
> > + *
> > + * 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.
> > + */
> 
> This is also confusing, new_domain is never NULL. It's
> to_smmu_domain_devices(new_domain) that can be null.

Yes, the comment didn't survive some of the edits..

/*
 * 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.
 *
 * If the device supports ATS then this determines if EATS should be enabled
 * in the STE, and starts sequencing EATS disable if required.
 *
 * 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 so that if PCI ATS is enabled then
 * STE.ETAS is enabled.
 *
 * new_domain can be a non-paging domain. In this case ATS will not be enabled,
 * and invalidations won't be tracked.
 */

?

Thanks,
Jason