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

[Jason Gunthorpe <jgg@nvidia.com>]

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?

Jason