Re: [PATCH RFC v1 08/11] iommu/arm-smmu-v3: Implement shared context alloc and free ops

From: Vivek Kumar Gautam
Date: Thu Sep 30 2021 - 05:51:13 EST

Next message: Johan Jonker: "[PATCH v2 1/3] dt-bindings: pinctrl: convert rockchip,pinctrl.txt to YAML"
Previous message: Lee Jones: "Re: [PATCH v7 2/5] dt-bindings: mfd: aspeed-lpc: Convert to YAML schema"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxx>
---
.../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

Next message: Johan Jonker: "[PATCH v2 1/3] dt-bindings: pinctrl: convert rockchip,pinctrl.txt to YAML"
Previous message: Lee Jones: "Re: [PATCH v7 2/5] dt-bindings: mfd: aspeed-lpc: Convert to YAML schema"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]