[v4,1/3] vfio: revert change that does intr eventfd setup at probe

Message ID 20190718143628.169623-2-ndabilpuram@marvell.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • vfio: fix broken msix interrupt initialization
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Nithin Dabilpuram July 18, 2019, 2:36 p.m.
This reverts commit 89aac60e0be9ed95a87b16e3595f102f9faaffb4.
"vfio: fix interrupts race condition"

The above mentioned commit moves the interrupt's eventfd setup
to probe time but only enables one interrupt for all types of
interrupt handles i.e VFIO_MSI, VFIO_LEGACY, VFIO_MSIX, UIO.
It works fine with default case but breaks below cases specifically
for MSIX based interrupt handles.

* Applications like l3fwd-power that request rxq interrupts
  while ethdev setup.
* Drivers that need > 1 MSIx interrupts to be configured for
  functionality to work.

VFIO PCI for MSIx expects all the possible vectors to be setup up
when using VFIO_IRQ_SET_ACTION_TRIGGER so that they can be
allocated from kernel pci subsystem. Only way to increase the number
of vectors later is first free all by using VFIO_IRQ_SET_DATA_NONE
with action trigger and then enable new vector count.

Above commit changes the behavior of rte_intr_[enable|disable] to
only mask and unmask unlike earlier behavior and thereby
breaking above two scenarios.

Fixes: 89aac60e0be9 ("vfio: fix interrupts race condition")
Cc: david.marchand@redhat.com

Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
Signed-off-by: Jerin Jacob <jerinj@marvell.com>
Tested-by: Stephen Hemminger <stephen@networkplumber.org>
Tested-by: Shahed Shaikh <shshaikh@marvell.com>
---
v4:
* No change.
v3:
* Update Shahed Shaikh's tested-by
v2:
* Include tested by sign from Stephen

 drivers/bus/pci/linux/pci_vfio.c          |  78 ++++++------
 lib/librte_eal/linux/eal/eal_interrupts.c | 201 +++++++++++++++++++++++-------
 2 files changed, 191 insertions(+), 88 deletions(-)

Comments

Yao, Lei A July 19, 2019, 8:28 a.m. | #1
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Nithin Dabilpuram
> Sent: Thursday, July 18, 2019 10:36 PM
> To: Hyong Youb Kim <hyonkim@cisco.com>; David Marchand
> <david.marchand@redhat.com>; Thomas Monjalon
> <thomas@monjalon.net>; Yigit, Ferruh <ferruh.yigit@intel.com>; Burakov,
> Anatoly <anatoly.burakov@intel.com>
> Cc: jerinj@marvell.com; John Daley <johndale@cisco.com>; Shahed Shaikh
> <shshaikh@marvell.com>; dev@dpdk.org; Nithin Dabilpuram
> <ndabilpuram@marvell.com>
> Subject: [dpdk-dev] [PATCH v4 1/3] vfio: revert change that does intr
> eventfd setup at probe
> 
> This reverts commit 89aac60e0be9ed95a87b16e3595f102f9faaffb4.
> "vfio: fix interrupts race condition"
> 
> The above mentioned commit moves the interrupt's eventfd setup
> to probe time but only enables one interrupt for all types of
> interrupt handles i.e VFIO_MSI, VFIO_LEGACY, VFIO_MSIX, UIO.
> It works fine with default case but breaks below cases specifically
> for MSIX based interrupt handles.
> 
> * Applications like l3fwd-power that request rxq interrupts
>   while ethdev setup.
> * Drivers that need > 1 MSIx interrupts to be configured for
>   functionality to work.
> 
> VFIO PCI for MSIx expects all the possible vectors to be setup up
> when using VFIO_IRQ_SET_ACTION_TRIGGER so that they can be
> allocated from kernel pci subsystem. Only way to increase the number
> of vectors later is first free all by using VFIO_IRQ_SET_DATA_NONE
> with action trigger and then enable new vector count.
> 
> Above commit changes the behavior of rte_intr_[enable|disable] to
> only mask and unmask unlike earlier behavior and thereby
> breaking above two scenarios.
> 
> Fixes: 89aac60e0be9 ("vfio: fix interrupts race condition")
> Cc: david.marchand@redhat.com
> 
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> Tested-by: Stephen Hemminger <stephen@networkplumber.org>
> Tested-by: Shahed Shaikh <shshaikh@marvell.com>
Tested-by: Lei Yao <lei.a.yao@Intel.com>

