diff mbox series

[v5,02/17] iommu/arm-smmu-v3: Consolidate the STE generation for abort/bypass

Message ID 2-v5-cd1be8dd9c71+3fa-smmuv3_newapi_p1_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series [v5,01/17] iommu/arm-smmu-v3: Make STE programming independent of the callers | expand

Commit Message

Jason Gunthorpe Feb. 6, 2024, 3:12 p.m. UTC 
This allows writing the flow of arm_smmu_write_strtab_ent() around abort
and bypass domains more naturally.

Note that the core code no longer supplies NULL domains, though there is
still a flow in the driver that end up in arm_smmu_write_strtab_ent() with
NULL. A later patch will remove it.

Remove the duplicate calculation of the STE in arm_smmu_init_bypass_stes()
and remove the force parameter. arm_smmu_rmr_install_bypass_ste() can now
simply invoke arm_smmu_make_bypass_ste() directly.

Reviewed-by: Michael Shavit <mshavit@google.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Mostafa Saleh <smostafa@google.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 | 97 ++++++++++++---------
 1 file changed, 55 insertions(+), 42 deletions(-)

Comments

Robin Murphy Feb. 15, 2024, 5:27 p.m. UTC | #1 
On 06/02/2024 3:12 pm, Jason Gunthorpe wrote:
> This allows writing the flow of arm_smmu_write_strtab_ent() around abort
> and bypass domains more naturally.
> 
> Note that the core code no longer supplies NULL domains, though there is
> still a flow in the driver that end up in arm_smmu_write_strtab_ent() with
> NULL. A later patch will remove it.
> 
> Remove the duplicate calculation of the STE in arm_smmu_init_bypass_stes()
> and remove the force parameter. arm_smmu_rmr_install_bypass_ste() can now
> simply invoke arm_smmu_make_bypass_ste() directly.
> 
> Reviewed-by: Michael Shavit <mshavit@google.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Reviewed-by: Mostafa Saleh <smostafa@google.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 | 97 ++++++++++++---------
>   1 file changed, 55 insertions(+), 42 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 f0b915567cbcdc..6123e5ad95822c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1498,6 +1498,24 @@ static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid,
>   	}
>   }
>   
> +static void arm_smmu_make_abort_ste(struct arm_smmu_ste *target)
> +{
> +	memset(target, 0, sizeof(*target));
> +	target->data[0] = cpu_to_le64(
> +		STRTAB_STE_0_V |
> +		FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT));
> +}
> +
> +static void arm_smmu_make_bypass_ste(struct arm_smmu_ste *target)
> +{
> +	memset(target, 0, sizeof(*target));
> +	target->data[0] = cpu_to_le64(
> +		STRTAB_STE_0_V |
> +		FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_BYPASS));
> +	target->data[1] = cpu_to_le64(
> +		FIELD_PREP(STRTAB_STE_1_SHCFG, STRTAB_STE_1_SHCFG_INCOMING));
> +}
> +
>   static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>   				      struct arm_smmu_ste *dst)
>   {
> @@ -1508,37 +1526,31 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>   	struct arm_smmu_domain *smmu_domain = master->domain;
>   	struct arm_smmu_ste target = {};
>   
> -	if (smmu_domain) {
> -		switch (smmu_domain->stage) {
> -		case ARM_SMMU_DOMAIN_S1:
> -			cd_table = &master->cd_table;
> -			break;
> -		case ARM_SMMU_DOMAIN_S2:
> -			s2_cfg = &smmu_domain->s2_cfg;
> -			break;
> -		default:
> -			break;
> -		}
> +	if (!smmu_domain) {
> +		if (disable_bypass)
> +			arm_smmu_make_abort_ste(&target);
> +		else
> +			arm_smmu_make_bypass_ste(&target);
> +		arm_smmu_write_ste(master, sid, dst, &target);
> +		return;
> +	}
> +
> +	switch (smmu_domain->stage) {
> +	case ARM_SMMU_DOMAIN_S1:
> +		cd_table = &master->cd_table;
> +		break;
> +	case ARM_SMMU_DOMAIN_S2:
> +		s2_cfg = &smmu_domain->s2_cfg;
> +		break;
> +	case ARM_SMMU_DOMAIN_BYPASS:
> +		arm_smmu_make_bypass_ste(&target);
> +		arm_smmu_write_ste(master, sid, dst, &target);
> +		return;
>   	}
>   
>   	/* Nuke the existing STE_0 value, as we're going to rewrite it */
>   	val = STRTAB_STE_0_V;
>   
> -	/* Bypass/fault */
> -	if (!smmu_domain || !(cd_table || s2_cfg)) {
> -		if (!smmu_domain && disable_bypass)
> -			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT);
> -		else
> -			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_BYPASS);
> -
> -		target.data[0] = cpu_to_le64(val);
> -		target.data[1] = cpu_to_le64(FIELD_PREP(STRTAB_STE_1_SHCFG,
> -						STRTAB_STE_1_SHCFG_INCOMING));
> -		target.data[2] = 0; /* Nuke the VMID */
> -		arm_smmu_write_ste(master, sid, dst, &target);
> -		return;
> -	}
> -
>   	if (cd_table) {
>   		u64 strw = smmu->features & ARM_SMMU_FEAT_E2H ?
>   			STRTAB_STE_1_STRW_EL2 : STRTAB_STE_1_STRW_NSEL1;
> @@ -1583,22 +1595,20 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>   	arm_smmu_write_ste(master, sid, dst, &target);
>   }
>   
> +/*
> + * This can safely directly manipulate the STE memory without a sync sequence
> + * because the STE table has not been installed in the SMMU yet.
> + */
>   static void arm_smmu_init_bypass_stes(struct arm_smmu_ste *strtab,

This name is long out-of-date - if we're refreshing this area, please 
rename to something relevant to what it actually does, e.g. 
s/bypass/initial/.

Although frankly I also think that at this point we should just get rid 
of the disable_bypass parameter altogether - it's been almost entirely 
meaningless since default domain support was added, and any tenuous 
cases for wanting inital STEs to be bypass should probably be using RMRs 
now anyway.

Thanks,
Robin.

> -				      unsigned int nent, bool force)
> +				      unsigned int nent)
>   {
>   	unsigned int i;
> -	u64 val = STRTAB_STE_0_V;
> -
> -	if (disable_bypass && !force)
> -		val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT);
> -	else
> -		val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_BYPASS);
>   
>   	for (i = 0; i < nent; ++i) {
> -		strtab->data[0] = cpu_to_le64(val);
> -		strtab->data[1] = cpu_to_le64(FIELD_PREP(
> -			STRTAB_STE_1_SHCFG, STRTAB_STE_1_SHCFG_INCOMING));
> -		strtab->data[2] = 0;
> +		if (disable_bypass)
> +			arm_smmu_make_abort_ste(strtab);
> +		else
> +			arm_smmu_make_bypass_ste(strtab);
>   		strtab++;
>   	}
>   }
> @@ -1626,7 +1636,7 @@ static int arm_smmu_init_l2_strtab(struct arm_smmu_device *smmu, u32 sid)
>   		return -ENOMEM;
>   	}
>   
> -	arm_smmu_init_bypass_stes(desc->l2ptr, 1 << STRTAB_SPLIT, false);
> +	arm_smmu_init_bypass_stes(desc->l2ptr, 1 << STRTAB_SPLIT);
>   	arm_smmu_write_strtab_l1_desc(strtab, desc);
>   	return 0;
>   }
> @@ -3245,7 +3255,7 @@ static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
>   	reg |= FIELD_PREP(STRTAB_BASE_CFG_LOG2SIZE, smmu->sid_bits);
>   	cfg->strtab_base_cfg = reg;
>   
> -	arm_smmu_init_bypass_stes(strtab, cfg->num_l1_ents, false);
> +	arm_smmu_init_bypass_stes(strtab, cfg->num_l1_ents);
>   	return 0;
>   }
>   
> @@ -3956,7 +3966,6 @@ static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)
>   	iort_get_rmr_sids(dev_fwnode(smmu->dev), &rmr_list);
>   
>   	list_for_each_entry(e, &rmr_list, list) {
> -		struct arm_smmu_ste *step;
>   		struct iommu_iort_rmr_data *rmr;
>   		int ret, i;
>   
> @@ -3969,8 +3978,12 @@ static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)
>   				continue;
>   			}
>   
> -			step = arm_smmu_get_step_for_sid(smmu, rmr->sids[i]);
> -			arm_smmu_init_bypass_stes(step, 1, true);
> +			/*
> +			 * STE table is not programmed to HW, see
> +			 * arm_smmu_init_bypass_stes()
> +			 */
> +			arm_smmu_make_bypass_ste(
> +				arm_smmu_get_step_for_sid(smmu, rmr->sids[i]));
>   		}
>   	}
>
Will Deacon Feb. 22, 2024, 5:40 p.m. UTC | #2 
On Thu, Feb 15, 2024 at 05:27:09PM +0000, Robin Murphy wrote:
> On 06/02/2024 3:12 pm, Jason Gunthorpe wrote:
> > +/*
> > + * This can safely directly manipulate the STE memory without a sync sequence
> > + * because the STE table has not been installed in the SMMU yet.
> > + */
> >   static void arm_smmu_init_bypass_stes(struct arm_smmu_ste *strtab,
> 
> This name is long out-of-date - if we're refreshing this area, please rename
> to something relevant to what it actually does, e.g. s/bypass/initial/.
> 
> Although frankly I also think that at this point we should just get rid of
> the disable_bypass parameter altogether - it's been almost entirely
> meaningless since default domain support was added, and any tenuous cases
> for wanting inital STEs to be bypass should probably be using RMRs now
> anyway.

We probably can't drop it for SMMUv2, but I'd be more than happy to do so
on SMMUv3. I think one of the reasons for keeping it in the new driver was
that the Arm Fast Model used to need it for legacy virtio devices that were
downsteam of the SMMU but couldn't advertise F_ACCESS_PLATFORM. Was that
fixed?

Will
Jason Gunthorpe Feb. 23, 2024, 6:53 p.m. UTC | #3 
On Thu, Feb 15, 2024 at 05:27:09PM +0000, Robin Murphy wrote:
> > @@ -1583,22 +1595,20 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
> >   	arm_smmu_write_ste(master, sid, dst, &target);
> >   }
> > +/*
> > + * This can safely directly manipulate the STE memory without a sync sequence
> > + * because the STE table has not been installed in the SMMU yet.
> > + */
> >   static void arm_smmu_init_bypass_stes(struct arm_smmu_ste *strtab,
> 
> This name is long out-of-date - if we're refreshing this area, please rename
> to something relevant to what it actually does, e.g. s/bypass/initial/.

Done

> Although frankly I also think that at this point we should just get rid of
> the disable_bypass parameter altogether - it's been almost entirely
> meaningless since default domain support was added, and any tenuous cases
> for wanting inital STEs to be bypass should probably be using RMRs now
> anyway.

I can write the patch for this if you and Will agree

Thanks,
Jason
Will Deacon Feb. 27, 2024, 10:50 a.m. UTC | #4 
On Fri, Feb 23, 2024 at 02:53:58PM -0400, Jason Gunthorpe wrote:
> On Thu, Feb 15, 2024 at 05:27:09PM +0000, Robin Murphy wrote:
> > > @@ -1583,22 +1595,20 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
> > >   	arm_smmu_write_ste(master, sid, dst, &target);
> > >   }
> > > +/*
> > > + * This can safely directly manipulate the STE memory without a sync sequence
> > > + * because the STE table has not been installed in the SMMU yet.
> > > + */
> > >   static void arm_smmu_init_bypass_stes(struct arm_smmu_ste *strtab,
> > 
> > This name is long out-of-date - if we're refreshing this area, please rename
> > to something relevant to what it actually does, e.g. s/bypass/initial/.
> 
> Done
> 
> > Although frankly I also think that at this point we should just get rid of
> > the disable_bypass parameter altogether - it's been almost entirely
> > meaningless since default domain support was added, and any tenuous cases
> > for wanting inital STEs to be bypass should probably be using RMRs now
> > anyway.
> 
> I can write the patch for this if you and Will agree

Yes, please! I'll be glad to see the back of that option. Since you just
posted v6 of your "part 1", feel free to send a patch which applies on top
of that (rather than having to rebase the whole shebang).

Will
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 f0b915567cbcdc..6123e5ad95822c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1498,6 +1498,24 @@  static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid,
 	}
 }
 
