[RFC,2/8] iommufd: replace attach_fn with a structure

Commit Message

Yi Liu Sept. 26, 2023, 9:26 a.m. UTC

Most of the core logic before conducting the actual device attach/
replace operation can be shared with pasid attach/replace. Create
a new structure so more information (e.g. pasid) can be later added
along with the attach_fn.

Signed-off-by: Kevin Tian <[e-mail]>
Signed-off-by: Yi Liu <[e-mail]>
---
 drivers/iommu/iommufd/device.c          | 35 ++++++++++++++++---------
 drivers/iommu/iommufd/iommufd_private.h |  8 ++++++
 2 files changed, 30 insertions(+), 13 deletions(-)

Comments

Baolu Lu Sept. 27, 2023, 2:17 a.m. UTC | #1

On 9/26/23 5:26 PM, Yi Liu wrote:
> Most of the core logic before conducting the actual device attach/
> replace operation can be shared with pasid attach/replace. Create
> a new structure so more information (e.g. pasid) can be later added
> along with the attach_fn.
> 
> Signed-off-by: Kevin Tian<[e-mail]>
> Signed-off-by: Yi Liu<[e-mail]>
> ---
>   drivers/iommu/iommufd/device.c          | 35 ++++++++++++++++---------
>   drivers/iommu/iommufd/iommufd_private.h |  8 ++++++
>   2 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 645ab5d290fe..4fa4153c5df7 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -597,8 +597,11 @@ iommufd_device_do_replace(struct iommufd_device *idev,
>   	return ERR_PTR(rc);
>   }
>   
> -typedef struct iommufd_hw_pagetable *(*attach_fn)(
> -	struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt);
> +static struct iommufd_hw_pagetable *do_attach(struct iommufd_device *idev,
> +		struct iommufd_hw_pagetable *hwpt, struct attach_data *data)
> +{
> +	return data->attach_fn(idev, hwpt);
> +}

I assume that this change was made because we need to pass the pasid
value to the attach_fn() callback.

If so, how about passing it directly to attach_fn() function?

typedef struct iommufd_hw_pagetable *(*attach_fn)(
		struct iommufd_device *idev,
		struct iommufd_hw_pagetable *hwpt,
		ioasid_t pasid);

In no pasid case, use IOMMU_NO_PASID.

Best regards,
baolu

Tian, Kevin Sept. 27, 2023, 3:10 a.m. UTC | #2

> From: Baolu Lu <[e-mail]>
> Sent: Wednesday, September 27, 2023 10:18 AM
> 
> On 9/26/23 5:26 PM, Yi Liu wrote:
> >
> > -typedef struct iommufd_hw_pagetable *(*attach_fn)(
> > -	struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt);
> > +static struct iommufd_hw_pagetable *do_attach(struct iommufd_device
> *idev,
> > +		struct iommufd_hw_pagetable *hwpt, struct attach_data
> *data)
> > +{
> > +	return data->attach_fn(idev, hwpt);
> > +}
> 
> I assume that this change was made because we need to pass the pasid
> value to the attach_fn() callback.
> 
> If so, how about passing it directly to attach_fn() function?
> 
> typedef struct iommufd_hw_pagetable *(*attach_fn)(
> 		struct iommufd_device *idev,
> 		struct iommufd_hw_pagetable *hwpt,
> 		ioasid_t pasid);
> 
> In no pasid case, use IOMMU_NO_PASID.
> 

that is one option, but also means the existing device attach_fn() needs
to accept an unused pasid parameter which doesn't read good.

So this patch chooses to handle the pasid necessity in an outer wrapper
and contain the pasid awareness only in pasid related attach_fn.

Patch

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 645ab5d290fe..4fa4153c5df7 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -597,8 +597,11 @@  iommufd_device_do_replace(struct iommufd_device *idev,
 	return ERR_PTR(rc);
 }
 
