Re: [PATCH 4/7] iommu/arm-smmu-v3: Split struct arm_smmu_ctx_desc_cfg.cdtab

[Mostafa Saleh <smostafa@google.com>]

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