| Message ID | 4-v1-1b720dce51d1+4f44-smmuv3_tidy_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Tidy some minor things in the stream table/cd table area | expand |
Hi Jason, On Mon, Jun 03, 2024 at 07:31:30PM -0300, Jason Gunthorpe wrote: > This is being used as both an array of CDs and an array of L1 descriptors. > > Give the two usages different names and correct types. > > Remove CTXDESC_CD_DWORDS as most usages were indexing or sizing an array > of struct arm_smmu_cd. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 60 ++++++++++----------- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 17 ++++-- > 2 files changed, 41 insertions(+), 36 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 735dd9ff61890e..24c286a0834445 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1170,7 +1170,7 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master, > static int arm_smmu_alloc_cd_leaf_table(struct arm_smmu_device *smmu, > struct arm_smmu_l1_ctx_desc *l1_desc) > { > - size_t size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3); > + size_t size = CTXDESC_L2_ENTRIES * sizeof(struct arm_smmu_cd); > > l1_desc->l2ptr = dmam_alloc_coherent(smmu->dev, size, > &l1_desc->l2ptr_dma, GFP_KERNEL); > @@ -1198,12 +1198,11 @@ struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, > struct arm_smmu_l1_ctx_desc *l1_desc; > struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table; > > - if (!cd_table->cdtab) > + if (!arm_smmu_cdtab_allocated(cd_table)) > return NULL; > > if (cd_table->s1fmt == STRTAB_STE_0_S1FMT_LINEAR) > - return (struct arm_smmu_cd *)(cd_table->cdtab + > - ssid * CTXDESC_CD_DWORDS); > + return &cd_table->cdtab.linear[ssid]; > > l1_desc = &cd_table->l1_desc[ssid / CTXDESC_L2_ENTRIES]; > if (!l1_desc->l2ptr) > @@ -1220,7 +1219,7 @@ struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master, > might_sleep(); > iommu_group_mutex_assert(master->dev); > > - if (!cd_table->cdtab) { > + if (!arm_smmu_cdtab_allocated(cd_table)) { > if (arm_smmu_alloc_cd_tables(master)) > return NULL; > } > @@ -1236,7 +1235,7 @@ struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master, > if (arm_smmu_alloc_cd_leaf_table(smmu, l1_desc)) > return NULL; > > - l1ptr = cd_table->cdtab + idx * CTXDESC_L1_DESC_DWORDS; Similarly to patch 1, I believe we can get rid of CTXDESC_L1_DESC_DWORDS entirely. > + l1ptr = &cd_table->cdtab.l1_desc[idx]; > arm_smmu_write_cd_l1_desc(l1ptr, l1_desc); > /* An invalid L1CD can be cached */ > arm_smmu_sync_cd(master, ssid, false); > @@ -1342,7 +1341,7 @@ void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid) > struct arm_smmu_cd target = {}; > struct arm_smmu_cd *cdptr; > > - if (!master->cd_table.cdtab) > + if (!arm_smmu_cdtab_allocated(&master->cd_table)) > return; > cdptr = arm_smmu_get_cd_ptr(master, ssid); > if (WARN_ON(!cdptr)) > @@ -1352,8 +1351,6 @@ void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid) > > static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master) > { > - int ret; > - size_t l1size; > size_t max_contexts; > struct arm_smmu_device *smmu = master->smmu; > struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table; > @@ -1366,8 +1363,14 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master) > cd_table->s1fmt = STRTAB_STE_0_S1FMT_LINEAR; > cd_table->num_l1_ents = max_contexts; > > - l1size = max_contexts * (CTXDESC_CD_DWORDS << 3); > + cd_table->cdtab.linear = dmam_alloc_coherent( > + smmu->dev, max_contexts * sizeof(struct arm_smmu_cd), > + &cd_table->cdtab_dma, GFP_KERNEL); > + if (!cd_table->cdtab.linear) > + return -ENOMEM; > } else { > + size_t l1size; > + > cd_table->s1fmt = STRTAB_STE_0_S1FMT_64K_L2; > cd_table->num_l1_ents = DIV_ROUND_UP(max_contexts, > CTXDESC_L2_ENTRIES); > @@ -1379,24 +1382,15 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master) > return -ENOMEM; > > l1size = cd_table->num_l1_ents * (CTXDESC_L1_DESC_DWORDS << 3); > + cd_table->cdtab.l1_desc = dmam_alloc_coherent( > + smmu->dev, l1size, &cd_table->cdtab_dma, GFP_KERNEL); > + if (!cd_table->cdtab.l1_desc) { > + devm_kfree(smmu->dev, cd_table->l1_desc); > + cd_table->l1_desc = NULL; > + return -ENOMEM; > + } > } > - > - cd_table->cdtab = dmam_alloc_coherent(smmu->dev, l1size, &cd_table->cdtab_dma, > - GFP_KERNEL); > - if (!cd_table->cdtab) { > - dev_warn(smmu->dev, "failed to allocate context descriptor\n"); > - ret = -ENOMEM; > - goto err_free_l1; > - } > - > return 0; > - > -err_free_l1: > - if (cd_table->l1_desc) { > - devm_kfree(smmu->dev, cd_table->l1_desc); > - cd_table->l1_desc = NULL; > - } > - return ret; > } > > static void arm_smmu_free_cd_tables(struct arm_smmu_master *master) > @@ -1407,7 +1401,7 @@ static void arm_smmu_free_cd_tables(struct arm_smmu_master *master) > struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table; > > if (cd_table->l1_desc) { > - size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3); > + size = CTXDESC_L2_ENTRIES * sizeof(struct arm_smmu_cd); > > for (i = 0; i < cd_table->num_l1_ents; i++) { > if (!cd_table->l1_desc[i].l2ptr) > @@ -1421,13 +1415,17 @@ static void arm_smmu_free_cd_tables(struct arm_smmu_master *master) > cd_table->l1_desc = NULL; > > l1size = cd_table->num_l1_ents * (CTXDESC_L1_DESC_DWORDS << 3); > + dmam_free_coherent(smmu->dev, l1size, cd_table->cdtab.l1_desc, > + cd_table->cdtab_dma); > } else { > - l1size = cd_table->num_l1_ents * (CTXDESC_CD_DWORDS << 3); > + dmam_free_coherent(smmu->dev, > + cd_table->num_l1_ents * > + sizeof(struct arm_smmu_cd), > + cd_table->cdtab.linear, cd_table->cdtab_dma); > } > > - dmam_free_coherent(smmu->dev, l1size, cd_table->cdtab, cd_table->cdtab_dma); > cd_table->cdtab_dma = 0; > - cd_table->cdtab = NULL; > + cd_table->cdtab.linear = NULL; nit: AFAIU, arm_smmu_cdtab_allocated() was added to check the cd table existence regardless the config. So, I think we should be consistent, as here it is set as linear, while it can be 2lvl. > } > > bool arm_smmu_free_asid(struct arm_smmu_ctx_desc *cd) > @@ -2955,7 +2953,7 @@ static void arm_smmu_release_device(struct device *dev) > > arm_smmu_disable_pasid(master); > arm_smmu_remove_master(master); > - if (master->cd_table.cdtab) > + if (arm_smmu_cdtab_allocated(&master->cd_table)) > arm_smmu_free_cd_tables(master); > kfree(master); > } > 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 280a04bfb7230c..21c1acf34dd29c 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -279,10 +279,8 @@ struct arm_smmu_ste { > #define CTXDESC_L1_DESC_V (1UL << 0) > #define CTXDESC_L1_DESC_L2PTR_MASK GENMASK_ULL(51, 12) > > -#define CTXDESC_CD_DWORDS 8 > - > struct arm_smmu_cd { > - __le64 data[CTXDESC_CD_DWORDS]; > + __le64 data[8]; > }; > > #define CTXDESC_CD_0_TCR_T0SZ GENMASK_ULL(5, 0) > @@ -312,7 +310,7 @@ struct arm_smmu_cd { > * When the SMMU only supports linear context descriptor tables, pick a > * reasonable size limit (64kB). > */ > -#define CTXDESC_LINEAR_CDMAX ilog2(SZ_64K / (CTXDESC_CD_DWORDS << 3)) > +#define CTXDESC_LINEAR_CDMAX ilog2(SZ_64K / sizeof(struct arm_smmu_cd)) > > /* Command queue */ > #define CMDQ_ENT_SZ_SHIFT 4 > @@ -593,7 +591,10 @@ struct arm_smmu_l1_ctx_desc { > }; > > struct arm_smmu_ctx_desc_cfg { > - __le64 *cdtab; > + union { > + struct arm_smmu_cd *linear; > + __le64 *l1_desc; As patch-1 also, I think naming is confusing. > + } cdtab; > dma_addr_t cdtab_dma; > struct arm_smmu_l1_ctx_desc *l1_desc; > unsigned int num_l1_ents; > @@ -602,6 +603,12 @@ struct arm_smmu_ctx_desc_cfg { > u8 s1cdmax; > }; > > +static inline bool > +arm_smmu_cdtab_allocated(struct arm_smmu_ctx_desc_cfg *cd_table) > +{ > + return cd_table->cdtab.linear; > +} > + > struct arm_smmu_s2_cfg { > u16 vmid; > }; > -- > 2.45.2 > Thanks, Mostafa
On Tue, Jun 04, 2024 at 04:07:21PM +0000, Mostafa Saleh wrote: > > @@ -1236,7 +1235,7 @@ struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master, > > if (arm_smmu_alloc_cd_leaf_table(smmu, l1_desc)) > > return NULL; > > > > - l1ptr = cd_table->cdtab + idx * CTXDESC_L1_DESC_DWORDS; > > Similarly to patch 1, I believe we can get rid of CTXDESC_L1_DESC_DWORDS entirely. I gave CD the same treatment, it ends up like: struct arm_smmu_cd { __le64 data[8]; }; struct arm_smmu_cdtab_l2 { struct arm_smmu_cd cds[CTXDESC_L2_ENTRIES]; }; struct arm_smmu_cdtab_l1 { __le64 l2ptr; }; struct arm_smmu_ctx_desc_cfg { union { struct { struct arm_smmu_cd *table; unsigned int num_ents; } linear; struct { struct arm_smmu_cdtab_l1 *l1tab; struct arm_smmu_cdtab_l2 **l2ptrs; unsigned int num_l1_ents; } l2; }; dma_addr_t cdtab_dma; u8 s1fmt; /* log2 of the maximum number of CDs supported by this table */ u8 s1cdmax; }; And we can remove a bunch of the #defines in favour of sizeof() > > - dmam_free_coherent(smmu->dev, l1size, cd_table->cdtab, cd_table->cdtab_dma); > > cd_table->cdtab_dma = 0; > > - cd_table->cdtab = NULL; > > + cd_table->cdtab.linear = NULL; > > nit: AFAIU, arm_smmu_cdtab_allocated() was added to check the cd table existence > regardless the config. > So, I think we should be consistent, as here it is set as linear, while it can be 2lvl. I deleted this code, the tables are allocated once and stay allocated forever until release when the entire cd_table is deleted. No reason to 0 memory we are about to kfree. Thanks, Jason
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 735dd9ff61890e..24c286a0834445 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1170,7 +1170,7 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master, static int arm_smmu_alloc_cd_leaf_table(struct arm_smmu_device *smmu, struct arm_smmu_l1_ctx_desc *l1_desc) { - size_t size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3); + size_t size = CTXDESC_L2_ENTRIES * sizeof(struct arm_smmu_cd); l1_desc->l2ptr = dmam_alloc_coherent(smmu->dev, size, &l1_desc->l2ptr_dma, GFP_KERNEL); @@ -1198,12 +1198,11 @@ struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, struct arm_smmu_l1_ctx_desc *l1_desc; struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table; - if (!cd_table->cdtab) + if (!arm_smmu_cdtab_allocated(cd_table)) return NULL; if (cd_table->s1fmt == STRTAB_STE_0_S1FMT_LINEAR) - return (struct arm_smmu_cd *)(cd_table->cdtab + - ssid * CTXDESC_CD_DWORDS); + return &cd_table->cdtab.linear[ssid]; l1_desc = &cd_table->l1_desc[ssid / CTXDESC_L2_ENTRIES]; if (!l1_desc->l2ptr) @@ -1220,7 +1219,7 @@ struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master, might_sleep(); iommu_group_mutex_assert(master->dev); - if (!cd_table->cdtab) { + if (!arm_smmu_cdtab_allocated(cd_table)) { if (arm_smmu_alloc_cd_tables(master)) return NULL; } @@ -1236,7 +1235,7 @@ struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master, if (arm_smmu_alloc_cd_leaf_table(smmu, l1_desc)) return NULL; - l1ptr = cd_table->cdtab + idx * CTXDESC_L1_DESC_DWORDS; + l1ptr = &cd_table->cdtab.l1_desc[idx]; arm_smmu_write_cd_l1_desc(l1ptr, l1_desc); /* An invalid L1CD can be cached */ arm_smmu_sync_cd(master, ssid, false); @@ -1342,7 +1341,7 @@ void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid) struct arm_smmu_cd target = {}; struct arm_smmu_cd *cdptr; - if (!master->cd_table.cdtab) + if (!arm_smmu_cdtab_allocated(&master->cd_table)) return; cdptr = arm_smmu_get_cd_ptr(master, ssid); if (WARN_ON(!cdptr)) @@ -1352,8 +1351,6 @@ void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid) static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master) { - int ret; - size_t l1size; size_t max_contexts; struct arm_smmu_device *smmu = master->smmu; struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table; @@ -1366,8 +1363,14 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master) cd_table->s1fmt = STRTAB_STE_0_S1FMT_LINEAR; cd_table->num_l1_ents = max_contexts; - l1size = max_contexts * (CTXDESC_CD_DWORDS << 3); + cd_table->cdtab.linear = dmam_alloc_coherent( + smmu->dev, max_contexts * sizeof(struct arm_smmu_cd), + &cd_table->cdtab_dma, GFP_KERNEL); + if (!cd_table->cdtab.linear) + return -ENOMEM; } else { + size_t l1size; + cd_table->s1fmt = STRTAB_STE_0_S1FMT_64K_L2; cd_table->num_l1_ents = DIV_ROUND_UP(max_contexts, CTXDESC_L2_ENTRIES); @@ -1379,24 +1382,15 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master) return -ENOMEM; l1size = cd_table->num_l1_ents * (CTXDESC_L1_DESC_DWORDS << 3); + cd_table->cdtab.l1_desc = dmam_alloc_coherent( + smmu->dev, l1size, &cd_table->cdtab_dma, GFP_KERNEL); + if (!cd_table->cdtab.l1_desc) { + devm_kfree(smmu->dev, cd_table->l1_desc); + cd_table->l1_desc = NULL; + return -ENOMEM; + } } - - cd_table->cdtab = dmam_alloc_coherent(smmu->dev, l1size, &cd_table->cdtab_dma, - GFP_KERNEL); - if (!cd_table->cdtab) { - dev_warn(smmu->dev, "failed to allocate context descriptor\n"); - ret = -ENOMEM; - goto err_free_l1; - } - return 0; - -err_free_l1: - if (cd_table->l1_desc) { - devm_kfree(smmu->dev, cd_table->l1_desc); - cd_table->l1_desc = NULL; - } - return ret; } static void arm_smmu_free_cd_tables(struct arm_smmu_master *master) @@ -1407,7 +1401,7 @@ static void arm_smmu_free_cd_tables(struct arm_smmu_master *master) struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table; if (cd_table->l1_desc) { - size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3); + size = CTXDESC_L2_ENTRIES * sizeof(struct arm_smmu_cd); for (i = 0; i < cd_table->num_l1_ents; i++) { if (!cd_table->l1_desc[i].l2ptr) @@ -1421,13 +1415,17 @@ static void arm_smmu_free_cd_tables(struct arm_smmu_master *master) cd_table->l1_desc = NULL; l1size = cd_table->num_l1_ents * (CTXDESC_L1_DESC_DWORDS << 3); + dmam_free_coherent(smmu->dev, l1size, cd_table->cdtab.l1_desc, + cd_table->cdtab_dma); } else { - l1size = cd_table->num_l1_ents * (CTXDESC_CD_DWORDS << 3); + dmam_free_coherent(smmu->dev, + cd_table->num_l1_ents * + sizeof(struct arm_smmu_cd), + cd_table->cdtab.linear, cd_table->cdtab_dma); } - dmam_free_coherent(smmu->dev, l1size, cd_table->cdtab, cd_table->cdtab_dma); cd_table->cdtab_dma = 0; - cd_table->cdtab = NULL; + cd_table->cdtab.linear = NULL; } bool arm_smmu_free_asid(struct arm_smmu_ctx_desc *cd) @@ -2955,7 +2953,7 @@ static void arm_smmu_release_device(struct device *dev) arm_smmu_disable_pasid(master); arm_smmu_remove_master(master); - if (master->cd_table.cdtab) + if (arm_smmu_cdtab_allocated(&master->cd_table)) arm_smmu_free_cd_tables(master); kfree(master); } 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 280a04bfb7230c..21c1acf34dd29c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -279,10 +279,8 @@ struct arm_smmu_ste { #define CTXDESC_L1_DESC_V (1UL << 0) #define CTXDESC_L1_DESC_L2PTR_MASK GENMASK_ULL(51, 12) -#define CTXDESC_CD_DWORDS 8 - struct arm_smmu_cd { - __le64 data[CTXDESC_CD_DWORDS]; + __le64 data[8]; }; #define CTXDESC_CD_0_TCR_T0SZ GENMASK_ULL(5, 0) @@ -312,7 +310,7 @@ struct arm_smmu_cd { * When the SMMU only supports linear context descriptor tables, pick a * reasonable size limit (64kB). */ -#define CTXDESC_LINEAR_CDMAX ilog2(SZ_64K / (CTXDESC_CD_DWORDS << 3)) +#define CTXDESC_LINEAR_CDMAX ilog2(SZ_64K / sizeof(struct arm_smmu_cd)) /* Command queue */ #define CMDQ_ENT_SZ_SHIFT 4 @@ -593,7 +591,10 @@ struct arm_smmu_l1_ctx_desc { }; struct arm_smmu_ctx_desc_cfg { - __le64 *cdtab; + union { + struct arm_smmu_cd *linear; + __le64 *l1_desc; + } cdtab; dma_addr_t cdtab_dma; struct arm_smmu_l1_ctx_desc *l1_desc; unsigned int num_l1_ents; @@ -602,6 +603,12 @@ struct arm_smmu_ctx_desc_cfg { u8 s1cdmax; }; +static inline bool +arm_smmu_cdtab_allocated(struct arm_smmu_ctx_desc_cfg *cd_table) +{ + return cd_table->cdtab.linear; +} + struct arm_smmu_s2_cfg { u16 vmid; };
This is being used as both an array of CDs and an array of L1 descriptors. Give the two usages different names and correct types. Remove CTXDESC_CD_DWORDS as most usages were indexing or sizing an array of struct arm_smmu_cd. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 60 ++++++++++----------- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 17 ++++-- 2 files changed, 41 insertions(+), 36 deletions(-)