Re: [PATCH v2 6/8] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Jul 31, 2023 at 06:48:16PM +0800, Michael Shavit wrote:
> Update arm_smmu_write_ctx_desc and downstream functions to operate on
> a master instead of an smmu domain. We expect arm_smmu_write_ctx_desc()
> to only be called to write a CD entry into a CD table owned by the
> master. Under the hood, arm_smmu_write_ctx_desc still fetches the CD
> table from the domain that is attached to the master, but a subsequent
> commit will move that table's ownership to the master.
> 
> Note that this change isn't a nop refactor since SVA will call
> arm_smmu_write_ctx_desc in a loop for every master the domain is
> attached to despite the fact that they all share the same CD table. This
> loop may look weird but becomes necessary when the CD table becomes
> per-master in a subsequent commit.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
> Changes in v2:
> - minor style fixes
> 
> Changes in v1:
> - arm_smmu_write_ctx_desc now get's the CD table to write to from the
>   master parameter instead of a distinct parameter. This works well
>   because the CD table being written to should always be owned by the
>   master by the end of this series. This version no longer allows master
>   to be NULL.
> 
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 33 +++++++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 59 ++++++++-----------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
>  3 files changed, 53 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 968559d625c40..8242ee3405f2d 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -45,9 +45,11 @@ static struct arm_smmu_ctx_desc *
>  arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
>  {
>  	int ret;
> +	unsigned long flags;
>  	u32 new_asid;
>  	struct arm_smmu_ctx_desc *cd;
>  	struct arm_smmu_device *smmu;
> +	struct arm_smmu_master *master;
>  	struct arm_smmu_domain *smmu_domain;
>  
>  	cd = xa_load(&arm_smmu_asid_xa, asid);
> @@ -80,7 +82,11 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
>  	 * be some overlap between use of both ASIDs, until we invalidate the
>  	 * TLB.
>  	 */
> -	arm_smmu_write_ctx_desc(smmu_domain, 0, cd);
> +	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> +	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> +		arm_smmu_write_ctx_desc(master, 0, cd);
> +	}

I think it is typical kernel style to not include the single statement
{}

> +	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
>  	/* Invalidate TLB entries previously associated with that context */
>  	arm_smmu_tlb_inv_asid(smmu, asid);
> @@ -211,6 +217,8 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>  {
>  	struct arm_smmu_mmu_notifier *smmu_mn = mn_to_smmu(mn);
>  	struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
> +	struct arm_smmu_master *master;
> +	unsigned long flags;
>  
>  	mutex_lock(&sva_lock);
>  	if (smmu_mn->cleared) {
> @@ -222,7 +230,11 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>  	 * DMA may still be running. Keep the cd valid to avoid C_BAD_CD events,
>  	 * but disable translation.
>  	 */
> -	arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, &quiet_cd);
> +	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> +	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> +		arm_smmu_write_ctx_desc(master, mm->pasid, &quiet_cd);
> +	}
> +	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);

And here

>  	arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid);
>  	arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
> @@ -248,7 +260,9 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
>  			  struct mm_struct *mm)
>  {
>  	int ret;
> +	unsigned long flags;
>  	struct arm_smmu_ctx_desc *cd;
> +	struct arm_smmu_master *master;
>  	struct arm_smmu_mmu_notifier *smmu_mn;
>  
>  	list_for_each_entry(smmu_mn, &smmu_domain->mmu_notifiers, list) {
> @@ -279,7 +293,11 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
>  		goto err_free_cd;
>  	}
>  
> -	ret = arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, cd);
> +	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> +	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> +		ret = arm_smmu_write_ctx_desc(master, mm->pasid, cd);
> +	}
> +	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);

And here..

>  	if (ret)
>  		goto err_put_notifier;
>  
> @@ -296,6 +314,8 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
>  
>  static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
>  {
> +	unsigned long flags;
> +	struct arm_smmu_master *master;
>  	struct mm_struct *mm = smmu_mn->mn.mm;
>  	struct arm_smmu_ctx_desc *cd = smmu_mn->cd;
>  	struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
> @@ -304,7 +324,12 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
>  		return;
>  
>  	list_del(&smmu_mn->list);
> -	arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, NULL);
> +
> +	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> +	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> +		arm_smmu_write_ctx_desc(master, mm->pasid, NULL);
> +	}
> +	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
  

And here..

You know, you should try to keep the function instead of duplicating
these

arm_smmu_write_ctx_desc_devices()

And put the four lines in there?

> @@ -987,19 +985,14 @@ static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
>  		},
>  	};
>  
> -	if (!smmu_domain->cd_table.installed)
> +	if (!master->domain->cd_table.installed)
>  		return;

BTW, do you have locking for this? I didn't check carefully

Jason