[v2] Share sva domains with all devices bound to a mm

[PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains

Posted by Tina Zhang 2 years, 3 months ago

Each mm bound to devices gets a PASID and corresponding sva domains
allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
field of the mm. The PASID is released in __mmdrop(), while a sva domain
is released when no one is using it (the reference count is decremented
in iommu_sva_unbind_device()).

Since the required info of PASID and sva domains is kept in struct
iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
field in mm struct. The sva domain list is protected by iommu_sva_lock.

Besides, this patch removes mm_pasid_init(), as with the introduced
iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/iommu-sva.c | 38 +++++++++++++++++++++++++-------------
 include/linux/iommu.h     | 10 +++-------
 kernel/fork.c             |  1 -
 3 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 0a4a1ed40814c..35366f51ad27d 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -15,6 +15,7 @@ static DEFINE_IDA(iommu_global_pasid_ida);
 /* Allocate a PASID for the mm within range (inclusive) */
 static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
 {
+	struct iommu_mm_data *iommu_mm = NULL;
 	int ret = 0;
 
 	if (min == IOMMU_PASID_INVALID ||
@@ -33,11 +34,22 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
 		goto out;
 	}
 
+	iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL);
+	if (!iommu_mm) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
-	if (ret < 0)
+	if (ret < 0) {
+		kfree(iommu_mm);
 		goto out;
+	}
+
+	iommu_mm->pasid = ret;
+	mm->iommu_mm = iommu_mm;
+	INIT_LIST_HEAD(&mm->iommu_mm->sva_domains);
 
-	mm->pasid = ret;
 	ret = 0;
 out:
 	mutex_unlock(&iommu_sva_lock);
@@ -82,16 +94,12 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 
 	mutex_lock(&iommu_sva_lock);
 	/* Search for an existing domain. */
-	domain = iommu_get_domain_for_dev_pasid(dev, mm_get_pasid(mm),
-						IOMMU_DOMAIN_SVA);
-	if (IS_ERR(domain)) {
-		ret = PTR_ERR(domain);
-		goto out_unlock;
-	}
-
-	if (domain) {
-		domain->users++;
-		goto out;
+	list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
+		ret = iommu_attach_device_pasid(domain, dev, mm_get_pasid(mm));
+		if (!ret) {
+			domain->users++;
+			goto out;
+		}
 	}
 
 	/* Allocate a new domain and set it on device pasid. */
@@ -105,6 +113,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 	if (ret)
 		goto out_free_domain;
 	domain->users = 1;
+	list_add(&domain->next, &mm->iommu_mm->sva_domains);
+
 out:
 	mutex_unlock(&iommu_sva_lock);
 	handle->dev = dev;
@@ -137,8 +147,9 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
 	struct device *dev = handle->dev;
 
 	mutex_lock(&iommu_sva_lock);
+	iommu_detach_device_pasid(domain, dev, pasid);
 	if (--domain->users == 0) {
-		iommu_detach_device_pasid(domain, dev, pasid);
+		list_del(&domain->next);
 		iommu_domain_free(domain);
 	}
 	mutex_unlock(&iommu_sva_lock);
@@ -218,4 +229,5 @@ void mm_pasid_drop(struct mm_struct *mm)
 		return;
 
 	ida_free(&iommu_global_pasid_ida, mm_get_pasid(mm));
+	kfree(mm->iommu_mm);
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 937f3abc26f2e..cfbd35ceb375f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -109,6 +109,7 @@ struct iommu_domain {
 		struct {	/* IOMMU_DOMAIN_SVA */
 			struct mm_struct *mm;
 			int users;
+			struct list_head next;
 		};
 	};
 };
@@ -1177,17 +1178,13 @@ static inline bool tegra_dev_iommu_get_stream_id(struct device *dev, u32 *stream
 }
 
 #ifdef CONFIG_IOMMU_SVA
