Re: [PATCH v5 6/9] iommu/arm-smmu-v3: Move CD table to arm_smmu_master

[Will Deacon]

On Wed, Aug 09, 2023 at 01:12:02AM +0800, Michael Shavit wrote:
> @@ -2203,7 +2186,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
>  		ias = min_t(unsigned long, ias, VA_BITS);
>  		oas = smmu->ias;
>  		fmt = ARM_64_LPAE_S1;
> -		finalise_stage_fn = arm_smmu_domain_finalise_s1;
> +		finalise_stage_fn = arm_smmu_domain_finalise_cd;

Why is this a better name? Now we have inconsistency with
arm_smmu_domain_finalise_s2().

>  		break;
>  	case ARM_SMMU_DOMAIN_NESTED:
>  	case ARM_SMMU_DOMAIN_S2:
> @@ -2402,6 +2385,16 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>  	master->domain = NULL;
>  	master->ats_enabled = false;
>  	arm_smmu_install_ste_for_dev(master);
> +	/*
> +	 * The table is uninstalled before clearing the CD to prevent an
> +	 * unnecessary sync in arm_smmu_write_ctx_desc. Although clearing the
> +	 * CD entry isn't strictly required to detach the domain since the
> +	 * table is uninstalled anyway, it's more proper and helps avoid
> +	 * confusion in the call to arm_smmu_write_ctx_desc on the next attach

You can remove the "it's more proper" part.

> +	 * (which expects the entry to be empty).
> +	 */
> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && master->cd_table.cdtab)
> +		arm_smmu_write_ctx_desc(master, 0, NULL);
>  }
>  
>  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> @@ -2436,22 +2429,14 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	if (!smmu_domain->smmu) {
>  		smmu_domain->smmu = smmu;
>  		ret = arm_smmu_domain_finalise(domain, master);
> -		if (ret) {
> +		if (ret)
>  			smmu_domain->smmu = NULL;
> -			goto out_unlock;
> -		}
> -	} else if (smmu_domain->smmu != smmu) {
> -		ret = -EINVAL;
> -		goto out_unlock;
> -	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
> -		   master->ssid_bits != smmu_domain->cd_table.max_cds_bits) {
> +	} else if (smmu_domain->smmu != smmu)
>  		ret = -EINVAL;
> -		goto out_unlock;
> -	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
> -		   smmu_domain->cd_table.stall_enabled != master->stall_enabled) {
> -		ret = -EINVAL;
> -		goto out_unlock;
> -	}

Removing these checks on the domain is pretty nice.

> @@ -2465,6 +2450,22 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
>  		master->ats_enabled = arm_smmu_ats_supported(master);
>  
> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> +		if (!master->cd_table.cdtab) {
> +			ret = arm_smmu_alloc_cd_tables(master);
> +			if (ret) {
> +				master->domain = NULL;
> +				return ret;
> +			}
> +		}
> +
> +		ret = arm_smmu_write_ctx_desc(master, 0, &smmu_domain->cd);
> +		if (ret) {
> +			master->domain = NULL;
> +			return ret;

Can you leak the cd tables here if you just allocated them?

> @@ -2472,10 +2473,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
>  	arm_smmu_enable_ats(master);
> -
> -out_unlock:
> -	mutex_unlock(&smmu_domain->init_mutex);
> -	return ret;
> +	return 0;
>  }
>  
>  static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
> @@ -2719,6 +2717,8 @@ static void arm_smmu_release_device(struct device *dev)
>  	arm_smmu_detach_dev(master);
>  	arm_smmu_disable_pasid(master);
>  	arm_smmu_remove_master(master);
> +	if (master->cd_table.cdtab_dma)

Why are you checking 'cdtab_dma' here instead of just 'cdtab'?

Will