RE: [PATCH v4 12/16] iommu/arm-smmu-v3: Add a global static IDENTITY domain

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, January 25, 2024 11:57 PM
> To: 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>
> Cc: Moritz Fischer <mdf@kernel.org>; Moritz Fischer <moritzf@google.com>;
> Michael Shavit <mshavit@google.com>; Nicolin Chen <nicolinc@nvidia.com>;
> patches@lists.linux.dev; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Subject: [PATCH v4 12/16] iommu/arm-smmu-v3: Add a global static
> IDENTITY domain
> 
> Move to the new static global for identity domains. Move all the logic out
> of arm_smmu_attach_dev into an identity only function.
> 
> Reviewed-by: Michael Shavit <mshavit@google.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Moritz Fischer <moritzf@google.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 82 +++++++++++++++--
> ----
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -
>  2 files changed, 58 insertions(+), 25 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 f08cfa9b90b3eb..d35bf9655c9b1b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2226,8 +2226,7 @@ static struct iommu_domain
> *arm_smmu_domain_alloc(unsigned type)
>  		return arm_smmu_sva_domain_alloc();
> 
>  	if (type != IOMMU_DOMAIN_UNMANAGED &&
> -	    type != IOMMU_DOMAIN_DMA &&
> -	    type != IOMMU_DOMAIN_IDENTITY)
> +	    type != IOMMU_DOMAIN_DMA)
>  		return NULL;
> 
>  	/*
> @@ -2335,11 +2334,6 @@ static int arm_smmu_domain_finalise(struct
> iommu_domain *domain)
>  	struct arm_smmu_domain *smmu_domain =
> to_smmu_domain(domain);
>  	struct arm_smmu_device *smmu = smmu_domain->smmu;
> 
> -	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> -		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> -		return 0;
> -	}
> -
>  	/* Restrict the stage to what we can actually support */
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
>  		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
> @@ -2537,7 +2531,7 @@ static void arm_smmu_detach_dev(struct
> arm_smmu_master *master)
>  	struct arm_smmu_domain *smmu_domain;
>  	unsigned long flags;
> 
> -	if (!domain)
> +	if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING))
>  		return;
> 
>  	smmu_domain = to_smmu_domain(domain);
> @@ -2600,15 +2594,7 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
> 
>  	arm_smmu_detach_dev(master);
> 
> -	/*
> -	 * The SMMU does not support enabling ATS with bypass. When the
> STE is
> -	 * in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests and
> -	 * Translated transactions are denied as though ATS is disabled for
> the
> -	 * stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and
> -	 * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry).
> -	 */
> -	if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
> -		master->ats_enabled = arm_smmu_ats_supported(master);
> +	master->ats_enabled = arm_smmu_ats_supported(master);
> 
>  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
>  	list_add(&master->domain_head, &smmu_domain->devices);
> @@ -2645,13 +2631,6 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
>  			arm_smmu_write_ctx_desc(master,
> IOMMU_NO_PASID,
>  						      NULL);
>  		break;
> -	case ARM_SMMU_DOMAIN_BYPASS:
> -		arm_smmu_make_bypass_ste(&target);
> -		arm_smmu_install_ste_for_dev(master, &target);
> -		if (master->cd_table.cdtab)
> -			arm_smmu_write_ctx_desc(master,
> IOMMU_NO_PASID,
> -						      NULL);
> -		break;
>  	}
> 
>  	arm_smmu_enable_ats(master, smmu_domain);
> @@ -2667,6 +2646,60 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
>  	return ret;
>  }
> 
> +static int arm_smmu_attach_dev_ste(struct device *dev,
> +				   struct arm_smmu_ste *ste)
> +{
> +	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> +
> +	if (arm_smmu_master_sva_enabled(master))
> +		return -EBUSY;
> +
> +	/*
> +	 * Do not allow any ASID to be changed while are working on the STE,
> +	 * otherwise we could miss invalidations.
> +	 */
> +	mutex_lock(&arm_smmu_asid_lock);
> +
> +	/*
> +	 * The SMMU does not support enabling ATS with bypass/abort.
> When the
> +	 * STE is in bypass (STE.Config[2:0] == 0b100), ATS Translation
> Requests
> +	 * and Translated transactions are denied as though ATS is disabled
> for
> +	 * the stream (STE.EATS == 0b00), causing F_BAD_ATS_TREQ and
> +	 * F_TRANSL_FORBIDDEN events (IHI0070Ea 5.2 Stream Table Entry).
> +	 */
> +	arm_smmu_detach_dev(master);
> +
> +	arm_smmu_install_ste_for_dev(master, ste);
> +	mutex_unlock(&arm_smmu_asid_lock);
> +
> +	/*
> +	 * This has to be done after removing the master from the
> +	 * arm_smmu_domain->devices to avoid races updating the same
> context
> +	 * descriptor from arm_smmu_share_asid().
> +	 */
> +	if (master->cd_table.cdtab)
> +		arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> NULL);
> +	return 0;
> +}
> +
> +static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
> +					struct device *dev)
> +{
> +	struct arm_smmu_ste ste;
> +
> +	arm_smmu_make_bypass_ste(&ste);
> +	return arm_smmu_attach_dev_ste(dev, &ste);
> +}
> +
> +static const struct iommu_domain_ops arm_smmu_identity_ops = {
> +	.attach_dev = arm_smmu_attach_dev_identity,
> +};
> +
> +static struct iommu_domain arm_smmu_identity_domain = {
> +	.type = IOMMU_DOMAIN_IDENTITY,
> +	.ops = &arm_smmu_identity_ops,
> +};
> +
>  static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned
> long iova,
>  			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
>  			      int prot, gfp_t gfp, size_t *mapped)
> @@ -3056,6 +3089,7 @@ static void arm_smmu_remove_dev_pasid(struct
> device *dev, ioasid_t pasid)
>  }
> 
>  static struct iommu_ops arm_smmu_ops = {
> +	.identity_domain	= &arm_smmu_identity_domain,

This seems to create a problem when we have set the identity domain and
try to enable sva for the device. Since there is no smmu_domain for this case 
and there is no specific domain type checking in iommu_sva_bind_device() path,
it eventually crashes(hangs in my test) in,

iommu_sva_bind_device()
   ...
      arm_smmu_sva_set_dev_pasid()
        __arm_smmu_sva_bind()
           arm_smmu_mmu_notifier_get(smmu_domain, ..)  --> never exit the mmu notifier list loop.

I think we should check for the domain type in iommu_sva_bind_device() or later
before trying to use smmu_domain.  At present(ie, without this series) it returns error
while we are trying to write the CD. But that looks too late as well.

Thanks,
Shameer