diff mbox series

[17/17] vfio/pci: Remove duplicate interrupt management flow

Message ID 6ec901daffab4170d9740c7d066becd079925d53.1706849424.git.reinette.chatre@intel.com (mailing list archive)
State New, archived
Headers show
Series vfio/pci: Remove duplicate code and logic from VFIO PCI interrupt management | expand

Commit Message

Reinette Chatre Feb. 2, 2024, 4:57 a.m. UTC 
vfio_pci_set_intx_trigger() and vfio_pci_set_trigger() have the
same flow that calls interrupt type (INTx, MSI, MSI-X) specific
functions.

Create callbacks for the interrupt type specific code that
can be called by the shared code so that only one of these functions
are needed. Rename the final generic function shared by all
interrupt types vfio_pci_set_trigger().

Relocate the "IOCTL support" marker to correctly mark the
now generic code.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 104 ++++++++++--------------------
 1 file changed, 35 insertions(+), 69 deletions(-)

Comments

Alex Williamson Feb. 5, 2024, 10:35 p.m. UTC | #1 
On Thu,  1 Feb 2024 20:57:11 -0800
Reinette Chatre <reinette.chatre@intel.com> wrote:

> vfio_pci_set_intx_trigger() and vfio_pci_set_trigger() have the
> same flow that calls interrupt type (INTx, MSI, MSI-X) specific
> functions.
> 
> Create callbacks for the interrupt type specific code that
> can be called by the shared code so that only one of these functions
> are needed. Rename the final generic function shared by all
> interrupt types vfio_pci_set_trigger().
> 
> Relocate the "IOCTL support" marker to correctly mark the
> now generic code.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  drivers/vfio/pci/vfio_pci_intrs.c | 104 ++++++++++--------------------
>  1 file changed, 35 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index daa84a317f40..a5b337cfae60 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -32,6 +32,12 @@ struct vfio_pci_irq_ctx {
>  };
>  
>  struct vfio_pci_intr_ops {
> +	int (*enable)(struct vfio_pci_core_device *vdev, unsigned int start,
> +		      unsigned int count, unsigned int index);
> +	void (*disable)(struct vfio_pci_core_device *vdev,
> +			unsigned int index);
> +	void (*send_eventfd)(struct vfio_pci_core_device *vdev,
> +			     struct vfio_pci_irq_ctx *ctx);
>  	int (*request_interrupt)(struct vfio_pci_core_device *vdev,
>  				 struct vfio_pci_irq_ctx *ctx,
>  				 unsigned int vector, unsigned int index);
> @@ -356,6 +362,12 @@ static void vfio_intx_disable(struct vfio_pci_core_device *vdev,
>  /*
>   * MSI/MSI-X
>   */
> +static void vfio_send_msi_eventfd(struct vfio_pci_core_device *vdev,
> +				  struct vfio_pci_irq_ctx *ctx)
> +{
> +	eventfd_signal(ctx->trigger);
> +}
> +
>  static irqreturn_t vfio_msihandler(int irq, void *arg)
>  {
>  	struct eventfd_ctx *trigger = arg;
> @@ -544,13 +556,22 @@ static void vfio_msi_unregister_producer(struct vfio_pci_irq_ctx *ctx)
>  	irq_bypass_unregister_producer(&ctx->producer);
>  }
>  
> +/*
> + * IOCTL support
> + */
>  static struct vfio_pci_intr_ops intr_ops[] = {
>  	[VFIO_PCI_INTX_IRQ_INDEX] = {
> +		.enable = vfio_intx_enable,
> +		.disable = vfio_intx_disable,
> +		.send_eventfd = vfio_send_intx_eventfd_ctx,
>  		.request_interrupt = vfio_intx_request_interrupt,
>  		.free_interrupt = vfio_intx_free_interrupt,
>  		.device_name = vfio_intx_device_name,
>  	},
>  	[VFIO_PCI_MSI_IRQ_INDEX] = {
> +		.enable = vfio_msi_enable,
> +		.disable = vfio_msi_disable,
> +		.send_eventfd = vfio_send_msi_eventfd,
>  		.request_interrupt = vfio_msi_request_interrupt,
>  		.free_interrupt = vfio_msi_free_interrupt,
>  		.device_name = vfio_msi_device_name,
> @@ -558,6 +579,9 @@ static struct vfio_pci_intr_ops intr_ops[] = {
>  		.unregister_producer = vfio_msi_unregister_producer,
>  	},
>  	[VFIO_PCI_MSIX_IRQ_INDEX] = {
> +		.enable = vfio_msi_enable,
> +		.disable = vfio_msi_disable,
> +		.send_eventfd = vfio_send_msi_eventfd,
>  		.request_interrupt = vfio_msi_request_interrupt,
>  		.free_interrupt = vfio_msi_free_interrupt,
>  		.device_name = vfio_msi_device_name,
> @@ -646,9 +670,6 @@ static int vfio_irq_set_block(struct vfio_pci_core_device *vdev,
>  	return ret;
>  }
>  
> -/*
> - * IOCTL support
> - */
>  static int vfio_pci_set_intx_unmask(struct vfio_pci_core_device *vdev,
>  				    unsigned int index, unsigned int start,
>  				    unsigned int count, uint32_t flags,
> @@ -701,71 +722,16 @@ static int vfio_pci_set_intx_mask(struct vfio_pci_core_device *vdev,
>  	return 0;
>  }
>  
> -static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
> -				     unsigned int index, unsigned int start,
> -				     unsigned int count, uint32_t flags,
> -				     void *data)
> -{
> -	struct vfio_pci_irq_ctx *ctx;
> -	unsigned int i;
> -
> -	if (is_intx(vdev) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
> -		vfio_intx_disable(vdev, index);
> -		return 0;
> -	}
> -
> -	if (!(is_intx(vdev) || is_irq_none(vdev)))
> -		return -EINVAL;
> -
> -	if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> -		int32_t *fds = data;
> -		int ret;
> -
> -		if (is_intx(vdev))
> -			return vfio_irq_set_block(vdev, start, count, fds, index);
> -
> -		ret = vfio_intx_enable(vdev, start, count, index);
> -		if (ret)
> -			return ret;
> -
> -		ret = vfio_irq_set_block(vdev, start, count, fds, index);
> -		if (ret)
> -			vfio_intx_disable(vdev, index);
> -
> -		return ret;
> -	}
> -
> -	if (!is_intx(vdev))
> -		return -EINVAL;
> -
> -	/* temporary */
> -	for (i = start; i < start + count; i++) {
> -		ctx = vfio_irq_ctx_get(vdev, i);
> -		if (!ctx || !ctx->trigger)
> -			continue;
> -		if (flags & VFIO_IRQ_SET_DATA_NONE) {
> -			vfio_send_intx_eventfd_ctx(vdev, ctx);
> -		} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> -			uint8_t *bools = data;
> -
> -			if (bools[i - start])
> -				vfio_send_intx_eventfd_ctx(vdev, ctx);
> -		}
> -	}
> -
> -	return 0;
> -}
> -
> -static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
> -				    unsigned int index, unsigned int start,
> -				    unsigned int count, uint32_t flags,
> -				    void *data)
> +static int vfio_pci_set_trigger(struct vfio_pci_core_device *vdev,
> +				unsigned int index, unsigned int start,
> +				unsigned int count, uint32_t flags,
> +				void *data)
>  {
>  	struct vfio_pci_irq_ctx *ctx;
>  	unsigned int i;
>  
>  	if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
> -		vfio_msi_disable(vdev, index);
> +		intr_ops[index].disable(vdev, index);
>  		return 0;
>  	}
>  
> @@ -780,13 +746,13 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
>  			return vfio_irq_set_block(vdev, start, count,
>  						  fds, index);
>  
> -		ret = vfio_msi_enable(vdev, start, count, index);
> +		ret = intr_ops[index].enable(vdev, start, count, index);
>  		if (ret)
>  			return ret;
>  
>  		ret = vfio_irq_set_block(vdev, start, count, fds, index);
>  		if (ret)
> -			vfio_msi_disable(vdev, index);
> +			intr_ops[index].disable(vdev, index);
>  
>  		return ret;
>  	}
> @@ -799,11 +765,11 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
>  		if (!ctx || !ctx->trigger)
>  			continue;
>  		if (flags & VFIO_IRQ_SET_DATA_NONE) {
> -			eventfd_signal(ctx->trigger);
> +			intr_ops[index].send_eventfd(vdev, ctx);
>  		} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
>  			uint8_t *bools = data;

Nit, an opportunity to add the missing new line between variable
declaration and code here.  Thanks,

Alex

>  			if (bools[i - start])
> -				eventfd_signal(ctx->trigger);
> +				intr_ops[index].send_eventfd(vdev, ctx);
>  		}
>  	}
>  	return 0;
> @@ -912,7 +878,7 @@ int vfio_pci_set_irqs_ioctl(struct vfio_pci_core_device *vdev, uint32_t flags,
>  			func = vfio_pci_set_intx_unmask;
>  			break;
>  		case VFIO_IRQ_SET_ACTION_TRIGGER:
> -			func = vfio_pci_set_intx_trigger;
> +			func = vfio_pci_set_trigger;
>  			break;
>  		}
>  		break;
> @@ -924,7 +890,7 @@ int vfio_pci_set_irqs_ioctl(struct vfio_pci_core_device *vdev, uint32_t flags,
>  			/* XXX Need masking support exported */
>  			break;
>  		case VFIO_IRQ_SET_ACTION_TRIGGER:
> -			func = vfio_pci_set_msi_trigger;
> +			func = vfio_pci_set_trigger;
>  			break;
>  		}
>  		break;
Reinette Chatre Feb. 6, 2024, 9:46 p.m. UTC | #2 
Hi Alex,

