Messages in this thread | | | Subject | Re: [PATCH RFC v1 08/11] iommu/arm-smmu-v3: Implement shared context alloc and free ops | From | Vivek Kumar Gautam <> | Date | Thu, 30 Sep 2021 15:20:31 +0530 |
| |
Hi Jean,
On 9/21/21 9:37 PM, Jean-Philippe Brucker wrote: > On Fri, Apr 23, 2021 at 03:21:44PM +0530, Vivek Gautam wrote: >> Implementing the alloc_shared_cd and free_shared_cd in cd-lib, and >> start using them for arm-smmu-v3-sva implementation. >> >> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com> >> --- >> .../arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c | 71 ++++++++-------- >> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 83 ++++++++----------- >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 - >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 14 ---- >> 4 files changed, 73 insertions(+), 96 deletions(-) >> >> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c >> index 537b7c784d40..b87829796596 100644 >> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c >> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-cd-lib.c >> @@ -285,16 +285,14 @@ static bool arm_smmu_free_asid(struct xarray *xa, void *cookie_cd) >> * descriptor is using it, try to replace it. >> */ >> static struct arm_smmu_ctx_desc * >> -arm_smmu_share_asid(struct mm_struct *mm, u16 asid) >> +arm_smmu_share_asid(struct iommu_pasid_table *tbl, struct mm_struct *mm, >> + struct xarray *xa, u16 asid, u32 asid_bits) > > xa and asid_bits could be stored in some arch-specific section of the > iommu_pasid_table struct. Other table drivers wouldn't need those > arguments.
Okay, will move them to a separate structure section.
> > More a comment for the parent series: it may be clearer to give a > different prefix to functions in this file (arm_smmu_cd_, pst_arm_?). > Reading this patch I'm a little confused by what belongs in the IOMMU > driver and what is done by this library. (I also keep reading 'tbl' as > 'tlb'. Maybe we could make it 'table' since that doesn't take a lot more > space)
Yea, this may be confusing. I will fix these namings in my next version.
> >> { >> int ret; >> u32 new_asid; >> struct arm_smmu_ctx_desc *cd; >> - struct arm_smmu_device *smmu; >> - struct arm_smmu_domain *smmu_domain; >> - struct iommu_pasid_table *tbl; >> >> - cd = xa_load(&arm_smmu_asid_xa, asid); >> + cd = xa_load(xa, asid); >> if (!cd) >> return NULL; >> >> @@ -306,12 +304,8 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid) >> return cd; >> } >> >> - smmu_domain = container_of(cd, struct arm_smmu_domain, s1_cfg.cd); >> - smmu = smmu_domain->smmu; >> - tbl = smmu_domain->tbl; >> - >> - ret = xa_alloc(&arm_smmu_asid_xa, &new_asid, cd, >> - XA_LIMIT(1, (1 << smmu->asid_bits) - 1), GFP_KERNEL); >> + ret = xa_alloc(xa, &new_asid, cd, XA_LIMIT(1, (1 << asid_bits) - 1), >> + GFP_KERNEL); >> if (ret) >> return ERR_PTR(-ENOSPC); >> /* >> @@ -325,48 +319,52 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid) >> * be some overlap between use of both ASIDs, until we invalidate the >> * TLB. >> */ >> - ret = iommu_psdtable_write(tbl, &tbl->cfg, 0, cd); >> + ret = arm_smmu_write_ctx_desc(&tbl->cfg, 0, cd); >> if (ret) >> return ERR_PTR(-ENOSYS); >> >> /* Invalidate TLB entries previously associated with that context */ >> - iommu_psdtable_flush_tlb(tbl, smmu_domain, asid); >> + iommu_psdtable_flush_tlb(tbl, tbl->cookie, asid); >> >> - xa_erase(&arm_smmu_asid_xa, asid); >> + xa_erase(xa, asid); >> return NULL; >> } >> >> -struct arm_smmu_ctx_desc * >> -arm_smmu_alloc_shared_cd(struct iommu_pasid_table *tbl, struct mm_struct *mm) >> +static struct iommu_psdtable_mmu_notifier * >> +arm_smmu_alloc_shared_cd(struct iommu_pasid_table *tbl, struct mm_struct *mm, >> + struct xarray *xa, u32 asid_bits) >> { >> u16 asid; >> int err = 0; >> u64 tcr, par, reg; >> struct arm_smmu_ctx_desc *cd; >> struct arm_smmu_ctx_desc *ret = NULL; >> + struct iommu_psdtable_mmu_notifier *pst_mn; >> >> asid = arm64_mm_context_get(mm); >> if (!asid) >> return ERR_PTR(-ESRCH); >> >> + pst_mn = kzalloc(sizeof(*pst_mn), GFP_KERNEL); >> + if (!pst_mn) { >> + err = -ENOMEM; >> + goto out_put_context; >> + } >> + >> cd = kzalloc(sizeof(*cd), GFP_KERNEL); >> if (!cd) { >> err = -ENOMEM; >> - goto out_put_context; >> + goto out_free_mn; >> } >> >> refcount_set(&cd->refs, 1); >> >> - mutex_lock(&arm_smmu_asid_lock); >> - ret = arm_smmu_share_asid(mm, asid); >> + ret = arm_smmu_share_asid(tbl, mm, xa, asid, asid_bits); >> if (ret) { >> - mutex_unlock(&arm_smmu_asid_lock); >> goto out_free_cd; >> } >> >> - err = xa_insert(&arm_smmu_asid_xa, asid, cd, GFP_KERNEL); >> - mutex_unlock(&arm_smmu_asid_lock); >> - >> + err = xa_insert(xa, asid, cd, GFP_KERNEL); >> if (err) >> goto out_free_asid; >> >> @@ -406,21 +404,26 @@ arm_smmu_alloc_shared_cd(struct iommu_pasid_table *tbl, struct mm_struct *mm) >> cd->asid = asid; >> cd->mm = mm; >> >> - return cd; >> + pst_mn->vendor.cd = cd; >> + return pst_mn; >> >> out_free_asid: >> - iommu_psdtable_free_asid(tbl, &arm_smmu_asid_xa, cd); >> + arm_smmu_free_asid(xa, cd); >> out_free_cd: >> kfree(cd); >> +out_free_mn: >> + kfree(pst_mn); >> out_put_context: >> arm64_mm_context_put(mm); >> return err < 0 ? ERR_PTR(err) : ret; >> } >> >> -void arm_smmu_free_shared_cd(struct iommu_pasid_table *tbl, >> - struct arm_smmu_ctx_desc *cd) >> +static void arm_smmu_free_shared_cd(struct iommu_pasid_table *tbl, >> + struct xarray *xa, void *cookie) > > Could we pass a struct iommu_psdtable_mmu_notifier, since that's what > alloc_shared() returns?
Sure, will update this.
> >> { >> - if (iommu_psdtable_free_asid(tbl, &arm_smmu_asid_xa, cd)) { >> + struct arm_smmu_ctx_desc *cd = cookie; >> + >> + if (iommu_psdtable_free_asid(tbl, xa, cd)) { >> /* Unpin ASID */ >> arm64_mm_context_put(cd->mm); >> kfree(cd); >> @@ -428,11 +431,13 @@ void arm_smmu_free_shared_cd(struct iommu_pasid_table *tbl, >> } >> >> struct iommu_vendor_psdtable_ops arm_cd_table_ops = { >> - .alloc = arm_smmu_alloc_cd_tables, >> - .free = arm_smmu_free_cd_tables, >> - .prepare = arm_smmu_prepare_cd, >> - .write = arm_smmu_write_ctx_desc, >> - .free_asid = arm_smmu_free_asid, >> + .alloc = arm_smmu_alloc_cd_tables, >> + .free = arm_smmu_free_cd_tables, >> + .prepare = arm_smmu_prepare_cd, >> + .write = arm_smmu_write_ctx_desc, >> + .free_asid = arm_smmu_free_asid, >> + .alloc_shared = arm_smmu_alloc_shared_cd, >> + .free_shared = arm_smmu_free_shared_cd, >> }; >> >> struct iommu_pasid_table * >> 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 da35d4cc0c1e..ef28d0c409da 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 >> @@ -13,23 +13,12 @@ >> #include "../../io-pgtable-arm.h" >> #include "../../iommu-pasid-table.h" >> >> -struct arm_smmu_mmu_notifier { >> - struct mmu_notifier mn; >> - struct arm_smmu_ctx_desc *cd; >> - bool cleared; >> - refcount_t refs; >> - struct list_head list; >> - struct arm_smmu_domain *domain; >> -}; >> - >> -#define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn) >> - >> struct arm_smmu_bond { >> - struct iommu_sva sva; >> - struct mm_struct *mm; >> - struct arm_smmu_mmu_notifier *smmu_mn; >> - struct list_head list; >> - refcount_t refs; >> + struct iommu_sva sva; >> + struct mm_struct *mm; >> + struct iommu_psdtable_mmu_notifier *smmu_mn; >> + struct list_head list; >> + refcount_t refs; >> }; >> >> #define sva_to_bond(handle) \ >> @@ -41,20 +30,22 @@ static void arm_smmu_mm_invalidate_range(struct mmu_notifier *mn, >> struct mm_struct *mm, >> unsigned long start, unsigned long end) >> { >> - struct arm_smmu_mmu_notifier *smmu_mn = mn_to_smmu(mn); >> - struct arm_smmu_domain *smmu_domain = smmu_mn->domain; >> + struct iommu_psdtable_mmu_notifier *smmu_mn = mn_to_pstiommu(mn); >> + struct arm_smmu_domain *smmu_domain = smmu_mn->cookie; >> + struct arm_smmu_ctx_desc *cd = smmu_mn->vendor.cd; >> size_t size = end - start + 1; >> >> if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_BTM)) >> - arm_smmu_tlb_inv_range_asid(start, size, smmu_mn->cd->asid, >> + arm_smmu_tlb_inv_range_asid(start, size, cd->asid, >> PAGE_SIZE, false, smmu_domain); >> arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, start, size); >> } >> >> 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 iommu_psdtable_mmu_notifier *smmu_mn = mn_to_pstiommu(mn); >> + struct arm_smmu_domain *smmu_domain = smmu_mn->cookie; >> + struct arm_smmu_ctx_desc *cd = smmu_mn->vendor.cd; >> struct iommu_pasid_table *tbl = smmu_domain->tbl; >> >> mutex_lock(&sva_lock); >> @@ -69,7 +60,7 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm) >> */ >> iommu_psdtable_write(tbl, &tbl->cfg, mm->pasid, &quiet_cd); > > Another comment for the parent series: I'd prefer making this a > "iommu_psdtable_quiesce()" call, instead of passing "quiet_cd" between > driver and library. Because that won't work if the SMMU driver is a module > or disabled - build of virtio-iommu will probably fail since quiet_cd will > be undefined. We could make the library built-in and move quiet_cd there, > but an explicit library call seems cleaner.
Right, having a separte library method would look cleaner. I will update this and the below flush_tlb() call.
> >> >> - iommu_psdtable_flush_tlb(tbl, smmu_domain, smmu_mn->cd->asid); >> + iommu_psdtable_flush_tlb(tbl, smmu_domain, cd->asid); > > We can directly call arm_smmu_tlb_inv* here. iommu_psdtable_flush_tlb() > should only be called from the library. But with the previous comment, > this invalidation would move to the library. > >> arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0); >> >> smmu_mn->cleared = true; >> @@ -78,7 +69,7 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm) >> >> static void arm_smmu_mmu_notifier_free(struct mmu_notifier *mn) >> { >> - kfree(mn_to_smmu(mn)); >> + kfree(mn_to_pstiommu(mn)); >> } >> >> static struct mmu_notifier_ops arm_smmu_mmu_notifier_ops = { >> @@ -88,63 +79,59 @@ static struct mmu_notifier_ops arm_smmu_mmu_notifier_ops = { >> }; >> >> /* Allocate or get existing MMU notifier for this {domain, mm} pair */ >> -static struct arm_smmu_mmu_notifier * >> +static struct iommu_psdtable_mmu_notifier * >> arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain, >> struct mm_struct *mm) >> { >> int ret; >> - struct arm_smmu_ctx_desc *cd; >> - struct arm_smmu_mmu_notifier *smmu_mn; >> + struct iommu_psdtable_mmu_notifier *smmu_mn; >> + struct arm_smmu_device *smmu = smmu_domain->smmu; >> struct iommu_pasid_table *tbl = smmu_domain->tbl; >> >> - list_for_each_entry(smmu_mn, &smmu_domain->mmu_notifiers, list) { >> + list_for_each_entry(smmu_mn, &tbl->mmu_notifiers, list) { >> if (smmu_mn->mn.mm == mm) { >> refcount_inc(&smmu_mn->refs); >> return smmu_mn; >> } >> } >> >> - cd = arm_smmu_alloc_shared_cd(tbl, mm); >> - if (IS_ERR(cd)) >> - return ERR_CAST(cd); >> - >> - smmu_mn = kzalloc(sizeof(*smmu_mn), GFP_KERNEL); >> - if (!smmu_mn) { >> - ret = -ENOMEM; >> - goto err_free_cd; >> - } >> + mutex_lock(&arm_smmu_asid_lock); >> + smmu_mn = iommu_psdtable_alloc_shared(tbl, mm, &arm_smmu_asid_xa, >> + smmu->asid_bits); >> + mutex_unlock(&arm_smmu_asid_lock); >> + if (IS_ERR(smmu_mn)) >> + return ERR_CAST(smmu_mn); >> >> refcount_set(&smmu_mn->refs, 1); >> - smmu_mn->cd = cd; >> - smmu_mn->domain = smmu_domain; >> + smmu_mn->cookie = smmu_domain; >> smmu_mn->mn.ops = &arm_smmu_mmu_notifier_ops; >> >> ret = mmu_notifier_register(&smmu_mn->mn, mm); >> - if (ret) { >> - kfree(smmu_mn); >> + if (ret) >> goto err_free_cd; >> - } >> >> - ret = iommu_psdtable_write(tbl, &tbl->cfg, mm->pasid, cd); >> + ret = iommu_psdtable_write(tbl, &tbl->cfg, mm->pasid, >> + smmu_mn->vendor.cd); > > Pass smmu_mn here, and let the library code get the cd (to allow for other > pasid table implementations)
Okay.
> >> if (ret) >> goto err_put_notifier; >> >> - list_add(&smmu_mn->list, &smmu_domain->mmu_notifiers); >> + list_add(&smmu_mn->list, &tbl->mmu_notifiers); > > I'd keep the mmu_notifiers list in domain if the library doesn't use it > for anything. > > That made me wonder whether the whole of arm_smmu_mmu_notifer_get/put() > could move to the library, since the virtio-iommu version seems to be the > same. They probably belong in iommu-sva-lib but we can revisit that when > there are more users.
Yea, I will move these notifier calls to the library. This makes it easier for virtio-iommu too.
Best regards Vivek
> > Thanks, > Jean >
[snip]
>> -- >> 2.17.1 >>
| |