diff mbox series

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

Message ID 12-v4-c93b774edcc4+42d2b-smmuv3_newapi_p1_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Update SMMUv3 to the modern iommu API (part 1/3) | expand

Commit Message

Jason Gunthorpe Jan. 25, 2024, 11:57 p.m. UTC 
Move to the new static global for identity domains. Move all the logic out
of arm_smmu_attach_dev into an identity only function.

Reviewed-by: Michael Shavit <mshavit@google.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Tested-by: Moritz Fischer <moritzf@google.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 82 +++++++++++++++------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -
 2 files changed, 58 insertions(+), 25 deletions(-)

Comments

Shameerali Kolothum Thodi Jan. 29, 2024, 6:11 p.m. UTC | #1 
> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, January 25, 2024 11:57 PM
> To: 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>
> Cc: Moritz Fischer <mdf@kernel.org>; Moritz Fischer <moritzf@google.com>;
> Michael Shavit <mshavit@google.com>; Nicolin Chen <nicolinc@nvidia.com>;
> patches@lists.linux.dev; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Subject: [PATCH v4 12/16] iommu/arm-smmu-v3: Add a global static
> IDENTITY domain
> 
> Move to the new static global for identity domains. Move all the logic out
> of arm_smmu_attach_dev into an identity only function.
> 
> Reviewed-by: Michael Shavit <mshavit@google.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Moritz Fischer <moritzf@google.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 82 +++++++++++++++--
> ----
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -
>  2 files changed, 58 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index f08cfa9b90b3eb..d35bf9655c9b1b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2226,8 +2226,7 @@ static struct iommu_domain
> *arm_smmu_domain_alloc(unsigned type)
>  		return arm_smmu_sva_domain_alloc();
> 
>  	if (type != IOMMU_DOMAIN_UNMANAGED &&
> -	    type != IOMMU_DOMAIN_DMA &&
> -	    type != IOMMU_DOMAIN_IDENTITY)
> +	    type != IOMMU_DOMAIN_DMA)
>  		return NULL;
> 
>  	/*
> @@ -2335,11 +2334,6 @@ static int arm_smmu_domain_finalise(struct
> iommu_domain *domain)
>  	struct arm_smmu_domain *smmu_domain =
> to_smmu_domain(domain);
>  	struct arm_smmu_device *smmu = smmu_domain->smmu;
> 
> -	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> -		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> -		return 0;
> -	}
> -
>  	/* Restrict the stage to what we can actually support */
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
>  		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
> @@ -2537,7 +2531,7 @@ static void arm_smmu_detach_dev(struct
> arm_smmu_master *master)
>  	struct arm_smmu_domain *smmu_domain;
>  	unsigned long flags;
> 
> -	if (!domain)
> +	if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING))
>  		return;
> 
>  	smmu_domain = to_smmu_domain(domain);
> @@ -2600,15 +2594,7 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
> 
>  	arm_smmu_detach_dev(master);
> 
> -	/*
> -	 * The SMMU does not support enabling ATS with bypass. When the
> STE is
> -	 * in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests and
> -	 * Translated transactions are denied as though ATS is disabled for
> the
> -	 * stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and
> -	 * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry).
> -	 */
> -	if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
> -		master->ats_enabled = arm_smmu_ats_supported(master);
> +	master->ats_enabled = arm_smmu_ats_supported(master);
> 
>  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
>  	list_add(&master->domain_head, &smmu_domain->devices);
> @@ -2645,13 +2631,6 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
>  			arm_smmu_write_ctx_desc(master,
> IOMMU_NO_PASID,
>  						      NULL);
>  		break;
> -	case ARM_SMMU_DOMAIN_BYPASS:
> -		arm_smmu_make_bypass_ste(&target);
> -		arm_smmu_install_ste_for_dev(master, &target);
> -		if (master->cd_table.cdtab)
> -			arm_smmu_write_ctx_desc(master,
> IOMMU_NO_PASID,
> -						      NULL);
> -		break;
>  	}
> 
>  	arm_smmu_enable_ats(master, smmu_domain);
> @@ -2667,6 +2646,60 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
>  	return ret;
>  }
> 
> +static int arm_smmu_attach_dev_ste(struct device *dev,
> +				   struct arm_smmu_ste *ste)
> +{
> +	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> +
> +	if (arm_smmu_master_sva_enabled(master))
> +		return -EBUSY;
> +
> +	/*
> +	 * Do not allow any ASID to be changed while are working on the STE,
> +	 * otherwise we could miss invalidations.
> +	 */
> +	mutex_lock(&arm_smmu_asid_lock);
> +
> +	/*
> +	 * The SMMU does not support enabling ATS with bypass/abort.
> When the
> +	 * STE is in bypass (STE.Config[2:0] == 0b100), ATS Translation
> Requests
> +	 * and Translated transactions are denied as though ATS is disabled
> for
> +	 * the stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and
> +	 * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry).
> +	 */
> +	arm_smmu_detach_dev(master);
> +
> +	arm_smmu_install_ste_for_dev(master, ste);
> +	mutex_unlock(&arm_smmu_asid_lock);
> +
> +	/*
> +	 * This has to be done after removing the master from the
> +	 * arm_smmu_domain->devices to avoid races updating the same
> context
> +	 * descriptor from arm_smmu_share_asid().
> +	 */
> +	if (master->cd_table.cdtab)
> +		arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> NULL);
> +	return 0;
> +}
> +
> +static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
> +					struct device *dev)
> +{
> +	struct arm_smmu_ste ste;
> +
> +	arm_smmu_make_bypass_ste(&ste);
> +	return arm_smmu_attach_dev_ste(dev, &ste);
> +}
> +
> +static const struct iommu_domain_ops arm_smmu_identity_ops = {
> +	.attach_dev = arm_smmu_attach_dev_identity,
> +};
> +
> +static struct iommu_domain arm_smmu_identity_domain = {
> +	.type = IOMMU_DOMAIN_IDENTITY,
> +	.ops = &arm_smmu_identity_ops,
> +};
> +
>  static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned
> long iova,
>  			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
>  			      int prot, gfp_t gfp, size_t *mapped)
> @@ -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,

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.

