[PATCH v4 12/16] iommu/arm-smmu-v3: Add a global static IDENTITY domain

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Jason Gunthorpe <jgg at nvidia.com>
> Sent: Monday, January 29, 2024 6:38 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>
> Cc: iommu at lists.linux.dev; Joerg Roedel <joro at 8bytes.org>; linux-arm-
> kernel at lists.infradead.org; Robin Murphy <robin.murphy at arm.com>; Will
> Deacon <will at kernel.org>; Moritz Fischer <mdf at kernel.org>; Moritz Fischer
> <moritzf at google.com>; Michael Shavit <mshavit at google.com>; Nicolin Chen
> <nicolinc at nvidia.com>; patches at lists.linux.dev
> Subject: Re: [PATCH v4 12/16] iommu/arm-smmu-v3: Add a global static
> IDENTITY domain
> 
> On Mon, Jan 29, 2024 at 06:11:48PM +0000, Shameerali Kolothum Thodi
> wrote:
> 
> > > @@ -3056,6 +3089,7 @@ static void
> arm_smmu_remove_dev_pasid(struct
> > > device *dev, ioasid_t pasid)
> > >  }
> > >
> > >  static struct iommu_ops arm_smmu_ops = {
> > > +	.identity_domain	= &arm_smmu_identity_domain,
> >
> > This seems to create a problem when we have set the identity domain and
> > try to enable sva for the device. Since there is no smmu_domain for this
> case
> > and there is no specific domain type checking in iommu_sva_bind_device()
> path,
> > it eventually crashes(hangs in my test) in,
> 
> Yeah, that is a longstanding issue in the SVA implementation, it only
> works if the RID is set to a S1 paging domain.
> 
> I cleaned it up here so that the SVA series was cleaer:
> 
> https://lore.kernel.org/linux-iommu/1-v4-e7091cdd9e8d+43b1-
> smmuv3_newapi_p2_jgg at nvidia.com/

Yes, this will do. But I think it is not complete. I will comment on that one.

> 
> > iommu_sva_bind_device()
> >    ...
> >       arm_smmu_sva_set_dev_pasid()
> >         __arm_smmu_sva_bind()
> >            arm_smmu_mmu_notifier_get(smmu_domain, ..)  --> never exit the
> mmu notifier list loop.
> >
> > I think we should check for the domain type in iommu_sva_bind_device()
> or later
> > before trying to use smmu_domain.  At present(ie, without this series) it
> returns error
> > while we are trying to write the CD. But that looks too late as well.
> 
> Oh wow, is that how it worked? OK, I figured it was just broken but if
> there was some error code that happened indirectly then lets've move
> the above patch ahead of this one.

Yes, indirectly indeed :)
https://elixir.bootlin.com/linux/v6.8-rc2/source/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c#L1068

Thanks,
Shameer