Re: [PATCH v5 11/17] iommu/arm-smmu-v3: Remove arm_smmu_master->domain

From: Jason Gunthorpe <jgg@nvidia.com>
To: Mostafa Saleh <smostafa@google.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>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Joerg Roedel <jroedel@suse.de>, Moritz Fischer <mdf@kernel.org>,
	Moritz Fischer <moritzf@google.com>,
	Michael Shavit <mshavit@google.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	patches@lists.linux.dev,
	Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>,
	Zhangfei Gao <zhangfei.gao@linaro.org>
Subject: Re: [PATCH v5 11/17] iommu/arm-smmu-v3: Remove arm_smmu_master->domain
Date: Tue, 13 Feb 2024 12:37:39 -0400	[thread overview]
Message-ID: <20240213163739.GE1088888@nvidia.com> (raw)
In-Reply-To: <ZcuOnnrWN3ZxJY8x@google.com>

On Tue, Feb 13, 2024 at 03:45:34PM +0000, Mostafa Saleh wrote:
> Hi Jason,
> 
> On Tue, Feb 06, 2024 at 11:12:48AM -0400, Jason Gunthorpe wrote:
> > Introducing global statics which are of type struct iommu_domain, not
> > struct arm_smmu_domain makes it difficult to retain
> > arm_smmu_master->domain, as it can no longer point to an IDENTITY or
> > BLOCKED domain.
> > 
> > The only place that uses the value is arm_smmu_detach_dev(). Change things
> > to work like other drivers and call iommu_get_domain_for_dev() to obtain
> > the current domain.
> > 
> > The master->domain is subtly protecting the domain_head against being
> > unused, change the domain_head to be INIT'd when the master is not
> > attached to a domain instead of garbage/zero.
> 
> I don't this the problem here, neither the reason for initialising the
> domain_head, can you please clarify the issue?

I didn't notice it either. Eric found it:

https://lore.kernel.org/linux-iommu/6fff20dd-46d5-4974-a4a5-fb4e7a59ce44@redhat.com/

> > @@ -2560,19 +2560,20 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
> >  
> >  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> >  {
> > +	struct iommu_domain *domain = iommu_get_domain_for_dev(master->dev);
> > +	struct arm_smmu_domain *smmu_domain;
> >  	unsigned long flags;
> > -	struct arm_smmu_domain *smmu_domain = master->domain;

master->domain is NULL here which happens in cases where the current
RID domain is not a PAGING domain.

> > -	if (!smmu_domain)
> > +	if (!domain)
> >  		return;

Which used to early exit

> >  
> > +	smmu_domain = to_smmu_domain(domain);
> >  	arm_smmu_disable_ats(master, smmu_domain);
> >  
> >  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> > -	list_del(&master->domain_head);
> > +	list_del_init(&master->domain_head);
> >  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);

But now would cause the list_del() to hit a non-inited list_head and
explode.

Instead we keep the list head init'd and the list_del is a NOP.

Tricky right??

I changed the comment like this:

The master->domain is subtly protecting the master->domain_head against
being unused as only PAGING domains will set master->domain and only
paging domains use the master->domain_head. To make it simple keep the
master->domain_head initialized so that the list_del() logic just does
nothing for non-PAGING domains.

OK?

Jason