-typedef struct iommufd_hw_pagetable *(*attach_fn)(
-	struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt);
+static struct iommufd_hw_pagetable *do_attach(struct iommufd_device *idev,
+		struct iommufd_hw_pagetable *hwpt, struct attach_data *data)
+{
+	return data->attach_fn(idev, hwpt);
+}
 
 /*
  * When automatically managing the domains we search for a compatible domain in
@@ -608,7 +611,7 @@  typedef struct iommufd_hw_pagetable *(*attach_fn)(
 static struct iommufd_hw_pagetable *
 iommufd_device_auto_get_domain(struct iommufd_device *idev,
 			       struct iommufd_ioas *ioas, u32 *pt_id,
-			       attach_fn do_attach)
+			       struct attach_data *data)
 {
 	/*
 	 * iommufd_hw_pagetable_attach() is called by
@@ -617,7 +620,7 @@  iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	 * to use the immediate_attach path as it supports drivers that can't
 	 * directly allocate a domain.
 	 */
-	bool immediate_attach = do_attach == iommufd_device_do_attach;
+	bool immediate_attach = data->attach_fn == iommufd_device_do_attach;
 	struct iommufd_hw_pagetable *destroy_hwpt;
 	struct iommufd_hw_pagetable *hwpt;
 
@@ -633,7 +636,7 @@  iommufd_device_auto_get_domain(struct iommufd_device *idev,
 
 		if (!iommufd_lock_obj(&hwpt->obj))
 			continue;
-		destroy_hwpt = (*do_attach)(idev, hwpt);
+		destroy_hwpt = do_attach(idev, hwpt, data);
 		if (IS_ERR(destroy_hwpt)) {
 			iommufd_put_object(&hwpt->obj);
 			/*
@@ -660,7 +663,7 @@  iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	}
 
 	if (!immediate_attach) {
-		destroy_hwpt = (*do_attach)(idev, hwpt);
+		destroy_hwpt = do_attach(idev, hwpt, data);
 		if (IS_ERR(destroy_hwpt))
 			goto out_abort;
 	} else {
@@ -681,8 +684,8 @@  iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	return destroy_hwpt;
 }
 
-static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
-				    attach_fn do_attach)
+int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
+			     struct attach_data *data)
 {
 	struct iommufd_hw_pagetable *destroy_hwpt;
 	struct iommufd_object *pt_obj;
@@ -696,7 +699,7 @@  static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
 		struct iommufd_hw_pagetable *hwpt =
 			container_of(pt_obj, struct iommufd_hw_pagetable, obj);
 
-		destroy_hwpt = (*do_attach)(idev, hwpt);
+		destroy_hwpt = do_attach(idev, hwpt, data);
 		if (IS_ERR(destroy_hwpt))
 			goto out_put_pt_obj;
 		break;
@@ -706,7 +709,7 @@  static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
 			container_of(pt_obj, struct iommufd_ioas, obj);
 
 		destroy_hwpt = iommufd_device_auto_get_domain(idev, ioas, pt_id,
-							      do_attach);
+							      data);
 		if (IS_ERR(destroy_hwpt))
 			goto out_put_pt_obj;
 		break;
@@ -742,8 +745,11 @@  static int iommufd_device_change_pt(struct iommufd_device *idev, u32 *pt_id,
 int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
 {
 	int rc;
+	struct attach_data data = {
+		.attach_fn = &iommufd_device_do_attach,
+	};
 
-	rc = iommufd_device_change_pt(idev, pt_id, &iommufd_device_do_attach);
+	rc = iommufd_device_change_pt(idev, pt_id, &data);
 	if (rc)
 		return rc;
 
@@ -773,8 +779,11 @@  EXPORT_SYMBOL_NS_GPL(iommufd_device_attach, IOMMUFD);
  */
 int iommufd_device_replace(struct iommufd_device *idev, u32 *pt_id)
 {
-	return iommufd_device_change_pt(idev, pt_id,
-					&iommufd_device_do_replace);
+	struct attach_data data = {
+		.attach_fn = &iommufd_device_do_replace,
+	};
+
+	return iommufd_device_change_pt(idev, pt_id, &data);
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_device_replace, IOMMUFD);
 
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 1bd412cff2d6..f1fe4120c3b1 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -353,6 +353,14 @@  int iommufd_get_hw_info(struct iommufd_ucmd *ucmd);
 int iommufd_set_dev_data(struct iommufd_ucmd *ucmd);
 int iommufd_unset_dev_data(struct iommufd_ucmd *ucmd);
 
+struct attach_data {
+	union {
+		struct iommufd_hw_pagetable *(*attach_fn)(
+				struct iommufd_device *idev,
+				struct iommufd_hw_pagetable *hwpt);
+	};
+};
+
 struct iommufd_access {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;