Re: [PATCH 2/7] iommu/arm-smmu-v3: Do not zero the strtab twice

From: Mostafa Saleh <smostafa@google.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>,
	Michael Shavit <mshavit@google.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	patches@lists.linux.dev, Ryan Roberts <ryan.roberts@arm.com>
Subject: Re: [PATCH 2/7] iommu/arm-smmu-v3: Do not zero the strtab twice
Date: Tue, 4 Jun 2024 15:56:04 +0000	[thread overview]
Message-ID: <Zl85FOpIrQSeto4W@google.com> (raw)
In-Reply-To: <2-v1-1b720dce51d1+4f44-smmuv3_tidy_jgg@nvidia.com>

Hi Jason,

On Mon, Jun 03, 2024 at 07:31:28PM -0300, Jason Gunthorpe wrote:
> dmam_alloc_coherent() already returns zero'd memory so cfg->strtab.l1_desc
> (the list of DMA addresses for the L2 entries) is already zero'd.
> 
> arm_smmu_init_l1_strtab() goes through and calls
> arm_smmu_write_strtab_l1_desc() on the newly allocated (and zero'd) struct
> arm_smmu_strtab_l1_desc, which ends up computing 'val = 0' and zeroing it
> again.
> 
> Remove arm_smmu_init_l1_strtab() and just call devm_kcalloc() from
> arm_smmu_init_strtab_2lvl to allocate the companion struct.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Looking at the code for dmam_alloc_coherent(basically dma_alloc_coherent)
I see that the memory is zeroed for both DMA direct and IOMMU, however
I don’t see that documented (in DMA-API.txt).

Assuming that’s guaranteed to be zeroed (maybe we should update the docs
if I am not missing something)
Reviewed-by: Mostafa Saleh <smostafa@google.com>

> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 27 +++++++--------------
>  1 file changed, 9 insertions(+), 18 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 6b4f1a664288db..d27dd0600bf1df 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3220,23 +3220,6 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  				       PRIQ_ENT_DWORDS, "priq");
>  }
>  
> -static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu)
> -{
> -	unsigned int i;
> -	struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
> -
> -	cfg->l1_desc = devm_kcalloc(smmu->dev, cfg->num_l1_ents,
> -				    sizeof(*cfg->l1_desc), GFP_KERNEL);
> -	if (!cfg->l1_desc)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < cfg->num_l1_ents; ++i)
> -		arm_smmu_write_strtab_l1_desc(
> -			&smmu->strtab_cfg.strtab.l1_desc[i], &cfg->l1_desc[i]);
> -
> -	return 0;
> -}
> -
>  static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
>  {
>  	void *strtab;
> @@ -3272,7 +3255,15 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
>  	reg |= FIELD_PREP(STRTAB_BASE_CFG_SPLIT, STRTAB_SPLIT);
>  	cfg->strtab_base_cfg = reg;
>  
> -	return arm_smmu_init_l1_strtab(smmu);
> +	cfg->l1_desc = devm_kcalloc(smmu->dev, cfg->num_l1_ents,
> +				    sizeof(*cfg->l1_desc), GFP_KERNEL);
> +	if (!cfg->l1_desc) {
> +		dev_err(smmu->dev,
> +			"failed to allocate l1 stream table (%zu bytes)\n",
> +			cfg->num_l1_ents * sizeof(*cfg->l1_desc));
> +		return -ENOMEM;
> +	}
> +	return 0;
>  }
>  
>  static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
> -- 
> 2.45.2
> 