+static void arm_smmu_make_abort_ste(struct arm_smmu_ste *target)
+{
+	memset(target, 0, sizeof(*target));
+	target->data[0] = cpu_to_le64(
+		STRTAB_STE_0_V |
+		FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT));
+}
+
+static void arm_smmu_make_bypass_ste(struct arm_smmu_ste *target)
+{
+	memset(target, 0, sizeof(*target));
+	target->data[0] = cpu_to_le64(
+		STRTAB_STE_0_V |
+		FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_BYPASS));
+	target->data[1] = cpu_to_le64(
+		FIELD_PREP(STRTAB_STE_1_SHCFG, STRTAB_STE_1_SHCFG_INCOMING));
+}
+
 static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
 				      struct arm_smmu_ste *dst)
 {
@@ -1508,37 +1526,31 @@  static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
 	struct arm_smmu_domain *smmu_domain = master->domain;
 	struct arm_smmu_ste target = {};
 
-	if (smmu_domain) {
-		switch (smmu_domain->stage) {
-		case ARM_SMMU_DOMAIN_S1:
-			cd_table = &master->cd_table;
-			break;
-		case ARM_SMMU_DOMAIN_S2:
-			s2_cfg = &smmu_domain->s2_cfg;
-			break;
-		default:
-			break;
-		}
+	if (!smmu_domain) {
+		if (disable_bypass)
+			arm_smmu_make_abort_ste(&target);
+		else
+			arm_smmu_make_bypass_ste(&target);
+		arm_smmu_write_ste(master, sid, dst, &target);
+		return;
+	}
+
+	switch (smmu_domain->stage) {
+	case ARM_SMMU_DOMAIN_S1:
+		cd_table = &master->cd_table;
+		break;
+	case ARM_SMMU_DOMAIN_S2:
+		s2_cfg = &smmu_domain->s2_cfg;
+		break;
+	case ARM_SMMU_DOMAIN_BYPASS:
+		arm_smmu_make_bypass_ste(&target);
+		arm_smmu_write_ste(master, sid, dst, &target);
+		return;
 	}
 
 	/* Nuke the existing STE_0 value, as we're going to rewrite it */
 	val = STRTAB_STE_0_V;
 
-	/* Bypass/fault */
-	if (!smmu_domain || !(cd_table || s2_cfg)) {
-		if (!smmu_domain && disable_bypass)
-			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT);
-		else
-			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_BYPASS);
-
-		target.data[0] = cpu_to_le64(val);
-		target.data[1] = cpu_to_le64(FIELD_PREP(STRTAB_STE_1_SHCFG,
-						STRTAB_STE_1_SHCFG_INCOMING));
-		target.data[2] = 0; /* Nuke the VMID */
-		arm_smmu_write_ste(master, sid, dst, &target);
-		return;
-	}
-
 	if (cd_table) {
 		u64 strw = smmu->features & ARM_SMMU_FEAT_E2H ?
 			STRTAB_STE_1_STRW_EL2 : STRTAB_STE_1_STRW_NSEL1;
@@ -1583,22 +1595,20 @@  static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
 	arm_smmu_write_ste(master, sid, dst, &target);
 }
 