This patch set  pass the interrupt test with ixgbe, i40e and virtio.
> ---
> v4:
> * No change.
> v3:
> * Update Shahed Shaikh's tested-by
> v2:
> * Include tested by sign from Stephen
> 
>  drivers/bus/pci/linux/pci_vfio.c          |  78 ++++++------
>  lib/librte_eal/linux/eal/eal_interrupts.c | 201 +++++++++++++++++++++++-
> ------
>  2 files changed, 191 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index ee31239..1ceb1c0 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -187,11 +187,8 @@ pci_vfio_set_bus_master(int dev_fd, bool op)
>  static int
>  pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
>  {
> -	char irq_set_buf[sizeof(struct vfio_irq_set) + sizeof(int)];
> -	struct vfio_irq_set *irq_set;
> -	enum rte_intr_mode intr_mode;
>  	int i, ret, intr_idx;
> -	int fd;
> +	enum rte_intr_mode intr_mode;
> 
>  	/* default to invalid index */
>  	intr_idx = VFIO_PCI_NUM_IRQS;
> @@ -223,6 +220,7 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev,
> int vfio_dev_fd)
>  	/* start from MSI-X interrupt type */
>  	for (i = VFIO_PCI_MSIX_IRQ_INDEX; i >= 0; i--) {
>  		struct vfio_irq_info irq = { .argsz = sizeof(irq) };
> +		int fd = -1;
> 
>  		/* skip interrupt modes we don't want */
>  		if (intr_mode != RTE_INTR_MODE_NONE &&
> @@ -238,51 +236,51 @@ pci_vfio_setup_interrupts(struct rte_pci_device
> *dev, int vfio_dev_fd)
>  			return -1;
>  		}
> 
> -		/* found a usable interrupt mode */
> -		if ((irq.flags & VFIO_IRQ_INFO_EVENTFD) != 0)
> -			break;
> -
>  		/* if this vector cannot be used with eventfd, fail if we
> explicitly
>  		 * specified interrupt type, otherwise continue */
> -		if (intr_mode != RTE_INTR_MODE_NONE) {
> -			RTE_LOG(ERR, EAL, "  interrupt vector does not
> support eventfd!\n");
> +		if ((irq.flags & VFIO_IRQ_INFO_EVENTFD) == 0) {
> +			if (intr_mode != RTE_INTR_MODE_NONE) {
> +				RTE_LOG(ERR, EAL,
> +						"  interrupt vector does not
> support eventfd!\n");
> +				return -1;
> +			} else
> +				continue;
> +		}
> +
> +		/* set up an eventfd for interrupts */
> +		fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> +		if (fd < 0) {
> +			RTE_LOG(ERR, EAL, "  cannot set up eventfd, "
> +					"error %i (%s)\n", errno,
> strerror(errno));
>  			return -1;
>  		}
> -	}
> 
> -	if (i < 0)
> -		return -1;
> +		dev->intr_handle.fd = fd;
> +		dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
> 
> -	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> -	if (fd < 0) {
> -		RTE_LOG(ERR, EAL, "  cannot set up eventfd, error %i (%s)\n",
> -			errno, strerror(errno));
> -		return -1;
> -	}
> +		switch (i) {
> +		case VFIO_PCI_MSIX_IRQ_INDEX:
> +			intr_mode = RTE_INTR_MODE_MSIX;
> +			dev->intr_handle.type =
> RTE_INTR_HANDLE_VFIO_MSIX;
> +			break;
> +		case VFIO_PCI_MSI_IRQ_INDEX:
> +			intr_mode = RTE_INTR_MODE_MSI;
> +			dev->intr_handle.type =
> RTE_INTR_HANDLE_VFIO_MSI;
> +			break;
> +		case VFIO_PCI_INTX_IRQ_INDEX:
> +			intr_mode = RTE_INTR_MODE_LEGACY;
> +			dev->intr_handle.type =
> RTE_INTR_HANDLE_VFIO_LEGACY;
> +			break;
> +		default:
> +			RTE_LOG(ERR, EAL, "  unknown interrupt type!\n");
> +			return -1;
> +		}
> 
> -	irq_set = (struct vfio_irq_set *)irq_set_buf;
> -	irq_set->argsz = sizeof(irq_set_buf);
> -	irq_set->flags =
> VFIO_IRQ_SET_DATA_EVENTFD|VFIO_IRQ_SET_ACTION_TRIGGER;
> -	irq_set->index = i;
> -	irq_set->start = 0;
> -	irq_set->count = 1;
> -	memcpy(&irq_set->data, &fd, sizeof(int));
> -	if (ioctl(vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set) < 0) {
> -		RTE_LOG(ERR, EAL, "  error configuring interrupt\n");
> -		close(fd);
> -		return -1;
> +		return 0;
>  	}
> 
> -	dev->intr_handle.fd = fd;
> -	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
> -	if (i == VFIO_PCI_MSIX_IRQ_INDEX)
> -		dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_MSIX;
> -	else if (i == VFIO_PCI_MSI_IRQ_INDEX)
> -		dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_MSI;
> -	else if (i == VFIO_PCI_INTX_IRQ_INDEX)
> -		dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_LEGACY;
> -
> -	return 0;
> +	/* if we're here, we haven't found a suitable interrupt vector */
> +	return -1;
>  }
> 
>  #ifdef HAVE_VFIO_DEV_REQ_INTERFACE
> diff --git a/lib/librte_eal/linux/eal/eal_interrupts.c
> b/lib/librte_eal/linux/eal/eal_interrupts.c
> index 27976b3..79ad5e8 100644
> --- a/lib/librte_eal/linux/eal/eal_interrupts.c
> +++ b/lib/librte_eal/linux/eal/eal_interrupts.c
> @@ -109,19 +109,42 @@ static pthread_t intr_thread;
> 
>  /* enable legacy (INTx) interrupts */
>  static int
> -vfio_enable_intx(const struct rte_intr_handle *intr_handle)
> -{
> -	struct vfio_irq_set irq_set;
> -	int ret;
> -
> -	/* unmask INTx */
> -	irq_set.argsz = sizeof(irq_set);
> -	irq_set.flags = VFIO_IRQ_SET_DATA_NONE |
> VFIO_IRQ_SET_ACTION_UNMASK;
> -	irq_set.index = VFIO_PCI_INTX_IRQ_INDEX;
> -	irq_set.start = 0;
> -	irq_set.count = 1;
> -
> -	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> &irq_set);
> +vfio_enable_intx(const struct rte_intr_handle *intr_handle) {
> +	struct vfio_irq_set *irq_set;
> +	char irq_set_buf[IRQ_SET_BUF_LEN];
> +	int len, ret;
> +	int *fd_ptr;
> +
> +	len = sizeof(irq_set_buf);
> +
> +	/* enable INTx */
> +	irq_set = (struct vfio_irq_set *) irq_set_buf;
> +	irq_set->argsz = len;
> +	irq_set->count = 1;
> +	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> VFIO_IRQ_SET_ACTION_TRIGGER;
> +	irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
> +	irq_set->start = 0;
> +	fd_ptr = (int *) &irq_set->data;
> +	*fd_ptr = intr_handle->fd;
> +
> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> irq_set);
> +
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "Error enabling INTx interrupts for
> fd %d\n",
> +						intr_handle->fd);
> +		return -1;
> +	}
> +
> +	/* unmask INTx after enabling */
> +	memset(irq_set, 0, len);
> +	len = sizeof(struct vfio_irq_set);
> +	irq_set->argsz = len;
> +	irq_set->count = 1;
> +	irq_set->flags = VFIO_IRQ_SET_DATA_NONE |
> VFIO_IRQ_SET_ACTION_UNMASK;
> +	irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
> +	irq_set->start = 0;
> +
> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> irq_set);
> 
>  	if (ret) {
>  		RTE_LOG(ERR, EAL, "Error unmasking INTx interrupts for
> fd %d\n",
> @@ -133,51 +156,128 @@ vfio_enable_intx(const struct rte_intr_handle
> *intr_handle)
> 
>  /* disable legacy (INTx) interrupts */
>  static int
> -vfio_disable_intx(const struct rte_intr_handle *intr_handle)
> -{
> -	struct vfio_irq_set irq_set;
> -	int ret;
> +vfio_disable_intx(const struct rte_intr_handle *intr_handle) {
> +	struct vfio_irq_set *irq_set;
> +	char irq_set_buf[IRQ_SET_BUF_LEN];
> +	int len, ret;
> 
> -	/* mask interrupts */
> -	irq_set.argsz = sizeof(irq_set);
> -	irq_set.flags = VFIO_IRQ_SET_DATA_NONE |
> VFIO_IRQ_SET_ACTION_MASK;
> -	irq_set.index = VFIO_PCI_INTX_IRQ_INDEX;
> -	irq_set.start = 0;
> -	irq_set.count = 1;
> +	len = sizeof(struct vfio_irq_set);
> 
> -	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> &irq_set);
> +	/* mask interrupts before disabling */
> +	irq_set = (struct vfio_irq_set *) irq_set_buf;
> +	irq_set->argsz = len;
> +	irq_set->count = 1;
> +	irq_set->flags = VFIO_IRQ_SET_DATA_NONE |
> VFIO_IRQ_SET_ACTION_MASK;
> +	irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
> +	irq_set->start = 0;
> +
> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> irq_set);
> 
>  	if (ret) {
>  		RTE_LOG(ERR, EAL, "Error masking INTx interrupts for
> fd %d\n",
>  						intr_handle->fd);
>  		return -1;
>  	}
> +
> +	/* disable INTx*/
> +	memset(irq_set, 0, len);
> +	irq_set->argsz = len;
> +	irq_set->count = 0;
> +	irq_set->flags = VFIO_IRQ_SET_DATA_NONE |
> VFIO_IRQ_SET_ACTION_TRIGGER;
> +	irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
> +	irq_set->start = 0;
> +
> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> irq_set);
> +
> +	if (ret) {
> +		RTE_LOG(ERR, EAL,
> +			"Error disabling INTx interrupts for fd %d\n",
> intr_handle->fd);
> +		return -1;
> +	}
>  	return 0;
>  }
> 
> +/* enable MSI interrupts */
> +static int
> +vfio_enable_msi(const struct rte_intr_handle *intr_handle) {
> +	int len, ret;
> +	char irq_set_buf[IRQ_SET_BUF_LEN];
> +	struct vfio_irq_set *irq_set;
> +	int *fd_ptr;
> +
> +	len = sizeof(irq_set_buf);
> +
> +	irq_set = (struct vfio_irq_set *) irq_set_buf;
> +	irq_set->argsz = len;
> +	irq_set->count = 1;
> +	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> VFIO_IRQ_SET_ACTION_TRIGGER;
> +	irq_set->index = VFIO_PCI_MSI_IRQ_INDEX;
> +	irq_set->start = 0;
> +	fd_ptr = (int *) &irq_set->data;
> +	*fd_ptr = intr_handle->fd;
> +
> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> irq_set);
> +
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "Error enabling MSI interrupts for
> fd %d\n",
> +						intr_handle->fd);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +/* disable MSI interrupts */
> +static int
> +vfio_disable_msi(const struct rte_intr_handle *intr_handle) {
> +	struct vfio_irq_set *irq_set;
> +	char irq_set_buf[IRQ_SET_BUF_LEN];
> +	int len, ret;
> +
> +	len = sizeof(struct vfio_irq_set);
> +
> +	irq_set = (struct vfio_irq_set *) irq_set_buf;
> +	irq_set->argsz = len;
> +	irq_set->count = 0;
> +	irq_set->flags = VFIO_IRQ_SET_DATA_NONE |
> VFIO_IRQ_SET_ACTION_TRIGGER;
> +	irq_set->index = VFIO_PCI_MSI_IRQ_INDEX;
> +	irq_set->start = 0;
> +
> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> irq_set);
> +
> +	if (ret)
> +		RTE_LOG(ERR, EAL,
> +			"Error disabling MSI interrupts for fd %d\n",
> intr_handle->fd);
> +
> +	return ret;
> +}
> +
>  /* enable MSI-X interrupts */
>  static int
> -vfio_enable_msix(const struct rte_intr_handle *intr_handle)
> -{
> +vfio_enable_msix(const struct rte_intr_handle *intr_handle) {
> +	int len, ret;
>  	char irq_set_buf[MSIX_IRQ_SET_BUF_LEN];
>  	struct vfio_irq_set *irq_set;
> -	int len, ret;
> -
> -	if (intr_handle->nb_efd == 0)
> -		return 0;
> +	int *fd_ptr;
> 
>  	len = sizeof(irq_set_buf);
> 
>  	irq_set = (struct vfio_irq_set *) irq_set_buf;
>  	irq_set->argsz = len;
> +	/* 0 < irq_set->count < RTE_MAX_RXTX_INTR_VEC_ID + 1 */
> +	irq_set->count = intr_handle->max_intr ?
> +		(intr_handle->max_intr > RTE_MAX_RXTX_INTR_VEC_ID + 1 ?
> +		RTE_MAX_RXTX_INTR_VEC_ID + 1 : intr_handle->max_intr) :
> 1;
>  	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> VFIO_IRQ_SET_ACTION_TRIGGER;
>  	irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
> -	irq_set->start = RTE_INTR_VEC_RXTX_OFFSET;
> -	irq_set->count = intr_handle->nb_efd;
> -	memcpy(&irq_set->data, intr_handle->efds,
> -	       sizeof(*intr_handle->efds) * intr_handle->nb_efd);
> +	irq_set->start = 0;
> +	fd_ptr = (int *) &irq_set->data;
> +	/* INTR vector offset 0 reserve for non-efds mapping */
> +	fd_ptr[RTE_INTR_VEC_ZERO_OFFSET] = intr_handle->fd;
> +	memcpy(&fd_ptr[RTE_INTR_VEC_RXTX_OFFSET], intr_handle->efds,
> +		sizeof(*intr_handle->efds) * intr_handle->nb_efd);
> 
>  	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> irq_set);
> +
>  	if (ret) {
>  		RTE_LOG(ERR, EAL, "Error enabling MSI-X interrupts for
> fd %d\n",
>  						intr_handle->fd);
> @@ -189,21 +289,22 @@ vfio_enable_msix(const struct rte_intr_handle
> *intr_handle)
> 
>  /* disable MSI-X interrupts */
>  static int
> -vfio_disable_msix(const struct rte_intr_handle *intr_handle)
> -{
> -	struct vfio_irq_set irq_set;
> -	int ret;
> +vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
> +	struct vfio_irq_set *irq_set;
> +	char irq_set_buf[MSIX_IRQ_SET_BUF_LEN];
> +	int len, ret;
> 
> -	if (intr_handle->nb_efd == 0)
> -		return 0;
> +	len = sizeof(struct vfio_irq_set);
> 
> -	irq_set.argsz = sizeof(irq_set);
> -	irq_set.flags = VFIO_IRQ_SET_DATA_NONE |
> VFIO_IRQ_SET_ACTION_TRIGGER;
> -	irq_set.index = VFIO_PCI_MSIX_IRQ_INDEX;
> -	irq_set.start = RTE_INTR_VEC_RXTX_OFFSET;
> -	irq_set.count = intr_handle->nb_efd;
> +	irq_set = (struct vfio_irq_set *) irq_set_buf;
> +	irq_set->argsz = len;
> +	irq_set->count = 0;
> +	irq_set->flags = VFIO_IRQ_SET_DATA_NONE |
> VFIO_IRQ_SET_ACTION_TRIGGER;
> +	irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
> +	irq_set->start = 0;
> +
> +	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> irq_set);
> 
> -	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS,
> &irq_set);
>  	if (ret)
>  		RTE_LOG(ERR, EAL,
>  			"Error disabling MSI-X interrupts for fd %d\n",
> intr_handle->fd);
> @@ -564,7 +665,9 @@ rte_intr_enable(const struct rte_intr_handle
> *intr_handle)
>  			return -1;
>  		break;
>  	case RTE_INTR_HANDLE_VFIO_MSI:
> -		return 0;
> +		if (vfio_enable_msi(intr_handle))
> +			return -1;
> +		break;
>  	case RTE_INTR_HANDLE_VFIO_LEGACY:
>  		if (vfio_enable_intx(intr_handle))
>  			return -1;
> @@ -618,7 +721,9 @@ rte_intr_disable(const struct rte_intr_handle
> *intr_handle)
>  			return -1;
>  		break;
>  	case RTE_INTR_HANDLE_VFIO_MSI:
> -		return 0;
> +		if (vfio_disable_msi(intr_handle))
> +			return -1;
> +		break;
>  	case RTE_INTR_HANDLE_VFIO_LEGACY:
>  		if (vfio_disable_intx(intr_handle))
>  			return -1;
> --
> 2.8.4
David Marchand July 22, 2019, 7:27 p.m. | #2
On Thu, Jul 18, 2019 at 4:36 PM Nithin Dabilpuram
<ndabilpuram@marvell.com> wrote:
>
> This reverts commit 89aac60e0be9ed95a87b16e3595f102f9faaffb4.
> "vfio: fix interrupts race condition"
>
> The above mentioned commit moves the interrupt's eventfd setup
> to probe time but only enables one interrupt for all types of
> interrupt handles i.e VFIO_MSI, VFIO_LEGACY, VFIO_MSIX, UIO.
> It works fine with default case but breaks below cases specifically
> for MSIX based interrupt handles.
>
> * Applications like l3fwd-power that request rxq interrupts
>   while ethdev setup.
> * Drivers that need > 1 MSIx interrupts to be configured for
>   functionality to work.
>
> VFIO PCI for MSIx expects all the possible vectors to be setup up
> when using VFIO_IRQ_SET_ACTION_TRIGGER so that they can be
> allocated from kernel pci subsystem. Only way to increase the number
> of vectors later is first free all by using VFIO_IRQ_SET_DATA_NONE
> with action trigger and then enable new vector count.

Indeed, this is the part that I had missed.
Ok for this revert.
A bit sad to see the coding style issues back :-).


