Re: [PATCH v5 09/27] iommu/arm-smmu-v3: Allocate the CD table entry in advance

[Mostafa Saleh <smostafa@google.com>]

Hi Jason,

On Mon, Mar 04, 2024 at 07:43:57PM -0400, Jason Gunthorpe wrote:
> Avoid arm_smmu_attach_dev() having to undo the changes to the
> smmu_domain->devices list, acquire the cdptr earlier so we don't need to
> handle that error.
> 
> Now there is a clear break in arm_smmu_attach_dev() where all the
> prep-work has been done non-disruptively and we commit to making the HW
> change, which cannot fail.
> 
> This completes transforming arm_smmu_attach_dev() so that it does not
> disturb the HW if it fails.
> 
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++--------------
>  1 file changed, 8 insertions(+), 16 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 2dd6cb17112e98..39081d828a2132 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2676,6 +2676,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	struct arm_smmu_device *smmu;
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>  	struct arm_smmu_master *master;
> +	struct arm_smmu_cd *cdptr;
>  
>  	if (!fwspec)
>  		return -ENOENT;
> @@ -2704,6 +2705,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> +		cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID);
> +		if (!cdptr)
> +			return -ENOMEM;
> +	}
> +
>  	/*
>  	 * Prevent arm_smmu_share_asid() from trying to change the ASID
>  	 * of either the old or new domain while we are working on it.
> @@ -2723,13 +2730,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	switch (smmu_domain->stage) {
>  	case ARM_SMMU_DOMAIN_S1: {
>  		struct arm_smmu_cd target_cd;
> -		struct arm_smmu_cd *cdptr;
> -
> -		cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID);
> -		if (!cdptr) {
> -			ret = -ENOMEM;
> -			goto out_list_del;
> -		}
>  
>  		arm_smmu_make_s1_cd(&target_cd, master, smmu_domain);
>  		arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr,
> @@ -2746,16 +2746,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	}
>  
>  	arm_smmu_enable_ats(master, smmu_domain);
> -	goto out_unlock;
> -
> -out_list_del:
> -	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -	list_del_init(&master->domain_head);
> -	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> -
> -out_unlock:
>  	mutex_unlock(&arm_smmu_asid_lock);
> -	return ret;
> +	return 0;
>  }
>  
>  static int arm_smmu_attach_dev_ste(struct device *dev,
> -- 
> 2.43.2
>
I believe this is fine, I couldn’t break it. With the comment on the previous
patch, where we explicitly allocate the CD here and not inside
arm_smmu_get_cd_ptr().

Reviewed-by: Mostafa Saleh <smostafa@google.com>


Thanks,
Mostafa