+/*
+ * This can safely directly manipulate the STE memory without a sync sequence
+ * because the STE table has not been installed in the SMMU yet.
+ */
 static void arm_smmu_init_bypass_stes(struct arm_smmu_ste *strtab,
-				      unsigned int nent, bool force)
+				      unsigned int nent)
 {
 	unsigned int i;
-	u64 val = STRTAB_STE_0_V;
-
-	if (disable_bypass && !force)
-		val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT);
-	else
-		val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_BYPASS);
 
 	for (i = 0; i < nent; ++i) {
-		strtab->data[0] = cpu_to_le64(val);
-		strtab->data[1] = cpu_to_le64(FIELD_PREP(
-			STRTAB_STE_1_SHCFG, STRTAB_STE_1_SHCFG_INCOMING));
-		strtab->data[2] = 0;
+		if (disable_bypass)
+			arm_smmu_make_abort_ste(strtab);
+		else
+			arm_smmu_make_bypass_ste(strtab);
 		strtab++;
 	}
 }
@@ -1626,7 +1636,7 @@  static int arm_smmu_init_l2_strtab(struct arm_smmu_device *smmu, u32 sid)
 		return -ENOMEM;
 	}
 
-	arm_smmu_init_bypass_stes(desc->l2ptr, 1 << STRTAB_SPLIT, false);
+	arm_smmu_init_bypass_stes(desc->l2ptr, 1 << STRTAB_SPLIT);
 	arm_smmu_write_strtab_l1_desc(strtab, desc);
 	return 0;
 }