Thanks,
Shameer
Jason Gunthorpe Jan. 29, 2024, 6:37 p.m. UTC | #2 
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/

> 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.

Thanks,
Jason
Shameerali Kolothum Thodi Jan. 30, 2024, 8:35 a.m. UTC | #3 
> -----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
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index f08cfa9b90b3eb..d35bf9655c9b1b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2226,8 +2226,7 @@  static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 		return arm_smmu_sva_domain_alloc();
 
 	if (type != IOMMU_DOMAIN_UNMANAGED &&
-	    type != IOMMU_DOMAIN_DMA &&
-	    type != IOMMU_DOMAIN_IDENTITY)
+	    type != IOMMU_DOMAIN_DMA)
 		return NULL;
 
 	/*
@@ -2335,11 +2334,6 @@  static int arm_smmu_domain_finalise(struct iommu_domain *domain)
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 
-	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
-		return 0;
-	}
-
 	/* Restrict the stage to what we can actually support */
 	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
 		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
@@ -2537,7 +2531,7 @@  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 	struct arm_smmu_domain *smmu_domain;
 	unsigned long flags;
 
-	if (!domain)
+	if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING))
 		return;
 
 	smmu_domain = to_smmu_domain(domain);
@@ -2600,15 +2594,7 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	arm_smmu_detach_dev(master);
 
-	/*
-	 * The SMMU does not support enabling ATS with bypass. When the STE is
-	 * in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests and
-	 * Translated transactions are denied as though ATS is disabled for the
-	 * stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and
-	 * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry).
-	 */
-	if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
-		master->ats_enabled = arm_smmu_ats_supported(master);
+	master->ats_enabled = arm_smmu_ats_supported(master);
 
 	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
 	list_add(&master->domain_head, &smmu_domain->devices);
@@ -2645,13 +2631,6 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
 						      NULL);
 		break;
-	case ARM_SMMU_DOMAIN_BYPASS:
-		arm_smmu_make_bypass_ste(&target);
-		arm_smmu_install_ste_for_dev(master, &target);
-		if (master->cd_table.cdtab)
-			arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
-						      NULL);
-		break;
 	}
 
 	arm_smmu_enable_ats(master, smmu_domain);
@@ -2667,6 +2646,60 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return ret;
 }
 
+static int arm_smmu_attach_dev_ste(struct device *dev,
+				   struct arm_smmu_ste *ste)
+{
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+
+	if (arm_smmu_master_sva_enabled(master))
+		return -EBUSY;
+
+	/*
+	 * Do not allow any ASID to be changed while are working on the STE,
+	 * otherwise we could miss invalidations.
+	 */
+	mutex_lock(&arm_smmu_asid_lock);
+
+	/*
+	 * The SMMU does not support enabling ATS with bypass/abort. When the
+	 * STE is in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests
+	 * and Translated transactions are denied as though ATS is disabled for
+	 * the stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and
+	 * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry).
+	 */
+	arm_smmu_detach_dev(master);
+
+	arm_smmu_install_ste_for_dev(master, ste);
+	mutex_unlock(&arm_smmu_asid_lock);
+
+	/*
+	 * This has to be done after removing the master from the
+	 * arm_smmu_domain->devices to avoid races updating the same context
+	 * descriptor from arm_smmu_share_asid().
+	 */
+	if (master->cd_table.cdtab)
+		arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, NULL);
+	return 0;
+}
+
+static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
+					struct device *dev)
+{
+	struct arm_smmu_ste ste;
+
+	arm_smmu_make_bypass_ste(&ste);
+	return arm_smmu_attach_dev_ste(dev, &ste);
+}
+
+static const struct iommu_domain_ops arm_smmu_identity_ops = {
+	.attach_dev = arm_smmu_attach_dev_identity,
+};
+
+static struct iommu_domain arm_smmu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &arm_smmu_identity_ops,
+};
+
 static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			      int prot, gfp_t gfp, size_t *mapped)
@@ -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,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 6b63ea7dae72da..23baf117e7e4b5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -712,7 +712,6 @@  struct arm_smmu_master {
 enum arm_smmu_domain_stage {
 	ARM_SMMU_DOMAIN_S1 = 0,
 	ARM_SMMU_DOMAIN_S2,
-	ARM_SMMU_DOMAIN_BYPASS,
 };
 
 struct arm_smmu_domain {