Re: [PATCH v5 09/17] iommu/arm-smmu-v3: Put writing the context descriptor in the right order

Mostafa Saleh
2024-02-13T15:43:15Z

Hi Jason,

On Tue, Feb 06, 2024 at 11:12:46AM -0400, Jason Gunthorpe 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.
>
> Reviewed-by: Michael Shavit <mshavit@google.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Moritz Fischer <moritzf@google.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> 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 340f3dc82c9ce0..2a6ac0af932c54 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2575,14 +2575,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)
> @@ -2659,6 +2651,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;
> + }

Instead of having duplicate
if (ret) {
master->domain = NULL;
goto out_list_del;
}

In the if and the else, we can just move it outside.

> }
>
> ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd);
> @@ -2668,15 +2671,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> }
>
> arm_smmu_make_cdtable_ste(&target, master);
> + 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;
> }
This invalidates the CD while the STE is in bypass/S2 which is a new behavior
to the driver, I don’t see anything from the user manual about this, so I
believe that is fine.

Reviewed-by: Mostafa Saleh <smostafa@google.com>

> - arm_smmu_install_ste_for_dev(master, &target);
>
> arm_smmu_enable_ats(master);
> goto out_unlock;
> --
> 2.43.0
>

Thanks,
Mostafa