On 2/5/2024 2:35 PM, Alex Williamson wrote:
> On Thu,  1 Feb 2024 20:57:11 -0800
> Reinette Chatre <reinette.chatre@intel.com> wrote:

...

>> @@ -799,11 +765,11 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
>>  		if (!ctx || !ctx->trigger)
>>  			continue;
>>  		if (flags & VFIO_IRQ_SET_DATA_NONE) {
>> -			eventfd_signal(ctx->trigger);
>> +			intr_ops[index].send_eventfd(vdev, ctx);
>>  		} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
>>  			uint8_t *bools = data;
> 
> Nit, an opportunity to add the missing new line between variable
> declaration and code here.  Thanks,

Will do. Thank you.

Reinette
diff mbox series

Patch

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index daa84a317f40..a5b337cfae60 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -32,6 +32,12 @@  struct vfio_pci_irq_ctx {
 };
 
 struct vfio_pci_intr_ops {
+	int (*enable)(struct vfio_pci_core_device *vdev, unsigned int start,
+		      unsigned int count, unsigned int index);
+	void (*disable)(struct vfio_pci_core_device *vdev,
+			unsigned int index);
+	void (*send_eventfd)(struct vfio_pci_core_device *vdev,
+			     struct vfio_pci_irq_ctx *ctx);
 	int (*request_interrupt)(struct vfio_pci_core_device *vdev,
 				 struct vfio_pci_irq_ctx *ctx,
 				 unsigned int vector, unsigned int index);
@@ -356,6 +362,12 @@  static void vfio_intx_disable(struct vfio_pci_core_device *vdev,
 /*
  * MSI/MSI-X
  */
+static void vfio_send_msi_eventfd(struct vfio_pci_core_device *vdev,
+				  struct vfio_pci_irq_ctx *ctx)
+{
+	eventfd_signal(ctx->trigger);
+}
+
 static irqreturn_t vfio_msihandler(int irq, void *arg)
 {
 	struct eventfd_ctx *trigger = arg;
@@ -544,13 +556,22 @@  static void vfio_msi_unregister_producer(struct vfio_pci_irq_ctx *ctx)
 	irq_bypass_unregister_producer(&ctx->producer);
 }
 
+/*
+ * IOCTL support
+ */
 static struct vfio_pci_intr_ops intr_ops[] = {
 	[VFIO_PCI_INTX_IRQ_INDEX] = {
+		.enable = vfio_intx_enable,
+		.disable = vfio_intx_disable,
+		.send_eventfd = vfio_send_intx_eventfd_ctx,
 		.request_interrupt = vfio_intx_request_interrupt,
 		.free_interrupt = vfio_intx_free_interrupt,
 		.device_name = vfio_intx_device_name,
 	},
 	[VFIO_PCI_MSI_IRQ_INDEX] = {
+		.enable = vfio_msi_enable,
+		.disable = vfio_msi_disable,
+		.send_eventfd = vfio_send_msi_eventfd,
 		.request_interrupt = vfio_msi_request_interrupt,
 		.free_interrupt = vfio_msi_free_interrupt,
 		.device_name = vfio_msi_device_name,
@@ -558,6 +579,9 @@  static struct vfio_pci_intr_ops intr_ops[] = {
 		.unregister_producer = vfio_msi_unregister_producer,
 	},
 	[VFIO_PCI_MSIX_IRQ_INDEX] = {
+		.enable = vfio_msi_enable,
+		.disable = vfio_msi_disable,
+		.send_eventfd = vfio_send_msi_eventfd,
 		.request_interrupt = vfio_msi_request_interrupt,
 		.free_interrupt = vfio_msi_free_interrupt,
 		.device_name = vfio_msi_device_name,
@@ -646,9 +670,6 @@  static int vfio_irq_set_block(struct vfio_pci_core_device *vdev,
 	return ret;
 }
 
-/*
- * IOCTL support
- */
 static int vfio_pci_set_intx_unmask(struct vfio_pci_core_device *vdev,
 				    unsigned int index, unsigned int start,
 				    unsigned int count, uint32_t flags,
@@ -701,71 +722,16 @@  static int vfio_pci_set_intx_mask(struct vfio_pci_core_device *vdev,
 	return 0;
 }
 
-static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
-				     unsigned int index, unsigned int start,
-				     unsigned int count, uint32_t flags,
-				     void *data)
-{
-	struct vfio_pci_irq_ctx *ctx;
-	unsigned int i;
-
-	if (is_intx(vdev) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
-		vfio_intx_disable(vdev, index);
-		return 0;
-	}
-
-	if (!(is_intx(vdev) || is_irq_none(vdev)))
-		return -EINVAL;
-
-	if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
-		int32_t *fds = data;
-		int ret;
-
-		if (is_intx(vdev))
-			return vfio_irq_set_block(vdev, start, count, fds, index);
-
-		ret = vfio_intx_enable(vdev, start, count, index);
-		if (ret)
-			return ret;
-
-		ret = vfio_irq_set_block(vdev, start, count, fds, index);
-		if (ret)
-			vfio_intx_disable(vdev, index);
-
-		return ret;
-	}
-
-	if (!is_intx(vdev))
-		return -EINVAL;
-
-	/* temporary */
-	for (i = start; i < start + count; i++) {
-		ctx = vfio_irq_ctx_get(vdev, i);
-		if (!ctx || !ctx->trigger)
-			continue;
-		if (flags & VFIO_IRQ_SET_DATA_NONE) {
-			vfio_send_intx_eventfd_ctx(vdev, ctx);
-		} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
-			uint8_t *bools = data;
-
-			if (bools[i - start])
-				vfio_send_intx_eventfd_ctx(vdev, ctx);
-		}
-	}
-
-	return 0;
-}
-
-static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
-				    unsigned int index, unsigned int start,
-				    unsigned int count, uint32_t flags,
-				    void *data)
+static int vfio_pci_set_trigger(struct vfio_pci_core_device *vdev,
+				unsigned int index, unsigned int start,
+				unsigned int count, uint32_t flags,
+				void *data)
 {
 	struct vfio_pci_irq_ctx *ctx;
 	unsigned int i;
 
 	if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
-		vfio_msi_disable(vdev, index);
+		intr_ops[index].disable(vdev, index);
 		return 0;
 	}
 
@@ -780,13 +746,13 @@  static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
 			return vfio_irq_set_block(vdev, start, count,
 						  fds, index);
 
-		ret = vfio_msi_enable(vdev, start, count, index);
+		ret = intr_ops[index].enable(vdev, start, count, index);
 		if (ret)
 			return ret;
 
 		ret = vfio_irq_set_block(vdev, start, count, fds, index);
 		if (ret)
-			vfio_msi_disable(vdev, index);
+			intr_ops[index].disable(vdev, index);
 
 		return ret;
 	}
@@ -799,11 +765,11 @@  static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
 		if (!ctx || !ctx->trigger)
 			continue;
 		if (flags & VFIO_IRQ_SET_DATA_NONE) {
-			eventfd_signal(ctx->trigger);
+			intr_ops[index].send_eventfd(vdev, ctx);
 		} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
 			uint8_t *bools = data;
 			if (bools[i - start])
-				eventfd_signal(ctx->trigger);
+				intr_ops[index].send_eventfd(vdev, ctx);
 		}
 	}
 	return 0;
@@ -912,7 +878,7 @@  int vfio_pci_set_irqs_ioctl(struct vfio_pci_core_device *vdev, uint32_t flags,
 			func = vfio_pci_set_intx_unmask;
 			break;
 		case VFIO_IRQ_SET_ACTION_TRIGGER:
-			func = vfio_pci_set_intx_trigger;
+			func = vfio_pci_set_trigger;
 			break;
 		}
 		break;
@@ -924,7 +890,7 @@  int vfio_pci_set_irqs_ioctl(struct vfio_pci_core_device *vdev, uint32_t flags,
 			/* XXX Need masking support exported */
 			break;
 		case VFIO_IRQ_SET_ACTION_TRIGGER:
-			func = vfio_pci_set_msi_trigger;
+			func = vfio_pci_set_trigger;
 			break;
 		}
 		break;