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

Mostafa Saleh
2024-02-13T17:01:09Z

On Tue, Feb 13, 2024 at 12:37:39PM -0400, Jason Gunthorpe wrote:
> 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?

Ahh, I see, as iommu_get_domain_for_dev() now returns a valid domain.
Thanks for the explanation, that makes sense.

Reviewed-by: Mostafa Saleh <smostafa@google.com>

Thanks,
Mostafa