From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id C8EB44C8D for ; Tue, 2 Oct 2018 06:30:49 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Oct 2018 21:30:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,330,1534834800"; d="scan'208,217";a="93786879" Received: from jguo15x-mobl.ccr.corp.intel.com (HELO [10.249.174.2]) ([10.249.174.2]) by fmsmga004.fm.intel.com with ESMTP; 01 Oct 2018 21:30:44 -0700 To: Andrew Rybchenko , stephen@networkplumber.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, konstantin.ananyev@intel.com, gaetan.rivet@6wind.com, jingjing.wu@intel.com, thomas@monjalon.net, motih@mellanox.com, matan@mellanox.com, harry.van.haaren@intel.com, qi.z.zhang@intel.com, shaopeng.he@intel.com, bernard.iremonger@intel.com, anatoly.burakov@intel.com Cc: jblunck@infradead.org, shreyansh.jain@nxp.com, dev@dpdk.org, helin.zhang@intel.com 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> From: Jeff Guo Message-ID: <1f0a0bb8-8634-4412-1002-e84362b90fe2@intel.com> Date: Tue, 2 Oct 2018 12:30:43 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2 1/4] eal: add a new req notifier to eal interrupt X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Oct 2018 04:30:50 -0000 hi, andrew thanks for your review again, see comment as below. On 10/1/2018 5:46 PM, Andrew Rybchenko wrote: > 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? > ok, it should be consistent. >> 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. > make sense, and i found that there are many duplication could be modify here, maybe we could just add param index type to some common function to enable/disable each type of the interrupts/req. I suggest to make other patch set which is aim to modify eal interrupt to do it. The same as the your above comment about char array initialize.  Do you agree with that? >> + 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; >