Re: [PATCH 06/17] vfio/pci: Remove interrupt index interpretation from wrappers

[Alex Williamson]

On Thu,  1 Feb 2024 20:57:00 -0800
Reinette Chatre <reinette.chatre@xxxxxxxxx> wrote:

> vfio_pci_set_intx_trigger() and vfio_pci_set_msi_trigger() have similar
> enough flows that can be converged into one shared flow instead of
> duplicated.
> 
> To share code between management of interrupt types it is necessary that
> the type of the interrupt is only interpreted by the code specific to
> the interrupt type being managed.
> 
> Remove interrupt type interpretation from what will eventually be
> shared code (vfio_pci_set_msi_trigger(), vfio_msi_set_block()) by
> pushing this interpretation to be within the interrupt
> type specific code (vfio_msi_enable(),
> (temporary) vfio_msi_set_vector_signal()).
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> ---
> Note to maintainers:
> Originally formed part of the IMS submission below, but is not
> specific to IMS.
> https://lore.kernel.org/lkml/cover.1696609476.git.reinette.chatre@xxxxxxxxx
> 
>  drivers/vfio/pci/vfio_pci_intrs.c | 38 +++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 97a3bb22b186..31f73c70fcd2 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -346,16 +346,19 @@ static irqreturn_t vfio_msihandler(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> -static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msix)
> +static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec,
> +			   unsigned int index)
>  {
>  	struct pci_dev *pdev = vdev->pdev;
> -	unsigned int flag = msix ? PCI_IRQ_MSIX : PCI_IRQ_MSI;
> +	unsigned int flag;
>  	int ret;
>  	u16 cmd;
>  
>  	if (!is_irq_none(vdev))
>  		return -EINVAL;
>  
> +	flag = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? PCI_IRQ_MSIX : PCI_IRQ_MSI;
> +
>  	/* return the number of supported vectors if we can't get all: */
>  	cmd = vfio_pci_memory_lock_and_enable(vdev);
>  	ret = pci_alloc_irq_vectors(pdev, 1, nvec, flag);
> @@ -367,10 +370,9 @@ static int vfio_msi_enable(struct vfio_pci_core_device *vdev, int nvec, bool msi
>  	}
>  	vfio_pci_memory_unlock_and_restore(vdev, cmd);
>  
> -	vdev->irq_type = msix ? VFIO_PCI_MSIX_IRQ_INDEX :
> -				VFIO_PCI_MSI_IRQ_INDEX;
> +	vdev->irq_type = index;
>  
> -	if (!msix) {
> +	if (index == VFIO_PCI_MSI_IRQ_INDEX) {
>  		/*
>  		 * Compute the virtual hardware field for max msi vectors -
>  		 * it is the log base 2 of the number of vectors.
> @@ -413,8 +415,10 @@ static int vfio_msi_alloc_irq(struct vfio_pci_core_device *vdev,
>  }
>  
>  static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
> -				      unsigned int vector, int fd, bool msix)
> +				      unsigned int vector, int fd,
> +				      unsigned int index)
>  {
> +	bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false;

Cleanup the unnecessary ternary while here, this can just be:

	bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX);

Thanks,

Alex

>  	struct pci_dev *pdev = vdev->pdev;
>  	struct vfio_pci_irq_ctx *ctx;
>  	struct eventfd_ctx *trigger;
> @@ -505,25 +509,26 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
>  
>  static int vfio_msi_set_block(struct vfio_pci_core_device *vdev,
>  			      unsigned int start, unsigned int count,
> -			      int32_t *fds, bool msix)
> +			      int32_t *fds, unsigned int index)
>  {
>  	unsigned int i, j;
>  	int ret = 0;
>  
>  	for (i = 0, j = start; i < count && !ret; i++, j++) {
>  		int fd = fds ? fds[i] : -1;
> -		ret = vfio_msi_set_vector_signal(vdev, j, fd, msix);
> +		ret = vfio_msi_set_vector_signal(vdev, j, fd, index);
>  	}
>  
>  	if (ret) {
>  		for (i = start; i < j; i++)
> -			vfio_msi_set_vector_signal(vdev, i, -1, msix);
> +			vfio_msi_set_vector_signal(vdev, i, -1, index);
>  	}
>  
>  	return ret;
>  }
>  
> -static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix)
> +static void vfio_msi_disable(struct vfio_pci_core_device *vdev,
> +			     unsigned int index)
>  {
>  	struct pci_dev *pdev = vdev->pdev;
>  	struct vfio_pci_irq_ctx *ctx;
> @@ -533,7 +538,7 @@ static void vfio_msi_disable(struct vfio_pci_core_device *vdev, bool msix)
>  	xa_for_each(&vdev->ctx, i, ctx) {
>  		vfio_virqfd_disable(&ctx->unmask);
>  		vfio_virqfd_disable(&ctx->mask);
> -		vfio_msi_set_vector_signal(vdev, i, -1, msix);
> +		vfio_msi_set_vector_signal(vdev, i, -1, index);
>  	}
>  
>  	cmd = vfio_pci_memory_lock_and_enable(vdev);
> @@ -656,10 +661,9 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
>  {
>  	struct vfio_pci_irq_ctx *ctx;
>  	unsigned int i;
> -	bool msix = (index == VFIO_PCI_MSIX_IRQ_INDEX) ? true : false;
>  
>  	if (irq_is(vdev, index) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
> -		vfio_msi_disable(vdev, msix);
> +		vfio_msi_disable(vdev, index);
>  		return 0;
>  	}
>  
> @@ -672,15 +676,15 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_core_device *vdev,
>  
>  		if (vdev->irq_type == index)
>  			return vfio_msi_set_block(vdev, start, count,
> -						  fds, msix);
> +						  fds, index);
>  
> -		ret = vfio_msi_enable(vdev, start + count, msix);
> +		ret = vfio_msi_enable(vdev, start + count, index);
>  		if (ret)
>  			return ret;
>  
> -		ret = vfio_msi_set_block(vdev, start, count, fds, msix);
> +		ret = vfio_msi_set_block(vdev, start, count, fds, index);
>  		if (ret)
> -			vfio_msi_disable(vdev, msix);
> +			vfio_msi_disable(vdev, index);
>  
>  		return ret;
>  	}