Re: [PATCH v12 04/15] iommu/smmuv3: Dynamically allocate s1_cfg and s2_cfg
Hi Shameer, On 11/17/20 12:39 PM, Shameerali Kolothum Thodi wrote: > Hi Eric, > >> -Original Message- >> From: Eric Auger [mailto:eric.au...@redhat.com] >> Sent: 16 November 2020 10:43 >> To: eric.auger@gmail.com; eric.au...@redhat.com; >> io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org; >> k...@vger.kernel.org; kvmarm@lists.cs.columbia.edu; w...@kernel.org; >> j...@8bytes.org; m...@kernel.org; robin.mur...@arm.com >> Cc: jean-phili...@linaro.org; zhangfei@linaro.org; >> zhangfei@gmail.com; vivek.gau...@arm.com; Shameerali Kolothum >> Thodi ; >> alex.william...@redhat.com; jacob.jun@linux.intel.com; >> yi.l@intel.com; t...@semihalf.com; nicoleots...@gmail.com >> Subject: [PATCH v12 04/15] iommu/smmuv3: Dynamically allocate s1_cfg and >> s2_cfg >> >> In preparation for the introduction of nested stages >> let's turn s1_cfg and s2_cfg fields into pointers which are >> dynamically allocated depending on the smmu_domain stage. > > This will break compile if we have CONFIG_ARM_SMMU_V3_SVA > because , > https://github.com/eauger/linux/blob/5.10-rc4-2stage-v12/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c#L40 > > Do we really need to make these pointers? Thanks for reporting. I think I can do differently. Working on this now. Thanks Eric > > Thanks, > Shameer > >> In nested mode, both stages will coexist and s1_cfg will >> be allocated when the guest configuration gets passed. >> >> Signed-off-by: Eric Auger >> --- >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 83 - >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 6 +- >> 2 files changed, 48 insertions(+), 41 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 d828d6cbeb0e..4baf9fafe462 100644 >> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >> @@ -953,9 +953,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct >> arm_smmu_domain *smmu_domain, >> unsigned int idx; >> struct arm_smmu_l1_ctx_desc *l1_desc; >> struct arm_smmu_device *smmu = smmu_domain->smmu; >> -struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg; >> +struct arm_smmu_ctx_desc_cfg *cdcfg = >> _domain->s1_cfg->cdcfg; >> >> -if (smmu_domain->s1_cfg.s1fmt == STRTAB_STE_0_S1FMT_LINEAR) >> +if (smmu_domain->s1_cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR) >> return cdcfg->cdtab + ssid * CTXDESC_CD_DWORDS; >> >> idx = ssid >> CTXDESC_SPLIT; >> @@ -990,7 +990,7 @@ int arm_smmu_write_ctx_desc(struct >> arm_smmu_domain *smmu_domain, int ssid, >> __le64 *cdptr; >> struct arm_smmu_device *smmu = smmu_domain->smmu; >> >> -if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg.s1cdmax))) >> +if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg->s1cdmax))) >> return -E2BIG; >> >> cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid); >> @@ -1056,7 +1056,7 @@ static int arm_smmu_alloc_cd_tables(struct >> arm_smmu_domain *smmu_domain) >> size_t l1size; >> size_t max_contexts; >> struct arm_smmu_device *smmu = smmu_domain->smmu; >> -struct arm_smmu_s1_cfg *cfg = _domain->s1_cfg; >> +struct arm_smmu_s1_cfg *cfg = smmu_domain->s1_cfg; >> struct arm_smmu_ctx_desc_cfg *cdcfg = >cdcfg; >> >> max_contexts = 1 << cfg->s1cdmax; >> @@ -1104,7 +1104,7 @@ static void arm_smmu_free_cd_tables(struct >> arm_smmu_domain *smmu_domain) >> int i; >> size_t size, l1size; >> struct arm_smmu_device *smmu = smmu_domain->smmu; >> -struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg; >> +struct arm_smmu_ctx_desc_cfg *cdcfg = >> _domain->s1_cfg->cdcfg; >> >> if (cdcfg->l1_desc) { >> size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3); >> @@ -1211,17 +1211,8 @@ static void arm_smmu_write_strtab_ent(struct >> arm_smmu_master *master, u32 sid, >> } >> >> if (smmu_domain) { >> -switch (smmu_domain->stage) { >> -case ARM_SMMU_DOMAIN_S1: >> -s1_cfg = _domain->s1_cfg; >> -break; >> -case ARM_SMMU_DOMAIN_S2: >> -case ARM_SMMU_DOMAIN_NESTED: >> -s2_cfg = _domain->s2_cfg; >> -break; >> -default: >> -break; >> -} >> +s1_cfg = smmu_domain->s1_cfg; >> +s2_cfg = smmu_domain->s2_cfg; >> } >> >> if (val & STRTAB_STE_0_V) { >> @@ -1664,10 +1655,10 @@ static void arm_smmu_tlb_inv_context(void >> *cookie) >> * careful, 007. >> */ >> if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { >> -arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); >> +arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg->cd.asid); >> } else { >> cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; >> -cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; >> +
RE: [PATCH v12 04/15] iommu/smmuv3: Dynamically allocate s1_cfg and s2_cfg
Hi Eric, > -Original Message- > From: Eric Auger [mailto:eric.au...@redhat.com] > Sent: 16 November 2020 10:43 > To: eric.auger@gmail.com; eric.au...@redhat.com; > io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org; > k...@vger.kernel.org; kvmarm@lists.cs.columbia.edu; w...@kernel.org; > j...@8bytes.org; m...@kernel.org; robin.mur...@arm.com > Cc: jean-phili...@linaro.org; zhangfei@linaro.org; > zhangfei@gmail.com; vivek.gau...@arm.com; Shameerali Kolothum > Thodi ; > alex.william...@redhat.com; jacob.jun@linux.intel.com; > yi.l@intel.com; t...@semihalf.com; nicoleots...@gmail.com > Subject: [PATCH v12 04/15] iommu/smmuv3: Dynamically allocate s1_cfg and > s2_cfg > > In preparation for the introduction of nested stages > let's turn s1_cfg and s2_cfg fields into pointers which are > dynamically allocated depending on the smmu_domain stage. This will break compile if we have CONFIG_ARM_SMMU_V3_SVA because , https://github.com/eauger/linux/blob/5.10-rc4-2stage-v12/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c#L40 Do we really need to make these pointers? Thanks, Shameer > In nested mode, both stages will coexist and s1_cfg will > be allocated when the guest configuration gets passed. > > Signed-off-by: Eric Auger > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 83 - > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 6 +- > 2 files changed, 48 insertions(+), 41 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 d828d6cbeb0e..4baf9fafe462 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -953,9 +953,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct > arm_smmu_domain *smmu_domain, > unsigned int idx; > struct arm_smmu_l1_ctx_desc *l1_desc; > struct arm_smmu_device *smmu = smmu_domain->smmu; > - struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg; > + struct arm_smmu_ctx_desc_cfg *cdcfg = > _domain->s1_cfg->cdcfg; > > - if (smmu_domain->s1_cfg.s1fmt == STRTAB_STE_0_S1FMT_LINEAR) > + if (smmu_domain->s1_cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR) > return cdcfg->cdtab + ssid * CTXDESC_CD_DWORDS; > > idx = ssid >> CTXDESC_SPLIT; > @@ -990,7 +990,7 @@ int arm_smmu_write_ctx_desc(struct > arm_smmu_domain *smmu_domain, int ssid, > __le64 *cdptr; > struct arm_smmu_device *smmu = smmu_domain->smmu; > > - if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg.s1cdmax))) > + if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg->s1cdmax))) > return -E2BIG; > > cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid); > @@ -1056,7 +1056,7 @@ static int arm_smmu_alloc_cd_tables(struct > arm_smmu_domain *smmu_domain) > size_t l1size; > size_t max_contexts; > struct arm_smmu_device *smmu = smmu_domain->smmu; > - struct arm_smmu_s1_cfg *cfg = _domain->s1_cfg; > + struct arm_smmu_s1_cfg *cfg = smmu_domain->s1_cfg; > struct arm_smmu_ctx_desc_cfg *cdcfg = >cdcfg; > > max_contexts = 1 << cfg->s1cdmax; > @@ -1104,7 +1104,7 @@ static void arm_smmu_free_cd_tables(struct > arm_smmu_domain *smmu_domain) > int i; > size_t size, l1size; > struct arm_smmu_device *smmu = smmu_domain->smmu; > - struct arm_smmu_ctx_desc_cfg *cdcfg = _domain->s1_cfg.cdcfg; > + struct arm_smmu_ctx_desc_cfg *cdcfg = > _domain->s1_cfg->cdcfg; > > if (cdcfg->l1_desc) { > size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3); > @@ -1211,17 +1211,8 @@ static void arm_smmu_write_strtab_ent(struct > arm_smmu_master *master, u32 sid, > } > > if (smmu_domain) { > - switch (smmu_domain->stage) { > - case ARM_SMMU_DOMAIN_S1: > - s1_cfg = _domain->s1_cfg; > - break; > - case ARM_SMMU_DOMAIN_S2: > - case ARM_SMMU_DOMAIN_NESTED: > - s2_cfg = _domain->s2_cfg; > - break; > - default: > - break; > - } > + s1_cfg = smmu_domain->s1_cfg; > + s2_cfg = smmu_domain->s2_cfg; > } > > if (val & STRTAB_STE_0_V) { > @@ -1664,10 +1655,10 @@ static void arm_smmu_tlb_inv_context(void > *cookie) >* careful, 007. >*/ > if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > - arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); > + arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg->cd.asid); > } else { > cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; > - cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; > + cmd.tlbi.vmid = smmu_domain->s2_cfg->vmid; > arm_smmu_cmdq_issue_cmd(smmu, ); > arm_smmu_cmdq_issue_sync(smmu); > } > @@ -1693,10 +1684,10 @@ static void arm_smmu_tlb_inv_range(unsigned