diff mbox series

[15/19] iommu/arm-smmu-v3: Add a global static IDENTITY domain

Message ID 15-v1-e289ca9121be+2be-smmuv3_newapi_p1_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Update SMMUv3 to the modern iommu API (part 1/2) | expand

Commit Message

Jason Gunthorpe Oct. 11, 2023, 12:33 a.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.

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

Michael Shavit Oct. 18, 2023, 11:06 a.m. UTC | #1 
On Wed, Oct 11, 2023 at 8:33 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> Move to the new static global for identity domains. Move all the logic out
> of arm_smmu_attach_dev into an identity only function.
>
> 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 26d3200c127450..1e03bdedfabad1 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2149,8 +2149,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;
>
>         /*
> @@ -2258,11 +2257,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;
> @@ -2459,7 +2453,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;

Confused me why we were checking against __IOMMU_DOMAIN_PAGING instead
 of IOMMU_DOMAIN_UNMANAGED/DMA to match domain_alloc, but ok it's
clarified by the final patch in the series.


>
>         smmu_domain = to_smmu_domain(domain);
> @@ -2522,15 +2516,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);
> @@ -2567,13 +2553,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);
> @@ -2589,6 +2568,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)
> @@ -2981,6 +3014,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 21f2f73501019a..154808f96718df 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 {
> --
> 2.42.0
>
Jason Gunthorpe Oct. 18, 2023, 12:26 p.m. UTC | #2 
On Wed, Oct 18, 2023 at 07:06:55PM +0800, Michael Shavit wrote:
> On Wed, Oct 11, 2023 at 8:33 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > Move to the new static global for identity domains. Move all the logic out
> > of arm_smmu_attach_dev into an identity only function.
> >
> > 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 26d3200c127450..1e03bdedfabad1 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -2149,8 +2149,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;
> >
> >         /*
> > @@ -2258,11 +2257,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;
> > @@ -2459,7 +2453,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;
> 
> Confused me why we were checking against __IOMMU_DOMAIN_PAGING instead
>  of IOMMU_DOMAIN_UNMANAGED/DMA to match domain_alloc, but ok it's
> clarified by the final patch in the series.

Long term I am trying to remove DMA/UNMANAGED from the drivers (we are
actually getting quite close!). A domain created by
domain_alloc_paging (later patch) should be tested like this.

Jason
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 26d3200c127450..1e03bdedfabad1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2149,8 +2149,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;
 
 	/*
@@ -2258,11 +2257,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;
@@ -2459,7 +2453,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);
@@ -2522,15 +2516,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);
@@ -2567,13 +2553,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);
@@ -2589,6 +2568,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)
@@ -2981,6 +3014,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 21f2f73501019a..154808f96718df 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 {