Messages in this thread | | | From | Michael Shavit <> | Date | Wed, 2 Aug 2023 01:03:42 +0800 | Subject | Re: [PATCH v2 6/8] iommu/arm-smmu-v3: Refactor write_ctx_desc |
| |
On Tue, Aug 1, 2023 at 10:18 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > You know, you should try to keep the function instead of duplicating > these > > arm_smmu_write_ctx_desc_devices() > > And put the four lines in there?
Urhhh yes, I thought I had a reason for this but probably just a lapse of judgement. Done.
> - ret = arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, cd); > + 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, mm->pasid, cd); > + } > + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
Just noticed that this is problematic; we may not notice a failure if it occurs on an earlier iteration of the loop. Will fix.
> > > @@ -987,19 +985,14 @@ static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain, > > }, > > }; > > > > - if (!smmu_domain->cd_table.installed) > > + if (!master->domain->cd_table.installed) > > return; > > BTW, do you have locking for this? I didn't check carefully
This is one of the reasons I wanted to take this as a parameter to the function. This relies on callers guaranteeing that the cd table not be attached/detached while this call is in progress. This works now because: 1. No domain may be attached/detached while SVA is enabled, which is most of the calls that lead to arm_smmu_sync_cd 2. The other call to arm_smmu_write_ctx_desc in arm-smmu-v3.c is more obviously serialized with operations that detach/attach the cd table.
Maybe this should at least be a comment on arm_smmu_write_ctx_desc, if not a lock?
Speaking of.... I should probably flip this bit to false in patch 5 when the cd table is detached.
| |