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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, January 31, 2024 2:12 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: iommu@lists.linux.dev; Joerg Roedel <joro@8bytes.org>; linux-arm-
> kernel@lists.infradead.org; Robin Murphy <robin.murphy@arm.com>; Will
> Deacon <will@kernel.org>; Eric Auger <eric.auger@redhat.com>; Jean-
> Philippe Brucker <jean-philippe@linaro.org>; Moritz Fischer
> <mdf@kernel.org>; Michael Shavit <mshavit@google.com>; Nicolin Chen
> <nicolinc@nvidia.com>; patches@lists.linux.dev
> Subject: Re: [PATCH v4 14/27] iommu/arm-smmu-v3: Make changing
> domains be hitless for ATS
> 
> On Wed, Jan 31, 2024 at 11:20:48AM +0000, Shameerali Kolothum Thodi
> wrote:
> 
> > > +static void arm_smmu_attach_commit(struct arm_smmu_master
> *master,
> > > +				   struct attach_state *state)
> > > +{
> > > +	lockdep_assert_held(&arm_smmu_asid_lock);
> > > +
> > > +	if (state->want_ats && !master->ats_enabled) {
> > > +		arm_smmu_enable_ats(master);
> > > +	} else if (master->ats_enabled) {
> > > +		/*
> > > +		 * The translation has changed, flush the ATC. At this point
> the
> > > +		 * SMMU is translating for the new domain and both the
> > > old&new
> > > +		 * domain will issue invalidations.
> > > +		 */
> > > +		arm_smmu_atc_inv_master(master);
> > > +	}
> > > +	master->ats_enabled = state->want_ats;
> > > +
> > > +	arm_smmu_remove_master_domain(master,
> > > +				      iommu_get_domain_for_dev(master-
> > > >dev));
> >
> > I think the core has not yet set the group->domain since we are still in
> attach().
> 
> Any chance you are not running exactly v4? There was an earlier
> version on github that had exactly what you said, Nicolin
> found it and it was fixed before v4 was posted:
> 
> static struct arm_smmu_domain *
> to_smmu_domain_devices(struct iommu_domain *domain)
> {
>         /* The domain can be NULL only when processing the first attach */
>         if (!domain)
>                 return NULL;

Ah...I think I am on an older version as the above check is missing. That should
fix it.

Thanks,
Shameer.