Re: [PATCH v3 10/13] iommu/arm-smmu-v3: Add second level of context descriptor table
On Wed, Dec 18, 2019 at 10:59:36AM +0100, Auger Eric wrote: > > struct arm_smmu_s1_cfg { > > - struct arm_smmu_cd_tabletable; > > + struct arm_smmu_cd_table*tables; > > + size_t num_tables; > > + __le64 *l1ptr; > you may add a comment saying that l1ptr and l1ptr_dma are only set/used > in non linear case and one comment saying that "tables" represent leaf > tables. I now have /* Leaf tables or linear table */ and /* First level tables, when two level are used */ but I'm not entirely convinced it adds value > > + dma_addr_t l1ptr_dma; > > struct arm_smmu_ctx_desccd; > > u8 s1fmt; > > u8 s1cdmax; > > @@ -1521,9 +1538,53 @@ static void arm_smmu_free_cd_leaf_table(struct > > arm_smmu_device *smmu, > > { > > size_t size = num_entries * (CTXDESC_CD_DWORDS << 3); > > > > + if (!table->ptr) > > + return; > > dmam_free_coherent(smmu->dev, size, table->ptr, table->ptr_dma); > > } > > > > +static void arm_smmu_write_cd_l1_desc(__le64 *dst, > > + struct arm_smmu_cd_table *table) > > +{ > > + u64 val = (table->ptr_dma & CTXDESC_L1_DESC_L2PTR_MASK) | > > + CTXDESC_L1_DESC_VALID; > > + > > + WRITE_ONCE(*dst, cpu_to_le64(val)); > > +} > > + > > +static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain, > > + u32 ssid) > > +{ > > + __le64 *l1ptr; > > + unsigned int idx; > > + struct arm_smmu_cd_table *table; > > + struct arm_smmu_device *smmu = smmu_domain->smmu; > > + struct arm_smmu_s1_cfg *cfg = _domain->s1_cfg; > > + > > + if (cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR) { > > + table = >tables[0]; > > + idx = ssid; > > + } else { > nit: you may avoid this extra indent by either returning above or go to > a label. > > + idx = ssid >> CTXDESC_SPLIT; > > + if (idx >= cfg->num_tables) > > + return NULL; > > + > > + table = >tables[idx]; > > + if (!table->ptr) { > > + if (arm_smmu_alloc_cd_leaf_table(smmu, table, > > +CTXDESC_L2_ENTRIES)) > > + return NULL; > > + > > + l1ptr = cfg->l1ptr + idx * CTXDESC_L1_DESC_DWORDS; > > + arm_smmu_write_cd_l1_desc(l1ptr, table); > > + /* An invalid L1CD can be cached */ > > + arm_smmu_sync_cd(smmu_domain, ssid, false); > > + } > > + idx = ssid & (CTXDESC_L2_ENTRIES - 1); > > + } > > + return table->ptr + idx * CTXDESC_CD_DWORDS;> +} > > + > > static u64 arm_smmu_cpu_tcr_to_cd(u64 tcr) > > { > > u64 val = 0; > > @@ -1556,8 +1617,10 @@ static int arm_smmu_write_ctx_desc(struct > > arm_smmu_domain *smmu_domain, > > u64 val; > > bool cd_live; > > struct arm_smmu_device *smmu = smmu_domain->smmu; > > - __le64 *cdptr = smmu_domain->s1_cfg.table.ptr + ssid * > > - CTXDESC_CD_DWORDS; > > + __le64 *cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid); > > + > > + if (!cdptr) > > + return -ENOMEM; > -ENOMEM does not fit well with (idx >= cfg->num_tables) case > Besides the idx is checked against the max table capacity only in non > linear mode. Can't you check the ssid against cfg->s1cdmax earlier? Ok, I'll move that check here Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 10/13] iommu/arm-smmu-v3: Add second level of context descriptor table
Hi Jean, On 12/9/19 7:05 PM, Jean-Philippe Brucker wrote: > The SMMU can support up to 20 bits of SSID. Add a second level of page > tables to accommodate this. Devices that support more than 1024 SSIDs now > have a table of 1024 L1 entries (8kB), pointing to tables of 1024 context > descriptors (64kB), allocated on demand. > > Signed-off-by: Jean-Philippe Brucker > --- > drivers/iommu/arm-smmu-v3.c | 153 +--- > 1 file changed, 143 insertions(+), 10 deletions(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index fc5119f34187..52adcdfda58b 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -224,6 +224,7 @@ > > #define STRTAB_STE_0_S1FMT GENMASK_ULL(5, 4) > #define STRTAB_STE_0_S1FMT_LINEAR0 > +#define STRTAB_STE_0_S1FMT_64K_L22 > #define STRTAB_STE_0_S1CTXPTR_MASK GENMASK_ULL(51, 6) > #define STRTAB_STE_0_S1CDMAX GENMASK_ULL(63, 59) > > @@ -263,7 +264,20 @@ > > #define STRTAB_STE_3_S2TTB_MASK GENMASK_ULL(51, 4) > > -/* Context descriptor (stage-1 only) */ > +/* > + * Context descriptors. > + * > + * Linear: when less than 1024 SSIDs are supported > + * 2lvl: at most 1024 L1 entries, > + * 1024 lazy entries per table. > + */ > +#define CTXDESC_SPLIT10 > +#define CTXDESC_L2_ENTRIES (1 << CTXDESC_SPLIT) > + > +#define CTXDESC_L1_DESC_DWORDS 1 > +#define CTXDESC_L1_DESC_VALID1 > +#define CTXDESC_L1_DESC_L2PTR_MASK GENMASK_ULL(51, 12) > + > #define CTXDESC_CD_DWORDS8 > #define CTXDESC_CD_0_TCR_T0SZGENMASK_ULL(5, 0) > #define ARM64_TCR_T0SZ GENMASK_ULL(5, 0) > @@ -575,7 +589,10 @@ struct arm_smmu_cd_table { > }; > > struct arm_smmu_s1_cfg { > - struct arm_smmu_cd_tabletable; > + struct arm_smmu_cd_table*tables; > + size_t num_tables; > + __le64 *l1ptr; you may add a comment saying that l1ptr and l1ptr_dma are only set/used in non linear case and one comment saying that "tables" represent leaf tables. > + dma_addr_t l1ptr_dma; > struct arm_smmu_ctx_desccd; > u8 s1fmt; > u8 s1cdmax; > @@ -1521,9 +1538,53 @@ static void arm_smmu_free_cd_leaf_table(struct > arm_smmu_device *smmu, > { > size_t size = num_entries * (CTXDESC_CD_DWORDS << 3); > > + if (!table->ptr) > + return; > dmam_free_coherent(smmu->dev, size, table->ptr, table->ptr_dma); > } > > +static void arm_smmu_write_cd_l1_desc(__le64 *dst, > + struct arm_smmu_cd_table *table) > +{ > + u64 val = (table->ptr_dma & CTXDESC_L1_DESC_L2PTR_MASK) | > + CTXDESC_L1_DESC_VALID; > + > + WRITE_ONCE(*dst, cpu_to_le64(val)); > +} > + > +static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain, > +u32 ssid) > +{ > + __le64 *l1ptr; > + unsigned int idx; > + struct arm_smmu_cd_table *table; > + struct arm_smmu_device *smmu = smmu_domain->smmu; > + struct arm_smmu_s1_cfg *cfg = _domain->s1_cfg; > + > + if (cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR) { > + table = >tables[0]; > + idx = ssid; > + } else { nit: you may avoid this extra indent by either returning above or go to a label. > + idx = ssid >> CTXDESC_SPLIT; > + if (idx >= cfg->num_tables) > + return NULL; > + > + table = >tables[idx]; > + if (!table->ptr) { > + if (arm_smmu_alloc_cd_leaf_table(smmu, table, > + CTXDESC_L2_ENTRIES)) > + return NULL; > + > + l1ptr = cfg->l1ptr + idx * CTXDESC_L1_DESC_DWORDS; > + arm_smmu_write_cd_l1_desc(l1ptr, table); > + /* An invalid L1CD can be cached */ > + arm_smmu_sync_cd(smmu_domain, ssid, false); > + } > + idx = ssid & (CTXDESC_L2_ENTRIES - 1); > + } > + return table->ptr + idx * CTXDESC_CD_DWORDS;> +} > + > static u64 arm_smmu_cpu_tcr_to_cd(u64 tcr) > { > u64 val = 0; > @@ -1556,8 +1617,10 @@ static int arm_smmu_write_ctx_desc(struct > arm_smmu_domain *smmu_domain, > u64 val; > bool cd_live; > struct arm_smmu_device *smmu = smmu_domain->smmu; > - __le64 *cdptr = smmu_domain->s1_cfg.table.ptr + ssid * > - CTXDESC_CD_DWORDS; > + __le64 *cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid); > + > + if (!cdptr) > + return -ENOMEM; -ENOMEM does not fit well with (idx >= cfg->num_tables) case Besides the idx is checked against the max table capacity only in non linear mode. Can't you check the
Re: [PATCH v3 10/13] iommu/arm-smmu-v3: Add second level of context descriptor table
On Mon, 9 Dec 2019 19:05:11 +0100 Jean-Philippe Brucker wrote: > The SMMU can support up to 20 bits of SSID. Add a second level of page > tables to accommodate this. Devices that support more than 1024 SSIDs now > have a table of 1024 L1 entries (8kB), pointing to tables of 1024 context > descriptors (64kB), allocated on demand. > > Signed-off-by: Jean-Philippe Brucker One tiny little comment inline. I really don't mind if you ignore it ;) Reviewed-by: Jonathan Cameron > --- > drivers/iommu/arm-smmu-v3.c | 153 +--- > 1 file changed, 143 insertions(+), 10 deletions(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index fc5119f34187..52adcdfda58b 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -224,6 +224,7 @@ > > #define STRTAB_STE_0_S1FMT GENMASK_ULL(5, 4) > #define STRTAB_STE_0_S1FMT_LINEAR0 > +#define STRTAB_STE_0_S1FMT_64K_L22 > #define STRTAB_STE_0_S1CTXPTR_MASK GENMASK_ULL(51, 6) > #define STRTAB_STE_0_S1CDMAX GENMASK_ULL(63, 59) > > @@ -263,7 +264,20 @@ > > #define STRTAB_STE_3_S2TTB_MASK GENMASK_ULL(51, 4) > > -/* Context descriptor (stage-1 only) */ > +/* > + * Context descriptors. > + * > + * Linear: when less than 1024 SSIDs are supported > + * 2lvl: at most 1024 L1 entries, > + * 1024 lazy entries per table. > + */ > +#define CTXDESC_SPLIT10 > +#define CTXDESC_L2_ENTRIES (1 << CTXDESC_SPLIT) > + > +#define CTXDESC_L1_DESC_DWORDS 1 > +#define CTXDESC_L1_DESC_VALID1 > +#define CTXDESC_L1_DESC_L2PTR_MASK GENMASK_ULL(51, 12) > + > #define CTXDESC_CD_DWORDS8 > #define CTXDESC_CD_0_TCR_T0SZGENMASK_ULL(5, 0) > #define ARM64_TCR_T0SZ GENMASK_ULL(5, 0) > @@ -575,7 +589,10 @@ struct arm_smmu_cd_table { > }; > > struct arm_smmu_s1_cfg { > - struct arm_smmu_cd_tabletable; > + struct arm_smmu_cd_table*tables; > + size_t num_tables; > + __le64 *l1ptr; > + dma_addr_t l1ptr_dma; > struct arm_smmu_ctx_desccd; > u8 s1fmt; > u8 s1cdmax; > @@ -1521,9 +1538,53 @@ static void arm_smmu_free_cd_leaf_table(struct > arm_smmu_device *smmu, > { > size_t size = num_entries * (CTXDESC_CD_DWORDS << 3); > > + if (!table->ptr) > + return; > dmam_free_coherent(smmu->dev, size, table->ptr, table->ptr_dma); > } > > +static void arm_smmu_write_cd_l1_desc(__le64 *dst, > + struct arm_smmu_cd_table *table) > +{ > + u64 val = (table->ptr_dma & CTXDESC_L1_DESC_L2PTR_MASK) | > + CTXDESC_L1_DESC_VALID; > + > + WRITE_ONCE(*dst, cpu_to_le64(val)); > +} > + > +static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain, > +u32 ssid) > +{ > + __le64 *l1ptr; > + unsigned int idx; > + struct arm_smmu_cd_table *table; > + struct arm_smmu_device *smmu = smmu_domain->smmu; > + struct arm_smmu_s1_cfg *cfg = _domain->s1_cfg; > + > + if (cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR) { > + table = >tables[0]; > + idx = ssid; > + } else { > + idx = ssid >> CTXDESC_SPLIT; > + if (idx >= cfg->num_tables) > + return NULL; > + > + table = >tables[idx]; > + if (!table->ptr) { > + if (arm_smmu_alloc_cd_leaf_table(smmu, table, > + CTXDESC_L2_ENTRIES)) > + return NULL; > + > + l1ptr = cfg->l1ptr + idx * CTXDESC_L1_DESC_DWORDS; > + arm_smmu_write_cd_l1_desc(l1ptr, table); > + /* An invalid L1CD can be cached */ > + arm_smmu_sync_cd(smmu_domain, ssid, false); > + } > + idx = ssid & (CTXDESC_L2_ENTRIES - 1); > + } > + return table->ptr + idx * CTXDESC_CD_DWORDS; > +} > + > static u64 arm_smmu_cpu_tcr_to_cd(u64 tcr) > { > u64 val = 0; > @@ -1556,8 +1617,10 @@ static int arm_smmu_write_ctx_desc(struct > arm_smmu_domain *smmu_domain, > u64 val; > bool cd_live; > struct arm_smmu_device *smmu = smmu_domain->smmu; > - __le64 *cdptr = smmu_domain->s1_cfg.table.ptr + ssid * > - CTXDESC_CD_DWORDS; > + __le64 *cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid); > + > + if (!cdptr) > + return -ENOMEM; > > val = le64_to_cpu(cdptr[0]); > cd_live = !!(val & CTXDESC_CD_0_V); > @@ -1604,20 +1667,87 @@ static int arm_smmu_write_ctx_desc(struct > arm_smmu_domain *smmu_domain, > > static int arm_smmu_alloc_cd_tables(struct arm_smmu_domain *smmu_domain) > { > + int