diff mbox series

[v2,10/19] iommu/arm-smmu-v3: Compute the STE only once for each master

Message ID 10-v2-de8b10590bf5+400-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 Nov. 13, 2023, 5:53 p.m. UTC 
Currently arm_smmu_install_ste_for_dev() iterates over every SID and
computes from scratch an identical STE. Every SID should have the same STE
contents. Turn this inside out so that the STE is supplied by the caller
and arm_smmu_install_ste_for_dev() simply installs it to every SID.

This is possible now that the STE generation does not inform what sequence
should be used to program it.

This allows splitting the STE calculation up according to the call site,
which following patches will make use of, and removes the confusing NULL
domain special case that only supported arm_smmu_detach_dev().

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 58 ++++++++-------------
 1 file changed, 22 insertions(+), 36 deletions(-)

Comments

Michael Shavit Nov. 15, 2023, 2:16 p.m. UTC | #1 
On Tue, Nov 14, 2023 at 1:53 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> Currently arm_smmu_install_ste_for_dev() iterates over every SID and
> computes from scratch an identical STE. Every SID should have the same STE
> contents. Turn this inside out so that the STE is supplied by the caller
> and arm_smmu_install_ste_for_dev() simply installs it to every SID.
>
> This is possible now that the STE generation does not inform what sequence
> should be used to program it.
>
> This allows splitting the STE calculation up according to the call site,
> which following patches will make use of, and removes the confusing NULL
> domain special case that only supported arm_smmu_detach_dev().
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Michael Shavit <mshavit@google.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 58 ++++++++-------------
>  1 file changed, 22 insertions(+), 36 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 b11dc03ee16880..4b157c2ddf9a80 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1536,36 +1536,6 @@ static void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
>                                       STRTAB_STE_3_S2TTB_MASK);
>  }
>
> -static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
> -                                     struct arm_smmu_ste *dst)
> -{
> -       struct arm_smmu_device *smmu = master->smmu;
> -       struct arm_smmu_domain *smmu_domain = master->domain;
> -       struct arm_smmu_ste target = {};
> -
> -       if (!smmu_domain) {
> -               if (disable_bypass)
> -                       arm_smmu_make_abort_ste(&target);
> -               else
> -                       arm_smmu_make_bypass_ste(&target);
> -               arm_smmu_write_ste(smmu, sid, dst, &target);
> -               return;
> -       }
> -
> -       switch (smmu_domain->stage) {
> -       case ARM_SMMU_DOMAIN_S1:
> -               arm_smmu_make_cdtable_ste(&target, master, &master->cd_table);
> -               break;
> -       case ARM_SMMU_DOMAIN_S2:
> -               arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
> -               break;
> -       case ARM_SMMU_DOMAIN_BYPASS:
> -               arm_smmu_make_bypass_ste(&target);
> -               break;
> -       }
> -       arm_smmu_write_ste(smmu, sid, dst, &target);
> -}
> -
>  static void arm_smmu_init_bypass_stes(struct arm_smmu_ste *strtab,
>                                       unsigned int nent)
>  {
> @@ -2387,7 +2357,8 @@ arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid)
>         }
>  }
>
> -static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
> +static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master,
> +                                        const struct arm_smmu_ste *target)
>  {
>         int i, j;
>         struct arm_smmu_device *smmu = master->smmu;
> @@ -2404,7 +2375,7 @@ static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
>                 if (j < i)
>                         continue;
>
> -               arm_smmu_write_strtab_ent(master, sid, step);
> +               arm_smmu_write_ste(smmu, sid, step, target);
>         }
>  }
>
> @@ -2511,6 +2482,7 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>  {
>         unsigned long flags;
> +       struct arm_smmu_ste target;
>         struct arm_smmu_domain *smmu_domain = master->domain;
>
>         if (!smmu_domain)
> @@ -2524,7 +2496,11 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>
>         master->domain = NULL;
>         master->ats_enabled = false;
> -       arm_smmu_install_ste_for_dev(master);
> +       if (disable_bypass)
> +               arm_smmu_make_abort_ste(&target);
> +       else
> +               arm_smmu_make_bypass_ste(&target);
> +       arm_smmu_install_ste_for_dev(master, &target);
>         /*
>          * Clearing the CD entry isn't strictly required to detach the domain
>          * since the table is uninstalled anyway, but it helps avoid confusion
> @@ -2539,6 +2515,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  {
>         int ret = 0;
>         unsigned long flags;
> +       struct arm_smmu_ste target;
>         struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>         struct arm_smmu_device *smmu;
>         struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> @@ -2600,7 +2577,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>         list_add(&master->domain_head, &smmu_domain->devices);
>         spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>
> -       if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> +       switch (smmu_domain->stage) {
> +       case ARM_SMMU_DOMAIN_S1:
>                 if (!master->cd_table.cdtab) {
>                         ret = arm_smmu_alloc_cd_tables(master);
>                         if (ret) {
> @@ -2614,9 +2592,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>                         master->domain = NULL;
>                         goto out_list_del;
>                 }
> -       }
>
> -       arm_smmu_install_ste_for_dev(master);
> +               arm_smmu_make_cdtable_ste(&target, master, &master->cd_table);
> +               break;
> +       case ARM_SMMU_DOMAIN_S2:
> +               arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
> +               break;
> +       case ARM_SMMU_DOMAIN_BYPASS:
> +               arm_smmu_make_bypass_ste(&target);
> +               break;
> +       }
> +       arm_smmu_install_ste_for_dev(master, &target);
>
>         arm_smmu_enable_ats(master);
>         goto out_unlock;
> --
> 2.42.0
>
Nicolin Chen Dec. 5, 2023, 2:13 a.m. UTC | #2 
On Mon, Nov 13, 2023 at 01:53:17PM -0400, Jason Gunthorpe wrote:
> Currently arm_smmu_install_ste_for_dev() iterates over every SID and
> computes from scratch an identical STE. Every SID should have the same STE
> contents. Turn this inside out so that the STE is supplied by the caller
> and arm_smmu_install_ste_for_dev() simply installs it to every SID.
> 
> This is possible now that the STE generation does not inform what sequence
> should be used to program it.
> 
> This allows splitting the STE calculation up according to the call site,
> which following patches will make use of, and removes the confusing NULL
> domain special case that only supported arm_smmu_detach_dev().
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
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 b11dc03ee16880..4b157c2ddf9a80 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1536,36 +1536,6 @@  static void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
 				      STRTAB_STE_3_S2TTB_MASK);
 }
 
-static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
-				      struct arm_smmu_ste *dst)
-{
-	struct arm_smmu_device *smmu = master->smmu;
-	struct arm_smmu_domain *smmu_domain = master->domain;
-	struct arm_smmu_ste target = {};
-
-	if (!smmu_domain) {
-		if (disable_bypass)
-			arm_smmu_make_abort_ste(&target);
-		else
-			arm_smmu_make_bypass_ste(&target);
-		arm_smmu_write_ste(smmu, sid, dst, &target);
-		return;
-	}
-
-	switch (smmu_domain->stage) {
-	case ARM_SMMU_DOMAIN_S1:
-		arm_smmu_make_cdtable_ste(&target, master, &master->cd_table);
-		break;
-	case ARM_SMMU_DOMAIN_S2:
-		arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
-		break;
-	case ARM_SMMU_DOMAIN_BYPASS:
-		arm_smmu_make_bypass_ste(&target);
-		break;
-	}
-	arm_smmu_write_ste(smmu, sid, dst, &target);
-}
-
 static void arm_smmu_init_bypass_stes(struct arm_smmu_ste *strtab,
 				      unsigned int nent)
 {
@@ -2387,7 +2357,8 @@  arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid)
 	}
 }
 
-static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
+static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master,
+					 const struct arm_smmu_ste *target)
 {
 	int i, j;
 	struct arm_smmu_device *smmu = master->smmu;
@@ -2404,7 +2375,7 @@  static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
 		if (j < i)
 			continue;
 
-		arm_smmu_write_strtab_ent(master, sid, step);
+		arm_smmu_write_ste(smmu, sid, step, target);
 	}
 }
 
@@ -2511,6 +2482,7 @@  static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 {
 	unsigned long flags;
+	struct arm_smmu_ste target;
 	struct arm_smmu_domain *smmu_domain = master->domain;
 
 	if (!smmu_domain)
@@ -2524,7 +2496,11 @@  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 
 	master->domain = NULL;
 	master->ats_enabled = false;
-	arm_smmu_install_ste_for_dev(master);
+	if (disable_bypass)
+		arm_smmu_make_abort_ste(&target);
+	else
+		arm_smmu_make_bypass_ste(&target);
+	arm_smmu_install_ste_for_dev(master, &target);
 	/*
 	 * Clearing the CD entry isn't strictly required to detach the domain
 	 * since the table is uninstalled anyway, but it helps avoid confusion
@@ -2539,6 +2515,7 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
 	int ret = 0;
 	unsigned long flags;
+	struct arm_smmu_ste target;
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
@@ -2600,7 +2577,8 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	list_add(&master->domain_head, &smmu_domain->devices);
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
-	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
+	switch (smmu_domain->stage) {
+	case ARM_SMMU_DOMAIN_S1:
 		if (!master->cd_table.cdtab) {
 			ret = arm_smmu_alloc_cd_tables(master);
 			if (ret) {
@@ -2614,9 +2592,17 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			master->domain = NULL;
 			goto out_list_del;
 		}
-	}
 
-	arm_smmu_install_ste_for_dev(master);
+		arm_smmu_make_cdtable_ste(&target, master, &master->cd_table);
+		break;
+	case ARM_SMMU_DOMAIN_S2:
+		arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
+		break;
+	case ARM_SMMU_DOMAIN_BYPASS:
+		arm_smmu_make_bypass_ste(&target);
+		break;
+	}
+	arm_smmu_install_ste_for_dev(master, &target);
 
 	arm_smmu_enable_ats(master);
 	goto out_unlock;