-static inline void mm_pasid_init(struct mm_struct *mm)
-{
-	mm->pasid = IOMMU_PASID_INVALID;
-}
 static inline bool mm_valid_pasid(struct mm_struct *mm)
 {
-	return mm->pasid != IOMMU_PASID_INVALID;
+	return mm->iommu_mm ? true : false;
 }
 static inline u32 mm_get_pasid(struct mm_struct *mm)
 {
-	return mm->pasid;
+	return mm->iommu_mm ? mm->iommu_mm->pasid : IOMMU_PASID_INVALID;
 }
 static inline u32 mm_get_enqcmd_pasid(struct mm_struct *mm)
 {
@@ -1213,7 +1210,6 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
 {
 	return IOMMU_PASID_INVALID;
 }
-static inline void mm_pasid_init(struct mm_struct *mm) {}
 static inline bool mm_valid_pasid(struct mm_struct *mm) { return false; }
 static inline u32 mm_get_pasid(struct mm_struct *mm)
 {
diff --git a/kernel/fork.c b/kernel/fork.c
index d2e12b6d2b180..f06392dd1ca8a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1274,7 +1274,6 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
 	mm_init_cpumask(mm);
 	mm_init_aio(mm);
 	mm_init_owner(mm, p);
-	mm_pasid_init(mm);
 	RCU_INIT_POINTER(mm->exe_file, NULL);
 	mmu_notifier_subscriptions_init(mm);
 	init_tlb_flush_pending(mm);
-- 
2.34.1

Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains

Posted by Baolu Lu 2 years, 3 months ago

On 2023/8/27 16:44, Tina Zhang wrote:
> Each mm bound to devices gets a PASID and corresponding sva domains
> allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> field of the mm. The PASID is released in __mmdrop(), while a sva domain
> is released when no one is using it (the reference count is decremented
> in iommu_sva_unbind_device()).
> 
> Since the required info of PASID and sva domains is kept in struct
> iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
> field in mm struct. The sva domain list is protected by iommu_sva_lock.
> 
> Besides, this patch removes mm_pasid_init(), as with the introduced
> iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>   drivers/iommu/iommu-sva.c | 38 +++++++++++++++++++++++++-------------
>   include/linux/iommu.h     | 10 +++-------
>   kernel/fork.c             |  1 -
>   3 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 0a4a1ed40814c..35366f51ad27d 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -15,6 +15,7 @@ static DEFINE_IDA(iommu_global_pasid_ida);
>   /* Allocate a PASID for the mm within range (inclusive) */
>   static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
>   {
> +	struct iommu_mm_data *iommu_mm = NULL;

No need to initialize this variable.

>   	int ret = 0;
>   
>   	if (min == IOMMU_PASID_INVALID ||
> @@ -33,11 +34,22 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t ma
>   		goto out;
>   	}
>   
> +	iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL);
> +	if (!iommu_mm) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
>   	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		kfree(iommu_mm);
>   		goto out;
> +	}
> +
> +	iommu_mm->pasid = ret;
> +	mm->iommu_mm = iommu_mm;
> +	INIT_LIST_HEAD(&mm->iommu_mm->sva_domains);

If you change the order of the two lines above, it will look more
comfortable.

>   
> -	mm->pasid = ret;
>   	ret = 0;
>   out:
>   	mutex_unlock(&iommu_sva_lock);
> @@ -82,16 +94,12 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>   
>   	mutex_lock(&iommu_sva_lock);
>   	/* Search for an existing domain. */
> -	domain = iommu_get_domain_for_dev_pasid(dev, mm_get_pasid(mm),
> -						IOMMU_DOMAIN_SVA);
> -	if (IS_ERR(domain)) {
> -		ret = PTR_ERR(domain);
> -		goto out_unlock;
> -	}
> -
> -	if (domain) {
> -		domain->users++;
> -		goto out;
> +	list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
> +		ret = iommu_attach_device_pasid(domain, dev, mm_get_pasid(mm));
> +		if (!ret) {
> +			domain->users++;
> +			goto out;
> +		}
>   	}
>   
>   	/* Allocate a new domain and set it on device pasid. */
> @@ -105,6 +113,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>   	if (ret)
>   		goto out_free_domain;
>   	domain->users = 1;
> +	list_add(&domain->next, &mm->iommu_mm->sva_domains);
> +
>   out:
>   	mutex_unlock(&iommu_sva_lock);
>   	handle->dev = dev;
> @@ -137,8 +147,9 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
>   	struct device *dev = handle->dev;
>   
>   	mutex_lock(&iommu_sva_lock);
> +	iommu_detach_device_pasid(domain, dev, pasid);
>   	if (--domain->users == 0) {
> -		iommu_detach_device_pasid(domain, dev, pasid);
> +		list_del(&domain->next);
>   		iommu_domain_free(domain);
>   	}
>   	mutex_unlock(&iommu_sva_lock);
> @@ -218,4 +229,5 @@ void mm_pasid_drop(struct mm_struct *mm)
>   		return;
>   
>   	ida_free(&iommu_global_pasid_ida, mm_get_pasid(mm));
> +	kfree(mm->iommu_mm);
>   }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 937f3abc26f2e..cfbd35ceb375f 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -109,6 +109,7 @@ struct iommu_domain {
>   		struct {	/* IOMMU_DOMAIN_SVA */
>   			struct mm_struct *mm;
>   			int users;
> +			struct list_head next;

It looks better if you add a comment to describe how this list node is
used and how the list is protected.

>   		};
>   	};
>   };
> @@ -1177,17 +1178,13 @@ static inline bool tegra_dev_iommu_get_stream_id(struct device *dev, u32 *stream
>   }
>   
>   #ifdef CONFIG_IOMMU_SVA
> -static inline void mm_pasid_init(struct mm_struct *mm)
> -{
> -	mm->pasid = IOMMU_PASID_INVALID;
> -}
>   static inline bool mm_valid_pasid(struct mm_struct *mm)
>   {
> -	return mm->pasid != IOMMU_PASID_INVALID;
> +	return mm->iommu_mm ? true : false;
>   }
>   static inline u32 mm_get_pasid(struct mm_struct *mm)
>   {
> -	return mm->pasid;
> +	return mm->iommu_mm ? mm->iommu_mm->pasid : IOMMU_PASID_INVALID;
>   }
>   static inline u32 mm_get_enqcmd_pasid(struct mm_struct *mm)
>   {
> @@ -1213,7 +1210,6 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
>   {
>   	return IOMMU_PASID_INVALID;
>   }
> -static inline void mm_pasid_init(struct mm_struct *mm) {}
>   static inline bool mm_valid_pasid(struct mm_struct *mm) { return false; }
>   static inline u32 mm_get_pasid(struct mm_struct *mm)
>   {
> diff --git a/kernel/fork.c b/kernel/fork.c
> index d2e12b6d2b180..f06392dd1ca8a 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1274,7 +1274,6 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
>   	mm_init_cpumask(mm);
>   	mm_init_aio(mm);
>   	mm_init_owner(mm, p);
> -	mm_pasid_init(mm);
>   	RCU_INIT_POINTER(mm->exe_file, NULL);
>   	mmu_notifier_subscriptions_init(mm);
>   	init_tlb_flush_pending(mm);

This patch overall looks good to me. With above nits addressed,

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains

Posted by Vasant Hegde 2 years, 3 months ago

Hi Tina,

On 8/27/2023 2:14 PM, Tina Zhang wrote:
> Each mm bound to devices gets a PASID and corresponding sva domains
> allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> field of the mm. The PASID is released in __mmdrop(), while a sva domain
> is released when no one is using it (the reference count is decremented
> in iommu_sva_unbind_device()).
> 
> Since the required info of PASID and sva domains is kept in struct
> iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
> field in mm struct. The sva domain list is protected by iommu_sva_lock.
> 
> Besides, this patch removes mm_pasid_init(), as with the introduced
> iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> ---
>  drivers/iommu/iommu-sva.c | 38 +++++++++++++++++++++++++-------------
>  include/linux/iommu.h     | 10 +++-------
>  kernel/fork.c             |  1 -
>  3 files changed, 28 insertions(+), 21 deletions(-)
> 


.../...

>  
>  	/* Allocate a new domain and set it on device pasid. */
> @@ -105,6 +113,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>  	if (ret)
>  		goto out_free_domain;
>  	domain->users = 1;
> +	list_add(&domain->next, &mm->iommu_mm->sva_domains);
> +
>  out:
>  	mutex_unlock(&iommu_sva_lock);
>  	handle->dev = dev;
> @@ -137,8 +147,9 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
>  	struct device *dev = handle->dev;
>  
>  	mutex_lock(&iommu_sva_lock);
> +	iommu_detach_device_pasid(domain, dev, pasid);
>  	if (--domain->users == 0) {
> -		iommu_detach_device_pasid(domain, dev, pasid);
> +		list_del(&domain->next);
>  		iommu_domain_free(domain);
>  	}
>  	mutex_unlock(&iommu_sva_lock);
> @@ -218,4 +229,5 @@ void mm_pasid_drop(struct mm_struct *mm)
>  		return;
>  
>  	ida_free(&iommu_global_pasid_ida, mm_get_pasid(mm));
> +	kfree(mm->iommu_mm);


I am not sure whether I understood the flow completely. Just wondering why you
are not freeing pasid in iommu_sva_unbind_device().
I mean once iommu_mm->sva_domains becomes free shouldn't we free the
iommu_mm->pasid?

Also in this function (mm_pasid_drop()), should we check/free sva domains?

-Vasant

Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains

Posted by Jason Gunthorpe 2 years, 3 months ago

On Mon, Aug 28, 2023 at 02:02:52PM +0530, Vasant Hegde wrote:

> I am not sure whether I understood the flow completely. Just wondering why you
> are not freeing pasid in iommu_sva_unbind_device().
> I mean once iommu_mm->sva_domains becomes free shouldn't we free the
> iommu_mm->pasid?

No, for Intel use cases that PASID permanently becomes part of the
ENQCMD MSR and cannot be revoked once it has been set
 
> Also in this function (mm_pasid_drop()), should we check/free sva domains?

No, the driver must support a SVA domain with an empty mm_struct, eg
by hooking release.

Jason

Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains

Posted by Vasant Hegde 2 years, 3 months ago

On 8/31/2023 2:06 AM, Jason Gunthorpe wrote:
> On Mon, Aug 28, 2023 at 02:02:52PM +0530, Vasant Hegde wrote:
> 
>> I am not sure whether I understood the flow completely. Just wondering why you
>> are not freeing pasid in iommu_sva_unbind_device().
>> I mean once iommu_mm->sva_domains becomes free shouldn't we free the
>> iommu_mm->pasid?
> 
> No, for Intel use cases that PASID permanently becomes part of the
> ENQCMD MSR and cannot be revoked once it has been set
>  

Fair enough.

Patch description did explain it to some extent. ("The PASID is released in
__mmdrop()"). May be this needs to be expanded to cover why pasid is not
released in iommu_sva_unbind_device().

>> Also in this function (mm_pasid_drop()), should we check/free sva domains?
> 
> No, the driver must support a SVA domain with an empty mm_struct, eg
> by hooking release.

Sorry. Looks like confused you. Looking into code I got this.

My question was: when PASID is enabled, is there any chance of mm_pasid_drop()
getting called before freeing all SVA domains?

-Vasant

Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains

Posted by Baolu Lu 2 years, 3 months ago

On 2023/8/31 14:42, Vasant Hegde wrote:
>>> Also in this function (mm_pasid_drop()), should we check/free sva domains?
>> No, the driver must support a SVA domain with an empty mm_struct, eg
>> by hooking release.
> Sorry. Looks like confused you. Looking into code I got this.
> 
> My question was: when PASID is enabled, is there any chance of mm_pasid_drop()
> getting called before freeing all SVA domains?

I remember we have discussed this during sva development. If I remember
it correctly, in any case, mm_pasid_drop() will be called after the
device is closed. The device driver will detach the sva domains in the
close path.

Best regards,
baolu

Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains

Posted by Jason Gunthorpe 2 years, 3 months ago

On Thu, Aug 31, 2023 at 03:35:36PM +0800, Baolu Lu wrote:
> On 2023/8/31 14:42, Vasant Hegde wrote:
> > > > Also in this function (mm_pasid_drop()), should we check/free sva domains?
> > > No, the driver must support a SVA domain with an empty mm_struct, eg
> > > by hooking release.
> > Sorry. Looks like confused you. Looking into code I got this.
> > 
> > My question was: when PASID is enabled, is there any chance of mm_pasid_drop()
> > getting called before freeing all SVA domains?
> 
> I remember we have discussed this during sva development. If I remember
> it correctly, in any case, mm_pasid_drop() will be called after the
> device is closed. 

Yes, we guarentee this because the SVA domain should be holding a
mmgrab on the mm_struct while it exists. The mmdrop cannot happen
until the SVA domain is freed which puts back that ref.

But drivers must not make any assumptions about this, the lifecycle of
the PASID is a core concern, so long as the driver is asked to attach
a domain to a PASID it should assume the PASID is valid.

A driver should *never* look at the mm_struct PASID, all the examples
of this in-tree today are wrong.

Jason

RE: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains

Posted by Zhang, Tina 2 years, 3 months ago

Hi Vasant,

> -----Original Message-----
> From: Vasant Hegde <vasant.hegde@amd.com>
> Sent: Monday, August 28, 2023 4:33 PM
> To: Zhang, Tina <tina.zhang@intel.com>; Jason Gunthorpe <jgg@ziepe.ca>;
> Tian, Kevin <kevin.tian@intel.com>; Lu Baolu <baolu.lu@linux.intel.com>;
> Michael Shavit <mshavit@google.com>
> Cc: iommu@lists.linux.dev; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains
> 
> Hi Tina,
> 
> On 8/27/2023 2:14 PM, Tina Zhang wrote:
> > Each mm bound to devices gets a PASID and corresponding sva domains
> > allocated in iommu_sva_bind_device(), which are referenced by
> iommu_mm
> > field of the mm. The PASID is released in __mmdrop(), while a sva
> > domain is released when no one is using it (the reference count is
> > decremented in iommu_sva_unbind_device()).
> >
> > Since the required info of PASID and sva domains is kept in struct
> > iommu_mm_data of a mm, use mm->iommu_mm field instead of the old
> pasid
> > field in mm struct. The sva domain list is protected by iommu_sva_lock.
> >
> > Besides, this patch removes mm_pasid_init(), as with the introduced
> > iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.
> >
> > Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> > ---
> >  drivers/iommu/iommu-sva.c | 38 +++++++++++++++++++++++++-------------
> >  include/linux/iommu.h     | 10 +++-------
> >  kernel/fork.c             |  1 -
> >  3 files changed, 28 insertions(+), 21 deletions(-)
> >
> 
> 
> .../...
> 
> >
> >  	/* Allocate a new domain and set it on device pasid. */ @@ -105,6
> > +113,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> struct mm_struct *mm
> >  	if (ret)
> >  		goto out_free_domain;
> >  	domain->users = 1;
> > +	list_add(&domain->next, &mm->iommu_mm->sva_domains);
> > +
> >  out:
> >  	mutex_unlock(&iommu_sva_lock);
> >  	handle->dev = dev;
> > @@ -137,8 +147,9 @@ void iommu_sva_unbind_device(struct iommu_sva
> *handle)
> >  	struct device *dev = handle->dev;
> >
> >  	mutex_lock(&iommu_sva_lock);
> > +	iommu_detach_device_pasid(domain, dev, pasid);
> >  	if (--domain->users == 0) {
> > -		iommu_detach_device_pasid(domain, dev, pasid);
> > +		list_del(&domain->next);
> >  		iommu_domain_free(domain);
> >  	}
> >  	mutex_unlock(&iommu_sva_lock);
> > @@ -218,4 +229,5 @@ void mm_pasid_drop(struct mm_struct *mm)
> >  		return;
> >
> >  	ida_free(&iommu_global_pasid_ida, mm_get_pasid(mm));
> > +	kfree(mm->iommu_mm);
> 
> 
> I am not sure whether I understood the flow completely. Just wondering why
> you are not freeing pasid in iommu_sva_unbind_device().
> I mean once iommu_mm->sva_domains becomes free shouldn't we free the
> iommu_mm->pasid?
No, the sva domain and the PASID are separate objects with their own lifecycles.
The iommu_mm->pasid is released when the mm is being released, meanwhile the sva_domain is released when no one is using it.

Regards,
-Tina
> 
> Also in this function (mm_pasid_drop()), should we check/free sva domains?
> 
> -Vasant

Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains

Posted by Vasant Hegde 2 years, 3 months ago

Hi Tina,

On 8/28/2023 2:40 PM, Zhang, Tina wrote:
> Hi Vasant,
> 
>> -----Original Message-----
>> From: Vasant Hegde <vasant.hegde@amd.com>
>> Sent: Monday, August 28, 2023 4:33 PM
>> To: Zhang, Tina <tina.zhang@intel.com>; Jason Gunthorpe <jgg@ziepe.ca>;
>> Tian, Kevin <kevin.tian@intel.com>; Lu Baolu <baolu.lu@linux.intel.com>;
>> Michael Shavit <mshavit@google.com>
>> Cc: iommu@lists.linux.dev; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v2 4/5] iommu: Support mm PASID 1:n with sva domains
>>
>> Hi Tina,
>>
>> On 8/27/2023 2:14 PM, Tina Zhang wrote:
>>> Each mm bound to devices gets a PASID and corresponding sva domains
>>> allocated in iommu_sva_bind_device(), which are referenced by
>> iommu_mm
>>> field of the mm. The PASID is released in __mmdrop(), while a sva
>>> domain is released when no one is using it (the reference count is
>>> decremented in iommu_sva_unbind_device()).
>>>
>>> Since the required info of PASID and sva domains is kept in struct
>>> iommu_mm_data of a mm, use mm->iommu_mm field instead of the old
>> pasid
>>> field in mm struct. The sva domain list is protected by iommu_sva_lock.
>>>
>>> Besides, this patch removes mm_pasid_init(), as with the introduced
>>> iommu_mm structure, initializing mm pasid in mm_init() is unnecessary.
>>>
>>> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
>>> ---
>>>  drivers/iommu/iommu-sva.c | 38 +++++++++++++++++++++++++-------------
>>>  include/linux/iommu.h     | 10 +++-------
>>>  kernel/fork.c             |  1 -
>>>  3 files changed, 28 insertions(+), 21 deletions(-)
>>>
>>
>>
>> .../...
>>
>>>
>>>  	/* Allocate a new domain and set it on device pasid. */ @@ -105,6
>>> +113,8 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev,
>> struct mm_struct *mm
>>>  	if (ret)
>>>  		goto out_free_domain;
>>>  	domain->users = 1;
>>> +	list_add(&domain->next, &mm->iommu_mm->sva_domains);
>>> +
>>>  out:
>>>  	mutex_unlock(&iommu_sva_lock);
>>>  	handle->dev = dev;
>>> @@ -137,8 +147,9 @@ void iommu_sva_unbind_device(struct iommu_sva
>> *handle)
>>>  	struct device *dev = handle->dev;
>>>
>>>  	mutex_lock(&iommu_sva_lock);
>>> +	iommu_detach_device_pasid(domain, dev, pasid);
>>>  	if (--domain->users == 0) {
>>> -		iommu_detach_device_pasid(domain, dev, pasid);
>>> +		list_del(&domain->next);
>>>  		iommu_domain_free(domain);
>>>  	}
>>>  	mutex_unlock(&iommu_sva_lock);
>>> @@ -218,4 +229,5 @@ void mm_pasid_drop(struct mm_struct *mm)
>>>  		return;
>>>
>>>  	ida_free(&iommu_global_pasid_ida, mm_get_pasid(mm));
>>> +	kfree(mm->iommu_mm);
>>
>>
>> I am not sure whether I understood the flow completely. Just wondering why
>> you are not freeing pasid in iommu_sva_unbind_device().
>> I mean once iommu_mm->sva_domains becomes free shouldn't we free the
>> iommu_mm->pasid?
> No, the sva domain and the PASID are separate objects with their own lifecycles.
> The iommu_mm->pasid is released when the mm is being released, meanwhile the sva_domain is released when no one is using it.

Thanks for the explanation.

-Vasant