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

[Michael Shavit]

On Wed, Oct 25, 2023 at 7:56 AM Jason Gunthorpe <jgg at nvidia.com> wrote:
>
> On Tue, Oct 24, 2023 at 04:09:06PM +0800, Michael Shavit wrote:
>
> > > +       /*
> > > +        * During prepare we want the current smmu_domain and new smmu_domain to
> > > +        * be in the devices list before we change any HW. This ensures that
> > > +        * both domains will send ATS invalidations to the master until we are
> > > +        * done.
> > > +        *
> > > +        * It is tempting to make this list only track masters that are using
> > > +        * ATS, but arm_smmu_share_asid() also uses this to change the ASID of a
> > > +        * domain, unrelated to ATS.
> > > +        */
> > > +       spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> > > +       cur_master_domain = arm_smmu_find_master_domain(smmu_domain, master);
> > > +       if (cur_master_domain) {
> > > +               kfree(master_domain);
> > > +               state->existing_master_domain = true;
> >
> > I'm confused, why would we expect to find that the domain is already
> > attached to the master???
>
> Yes, it can happen, I agree it is
> surprising. __iommu_group_set_domain_internal()'s error handling is
> designed to support the wonky drivers (like smmu before this series)
> which would destroy the domain attachment and then fail
> attach_dev().
>
> To accommodate this the error unwind for
> __iommu_group_set_domain_internal() deliberately resets the correct
> domain again, expecting good drivers to make it into a NOP and wonky
> drivers to restore the attachment they threw away.
>
> > If the iommu core really allows such attach() calls, can we check
> > for that earlier and simply return early from attach_dev()?
>
> We could, but that looked more complex.. We trade an extra if
> under arm_smmu_attach_commit() for an extra if in two callers.
>
> What do you think?

Huh. Can this be checked by calling iommu_get_domain_for_dev() and
comparing against the input iommu_domain parameter, or is
iommu_get_domain_for_dev() cleared in this scenario?