diff mbox series

[v4,05/13] iommu/arm-smmu-v3: Use the master-owned s1_cfg

Message ID 20230621063825.268890-6-mshavit@google.com (mailing list archive)
State New, archived
Headers show
Series Add PASID support to SMMUv3 unmanaged domains | expand

Commit Message

Michael Shavit June 21, 2023, 6:37 a.m. UTC 
Insert CDs for STAGE_1 domains into a CD table owned by the
arm_smmu_master. Remove the CD table that was owned by arm_smmu_domain.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 43 ++++++---------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 --
 2 files changed, 12 insertions(+), 34 deletions(-)

Comments

Nicolin Chen July 13, 2023, 1:57 a.m. UTC | #1 
On Wed, Jun 21, 2023 at 02:37:17PM +0800, Michael Shavit wrote:
 
> Insert CDs for STAGE_1 domains into a CD table owned by the
> arm_smmu_master. Remove the CD table that was owned by arm_smmu_domain.

> @@ -718,10 +718,7 @@ struct arm_smmu_domain {
> 
>         enum arm_smmu_domain_stage              stage;
>         union {
> -               struct {
>                 struct arm_smmu_ctx_desc        cd;
> -               struct arm_smmu_s1_cfg          s1_cfg;
> -               };
>                 struct arm_smmu_s2_cfg          s2_cfg;
>         };

So the arm_smmu_domain looks like an object representing either:
1) a CD table + (if !IDENTITY) an S1 IOPT for default substream
2) an S2 IOPT

I wonder if we need something like struct arm_smmu_subdomain to
represent a substream, because now an IOMMU_DOMAIN_SVA domain is
represented by this struct arm_smmu_domain too, which is neither
of the two cases in the list above.

Thanks
Nicolin
Nicolin Chen July 13, 2023, 4:25 a.m. UTC | #2 
On Wed, Jul 12, 2023 at 06:57:33PM -0700, Nicolin Chen wrote:
> On Wed, Jun 21, 2023 at 02:37:17PM +0800, Michael Shavit wrote:
>  
> > Insert CDs for STAGE_1 domains into a CD table owned by the
> > arm_smmu_master. Remove the CD table that was owned by arm_smmu_domain.
> 
> > @@ -718,10 +718,7 @@ struct arm_smmu_domain {
> > 
> >         enum arm_smmu_domain_stage              stage;
> >         union {
> > -               struct {
> >                 struct arm_smmu_ctx_desc        cd;
> > -               struct arm_smmu_s1_cfg          s1_cfg;
> > -               };
> >                 struct arm_smmu_s2_cfg          s2_cfg;
> >         };
> 
> So the arm_smmu_domain looks like an object representing either:
> 1) a CD table + (if !IDENTITY) an S1 IOPT for default substream
> 2) an S2 IOPT

