Re: [PATCH v4 07/16] iommu/arm-smmu-v3: Compute the STE only once for each master

[Mostafa Saleh <smostafa@google.com>]

Hi Jason,

On Thu, Jan 25, 2024 at 07:57:17PM -0400, Jason Gunthorpe wrote:
> Currently arm_smmu_install_ste_for_dev() iterates over every SID and
> computes from scratch an identical STE. Every SID should have the same STE
> contents. Turn this inside out so that the STE is supplied by the caller
> and arm_smmu_install_ste_for_dev() simply installs it to every SID.
> 
> This is possible now that the STE generation does not inform what sequence
> should be used to program it.
> 
> This allows splitting the STE calculation up according to the call site,
> which following patches will make use of, and removes the confusing NULL
> domain special case that only supported arm_smmu_detach_dev().
> 
> 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 | 57 ++++++++-------------
>  1 file changed, 22 insertions(+), 35 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 539ef380f457fa..cf3e348cb9abe1 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1588,35 +1588,6 @@ static void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
>  				      STRTAB_STE_3_S2TTB_MASK);
>  }
>  
> -static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
> -				      struct arm_smmu_ste *dst)
> -{
> -	struct arm_smmu_domain *smmu_domain = master->domain;
> -	struct arm_smmu_ste target = {};
> -
> -	if (!smmu_domain) {
> -		if (disable_bypass)
> -			arm_smmu_make_abort_ste(&target);
> -		else
> -			arm_smmu_make_bypass_ste(&target);
> -		arm_smmu_write_ste(master, sid, dst, &target);
> -		return;
> -	}
> -
> -	switch (smmu_domain->stage) {
> -	case ARM_SMMU_DOMAIN_S1:
> -		arm_smmu_make_cdtable_ste(&target, master, &master->cd_table);
> -		break;
> -	case ARM_SMMU_DOMAIN_S2:
> -		arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
> -		break;
> -	case ARM_SMMU_DOMAIN_BYPASS:
> -		arm_smmu_make_bypass_ste(&target);
> -		break;
> -	}
> -	arm_smmu_write_ste(master, sid, dst, &target);
> -}
> -
>  static void arm_smmu_init_bypass_stes(struct arm_smmu_ste *strtab,
>  				      unsigned int nent)
>  {
> @@ -2439,7 +2410,8 @@ arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid)
>  	}
>  }
>  
> -static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
> +static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master,
> +					 const struct arm_smmu_ste *target)
>  {
>  	int i, j;
>  	struct arm_smmu_device *smmu = master->smmu;
> @@ -2456,7 +2428,7 @@ static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master)
>  		if (j < i)
>  			continue;
>  
> -		arm_smmu_write_strtab_ent(master, sid, step);
> +		arm_smmu_write_ste(master, sid, step, target);
>  	}
>  }
>  
> @@ -2563,6 +2535,7 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>  {
>  	unsigned long flags;
> +	struct arm_smmu_ste target;
>  	struct arm_smmu_domain *smmu_domain = master->domain;
>  
>  	if (!smmu_domain)
> @@ -2576,7 +2549,11 @@ 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);
> +	if (disable_bypass)
> +		arm_smmu_make_abort_ste(&target);
> +	else
> +		arm_smmu_make_bypass_ste(&target);
> +	arm_smmu_install_ste_for_dev(master, &target);
>  	/*
>  	 * Clearing the CD entry isn't strictly required to detach the domain
>  	 * since the table is uninstalled anyway, but it helps avoid confusion
> @@ -2591,6 +2568,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  {
>  	int ret = 0;
>  	unsigned long flags;
> +	struct arm_smmu_ste target;
>  	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>  	struct arm_smmu_device *smmu;
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> @@ -2652,7 +2630,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	list_add(&master->domain_head, &smmu_domain->devices);
>  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
> -	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
> +	switch (smmu_domain->stage) {
> +	case ARM_SMMU_DOMAIN_S1:
>  		if (!master->cd_table.cdtab) {
>  			ret = arm_smmu_alloc_cd_tables(master);
>  			if (ret) {
> @@ -2666,9 +2645,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  			master->domain = NULL;
>  			goto out_list_del;
>  		}
> -	}
>  
> -	arm_smmu_install_ste_for_dev(master);
> +		arm_smmu_make_cdtable_ste(&target, master, &master->cd_table);
> +		break;
> +	case ARM_SMMU_DOMAIN_S2:
> +		arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
> +		break;
> +	case ARM_SMMU_DOMAIN_BYPASS:
> +		arm_smmu_make_bypass_ste(&target);
> +		break;
> +	}
> +	arm_smmu_install_ste_for_dev(master, &target);
>  
>  	arm_smmu_enable_ats(master);
>  	goto out_unlock;
> -- 
> 2.43.0
>

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

Thanks,
Mostafa