Re: [PATCH 6/7] iommu/arm-smmu-v3: Shrink the cdtab l1_desc array

[Mostafa Saleh <smostafa@google.com>]

Hi Jason,

On Mon, Jun 03, 2024 at 07:31:32PM -0300, Jason Gunthorpe wrote:
> The top of the 2 level CD table is (at most) 1024 entries big, and two
> high order allocations are required. One of __le64 which is programmed
> into the HW (8k) and one of struct arm_smmu_l1_ctx_desc which holds the
> CPU pointer (16k).
> 
> There are two copies of the l2ptr_dma, one is stored in the struct
> arm_smmu_l1_ctx_desc, and another is encoded in the __le64 for the HW to
> use. Instead of storing two copies just decode the value from the __le64.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 35 ++++++++++-----------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -
>  2 files changed, 16 insertions(+), 20 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 3f2e0462433d2d..7a6c9aac4cd450 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1167,28 +1167,19 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
>  	arm_smmu_cmdq_batch_submit(smmu, &cmds);
>  }
>  
> -static int arm_smmu_alloc_cd_leaf_table(struct arm_smmu_device *smmu,
> -					struct arm_smmu_l1_ctx_desc *l1_desc)
> +static void arm_smmu_write_cd_l1_desc(__le64 *dst, dma_addr_t l2ptr_dma)
>  {
> -	size_t size = CTXDESC_L2_ENTRIES * sizeof(struct arm_smmu_cd);
> -
> -	l1_desc->l2ptr = dma_alloc_coherent(smmu->dev, size,
> -					    &l1_desc->l2ptr_dma, GFP_KERNEL);
> -	if (!l1_desc->l2ptr)
> -		return -ENOMEM;
> -	return 0;
> -}
> -
> -static void arm_smmu_write_cd_l1_desc(__le64 *dst,
> -				      struct arm_smmu_l1_ctx_desc *l1_desc)
> -{
> -	u64 val = (l1_desc->l2ptr_dma & CTXDESC_L1_DESC_L2PTR_MASK) |
> -		  CTXDESC_L1_DESC_V;
> +	u64 val = (l2ptr_dma & CTXDESC_L1_DESC_L2PTR_MASK) | CTXDESC_L1_DESC_V;
>  
>  	/* The HW has 64 bit atomicity with stores to the L2 CD table */
>  	WRITE_ONCE(*dst, cpu_to_le64(val));
>  }
>  
> +static dma_addr_t arm_smmu_cd_l1_get_desc(__le64 *src)
> +{
> +	return le64_to_cpu(*src) & CTXDESC_L1_DESC_L2PTR_MASK;
> +}
> +
>  struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
>  					u32 ssid)
>  {
> @@ -1227,13 +1218,18 @@ struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master,
>  
>  		l1_desc = &cd_table->l1_desc[idx];
>  		if (!l1_desc->l2ptr) {
> +			dma_addr_t l2ptr_dma;
>  			__le64 *l1ptr;
>  
> -			if (arm_smmu_alloc_cd_leaf_table(smmu, l1_desc))
> +			l1_desc->l2ptr = dma_alloc_coherent(
> +				smmu->dev,
> +				CTXDESC_L2_ENTRIES * sizeof(struct arm_smmu_cd),
> +				&l2ptr_dma, GFP_KERNEL);
> +			if (!l1_desc->l2ptr)
>  				return NULL;
>  
>  			l1ptr = &cd_table->cdtab.l1_desc[idx];
> -			arm_smmu_write_cd_l1_desc(l1ptr, l1_desc);
> +			arm_smmu_write_cd_l1_desc(l1ptr, l2ptr_dma);
>  			/* An invalid L1CD can be cached */
>  			arm_smmu_sync_cd(master, ssid, false);
>  		}
> @@ -1406,7 +1402,8 @@ static void arm_smmu_free_cd_tables(struct arm_smmu_master *master)
>  
>  			dma_free_coherent(smmu->dev, size,
>  					  cd_table->l1_desc[i].l2ptr,
> -					  cd_table->l1_desc[i].l2ptr_dma);
> +					  arm_smmu_cd_l1_get_desc(
> +						  &cd_table->cdtab.l1_desc[i]));
>  		}
>  		kfree(cd_table->l1_desc);
>  
> 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 21c1acf34dd29c..1ffe2fdfd3755f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -587,7 +587,6 @@ struct arm_smmu_ctx_desc {
>  
>  struct arm_smmu_l1_ctx_desc {
>  	struct arm_smmu_cd		*l2ptr;
> -	dma_addr_t			l2ptr_dma;

Maybe now we can also get rid of arm_smmu_l1_ctx_desc, and embed l2ptr directly
in arm_smmu_ctx_desc_cfg?

>  };
>  
>  struct arm_smmu_ctx_desc_cfg {
> -- 
> 2.45.2
> 

Thanks,
Mostafa