Re: [PATCH v5 5/9] iommu/arm-smmu-v3: Refactor write_ctx_desc

From: Will Deacon
Date: Wed Aug 09 2023 - 09:50:09 EST


On Wed, Aug 09, 2023 at 01:12:01AM +0800, Michael Shavit wrote:
> 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..e3992a0c16377 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
> @@ -37,6 +37,24 @@ struct arm_smmu_bond {
>
> static DEFINE_MUTEX(sva_lock);
>
> +static int arm_smmu_write_ctx_desc_devices(struct arm_smmu_domain *smmu_domain,
> + int ssid,
> + struct arm_smmu_ctx_desc *cd)
> +{
> + struct arm_smmu_master *master;
> + unsigned long flags;
> + int ret;
> +
> + 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, ssid, cd);
> + if (ret)
> + break;
> + }
> + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> + return ret;
> +}
> +
> /*
> * Check if the CPU ASID is available on the SMMU side. If a private context
> * descriptor is using it, try to replace it.
> @@ -80,7 +98,7 @@ 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);
> + arm_smmu_write_ctx_desc_devices(smmu_domain, 0, cd);
>
> /* Invalidate TLB entries previously associated with that context */
> arm_smmu_tlb_inv_asid(smmu, asid);
> @@ -222,7 +240,7 @@ 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);
> + arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, &quiet_cd);
>
> arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid);
> arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
> @@ -279,9 +297,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);
> - if (ret)
> + ret = arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, cd);
> + if (ret) {
> + arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, NULL);

Why is it safe to drop the lock between these two calls?

> 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 c01023404c26c..34bd7815aeb8e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -971,14 +971,12 @@ void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid)
> arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
> }
>
> -static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
> +static void arm_smmu_sync_cd(struct arm_smmu_master *master,
> int ssid, bool leaf)
> {
> size_t i;
> - unsigned long flags;
> - struct arm_smmu_master *master;
> struct arm_smmu_cmdq_batch cmds;
> - struct arm_smmu_device *smmu = smmu_domain->smmu;
> + struct arm_smmu_device *smmu = master->smmu;
> struct arm_smmu_cmdq_ent cmd = {
> .opcode = CMDQ_OP_CFGI_CD,
> .cfgi = {
> @@ -988,15 +986,10 @@ static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
> };
>
> cmds.num = 0;
> -
> - spin_lock_irqsave(&smmu_domain->devices_lock, flags);

Since you're dropping this and relying on the lock being taken higher up
callstack, can we add a lockdep assertion that we do actually hold the
devices_lock, please?

Will