--
David Marchand

Patch

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index ee31239..1ceb1c0 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -187,11 +187,8 @@  pci_vfio_set_bus_master(int dev_fd, bool op)
 static int
 pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
 {
-	char irq_set_buf[sizeof(struct vfio_irq_set) + sizeof(int)];
-	struct vfio_irq_set *irq_set;
-	enum rte_intr_mode intr_mode;
 	int i, ret, intr_idx;
-	int fd;
+	enum rte_intr_mode intr_mode;
 
 	/* default to invalid index */
 	intr_idx = VFIO_PCI_NUM_IRQS;
@@ -223,6 +220,7 @@  pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
 	/* start from MSI-X interrupt type */
 	for (i = VFIO_PCI_MSIX_IRQ_INDEX; i >= 0; i--) {
 		struct vfio_irq_info irq = { .argsz = sizeof(irq) };
+		int fd = -1;
 
 		/* skip interrupt modes we don't want */
 		if (intr_mode != RTE_INTR_MODE_NONE &&
@@ -238,51 +236,51 @@  pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
 			return -1;
 		}
 
-		/* found a usable interrupt mode */
-		if ((irq.flags & VFIO_IRQ_INFO_EVENTFD) != 0)
-			break;
-
 		/* if this vector cannot be used with eventfd, fail if we explicitly
 		 * specified interrupt type, otherwise continue */
-		if (intr_mode != RTE_INTR_MODE_NONE) {
-			RTE_LOG(ERR, EAL, "  interrupt vector does not support eventfd!\n");
+		if ((irq.flags & VFIO_IRQ_INFO_EVENTFD) == 0) {
+			if (intr_mode != RTE_INTR_MODE_NONE) {
+				RTE_LOG(ERR, EAL,
+						"  interrupt vector does not support eventfd!\n");
+				return -1;
+			} else
+				continue;
+		}
+
+		/* set up an eventfd for interrupts */
+		fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+		if (fd < 0) {
+			RTE_LOG(ERR, EAL, "  cannot set up eventfd, "
+					"error %i (%s)\n", errno, strerror(errno));
 			return -1;
 		}
-	}
 
-	if (i < 0)
-		return -1;
+		dev->intr_handle.fd = fd;
+		dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
 
-	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
-	if (fd < 0) {
-		RTE_LOG(ERR, EAL, "  cannot set up eventfd, error %i (%s)\n",
-			errno, strerror(errno));
-		return -1;
-	}
+		switch (i) {
+		case VFIO_PCI_MSIX_IRQ_INDEX:
+			intr_mode = RTE_INTR_MODE_MSIX;
+			dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_MSIX;
+			break;
+		case VFIO_PCI_MSI_IRQ_INDEX:
+			intr_mode = RTE_INTR_MODE_MSI;
+			dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_MSI;
+			break;
+		case VFIO_PCI_INTX_IRQ_INDEX:
+			intr_mode = RTE_INTR_MODE_LEGACY;
+			dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_LEGACY;
+			break;
+		default:
+			RTE_LOG(ERR, EAL, "  unknown interrupt type!\n");
+			return -1;
+		}
 
-	irq_set = (struct vfio_irq_set *)irq_set_buf;
-	irq_set->argsz = sizeof(irq_set_buf);
-	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD|VFIO_IRQ_SET_ACTION_TRIGGER;
-	irq_set->index = i;
-	irq_set->start = 0;
-	irq_set->count = 1;
-	memcpy(&irq_set->data, &fd, sizeof(int));
-	if (ioctl(vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set) < 0) {
-		RTE_LOG(ERR, EAL, "  error configuring interrupt\n");
-		close(fd);
-		return -1;
+		return 0;
 	}
 
-	dev->intr_handle.fd = fd;
-	dev->intr_handle.vfio_dev_fd = vfio_dev_fd;
-	if (i == VFIO_PCI_MSIX_IRQ_INDEX)
-		dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_MSIX;
-	else if (i == VFIO_PCI_MSI_IRQ_INDEX)
-		dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_MSI;
-	else if (i == VFIO_PCI_INTX_IRQ_INDEX)
-		dev->intr_handle.type = RTE_INTR_HANDLE_VFIO_LEGACY;
-
-	return 0;
+	/* if we're here, we haven't found a suitable interrupt vector */
+	return -1;
 }
 
 #ifdef HAVE_VFIO_DEV_REQ_INTERFACE
diff --git a/lib/librte_eal/linux/eal/eal_interrupts.c b/lib/librte_eal/linux/eal/eal_interrupts.c
index 27976b3..79ad5e8 100644
--- a/lib/librte_eal/linux/eal/eal_interrupts.c
+++ b/lib/librte_eal/linux/eal/eal_interrupts.c
@@ -109,19 +109,42 @@  static pthread_t intr_thread;
 
 /* enable legacy (INTx) interrupts */
 static int
-vfio_enable_intx(const struct rte_intr_handle *intr_handle)
-{
-	struct vfio_irq_set irq_set;
-	int ret;
-
-	/* unmask INTx */
-	irq_set.argsz = sizeof(irq_set);
-	irq_set.flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_UNMASK;
-	irq_set.index = VFIO_PCI_INTX_IRQ_INDEX;
-	irq_set.start = 0;
-	irq_set.count = 1;
-
-	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, &irq_set);
+vfio_enable_intx(const struct rte_intr_handle *intr_handle) {
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
+	int *fd_ptr;
+
+	len = sizeof(irq_set_buf);
+
+	/* enable INTx */
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
+	irq_set->start = 0;
+	fd_ptr = (int *) &irq_set->data;
+	*fd_ptr = intr_handle->fd;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Error enabling INTx interrupts for fd %d\n",
+						intr_handle->fd);
+		return -1;
+	}
+
+	/* unmask INTx after enabling */
+	memset(irq_set, 0, len);
+	len = sizeof(struct vfio_irq_set);
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_UNMASK;
+	irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
 
 	if (ret) {
 		RTE_LOG(ERR, EAL, "Error unmasking INTx interrupts for fd %d\n",
@@ -133,51 +156,128 @@  vfio_enable_intx(const struct rte_intr_handle *intr_handle)
 
 /* disable legacy (INTx) interrupts */
 static int
-vfio_disable_intx(const struct rte_intr_handle *intr_handle)
-{
-	struct vfio_irq_set irq_set;
-	int ret;
+vfio_disable_intx(const struct rte_intr_handle *intr_handle) {
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
 
-	/* mask interrupts */
-	irq_set.argsz = sizeof(irq_set);
-	irq_set.flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_MASK;
-	irq_set.index = VFIO_PCI_INTX_IRQ_INDEX;
-	irq_set.start = 0;
-	irq_set.count = 1;
+	len = sizeof(struct vfio_irq_set);
 
-	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, &irq_set);
+	/* mask interrupts before disabling */
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_MASK;
+	irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
 
 	if (ret) {
 		RTE_LOG(ERR, EAL, "Error masking INTx interrupts for fd %d\n",
 						intr_handle->fd);
 		return -1;
 	}
+
+	/* disable INTx*/
+	memset(irq_set, 0, len);
+	irq_set->argsz = len;
+	irq_set->count = 0;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret) {
+		RTE_LOG(ERR, EAL,
+			"Error disabling INTx interrupts for fd %d\n", intr_handle->fd);
+		return -1;
+	}
 	return 0;
 }
 
+/* enable MSI interrupts */
+static int
+vfio_enable_msi(const struct rte_intr_handle *intr_handle) {
+	int len, ret;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	struct vfio_irq_set *irq_set;
+	int *fd_ptr;
+
+	len = sizeof(irq_set_buf);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 1;
+	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_MSI_IRQ_INDEX;
+	irq_set->start = 0;
+	fd_ptr = (int *) &irq_set->data;
+	*fd_ptr = intr_handle->fd;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret) {
+		RTE_LOG(ERR, EAL, "Error enabling MSI interrupts for fd %d\n",
+						intr_handle->fd);
+		return -1;
+	}
+	return 0;
+}
+
+/* disable MSI interrupts */
+static int
+vfio_disable_msi(const struct rte_intr_handle *intr_handle) {
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[IRQ_SET_BUF_LEN];
+	int len, ret;
+
+	len = sizeof(struct vfio_irq_set);
+
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 0;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_MSI_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+	if (ret)
+		RTE_LOG(ERR, EAL,
+			"Error disabling MSI interrupts for fd %d\n", intr_handle->fd);
+
+	return ret;
+}
+
 /* enable MSI-X interrupts */
 static int
