| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6f8b1656-5ca0-c106-db7d-366e536f5575@arm.com>
Date: Thu, 30 Sep 2021 15:20:31 +0530
From: Vivek Kumar Gautam <vivek.gautam@....com>
To: Jean-Philippe Brucker <jean-philippe@...aro.org>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
iommu@...ts.linux-foundation.org,
virtualization@...ts.linux-foundation.org, joro@...tes.org,
will.deacon@....com, mst@...hat.com, robin.murphy@....com,
eric.auger@...hat.com, kevin.tian@...el.com,
jacob.jun.pan@...ux.intel.com, yi.l.liu@...el.com,
Lorenzo.Pieralisi@....com, shameerali.kolothum.thodi@...wei.com
Subject: Re: [PATCH RFC v1 08/11] iommu/arm-smmu-v3: Implement shared context
alloc and free ops
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@....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
>>
Powered by blists - more mailing lists