The struct sva_iommu represents a bond of an SVA domain and a device.
It is functionally equivalent to the iommu_attach_handle. To avoid
code duplication, replace sva_iommu with the iommu_attach_handle and
remove the code that manages sva_iommu.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h      | 28 +++++++------------------
 include/linux/uacce.h      |  2 +-
 drivers/dma/idxd/idxd.h    |  2 +-
 drivers/iommu/iommu-priv.h |  7 +++++++
 drivers/dma/idxd/cdev.c    |  4 ++--
 drivers/iommu/iommu-sva.c  | 43 ++++++++++++++++++--------------------
 drivers/misc/uacce/uacce.c |  2 +-
 7 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2e925b5eba53..be9c9a10169d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -39,7 +39,6 @@ struct iommu_domain;
 struct iommu_domain_ops;
 struct iommu_dirty_ops;
 struct notifier_block;
-struct iommu_sva;
 struct iommu_dma_cookie;
 struct iommu_fault_param;
 
@@ -986,20 +985,9 @@ struct iommu_fwspec {
 /* ATS is supported */
 #define IOMMU_FWSPEC_PCI_RC_ATS			(1 << 0)
 
-/**
- * struct iommu_sva - handle to a device-mm bond
- */
-struct iommu_sva {
-	struct device			*dev;
-	struct iommu_domain		*domain;
-	struct list_head		handle_item;
-	refcount_t			users;
-};
-
 struct iommu_mm_data {
 	u32			pasid;
 	struct list_head	sva_domains;
-	struct list_head	sva_handles;
 };
 
 int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
@@ -1527,24 +1515,24 @@ static inline u32 mm_get_enqcmd_pasid(struct mm_struct *mm)
 }
 
 void mm_pasid_drop(struct mm_struct *mm);
-struct iommu_sva *iommu_sva_bind_device(struct device *dev,
-					struct mm_struct *mm);
-void iommu_sva_unbind_device(struct iommu_sva *handle);
-u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+struct iommu_attach_handle *iommu_sva_bind_device(struct device *dev,
+						  struct mm_struct *mm);
+void iommu_sva_unbind_device(struct iommu_attach_handle *handle);
+u32 iommu_sva_get_pasid(struct iommu_attach_handle *handle);
 struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
 					    struct mm_struct *mm);
 #else
-static inline struct iommu_sva *
+static inline struct iommu_attach_handle *
 iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
 {
-	return NULL;
+	return ERR_PTR(-ENODEV);
 }
 
-static inline void iommu_sva_unbind_device(struct iommu_sva *handle)
+static inline void iommu_sva_unbind_device(struct iommu_attach_handle *handle)
 {
 }
 
-static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+static inline u32 iommu_sva_get_pasid(struct iommu_attach_handle *handle)
 {
 	return IOMMU_PASID_INVALID;
 }
