[PATCH 4/7] iommu/arm-smmu-v3: Split struct arm_smmu_ctx_desc_cfg.cdtab
Mostafa Saleh
smostafa at google.com
Tue Jun 4 09:07:21 PDT 2024
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 at 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
More information about the linux-arm-kernel
mailing list