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

[Vivek Kumar Gautam <vivek.gautam@arm.com>]

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
>>