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

[Alex Williamson <alex.williamson@redhat.com>]

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;