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

[Tina Zhang]

Hi Jason,

On 9/22/23 07:31, Jason Gunthorpe wrote:
> On Tue, Sep 12, 2023 at 08:59:35PM +0800, Tina Zhang wrote:
> > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> > index 0f956ecd0c9b..79779bbef653 100644
> > --- a/drivers/iommu/iommu-sva.c
> > +++ b/drivers/iommu/iommu-sva.c
> > @@ -15,6 +15,7 @@ static DEFINE_MUTEX(iommu_sva_lock);
> >   static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev)
> >   {
> >   	ioasid_t pasid;
> > +	struct iommu_mm_data *iommu_mm;
> >   	int ret = 0;
> >   
> >   	if (!arch_pgtable_dma_compat(mm))
> > @@ -28,12 +29,22 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev)
> >   		goto out;
> >   	}
> >   
> > +	iommu_mm = kzalloc(sizeof(struct iommu_mm_data), GFP_KERNEL);
> > +	if (!iommu_mm) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> >   	pasid = iommu_alloc_global_pasid(dev);
> >   	if (pasid == IOMMU_PASID_INVALID) {
> > +		kfree(iommu_mm);
> >   		ret = -ENOSPC;
> >   		goto out;
> >   	}
> > -	mm->pasid = pasid;
> > +	iommu_mm->pasid = pasid;
> > +	INIT_LIST_HEAD(&iommu_mm->sva_domains);
> > +	mm->iommu_mm = iommu_mm;
> > +
> >   	ret = 0;
> >   out:
> >   	mutex_unlock(&iommu_sva_lock);
> > @@ -73,16 +84,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. */
> > @@ -96,6 +103,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;
> > @@ -128,8 +137,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);
> > @@ -209,4 +219,5 @@ void mm_pasid_drop(struct mm_struct *mm)
> >   		return;
> >   
> >   	iommu_free_global_pasid(mm_get_pasid(mm));
> > +	kfree(mm->iommu_mm);
> >   }
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index b9c9f14a95cc..c61bc45d5a82 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -109,6 +109,11 @@ struct iommu_domain {
> >   		struct {	/* IOMMU_DOMAIN_SVA */
> >   			struct mm_struct *mm;
> >   			int users;
> > +			/*
> > +			 * Next iommu_domain in mm->iommu_mm->sva-domains list
> > +			 * protected by iommu_sva_lock.
> > +			 */
> > +			struct list_head next;
> >   		};
> >   	};
> >   };
> > @@ -1186,17 +1191,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;
> >   }
>
> HUm this isn't locked very nicely.
>
> Above do
>
> smp_store_release(&mm->iommu_mm, iommu_mm);
>
> And then do
>
>    return READ_ONCE(mm->iommu_mm)
Good suggestion. Thanks.

> (no need for ternaries with bools, compiler generates it automatically)
>
>
> >   static inline u32 mm_get_pasid(struct mm_struct *mm)
> >   {
> > -	return mm->pasid;
> > +	return mm->iommu_mm ? mm->iommu_mm->pasid : IOMMU_PASID_INVALID;
> >   }
>
> Then this should be
>
> struct iommu_mm_data *iommu_mm = READ_ONCE(mm->iommu_mm);
>
> if (!iommu_mm)
>     return IOMMU_PASID_INVALID;
> return iommu_mm->pasid;
>
> Keeping in mind that the kfree(mm->iommu_mm) being placed in the
> mm_drop is critical to this working safely.
Right. Thanks.


Regards,
-Tina
> Otherwise the logic looks OK.
>
> Jason