diff --git a/include/linux/uacce.h b/include/linux/uacce.h
index e290c0269944..1548119c89ae 100644
--- a/include/linux/uacce.h
+++ b/include/linux/uacce.h
@@ -97,7 +97,7 @@ struct uacce_queue {
 	struct mutex mutex;
 	enum uacce_q_state state;
 	u32 pasid;
-	struct iommu_sva *handle;
+	struct iommu_attach_handle *handle;
 	struct address_space *mapping;
 };
 
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 7b98944135eb..45588156ca60 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -335,7 +335,7 @@ struct idxd_device {
 	struct idxd_wq **wqs;
 	struct idxd_engine **engines;
 
-	struct iommu_sva *sva;
+	struct iommu_attach_handle *sva;
 	unsigned int pasid;
 
 	int num_groups;
diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index da1addaa1a31..ae65e0b85d69 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -30,6 +30,13 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
 
 struct iommu_attach_handle {
 	struct iommu_domain		*domain;
+	union {
+		/* attach data for SVA domain */
+		struct {
+			struct device	*dev;
+			refcount_t	users;
+		};
+	};
 };
 
 struct iommu_attach_handle *
diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index c095a2c8f659..f8ac47bdfc4c 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -45,7 +45,7 @@ struct idxd_user_context {
 	unsigned int pasid;
 	struct mm_struct *mm;
 	unsigned int flags;
-	struct iommu_sva *sva;
+	struct iommu_attach_handle *sva;
 	struct idxd_dev idxd_dev;
 	u64 counters[COUNTER_MAX];
 	int id;
@@ -225,7 +225,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
 	struct idxd_wq *wq;
 	struct device *dev, *fdev;
 	int rc = 0;
-	struct iommu_sva *sva;
+	struct iommu_attach_handle *sva;
 	unsigned int pasid;
 	struct idxd_cdev *idxd_cdev;
 
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 640acc804e8c..d49737e43de2 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -41,7 +41,6 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
 	}
 	iommu_mm->pasid = pasid;
 	INIT_LIST_HEAD(&iommu_mm->sva_domains);
-	INIT_LIST_HEAD(&iommu_mm->sva_handles);
 	/*
 	 * Make sure the write to mm->iommu_mm is not reordered in front of
 	 * initialization to iommu_mm fields. If it does, readers may see a
@@ -67,13 +66,17 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
  *
  * On error, returns an ERR_PTR value.
  */
-struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
+struct iommu_attach_handle *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
 {
+	struct iommu_group *group = dev->iommu_group;
+	struct iommu_attach_handle *handle;
 	struct iommu_mm_data *iommu_mm;
 	struct iommu_domain *domain;
-	struct iommu_sva *handle;
 	int ret;
 
+	if (!group)
+		return ERR_PTR(-ENODEV);
+
 	mutex_lock(&iommu_sva_lock);
 
 	/* Allocate mm->pasid if necessary. */
@@ -83,17 +86,15 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 		goto out_unlock;
 	}
 
-	list_for_each_entry(handle, &mm->iommu_mm->sva_handles, handle_item) {
-		if (handle->dev == dev) {
-			refcount_inc(&handle->users);
-			mutex_unlock(&iommu_sva_lock);
-			return handle;
-		}
+	/* A bond already exists, just take a reference`. */
+	handle = iommu_attach_handle_get(group, iommu_mm->pasid, IOMMU_DOMAIN_SVA);
+	if (!IS_ERR(handle)) {
+		refcount_inc(&handle->users);
+		mutex_unlock(&iommu_sva_lock);
+		return handle;
 	}
-
-	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
-	if (!handle) {
-		ret = -ENOMEM;
+	if (handle == ERR_PTR(-EBUSY)) {
+		ret = PTR_ERR(handle);
 		goto out_unlock;
 	}
 
@@ -110,7 +111,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 	domain = iommu_sva_domain_alloc(dev, mm);
 	if (!domain) {
 		ret = -ENOMEM;
-		goto out_free_handle;
+		goto out_unlock;
 	}
 
 	ret = iommu_attach_device_pasid(domain, dev, iommu_mm->pasid);
@@ -120,17 +121,15 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm
 	list_add(&domain->next, &mm->iommu_mm->sva_domains);
 
 out:
+	handle = iommu_attach_handle_get(group, iommu_mm->pasid, 0);
 	refcount_set(&handle->users, 1);
-	list_add(&handle->handle_item, &mm->iommu_mm->sva_handles);
-	mutex_unlock(&iommu_sva_lock);
 	handle->dev = dev;
-	handle->domain = domain;
+	mutex_unlock(&iommu_sva_lock);
+
 	return handle;
 
 out_free_domain:
 	iommu_domain_free(domain);
-out_free_handle:
-	kfree(handle);
 out_unlock:
 	mutex_unlock(&iommu_sva_lock);
 	return ERR_PTR(ret);
@@ -145,7 +144,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
  * not be issuing any more transaction for this PASID. All outstanding page
  * requests for this PASID must have been flushed to the IOMMU.
  */
-void iommu_sva_unbind_device(struct iommu_sva *handle)
+void iommu_sva_unbind_device(struct iommu_attach_handle *handle)
 {
 	struct iommu_domain *domain = handle->domain;
 	struct iommu_mm_data *iommu_mm = domain->mm->iommu_mm;
@@ -156,7 +155,6 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
 		mutex_unlock(&iommu_sva_lock);
 		return;
 	}
-	list_del(&handle->handle_item);
 
 	iommu_detach_device_pasid(domain, dev, iommu_mm->pasid);
 	if (--domain->users == 0) {
@@ -164,11 +162,10 @@ void iommu_sva_unbind_device(struct iommu_sva *handle)
 		iommu_domain_free(domain);
 	}
 	mutex_unlock(&iommu_sva_lock);
-	kfree(handle);
 }
 EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
 
-u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+u32 iommu_sva_get_pasid(struct iommu_attach_handle *handle)
 {
 	struct iommu_domain *domain = handle->domain;
 
diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index bdc2e6fda782..b325097421c1 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -106,7 +106,7 @@ static long uacce_fops_compat_ioctl(struct file *filep,
 static int uacce_bind_queue(struct uacce_device *uacce, struct uacce_queue *q)
 {
 	u32 pasid;
-	struct iommu_sva *handle;
+	struct iommu_attach_handle *handle;
 
 	if (!(uacce->flags & UACCE_DEV_SVA))
 		return 0;
-- 
2.34.1

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, April 30, 2024 10:57 PM
> 
>  #else
> -static inline struct iommu_sva *
> +static inline struct iommu_attach_handle *
>  iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
>  {
> -	return NULL;
> +	return ERR_PTR(-ENODEV);
>  }
> 

this should be a separate fix.

existing drivers (idxd and uacce) only check IS_ERR() on the return
value. A Null pointer may lead to an error reported at a later point.

On 5/15/24 3:21 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, April 30, 2024 10:57 PM
>>
>>   #else
>> -static inline struct iommu_sva *
>> +static inline struct iommu_attach_handle *
>>   iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
>>   {
>> -	return NULL;
>> +	return ERR_PTR(-ENODEV);
>>   }
>>
> 
> this should be a separate fix.

Yes. It could be a fix.

> 
> existing drivers (idxd and uacce) only check IS_ERR() on the return
> value. A Null pointer may lead to an error reported at a later point.

Though I don't think it could cause any real problem because this
interface should always be called after the sva enabling one.

Best regards,
baolu

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Sunday, May 19, 2024 6:14 PM
> 
> On 5/15/24 3:21 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Tuesday, April 30, 2024 10:57 PM
> >>
> >>   #else
> >> -static inline struct iommu_sva *
> >> +static inline struct iommu_attach_handle *
> >>   iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
> >>   {
> >> -	return NULL;
> >> +	return ERR_PTR(-ENODEV);
> >>   }
> >>
> >
> > this should be a separate fix.
> 
> Yes. It could be a fix.
> 
> >
> > existing drivers (idxd and uacce) only check IS_ERR() on the return
> > value. A Null pointer may lead to an error reported at a later point.
> 
> Though I don't think it could cause any real problem because this
> interface should always be called after the sva enabling one.
> 

correct.

On Tue, Apr 30, 2024 at 10:57:03PM +0800, Lu Baolu wrote:
> diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
> index da1addaa1a31..ae65e0b85d69 100644
> --- a/drivers/iommu/iommu-priv.h
> +++ b/drivers/iommu/iommu-priv.h
> @@ -30,6 +30,13 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
>  
>  struct iommu_attach_handle {
>  	struct iommu_domain		*domain;
> +	union {
> +		/* attach data for SVA domain */
> +		struct {
> +			struct device	*dev;
> +			refcount_t	users;
> +		};
> +	};
>  };

FWIW I was thinking of having the caller allocate the handle and pass it
down, but this seems workable too and is a bit simpler.

> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> index bdc2e6fda782..b325097421c1 100644
> --- a/drivers/misc/uacce/uacce.c
> +++ b/drivers/misc/uacce/uacce.c
> @@ -106,7 +106,7 @@ static long uacce_fops_compat_ioctl(struct file *filep,
>  static int uacce_bind_queue(struct uacce_device *uacce, struct uacce_queue *q)
>  {
>  	u32 pasid;
> -	struct iommu_sva *handle;
> +	struct iommu_attach_handle *handle;

Though I'm much less keen on this..

Maybe

  struct iommu_attach_handle {
  	struct iommu_domain		*domain;
 	union {
	      struct iommu_sva sva;
 	};
  };

?

Then container_of(sva) to get back to handle and keep the meaningful
type?

Jason