[RFC,5/6] iommu: Support mm PASID 1:1 with sva domain

  Each mm bound to devices gets a PASID and a corresponding sva domain
allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
field of the mm. And that PASID and sva domain get released in iommu_sva_
unbind_device() when no devices are binding to that mm. As a result,
during the life cycle, sva domain has 1:1 with mm PASID.

Since the required info of PASID and sva domain are kept in struct
iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
field in mm struct.

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

On Fri, Jul 07, 2023 at 09:34:40AM +0800, Tina Zhang wrote:
> Each mm bound to devices gets a PASID and a corresponding sva domain
> allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> field of the mm. And that PASID and sva domain get released in iommu_sva_
> unbind_device() when no devices are binding to that mm. As a result,
> during the life cycle, sva domain has 1:1 with mm PASID.
> 
> Since the required info of PASID and sva domain are kept in struct
> iommu_mm_data of a mm, use mm->iommu_mm field instead of the old pasid
> field in mm struct.

This is not technically right, the domains need to be a list, and we
need to search the list. Almost always the list will have 0 or 1
entries in it.

> @@ -88,31 +98,41 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>  		goto out_unlock;
>  	}
>  
> -	if (domain) {
> -		domain->users++;
> -		goto out;
> +	if (unlikely(domain)) {
> +		/* Re-attach the device to the same domain? */
> +		if (domain == sva_domain) {
> +			goto out;
> +		} else {
> +			/* Didn't get detached from the previous domain? */
> +			ret = -EBUSY;
> +			goto out_unlock;
> +		}
>  	}

And if we do all of this we should just get rid of the horrible
iommu_get_domain_for_dev_pasid() entirely.

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 20135912584ba..1511ded7bc910 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -1175,20 +1175,20 @@ 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;
> +	mm->iommu_mm = &default_iommu_mm;
>  }
>  static inline bool mm_valid_pasid(struct mm_struct *mm)
>  {
> -	return mm->pasid != IOMMU_PASID_INVALID;
> +	return mm->iommu_mm->pasid != IOMMU_PASID_INVALID;
>  }

And these can now just test if the iommu_mmu->sva_domain is NULL

Jaon
  

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, July 11, 2023 1:29 AM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; Joerg Roedel <joro@8bytes.org>; Will
> Deacon <will@kernel.org>; Lu Baolu <baolu.lu@linux.intel.com>; Michael
> Shavit <mshavit@google.com>; iommu@lists.linux.dev; linux-
> kernel@vger.kernel.org
> Subject: Re: [RFC PATCH 5/6] iommu: Support mm PASID 1:1 with sva domain
> 
> On Fri, Jul 07, 2023 at 09:34:40AM +0800, Tina Zhang wrote:
> > Each mm bound to devices gets a PASID and a corresponding sva domain
> > allocated in iommu_sva_bind_device(), which are referenced by iommu_mm
> > field of the mm. And that PASID and sva domain get released in
> > iommu_sva_
> > unbind_device() when no devices are binding to that mm. As a result,
> > during the life cycle, sva domain has 1:1 with mm PASID.
> >
> > Since the required info of PASID and sva domain are kept in struct
> > iommu_mm_data of a mm, use mm->iommu_mm field instead of the old
> pasid
> > field in mm struct.
> 
> This is not technically right, the domains need to be a list, and we need to
> search the list. Almost always the list will have 0 or 1 entries in it.
OK. Then, the mapping between sva_domain and pasid is n:1. IIUC, the reason why we want this n:1 is because sva_domain is a type of iommu_domain and therefore may have some hardware IOMMU specific configurations that cannot be shared between different IOMMUs.