["]
> I wonder if we need something like struct arm_smmu_subdomain to
> represent a substream, because now an IOMMU_DOMAIN_SVA domain is
> represented by this struct arm_smmu_domain too, which is neither
> of the two cases in the list above.
["]

I got this part incorrect: an SVA domain is actually represented
by an arm_smmu_sva_domain, so there's no nested arm_smmu_domain
situation. And we use arm_smmu_domain for other purpose, where I
got confused.

Please ignore this part, though I am still wondering if we could
name it in a way having a clear topology.

Thanks
Nicolin
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 b6f7cf60f8f3d..08f440fe1da6d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2065,12 +2065,8 @@  static void arm_smmu_domain_free(struct iommu_domain *domain)
 
 	/* Free the CD and ASID, if we allocated them */
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg;
-
 		/* Prevent SVA from touching the CD while we're freeing it */
 		mutex_lock(&arm_smmu_asid_lock);
-		if (cfg->cdcfg.cdtab)
-			arm_smmu_free_cd_tables(smmu, &cfg->cdcfg);
 		arm_smmu_free_asid(&smmu_domain->cd);
 		mutex_unlock(&arm_smmu_asid_lock);
 	} else {
@@ -2082,14 +2078,13 @@  static void arm_smmu_domain_free(struct iommu_domain *domain)
 	kfree(smmu_domain);
 }
 
-static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
+static int arm_smmu_domain_finalise_cd(struct arm_smmu_domain *smmu_domain,
 				       struct arm_smmu_master *master,
 				       struct io_pgtable_cfg *pgtbl_cfg)
 {
 	int ret;
 	u32 asid;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
-	struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg;
 	struct arm_smmu_ctx_desc *cd = &smmu_domain->cd;
 	typeof(&pgtbl_cfg->arm_lpae_s1_cfg.tcr) tcr = &pgtbl_cfg->arm_lpae_s1_cfg.tcr;
 
@@ -2102,10 +2097,6 @@  static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 	if (ret)
 		goto out_unlock;
 
-	ret = arm_smmu_init_s1_cfg(master, cfg);
-	if (ret)
-		goto out_free_asid;
-
 	cd->asid	= (u16)asid;
 	cd->ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
 	cd->tcr		= FIELD_PREP(CTXDESC_CD_0_TCR_T0SZ, tcr->tsz) |
@@ -2117,19 +2108,9 @@  static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 			  CTXDESC_CD_0_TCR_EPD1 | CTXDESC_CD_0_AA64;
 	cd->mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair;
 
-	ret = arm_smmu_write_ctx_desc(smmu, cfg,
-				      NULL /*Not attached to a master yet */,
-				      0, cd);
-	if (ret)
-		goto out_free_cd_tables;
-
 	mutex_unlock(&arm_smmu_asid_lock);
 	return 0;
 
-out_free_cd_tables:
-	arm_smmu_free_cd_tables(smmu, &cfg->cdcfg);
-out_free_asid:
-	arm_smmu_free_asid(cd);
 out_unlock:
 	mutex_unlock(&arm_smmu_asid_lock);
 	return ret;
@@ -2192,7 +2173,7 @@  static int arm_smmu_domain_finalise(struct iommu_domain *domain,
 		ias = min_t(unsigned long, ias, VA_BITS);
 		oas = smmu->ias;
 		fmt = ARM_64_LPAE_S1;
-		finalise_stage_fn = arm_smmu_domain_finalise_s1;
+		finalise_stage_fn = arm_smmu_domain_finalise_cd;
 		break;
 	case ARM_SMMU_DOMAIN_NESTED:
 	case ARM_SMMU_DOMAIN_S2:
@@ -2439,20 +2420,20 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	} else if (smmu_domain->smmu != smmu) {
 		ret = -EINVAL;
 		goto out_unlock;
-	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
-		   master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) {
-		ret = -EINVAL;
-		goto out_unlock;
-	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
-		   smmu_domain->s1_cfg.stall_enabled !=
-			   master->stall_enabled) {
-		ret = -EINVAL;
-		goto out_unlock;
 	}
 
 	master->domain = smmu_domain;
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		master->s1_cfg = &smmu_domain->s1_cfg;
+		master->s1_cfg = &master->owned_s1_cfg;
+		ret = arm_smmu_write_ctx_desc(
+			smmu,
+			master->s1_cfg, NULL /*Not attached to a master yet */,
+			0, &smmu_domain->cd);
+		if (ret) {
+			master->s1_cfg = NULL;
+			master->domain = NULL;
+			goto out_unlock;
+		}
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 ||
 		   smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED) {
 		master->s2_cfg = &smmu_domain->s2_cfg;
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 00a493442d6f9..dff0fa8345462 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -718,10 +718,7 @@  struct arm_smmu_domain {
 
 	enum arm_smmu_domain_stage		stage;
 	union {
-		struct {
 		struct arm_smmu_ctx_desc	cd;
-		struct arm_smmu_s1_cfg		s1_cfg;
-		};
 		struct arm_smmu_s2_cfg		s2_cfg;
 	};