From mboxrd@z Thu Jan 1 00:00:00 1970 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 2519D378E for ; Tue, 2 Oct 2018 08:52:29 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us3.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id C8D27B4005A; Tue, 2 Oct 2018 06:52:26 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 2 Oct 2018 07:52:14 +0100 To: Jeff Guo , , , , , , , , , , , , , , CC: , , , 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> <1f0a0bb8-8634-4412-1002-e84362b90fe2@intel.com> From: Andrew Rybchenko Message-ID: Date: Tue, 2 Oct 2018 09:51:30 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 In-Reply-To: <1f0a0bb8-8634-4412-1002-e84362b90fe2@intel.com> Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24130.003 X-TM-AS-Result: No-17.861400-8.000000-10 X-TMASE-MatchedRID: vbSD0OnL8/IOwH4pD14DsPHkpkyUphL9EtdrY/Wb3fPagsZM0qVv15as fA8Y/RCFW5yCgZB+gAlbAeHsEbW6gc637+A5hpnFA9lly13c/gEK3n1SHen81bStxTlX+hJ7ahP /1jRJ6pya5mB7xqs3y/JKsIzII36TE6Q6aj20RNhjoaO27r+3fVgy2ozNthE2Ydn5x3tXIpfVAc A0qi7Q8ECX2G18MwL+PCfqX6LxQk/DkHnJwIRKW37siEtWY367yeUl7aCTy8jb6Y+fnTZUL0NFJ C6C2PHmNlpmSQMjZB82iF1B6yka+PUKQfPzAUrjaDgPZBX/bMvqvccKLF+4pwwv1ZvdCH+FVM2p /cRDyjFic/5yraYWDlebGpjHs5mWxldSKOGXaXwZCVRU/aI9zLq0DjrLw08jeI8u4tqjXL3dKUS BW7I322+5ieh24ZYRX7bicKxRIU23sNbcHjySQbI7zVffJqTzO0+rTsp8ydZo6pGTKjx9OxV+Sh Q64MRbmzUYSaJyf+WP1tXkmp73XcFrOsKxf3J4p1jfXSeV8BIma3YGx6EEKsLoGGrUCJg8 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--17.861400-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24130.003 X-MDID: 1538463148-zgzkBBK4mtIj 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 06:52:29 -0000 On 10/2/18 7:30 AM, Jeff Guo wrote: > 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? Yes, I agree. > >>> +    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; >>