Re: [dpdk-dev] [PATCH v2 1/4] eal: add a new req notifier to eal interrupt

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 9/30/18 5:16 PM, Jeff Guo wrote:
> Add a new req notifier in eal interrupt for enable vfio hotplug.
>
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v2->v1:
> no change
> ---
>   lib/librte_eal/common/include/rte_eal_interrupts.h |  1 +
>   lib/librte_eal/linuxapp/eal/eal_interrupts.c       | 71 ++++++++++++++++++++++
>   2 files changed, 72 insertions(+)
>
> diff --git a/lib/librte_eal/common/include/rte_eal_interrupts.h b/lib/librte_eal/common/include/rte_eal_interrupts.h
> index 6eb4932..2c47738 100644
> --- a/lib/librte_eal/common/include/rte_eal_interrupts.h
> +++ b/lib/librte_eal/common/include/rte_eal_interrupts.h
> @@ -35,6 +35,7 @@ enum rte_intr_handle_type {
>   	RTE_INTR_HANDLE_EXT,          /**< external handler */
>   	RTE_INTR_HANDLE_VDEV,         /**< virtual device */
>   	RTE_INTR_HANDLE_DEV_EVENT,    /**< device event handle */
> +	RTE_INTR_HANDLE_VFIO_REQ,  /**< vfio device handle (req) */

Alignment looks inconsistent. Is there any reason?

>   	RTE_INTR_HANDLE_MAX           /**< count of elements */
>   };
>   
> diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> index 4076c6d..7f611b3 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
> @@ -308,6 +308,64 @@ vfio_disable_msix(const struct rte_intr_handle *intr_handle) {
>   
>   	return ret;
>   }
> +
> +/* enable req notifier */
> +static int
> +vfio_enable_req(const struct rte_intr_handle *intr_handle)
> +{
> +	int len, ret;
> +	char irq_set_buf[IRQ_SET_BUF_LEN];

I see that it is copied from similar functions in the file, but
I'd suggest to initialize it with zeros using '= {};' just to make
sure that uninitialized on-stack data never go to kernel.

> +	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_REQ_IRQ_INDEX;

It looks like it is the only difference (plus error log below) from
vfio_enable_msi(). May be it make sense to restructure code
to avoid duplication. Obviously it is not critical, but please, consider.

Similar comment is applicable to vfio_disable_req() below.

> +	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 req interrupts for fd %d\n",
> +						intr_handle->fd);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/* disable req notifier */
> +static int
> +vfio_disable_req(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_REQ_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 req interrupts for fd %d\n",
> +			intr_handle->fd);
> +
> +	return ret;
> +}
>   #endif
>   
>   static int
> @@ -556,6 +614,10 @@ rte_intr_enable(const struct rte_intr_handle *intr_handle)
>   		if (vfio_enable_intx(intr_handle))
>   			return -1;
>   		break;
> +	case RTE_INTR_HANDLE_VFIO_REQ:
> +		if (vfio_enable_req(intr_handle))
> +			return -1;
> +		break;
>   #endif
>   	/* not used at this moment */
>   	case RTE_INTR_HANDLE_DEV_EVENT:
> @@ -606,6 +668,11 @@ rte_intr_disable(const struct rte_intr_handle *intr_handle)
>   		if (vfio_disable_intx(intr_handle))
>   			return -1;
>   		break;
> +	case RTE_INTR_HANDLE_VFIO_REQ:
> +		if (vfio_disable_req(intr_handle))
> +			return -1;
> +		break;
> +
>   #endif
>   	/* not used at this moment */
>   	case RTE_INTR_HANDLE_DEV_EVENT:
> @@ -682,6 +749,10 @@ eal_intr_process_interrupts(struct epoll_event *events, int nfds)
>   			bytes_read = 0;
>   			call = true;
>   			break;
> +		case RTE_INTR_HANDLE_VFIO_REQ:
> +			bytes_read = 0;
> +			call = true;
> +			break;
>   		default:
>   			bytes_read = 1;
>   			break;