> 
> > @@ -88,31 +98,41 @@ struct iommu_sva *iommu_sva_bind_device(struct
> device *dev, struct mm_struct *mm
> >  		goto out_unlock;
> >  	}
> >
> > -	if (domain) {
> > -		domain->users++;
> > -		goto out;
> > +	if (unlikely(domain)) {
> > +		/* Re-attach the device to the same domain? */
> > +		if (domain == sva_domain) {
> > +			goto out;
> > +		} else {
> > +			/* Didn't get detached from the previous domain? */
> > +			ret = -EBUSY;
> > +			goto out_unlock;
> > +		}
> >  	}
> 
> And if we do all of this we should just get rid of the horrible
> iommu_get_domain_for_dev_pasid() entirely.
I think it depends on whether we could get rid of iommu_group->pasid_array, right? In the current implementation, it seems iommu_group->pasid_array is expected to identify the usages that need PASID-granular DMA address translation (i.e., not only for sva usage).

> 
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> > 20135912584ba..1511ded7bc910 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -1175,20 +1175,20 @@ 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;
> > +	mm->iommu_mm = &default_iommu_mm;
> >  }
> >  static inline bool mm_valid_pasid(struct mm_struct *mm)  {
> > -	return mm->pasid != IOMMU_PASID_INVALID;
> > +	return mm->iommu_mm->pasid != IOMMU_PASID_INVALID;
> >  }
> 
> And these can now just test if the iommu_mmu->sva_domain is NULL
Right. Baolu also shared a similar suggestion. Thanks.

Regards,
-Tina

> 
> Jaon
  

On 2023/7/11 1:28, Jason Gunthorpe wrote:
>> @@ -88,31 +98,41 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
>>   		goto out_unlock;
>>   	}
>>   
>> -	if (domain) {
>> -		domain->users++;
>> -		goto out;
>> +	if (unlikely(domain)) {
>> +		/* Re-attach the device to the same domain? */
>> +		if (domain == sva_domain) {
>> +			goto out;
>> +		} else {
>> +			/* Didn't get detached from the previous domain? */
>> +			ret = -EBUSY;
>> +			goto out_unlock;
>> +		}
>>   	}
> And if we do all of this we should just get rid of the horrible
> iommu_get_domain_for_dev_pasid() entirely.

At the core level, we have no idea about whether an sva domain allocated
for one device is compatible with another device. Hence, we should loop
the sva domains in the list and if the attach interface feeds back
-EINVAL's (not compatible), we should allocate a new domain for the
attached device and put it in the list if the new attachment is
successful.

Perhaps I'm too worried?

Best regards,
baolu
  

On Tue, Jul 11, 2023 at 10:43:43AM +0800, Baolu Lu wrote:
> On 2023/7/11 1:28, Jason Gunthorpe wrote:
> > > @@ -88,31 +98,41 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
> > >   		goto out_unlock;
> > >   	}
> > > -	if (domain) {
> > > -		domain->users++;
> > > -		goto out;
> > > +	if (unlikely(domain)) {
> > > +		/* Re-attach the device to the same domain? */
> > > +		if (domain == sva_domain) {
> > > +			goto out;
> > > +		} else {
> > > +			/* Didn't get detached from the previous domain? */
> > > +			ret = -EBUSY;
> > > +			goto out_unlock;
> > > +		}
> > >   	}
> > And if we do all of this we should just get rid of the horrible
> > iommu_get_domain_for_dev_pasid() entirely.
> 
> At the core level, we have no idea about whether an sva domain allocated
> for one device is compatible with another device. Hence, we should loop
> the sva domains in the list and if the attach interface feeds back
> -EINVAL's (not compatible), we should allocate a new domain for the
> attached device and put it in the list if the new attachment is
> successful.

Yes, generally.

It would be good to undertand if the drivers desire one sva domain per
instance or one sva domain per driver - but with this approach it
becomes a driver choice which to use. I would guess that one sva
domain per instance is slightly simpler in the drivers..

iommu_get_domain_for_dev_pasid() turns into a check if the group
already has a SVA domain bound from a list, ie we turn it upside down
and have it search the list under the xa_lock instead of trying to
return a domain pointer back out.

Jason
  

Hi Tina,

On 7/7/2023 9:34 AM, Tina Zhang wrote:
> @@ -33,9 +34,18 @@ 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;
> +	}
> +	mm->iommu_mm = iommu_mm;
> +
>   	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max, GFP_KERNEL);
> -	if (ret < min)
> +	if (ret < min) {
> +		kfree(iommu_mm);
>   		goto out;

Once pasid allocation fails at this part, we should reassign the 
mm->iommu_mm back to default value, or it will keep a unavailable 
pointer, right?

Thanks,
Yanfei
> +	}
>   	mm_set_pasid(mm, ret);
>   	ret = 0;
>   out: