[PATCH v2 12/19] iommu/arm-smmu-v3: Put writing the context descriptor in the right order

[Michael Shavit]

On Tue, Nov 14, 2023 at 1:53 AM Jason Gunthorpe <jgg at nvidia.com> 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.
>
> Signed-off-by: Jason Gunthorpe <jgg at nvidia.com>
Reviewed-by: Michael Shavit <mshavit at google.com>

This patch might be a better fit before the previous one. When going
from S1 to S2 or bypass:
Pre-both patches, attach_dev() installs a NULL STE, then clears the
now unused CDE, then installs a new STE.
After the previous patch, attach_dev() clears the *still used* CDE,
and then replaces the STE.
After this patch, attach_dev() replaces the STE, and then clears the CDE

Reordering the two patches removes the scenario where we could hit a
NULL-ed CDE.

> ---
>  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 f70862806211de..eb5dcd357a42b8 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2495,14 +2495,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)
> @@ -2579,6 +2571,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;
> +                       }
>                 }
>
>                 ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd);
> @@ -2588,15 +2591,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>                 }
>
>                 arm_smmu_make_cdtable_ste(&target, master, &master->cd_table);
> +               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;
>         }
> -       arm_smmu_install_ste_for_dev(master, &target);
>
>         arm_smmu_enable_ats(master);
>         goto out_unlock;
> --
> 2.42.0
>