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

From: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	Joerg Roedel <joro@8bytes.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>, Moritz Fischer <mdf@kernel.org>,
	"Moritz Fischer" <moritzf@google.com>,
	Michael Shavit <mshavit@google.com>,
	"Nicolin Chen" <nicolinc@nvidia.com>,
	"patches@lists.linux.dev" <patches@lists.linux.dev>
Subject: RE: [PATCH v4 12/16] iommu/arm-smmu-v3: Add a global static IDENTITY domain
Date: Tue, 30 Jan 2024 08:35:15 +0000	[thread overview]
Message-ID: <84bb5ddba8b04f28b4cec5cb7a544817@huawei.com> (raw)
In-Reply-To: <20240129183750.GZ1455070@nvidia.com>

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Monday, January 29, 2024 6:38 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>; Moritz Fischer <mdf@kernel.org>; Moritz Fischer
> <moritzf@google.com>; Michael Shavit <mshavit@google.com>; Nicolin Chen
> <nicolinc@nvidia.com>; patches@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@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