Re: [PATCH v5 09/17] iommu/arm-smmu-v3: Put writing the context descriptor in the right order

[Mostafa Saleh <smostafa@google.com>]

Hi Jason,

On Tue, Feb 06, 2024 at 11:12:46AM -0400, Jason Gunthorpe wrote:
> Get closer to the IOMMU API ideal that changes between domains can be
> hitless. The ordering for the CD table entry is not entirely clean from
> this perspective.
> 
> When switching away from a STE with a CD table programmed in it we should
> write the new STE first, then clear any old data in the CD entry.
> 
> If we are programming a CD table for the first time to a STE then the CD
> entry should be programmed before the STE is loaded.
> 
> If we are replacing a CD table entry when the STE already points at the CD
> entry then we just need to do the make/break sequence.
> 
> Lift this code out of arm_smmu_detach_dev() so it can all be sequenced
> properly. The only other caller is arm_smmu_release_device() and it is
> going to free the cdtable anyhow, so it doesn't matter what is in it.
> 
> 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 | 29 ++++++++++++++-------
>  1 file changed, 20 insertions(+), 9 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 340f3dc82c9ce0..2a6ac0af932c54 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2575,14 +2575,6 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>  
>  	master->domain = NULL;
>  	master->ats_enabled = false;
> -	/*
> -	 * Clearing the CD entry isn't strictly required to detach the domain
> -	 * since the table is uninstalled anyway, but it helps avoid confusion
> -	 * in the call to arm_smmu_write_ctx_desc on the next attach (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, IOMMU_NO_PASID, NULL);
>  }
>  
>  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> @@ -2659,6 +2651,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  				master->domain = NULL;
>  				goto out_list_del;
>  			}
> +		} else {
> +			/*
> +			 * arm_smmu_write_ctx_desc() relies on the entry being
> +			 * invalid to work, clear any existing entry.
> +			 */
> +			ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
> +						      NULL);
> +			if (ret) {
> +				master->domain = NULL;
> +				goto out_list_del;
> +			}

Instead of having duplicate
           if (ret) {
               master->domain = NULL;
               goto out_list_del;
           }

In the if and the else, we can just move it outside.

>  		}
>  
>  		ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd);
> @@ -2668,15 +2671,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  		}
>  
>  		arm_smmu_make_cdtable_ste(&target, master);
> +		arm_smmu_install_ste_for_dev(master, &target);
>  		break;
>  	case ARM_SMMU_DOMAIN_S2:
>  		arm_smmu_make_s2_domain_ste(&target, master, smmu_domain);
> +		arm_smmu_install_ste_for_dev(master, &target);
> +		if (master->cd_table.cdtab)
> +			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;
>  	}
This invalidates the CD while the STE is in bypass/S2 which is a new behavior
to the driver, I don’t see anything from the user manual about this, so I
believe that is fine.

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

> -	arm_smmu_install_ste_for_dev(master, &target);
>  
>  	arm_smmu_enable_ats(master);
>  	goto out_unlock;
> -- 
> 2.43.0
>

Thanks,
Mostafa