Re: [PATCH RFC v8 4/9] iommu/arm-smmu-v3: Use a per-domain CD table for non-PASID streams

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Nov 13, 2026 at 09:27:16PM +0800, Peng Fan (OSS) wrote:
> +static int arm_smmu_alloc_cd_ptr_l2(struct arm_smmu_master *master, u32 ssid)
> +{
> +	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
> +	unsigned int idx = arm_smmu_cdtab_l1_idx(ssid);
> +	struct arm_smmu_cdtab_l2 **l2ptr = &cd_table->l2.l2ptrs[idx];
> +	dma_addr_t l2ptr_dma;
> +
> +	if (*l2ptr)
> +		return 0;
> +
> +	*l2ptr = dma_alloc_coherent(master->smmu->dev, sizeof(**l2ptr),
> +				    &l2ptr_dma, GFP_KERNEL);
> +	if (!*l2ptr)
> +		return -ENOMEM;
> +
> +	arm_smmu_write_cd_l1_desc(&cd_table->l2.l1tab[idx], l2ptr_dma);
> +	/* An invalid L1CD can be cached */
> +	arm_smmu_sync_cd(master, ssid, false);
> +	return 0;
> +}

This patch is a bit too big. Move this into a prep patch please.

> @@ -1793,9 +1801,22 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_device *smmu,
>  
>  static int arm_smmu_master_alloc_cd_tables(struct arm_smmu_master *master)
>  {
> -	return arm_smmu_alloc_cd_tables(master->smmu, &master->cd_table,
> -					master->ssid_bits,
> -					master->ats_always_on);
> +	int ret;
> +
> +	ret = arm_smmu_alloc_cd_tables(master->smmu, &master->cd_table,
> +				       master->ssid_bits,
> +				       master->ats_always_on);
> +	if (ret)
> +		return ret;
> +
> +	if (master->cd_table.s1fmt == STRTAB_STE_0_S1FMT_64K_L2) {
> +		ret = arm_smmu_alloc_cd_ptr_l2(master, IOMMU_NO_PASID);
> +		if (ret) {
> +			arm_smmu_free_cd_tables(master->smmu, &master->cd_table);
> +			memset(&master->cd_table, 0, sizeof(master->cd_table));
> +		}

It'd be helpful to have a line of inline comments stating why this
is needed here.

> @@ -1830,6 +1851,12 @@ static void arm_smmu_free_cd_tables(struct arm_smmu_device *smmu,
>  	}
>  }
>  
> +static int arm_smmu_domain_alloc_cd_tables(struct arm_smmu_domain *smmu_domain)
> +{
> +	return arm_smmu_alloc_cd_tables(smmu_domain->smmu,

Hmm, that's not good. We are trying to decouple smmu_domain from an
SMMU instance so that one smmu_domain can be shared across multiple
SMMU instances.

Tying one instance to a domain is apparently going against it..

Instead, we would need an {smmu, cd_table, refcount} list. And it'd
make thing more complicated than letting master own CD table as-is.

Not sure if it's still worth doing so. Robin?

> @@ -3971,15 +4058,23 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
>  	if (smmu_domain->smmu != master->smmu)
>  		return -EINVAL;
>  
> -	if (!master->cd_table.in_ste &&
> +	if (!((sid_domain->type & __IOMMU_DOMAIN_PAGING) &&
> +	      to_smmu_domain(sid_domain)->stage == ARM_SMMU_DOMAIN_S1) &&
>  	    sid_domain->type != IOMMU_DOMAIN_IDENTITY &&
>  	    sid_domain->type != IOMMU_DOMAIN_BLOCKED)
>  		return -EINVAL;
>  
> +	first = !arm_smmu_ssids_in_use(&master->cd_table);
>  	cdptr = arm_smmu_alloc_cd_ptr(master, pasid);
>  	if (!cdptr)
>  		return -ENOMEM;
>  
> +	if (first) {
> +		cd0ptr = arm_smmu_master_get_cd_ptr(master, IOMMU_NO_PASID);
> +		if (WARN_ON(!cd0ptr))
> +			return -ENXIO;

You don't need "first" at all, given ...

>  	mutex_lock(&arm_smmu_asid_lock);
>  	ret = arm_smmu_attach_prepare(&state, &smmu_domain->domain);
>  	if (ret) {
> @@ -3995,7 +4090,30 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
>  	cd->data[0] |= cpu_to_le64(
>  		FIELD_PREP(CTXDESC_CD_0_ASID, smmu_domain->cd.asid));
>  
> +	/*
> +	 * The private CD table is taking over the STE now. Make sure its
> +	 * default substream matches the RID domain before the STE points at
> +	 * it, since non-PASID traffic follows CD[0].
> +	 */
> +	if (first) {
> +		if ((sid_domain->type & __IOMMU_DOMAIN_PAGING) &&
> +		    to_smmu_domain(sid_domain)->stage == ARM_SMMU_DOMAIN_S1) {
> +			struct arm_smmu_cd cd0;
> +
> +			arm_smmu_make_s1_cd(&cd0, master,
> +					    to_smmu_domain(sid_domain));
> +			arm_smmu_write_cd_entry(master, IOMMU_NO_PASID,
> +						cd0ptr, &cd0);

... there is only one place using the cd0ptr.

> @@ -4015,35 +4133,61 @@ static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,
[...] 
>  	/*
> -	 * When the last user of the CD table goes away downgrade the STE back
> -	 * to a non-cd_table one, by re-attaching its sid_domain.
> +	 * When the last user of the CD table goes away, switch the STE back to
> +	 * the RID domain's CD table (S1) or to the native bypass/abort STE
> +	 * (identity/blocked). Do not call attach_dev() for an S1 paging domain:
> +	 * this is a teardown path where allocation failures cannot be
> +	 * propagated. The private CD table is kept allocated for reuse and
> +	 * freed when the device goes away.
>  	 */
>  	if (!arm_smmu_ssids_in_use(&master->cd_table)) {
>  		struct iommu_domain *sid_domain =
>  			iommu_driver_get_domain_for_dev(master->dev);
> -		bool ats_always_on = master->ats_always_on &&
> -				     sid_domain->type != IOMMU_DOMAIN_BLOCKED;
> -		bool downgrade = sid_domain->type == IOMMU_DOMAIN_IDENTITY ||
> -				 sid_domain->type == IOMMU_DOMAIN_BLOCKED;
>  
> -		if (!ats_always_on && downgrade)
> -			sid_domain->ops->attach_dev(sid_domain, dev,
> -						    sid_domain);
> +		if (!master->ats_always_on ||
> +		    sid_domain->type == IOMMU_DOMAIN_BLOCKED) {
> +			if ((sid_domain->type & __IOMMU_DOMAIN_PAGING) &&
> +			    to_smmu_domain(sid_domain)->stage ==
> +				    ARM_SMMU_DOMAIN_S1) {
> +				struct arm_smmu_ste ste;
> +				struct arm_smmu_domain *sid_smmu_domain =
> +					to_smmu_domain(sid_domain);
> +
> +				mutex_lock(&arm_smmu_asid_lock);
> +				arm_smmu_make_cdtable_ste(
> +					&ste, master, &sid_smmu_domain->cd_table,
> +					master->ats_enabled,
> +					STRTAB_STE_1_S1DSS_SSID0);
> +				arm_smmu_install_ste_for_dev(master, &ste);
> +				mutex_unlock(&arm_smmu_asid_lock);
> +			} else if (sid_domain->type == IOMMU_DOMAIN_IDENTITY ||
> +				   sid_domain->type == IOMMU_DOMAIN_BLOCKED) {
> +				ret = sid_domain->ops->attach_dev(sid_domain, dev,
> +								  sid_domain);

I commented in v7 that attach_dev shouldn't propagate the ret..

Although your patch-9 clean it back to the right form, I think
there should be a way to prepare things cleanly so that you can
make a change here in one shot.

Nicolin