diff mbox series

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

Message ID ca249ad459c07a36cd64137ee1cc107dcd8b6f4c.1706849424.git.reinette.chatre@intel.com
State New
Headers
Series vfio/pci: Remove duplicate code and logic from VFIO PCI interrupt management |

Commit Message

Reinette Chatre Feb. 2, 2024, 4:57 a.m. UTC 
  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@intel.com>
---
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@intel.com

 drivers/vfio/pci/vfio_pci_intrs.c | 38 +++++++++++++++++--------------
 1 file changed, 21 insertions(+), 17 deletions(-)

Comments

Alex Williamson Feb. 5, 2024, 10:35 p.m. UTC | #1 
On Thu,  1 Feb 2024 20:57:00 -0800
Reinette Chatre <reinette.chatre@intel.com> 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@intel.com>
> ---
> 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@intel.com
> 
>  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;
>  	}
Reinette Chatre Feb. 6, 2024, 9:44 p.m. UTC | #2 
Hi Alex,

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

>> @@ -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);
> 

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 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;
 	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;
 	}