[v4,05/13] iommu/arm-smmu-v3: Use the master-owned s1_cfg
Commit Message
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
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
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
@@ -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;
@@ -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;
};