@@ -3245,7 +3255,7 @@  static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
 	reg |= FIELD_PREP(STRTAB_BASE_CFG_LOG2SIZE, smmu->sid_bits);
 	cfg->strtab_base_cfg = reg;
 
-	arm_smmu_init_bypass_stes(strtab, cfg->num_l1_ents, false);
+	arm_smmu_init_bypass_stes(strtab, cfg->num_l1_ents);
 	return 0;
 }
 
@@ -3956,7 +3966,6 @@  static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)
 	iort_get_rmr_sids(dev_fwnode(smmu->dev), &rmr_list);
 
 	list_for_each_entry(e, &rmr_list, list) {
-		struct arm_smmu_ste *step;
 		struct iommu_iort_rmr_data *rmr;
 		int ret, i;
 
@@ -3969,8 +3978,12 @@  static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)
 				continue;
 			}
 
-			step = arm_smmu_get_step_for_sid(smmu, rmr->sids[i]);
-			arm_smmu_init_bypass_stes(step, 1, true);
+			/*
+			 * STE table is not programmed to HW, see
+			 * arm_smmu_init_bypass_stes()
+			 */
+			arm_smmu_make_bypass_ste(
+				arm_smmu_get_step_for_sid(smmu, rmr->sids[i]));
 		}
 	}