Re: [PATCH v5 04/14] iommufd: Abstract an iommufd_eventq from iommufd_fault

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jan 07, 2025 at 09:10:07AM -0800, Nicolin Chen wrote:

> @@ -433,32 +434,35 @@ void iopt_remove_access(struct io_pagetable *iopt,
>  			u32 iopt_access_list_id);
>  void iommufd_access_destroy_object(struct iommufd_object *obj);
>  
> -/*
> - * An iommufd_fault object represents an interface to deliver I/O page faults
> - * to the user space. These objects are created/destroyed by the user space and
> - * associated with hardware page table objects during page-table allocation.
> - */
> -struct iommufd_fault {
> +struct iommufd_eventq_ops {
> +	ssize_t (*read)(struct iommufd_eventq *eventq, char __user *buf,
> +			size_t count, loff_t *ppos); /* Mandatory op */
> +	ssize_t (*write)(struct iommufd_eventq *eventq, const char __user *buf,
> +			 size_t count, loff_t *ppos); /* Optional op */
> +};

I think I recommend to avoid this, more like this:

diff --git a/drivers/iommu/iommufd/eventq.c b/drivers/iommu/iommufd/eventq.c
index b5be629f38eda7..73f5e8a6b17f54 100644
--- a/drivers/iommu/iommufd/eventq.c
+++ b/drivers/iommu/iommufd/eventq.c
@@ -341,11 +341,6 @@ static ssize_t iommufd_fault_fops_write(struct iommufd_eventq *eventq,
 	return done == 0 ? rc : done;
 }
 
-static const struct iommufd_eventq_ops iommufd_fault_ops = {
-	.read = &iommufd_fault_fops_read,
-	.write = &iommufd_fault_fops_write,
-};
-
 /* IOMMUFD_OBJ_VEVENTQ Functions */
 
 void iommufd_veventq_abort(struct iommufd_object *obj)
@@ -409,31 +404,8 @@ static ssize_t iommufd_veventq_fops_read(struct iommufd_eventq *eventq,
 	return done == 0 ? rc : done;
 }
 
-static const struct iommufd_eventq_ops iommufd_veventq_ops = {
-	.read = &iommufd_veventq_fops_read,
-};
-
 /* Common Event Queue Functions */
 
-static ssize_t iommufd_eventq_fops_read(struct file *filep, char __user *buf,
-					size_t count, loff_t *ppos)
-{
-	struct iommufd_eventq *eventq = filep->private_data;
-
-	return eventq->ops->read(eventq, buf, count, ppos);
-}
-
-static ssize_t iommufd_eventq_fops_write(struct file *filep,
-					 const char __user *buf, size_t count,
-					 loff_t *ppos)
-{
-	struct iommufd_eventq *eventq = filep->private_data;
-
-	if (!eventq->ops->write)
-		return -EOPNOTSUPP;
-	return eventq->ops->write(eventq, buf, count, ppos);
-}
-
 static __poll_t iommufd_eventq_fops_poll(struct file *filep,
 					 struct poll_table_struct *wait)
 {
@@ -458,34 +430,31 @@ static int iommufd_eventq_fops_release(struct inode *inode, struct file *filep)
 	return 0;
 }
 
-static const struct file_operations iommufd_eventq_fops = {
-	.owner		= THIS_MODULE,
-	.open		= nonseekable_open,
-	.read		= iommufd_eventq_fops_read,
-	.write		= iommufd_eventq_fops_write,
-	.poll		= iommufd_eventq_fops_poll,
-	.release	= iommufd_eventq_fops_release,
-};
+#define INIT_EVENTQ_FOPS(read_op, write_op)                     \
+	(struct file_operations){                               \
+		.owner = THIS_MODULE,                           \
+		.open = nonseekable_open,                       \
+		.read = read_op,                                \
+		.write = write_op,                              \
+		.poll = iommufd_eventq_fops_poll,               \
+		.release = iommufd_eventq_fops_release,         \
+	}
 
 static int iommufd_eventq_init(struct iommufd_eventq *eventq, char *name,
 			       struct iommufd_ctx *ictx,
-			       const struct iommufd_eventq_ops *ops)
+			       const struct file_operations *fops)
 {
 	struct file *filep;
 	int fdno;
 
-	if (WARN_ON_ONCE(!ops || !ops->read))
-		return -EINVAL;
-
 	mutex_init(&eventq->mutex);
 	INIT_LIST_HEAD(&eventq->deliver);
 	init_waitqueue_head(&eventq->wait_queue);
 
-	filep = anon_inode_getfile(name, &iommufd_eventq_fops, eventq, O_RDWR);
+	filep = anon_inode_getfile(name, fops, eventq, O_RDWR);
 	if (IS_ERR(filep))
 		return PTR_ERR(filep);
 
-	eventq->ops = ops;
 	eventq->ictx = ictx;
 	iommufd_ctx_get(eventq->ictx);
 	refcount_inc(&eventq->obj.users);
@@ -497,6 +466,9 @@ static int iommufd_eventq_init(struct iommufd_eventq *eventq, char *name,
 	return fdno;
 }
 
+static const struct file_operations iommufd_pgfault_fops =
+	INIT_EVENTQ_FOPS(iommufd_fault_fops_read, iommufd_fault_fops_write);
+
 int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 {
 	struct iommu_fault_alloc *cmd = ucmd->cmd;
@@ -515,7 +487,7 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 	xa_init_flags(&fault->response, XA_FLAGS_ALLOC1);
 
 	fdno = iommufd_eventq_init(&fault->common, "[iommufd-pgfault]",
-				   ucmd->ictx, &iommufd_fault_ops);
+				   ucmd->ictx, &iommufd_pgfault_fops);
 	if (fdno < 0) {
 		rc = fdno;
 		goto out_abort;
@@ -541,6 +513,9 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 	return rc;
 }
 
+static const struct file_operations iommufd_viommu_fops =
+	INIT_EVENTQ_FOPS(iommufd_veventq_fops_read, NULL);
+
 int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd)
 {
 	struct iommu_veventq_alloc *cmd = ucmd->cmd;
@@ -580,7 +555,7 @@ int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd)
 	list_add_tail(&veventq->node, &viommu->veventqs);
 
 	fdno = iommufd_eventq_init(&veventq->common, "[iommufd-viommu-event]",
-				   ucmd->ictx, &iommufd_veventq_ops);
+				   ucmd->ictx, &iommufd_viommu_fops);
 	if (fdno < 0) {
 		rc = fdno;
 		goto out_abort;
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 3c0374154a94d3..6c23d5b58901af 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -434,20 +434,11 @@ void iopt_remove_access(struct io_pagetable *iopt,
 			u32 iopt_access_list_id);
 void iommufd_access_destroy_object(struct iommufd_object *obj);
 
-struct iommufd_eventq_ops {
-	ssize_t (*read)(struct iommufd_eventq *eventq, char __user *buf,
-			size_t count, loff_t *ppos); /* Mandatory op */
-	ssize_t (*write)(struct iommufd_eventq *eventq, const char __user *buf,
-			 size_t count, loff_t *ppos); /* Optional op */
-};
-
 struct iommufd_eventq {
 	struct iommufd_object obj;
 	struct iommufd_ctx *ictx;
 	struct file *filep;
 
-	const struct iommufd_eventq_ops *ops;
-
 	/* The lists of outstanding events protected by below mutex. */
 	struct mutex mutex;
 	struct list_head deliver;