[PATCH v3 01/13] iommu/arm-smmu-v3: Add arm_smmu_attach_release()

[Jonathan Cameron]

> The IOPF teardown is done in arm_smmu_remove_master_domain() when releasing
> the master_domain on detach, under the global arm_smmu_asid_lock mutex. But
> the teardown must drain any in-flight IOPF (for the old domain), before the
> master_domain is freed via iopf_queue_flush_dev() calling flush_workqueue()
> that can block on user-faulting page-fault handlers. Doing so while holding
> the arm_smmu_asid_lock would stall any unrelated attachment in the system.
> 
> Split the teardown out of arm_smmu_remove_master_domain(), to a new helper
> arm_smmu_attach_release() that runs after arm_smmu_asid_lock is released.
> 
> Since no other device would use the old master_domain that is being freed,
> it's safe to move out of arm_smmu_asid_lock (still under the protection of
> iommu_group->mutex).

This is a lot of text if the next bit about being a refactor only
is accurate.  Seems that not blocking attachments is the issue and
to me that is a functional and useful change. However, is that
in this patch?  Anyhow to me this needs a rewrite to focus on just
what is actually changing here rather than the eventual picture.


> 
> Note: this is a pure refactor; no functional change.
> 
> Signed-off-by: Nicolin Chen <nicolinc at nvidia.com>
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> index 25982bdbcbd9..fce026efa44f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> @@ -194,6 +194,7 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
>  	arm_smmu_install_ste_for_dev(master, &ste);
>  	arm_smmu_attach_commit(&state);
>  	mutex_unlock(&arm_smmu_asid_lock);
> +	arm_smmu_attach_release(&state);
>  	return 0;
>  }
>  
> 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 5732f3ba0122..99baa59b39c9 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3285,9 +3285,9 @@ arm_smmu_master_build_invs(struct arm_smmu_master *master, bool ats_enabled,
>  	return master->build_invs;
>  }
>  
> -static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
> -					  struct iommu_domain *domain,
> -					  ioasid_t ssid)
> +static struct arm_smmu_master_domain *
> +arm_smmu_remove_master_domain(struct arm_smmu_master *master,
> +			      struct iommu_domain *domain, ioasid_t ssid)
>  {
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain_devices(domain);
>  	struct arm_smmu_master_domain *master_domain;
> @@ -3295,7 +3295,7 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
>  	unsigned long flags;
>  
>  	if (!smmu_domain)
> -		return;
> +		return NULL;
>  
>  	if (domain->type == IOMMU_DOMAIN_NESTED)
>  		nested_ats_flush = to_smmu_nested_domain(domain)->enable_ats;
> @@ -3310,8 +3310,23 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
>  	}
>  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
> +	/* arm_smmu_attach_release() will free it */
> +	return master_domain;
> +}
> +
> +/* Release the old master_domain detached by arm_smmu_remove_master_domain() */
> +void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
> +{
> +	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
> +	struct arm_smmu_master *master = state->master;
> +
> +	iommu_group_mutex_assert(master->dev);
> +
> +	if (!master_domain)

I guess this makes sense in later patches, but for now the local
variable seems more confusing than anything.

> +		return;

I'd add a blank line here to separate the sanity checks from bulk
code.


>  	arm_smmu_disable_iopf(master, master_domain);
>  	kfree(master_domain);
> +	state->old_master_domain = NULL;
>  }
>  

> @@ -3784,6 +3801,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
>
This path is hit from a failure of arm_smmu_attach_prepare()
At that point the old domain hasn't been detached.

Now it doesn't matter because of what is currently done in release,
but from a code flow / what that function is documented to be for
this seems wrong to me.  I'd separate the good and the bad
paths in the function.

-- 
Jonathan Cameron <jonathan.cameron at oss.qualcomm.com>