From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [PATCH v2 1/4] eal: add a new req notifier to eal interrupt Date: Mon, 1 Oct 2018 12:46:02 +0300 Message-ID: References: <1534503091-31910-1-git-send-email-jia.guo@intel.com> <1538316988-128382-1-git-send-email-jia.guo@intel.com> <1538316988-128382-2-git-send-email-jia.guo@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , , To: Jeff Guo , , , , , , , , , , , , , , Return-path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id EFBC42952 for ; Mon, 1 Oct 2018 11:46:59 +0200 (CEST) In-Reply-To: <1538316988-128382-2-git-send-email-jia.guo@intel.com> Content-Language: en-GB List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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;