Re: [PATCH v7 04/14] iommu/arm-smmu-v3: Make changing domains be hitless for ATS

[Nicolin Chen <nicolinc@nvidia.com>]

Hi Jason,

Overall this patch makes the driver look much cleaner. Just a
few small comments/nitpickings below.

On Wed, May 08, 2024 at 03:57:12PM -0300, Jason Gunthorpe wrote:


>  static void arm_smmu_v3_write_ste_test_s2_to_abort(struct kunit *test)
> 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 532fe17f28bfe5..01e970f6ee4363 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c

> -static void arm_smmu_disable_ats(struct arm_smmu_master *master,
> -				 struct arm_smmu_domain *smmu_domain)
> -{
> -	if (!master->ats_enabled)
> -		return;
> -
> -	pci_disable_ats(to_pci_dev(master->dev));
> -	/*
> -	 * Ensure ATS is disabled at the endpoint before we issue the
> -	 * ATC invalidation via the SMMU.
> -	 */
> -	wmb();
> -	arm_smmu_atc_inv_master(master);
> -	atomic_dec(&smmu_domain->nr_ats_masters);
> -}
> -

Maybe we could keep the function for symmetry, since prepare()
still calls pci_disable_ats() + wmb()?

> +/*
> + * Prepare to attach a domain to a master. If disable_ats is not set this will
> + * turn on ATS if supported. smmu_domain can be NULL if the domain being

This function doesn't turn on ATS actually, but commit() does.

Should it be more accurate "If disable_ats is set this will turn
off ATS if enabled"?

> + * attached does not have a page table and does not require invalidation
> + * tracking.
> + */
> +static int arm_smmu_attach_prepare(struct arm_smmu_master *master,
> +				   struct iommu_domain *domain,
> +				   struct attach_state *state)
> +{
> +	struct arm_smmu_domain *smmu_domain =
> +		to_smmu_domain_devices(domain);

This could fit into a single line.

> -static int arm_smmu_attach_dev_ste(struct device *dev,
> -				   struct arm_smmu_ste *ste)
> +static int arm_smmu_attach_dev_ste(struct iommu_domain *domain,
> +				   struct device *dev, struct arm_smmu_ste *ste)

How about arm_smmu_domain_attach_dev_ste?

>  void arm_smmu_make_cdtable_ste(struct arm_smmu_ste *target,
> -			       struct arm_smmu_master *master);
> +			       struct arm_smmu_master *master,
> +			       bool ats_enabled);
>  void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
>  				 struct arm_smmu_master *master,
> -				 struct arm_smmu_domain *smmu_domain);
> +				 struct arm_smmu_domain *smmu_domain,
> +				 bool ats_enabled);

We seem to pass state.want_ats all the time. So maybe just name
it "bool want_ats", given that "ats_enabled" is only true after
commit() that always comes after these two functions?

Thanks
Nicolin