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

[Michael Shavit]

On Thu, Jun 20, 2024 at 2:43 AM Jason Gunthorpe <jgg at nvidia.com> wrote:
>
> 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

Looks good to me.