Messages in this thread Patch in this message | | | Date | Tue, 1 Aug 2023 11:18:21 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH v2 6/8] iommu/arm-smmu-v3: Refactor write_ctx_desc |
| |
On Mon, Jul 31, 2023 at 06:48:16PM +0800, Michael Shavit wrote: > Update arm_smmu_write_ctx_desc and downstream functions to operate on > a master instead of an smmu domain. We expect arm_smmu_write_ctx_desc() > to only be called to write a CD entry into a CD table owned by the > master. Under the hood, arm_smmu_write_ctx_desc still fetches the CD > table from the domain that is attached to the master, but a subsequent > commit will move that table's ownership to the master. > > Note that this change isn't a nop refactor since SVA will call > arm_smmu_write_ctx_desc in a loop for every master the domain is > attached to despite the fact that they all share the same CD table. This > loop may look weird but becomes necessary when the CD table becomes > per-master in a subsequent commit. > > Signed-off-by: Michael Shavit <mshavit@google.com> > --- > > Changes in v2: > - minor style fixes > > Changes in v1: > - arm_smmu_write_ctx_desc now get's the CD table to write to from the > master parameter instead of a distinct parameter. This works well > because the CD table being written to should always be owned by the > master by the end of this series. This version no longer allows master > to be NULL. > > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 33 +++++++++-- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 59 ++++++++----------- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 +- > 3 files changed, 53 insertions(+), 41 deletions(-) > > 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..8242ee3405f2d 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 > @@ -45,9 +45,11 @@ static struct arm_smmu_ctx_desc * > arm_smmu_share_asid(struct mm_struct *mm, u16 asid) > { > int ret; > + unsigned long flags; > u32 new_asid; > struct arm_smmu_ctx_desc *cd; > struct arm_smmu_device *smmu; > + struct arm_smmu_master *master; > struct arm_smmu_domain *smmu_domain; > > cd = xa_load(&arm_smmu_asid_xa, asid); > @@ -80,7 +82,11 @@ 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); > + spin_lock_irqsave(&smmu_domain->devices_lock, flags); > + list_for_each_entry(master, &smmu_domain->devices, domain_head) { > + arm_smmu_write_ctx_desc(master, 0, cd); > + }
I think it is typical kernel style to not include the single statement {}
> + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > /* Invalidate TLB entries previously associated with that context */ > arm_smmu_tlb_inv_asid(smmu, asid); > @@ -211,6 +217,8 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm) > { > struct arm_smmu_mmu_notifier *smmu_mn = mn_to_smmu(mn); > struct arm_smmu_domain *smmu_domain = smmu_mn->domain; > + struct arm_smmu_master *master; > + unsigned long flags; > > mutex_lock(&sva_lock); > if (smmu_mn->cleared) { > @@ -222,7 +230,11 @@ 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); > + spin_lock_irqsave(&smmu_domain->devices_lock, flags); > + list_for_each_entry(master, &smmu_domain->devices, domain_head) { > + arm_smmu_write_ctx_desc(master, mm->pasid, &quiet_cd); > + } > + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
And here
> arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid); > arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0); > @@ -248,7 +260,9 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain, > struct mm_struct *mm) > { > int ret; > + unsigned long flags; > struct arm_smmu_ctx_desc *cd; > + struct arm_smmu_master *master; > struct arm_smmu_mmu_notifier *smmu_mn; > > list_for_each_entry(smmu_mn, &smmu_domain->mmu_notifiers, list) { > @@ -279,7 +293,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); > + 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);
And here..
> if (ret) > goto err_put_notifier; > > @@ -296,6 +314,8 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain, > > static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn) > { > + unsigned long flags; > + struct arm_smmu_master *master; > struct mm_struct *mm = smmu_mn->mn.mm; > struct arm_smmu_ctx_desc *cd = smmu_mn->cd; > struct arm_smmu_domain *smmu_domain = smmu_mn->domain; > @@ -304,7 +324,12 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn) > return; > > list_del(&smmu_mn->list); > - arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, NULL); > + > + spin_lock_irqsave(&smmu_domain->devices_lock, flags); > + list_for_each_entry(master, &smmu_domain->devices, domain_head) { > + arm_smmu_write_ctx_desc(master, mm->pasid, NULL); > + } > + spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
And here..
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?
> @@ -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
Jason
| |