-vfio_enable_msix(const struct rte_intr_handle *intr_handle)
-{
+vfio_enable_msix(const struct rte_intr_handle *intr_handle) {
+	int len, ret;
 	char irq_set_buf[MSIX_IRQ_SET_BUF_LEN];
 	struct vfio_irq_set *irq_set;
-	int len, ret;
-
-	if (intr_handle->nb_efd == 0)
-		return 0;
+	int *fd_ptr;
 
 	len = sizeof(irq_set_buf);
 
 	irq_set = (struct vfio_irq_set *) irq_set_buf;
 	irq_set->argsz = len;
+	/* 0 < irq_set->count < RTE_MAX_RXTX_INTR_VEC_ID + 1 */
+	irq_set->count = intr_handle->max_intr ?
+		(intr_handle->max_intr > RTE_MAX_RXTX_INTR_VEC_ID + 1 ?
+		RTE_MAX_RXTX_INTR_VEC_ID + 1 : intr_handle->max_intr) : 1;
 	irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_TRIGGER;
 	irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
-	irq_set->start = RTE_INTR_VEC_RXTX_OFFSET;
-	irq_set->count = intr_handle->nb_efd;
-	memcpy(&irq_set->data, intr_handle->efds,
-	       sizeof(*intr_handle->efds) * intr_handle->nb_efd);
+	irq_set->start = 0;
+	fd_ptr = (int *) &irq_set->data;
+	/* INTR vector offset 0 reserve for non-efds mapping */
+	fd_ptr[RTE_INTR_VEC_ZERO_OFFSET] = intr_handle->fd;
+	memcpy(&fd_ptr[RTE_INTR_VEC_RXTX_OFFSET], intr_handle->efds,
+		sizeof(*intr_handle->efds) * intr_handle->nb_efd);
 
 	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
 	if (ret) {
 		RTE_LOG(ERR, EAL, "Error enabling MSI-X interrupts for fd %d\n",
 						intr_handle->fd);
@@ -189,21 +289,22 @@  vfio_enable_msix(const struct rte_intr_handle *intr_handle)
 
 /* disable MSI-X interrupts */
 static int
-vfio_disable_msix(const struct rte_intr_handle *intr_handle)
-{
-	struct vfio_irq_set irq_set;
-	int ret;
+vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
+	struct vfio_irq_set *irq_set;
+	char irq_set_buf[MSIX_IRQ_SET_BUF_LEN];
+	int len, ret;
 
-	if (intr_handle->nb_efd == 0)
-		return 0;
+	len = sizeof(struct vfio_irq_set);
 
-	irq_set.argsz = sizeof(irq_set);
-	irq_set.flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
-	irq_set.index = VFIO_PCI_MSIX_IRQ_INDEX;
-	irq_set.start = RTE_INTR_VEC_RXTX_OFFSET;
-	irq_set.count = intr_handle->nb_efd;
+	irq_set = (struct vfio_irq_set *) irq_set_buf;
+	irq_set->argsz = len;
+	irq_set->count = 0;
+	irq_set->flags = VFIO_IRQ_SET_DATA_NONE | VFIO_IRQ_SET_ACTION_TRIGGER;
+	irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
+	irq_set->start = 0;
+
+	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, irq_set);
 
-	ret = ioctl(intr_handle->vfio_dev_fd, VFIO_DEVICE_SET_IRQS, &irq_set);
 	if (ret)
 		RTE_LOG(ERR, EAL,
 			"Error disabling MSI-X interrupts for fd %d\n", intr_handle->fd);
@@ -564,7 +665,9 @@  rte_intr_enable(const struct rte_intr_handle *intr_handle)
 			return -1;
 		break;
 	case RTE_INTR_HANDLE_VFIO_MSI:
-		return 0;
+		if (vfio_enable_msi(intr_handle))
+			return -1;
+		break;
 	case RTE_INTR_HANDLE_VFIO_LEGACY:
 		if (vfio_enable_intx(intr_handle))
 			return -1;
@@ -618,7 +721,9 @@  rte_intr_disable(const struct rte_intr_handle *intr_handle)
 			return -1;
 		break;
 	case RTE_INTR_HANDLE_VFIO_MSI:
-		return 0;
+		if (vfio_disable_msi(intr_handle))
+			return -1;
+		break;
 	case RTE_INTR_HANDLE_VFIO_LEGACY:
 		if (vfio_disable_intx(intr_handle))
 			return -1;