[PATCH v2 09/19] iommu/arm-smmu-v3: Hold arm_smmu_asid_lock during all of attach_dev

[Michael Shavit]

On Tue, Nov 14, 2023 at 1:53 AM Jason Gunthorpe <jgg at nvidia.com> wrote:
>
> The BTM support wants to be able to change the ASID of any smmu_domain.
> When it goes to do this it holds the arm_smmu_asid_lock and iterates over
> the target domain's devices list.
>
> During attach of a S1 domain we must ensure that the devices list and
> CD are in sync, otherwise we could miss CD updates or a parallel CD update
> could push an out of date CD.
>
> This is pretty complicated, and works today because arm_smmu_detach_dev()
> remove the CD table from the STE before working on the CD entries.
>
> The next patch will allow the CD table to remain in the STE so solve this
> racy by holding the lock for a longer period. The lock covers both of the
> changes to the device list and the CD table entries.
>
> Move arm_smmu_detach_dev() till after we have initialized the domain so
> the lock can be held for less time.
>
> Signed-off-by: Jason Gunthorpe <jgg at nvidia.com>
Reviewed-by: Michael Shavit <mshavit at google.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 ++++++++++++---------
>  1 file changed, 13 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 e80373885d8b19..b11dc03ee16880 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2560,8 +2560,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>                 return -EBUSY;
>         }
>
> -       arm_smmu_detach_dev(master);
> -
>         mutex_lock(&smmu_domain->init_mutex);
>
>         if (!smmu_domain->smmu) {
> @@ -2576,6 +2574,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>         if (ret)
>                 return ret;
>
> +       /*
> +        * Prevent arm_smmu_share_asid() from trying to change the ASID
> +        * of either the old or new domain while we are working on it.
> +        * This allows the STE and the smmu_domain->devices list to
> +        * be inconsistent during this routine.
> +        */
> +       mutex_lock(&arm_smmu_asid_lock);
> +
> +       arm_smmu_detach_dev(master);
> +
>         master->domain = smmu_domain;
>
>         /*
> @@ -2601,13 +2609,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>                         }
>                 }
>
> -               /*
> -                * Prevent SVA from concurrently modifying the CD or writing to
> -                * the CD entry
> -                */
> -               mutex_lock(&arm_smmu_asid_lock);
>                 ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd);
> -               mutex_unlock(&arm_smmu_asid_lock);
>                 if (ret) {
>                         master->domain = NULL;
>                         goto out_list_del;
> @@ -2617,13 +2619,15 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>         arm_smmu_install_ste_for_dev(master);
>
>         arm_smmu_enable_ats(master);
> -       return 0;
> +       goto out_unlock;
>
>  out_list_del:
>         spin_lock_irqsave(&smmu_domain->devices_lock, flags);
>         list_del(&master->domain_head);
>         spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>
> +out_unlock:
> +       mutex_unlock(&arm_smmu_asid_lock);
>         return ret;
>  }
>
> --
> 2.42.0
>