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 3DD332BBD for ; Mon, 1 Oct 2018 11:48:01 +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-us4.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 9CF2A800059; Mon, 1 Oct 2018 09:47:59 +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; Mon, 1 Oct 2018 10:47:48 +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-5-git-send-email-jia.guo@intel.com> From: Andrew Rybchenko Message-ID: <765984fb-213e-002f-209a-9165011540fb@solarflare.com> Date: Mon, 1 Oct 2018 12:47:05 +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: <1538316988-128382-5-git-send-email-jia.guo@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-24128.003 X-TM-AS-Result: No-20.088800-8.000000-10 X-TMASE-MatchedRID: qsaWi0FWcYsOwH4pD14DsPHkpkyUphL9Kx5ICGp/WtF+YesuCgkiXHGv R+V+glJsYE8U75Rh/1peSJEyhb2/xAFZiO4Ps7hJi+quUbDYb+RU+YqtAU+F2VwpnAAvAwaz7fK xaM2xqkBHDd+lAeDDe6v0P2cjKcedUDF47cqDQrUflhDI6DvVlkyQ5fRSh265CqIJhrrDy28RRu BBgUeHeEW6sgxhMI/sdwW4aA3TW+fVFtf7bVfns8u00lnG8+PWdSHY4x0UjQmOQJOkHmszfzZYs sIrYd00GmlyIPiFqiwEgFdNzW3n6W/gIggBOfOXQ1OcCEvT+bfRfRfl2l4F3pKulvdD3z13ymsy D5ZDYiJ6jpxAae9AU4r50uL/CeFYiZqs3zadcCmLzZSKyQypzHnJuTC2P+O+Blt4RZwvTdUoNQx YYLwBgMwBL5qPui48jf93Q249wjWaTrLrmn0xG6wxbZnudyr7GEfoClqBl847XVx9vK07ZfNO7f lRFqXmIGSHKu90xKWSU848M/hs6Me4Woyb+kVFyDp+jSvEtWsl3afZehJEWedlU2K5Jm9bXa822 VCK6b9Xjcu0wLDwhAUIzG/5oa1p/P+NVrYlIEOEMriRd+ARuhwMMtF4a3g9+ZQPGkWiBBejxYyR Ba/qJQPTK4qtAgwIAYt5KiTiutkgBwKKRHe+r+erqLbEpzu4bZgxxGMNa8sl4THdearsrq6zyCg caObIrTabkyv2Rho= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--20.088800-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24128.003 X-MDID: 1538387280-txKrccYfkr74 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2 4/4] vfio: enable vfio hotplug by req notifier handler 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: Mon, 01 Oct 2018 09:48:01 -0000 On 9/30/18 5:16 PM, Jeff Guo wrote: > When device is be hot-unplugged, the vfio kernel module will sent req > notifier to request user space to release the allocated resources at > first. After that, vfio kernel module will detect the device disappear, > and then delete the device in kernel. > > This patch aim to add req notifier processing to enable hotplug for vfio. > By enable the req notifier monitoring and register the notifier callback, > when device be hot-unplugged, the hot-unplug handler will be called to > process hotplug for vfio. > > Signed-off-by: Jeff Guo > --- > v2->v1: > refine some code logic. > --- > drivers/bus/pci/linux/pci_vfio.c | 95 ++++++++++++++++++++++++++++++++++++++++ > drivers/bus/pci/pci_common.c | 10 +++++ > 2 files changed, 105 insertions(+) > > diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c > index 686386d..c780860 100644 > --- a/drivers/bus/pci/linux/pci_vfio.c > +++ b/drivers/bus/pci/linux/pci_vfio.c > @@ -17,6 +17,8 @@ > #include > #include > #include > +#include > +#include > > #include "eal_filesystem.h" > > @@ -277,6 +279,89 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd) > return -1; > } > > +static void > +pci_vfio_req_handler(void *param) > +{ > + struct rte_bus *bus; > + int ret; > + struct rte_device *device = (struct rte_device *)param; > + > + bus = rte_bus_find_by_device(device); > + if (bus == NULL) { > + RTE_LOG(ERR, EAL, "Cannot find bus for device (%s)\n", > + device->name); > + return; > + } > + > + /** Why is doxygen style comment used here? > + * vfio kernel module request user space to release allocated > + * resources before device be deleted in kernel, so it can directly > + * call the vfio bus hot-unplug handler to process it. > + */ > + ret = bus->hot_unplug_handler(device); > + if (ret) > + RTE_LOG(ERR, EAL, "Can not handle hot-unplug for " > + "device (%s)\n", device->name); Consider to avoid format string split to simplify search using grep. > +} > + > +/* enable notifier (only enable req now) */ > +static int > +pci_vfio_enable_notifier(struct rte_pci_device *dev, int vfio_dev_fd) > +{ > + int ret; > + int fd = -1; > + > + /* set up an eventfd for req notifier */ > + 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)); Consider to avoid format string split to simplify search using grep. > + return -1; > + } > + > + dev->req_notifier_handler.fd = fd; > + dev->req_notifier_handler.type = RTE_INTR_HANDLE_VFIO_REQ; > + dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd; > + ret = rte_intr_callback_register(&dev->req_notifier_handler, > + pci_vfio_req_handler, > + (void *)&dev->device); > + if (ret) { > + RTE_LOG(ERR, EAL, "Fail to register req notifier handler.\n"); I think we should close(fd) here. > + return -1; > + } > + > + ret = rte_intr_enable(&dev->req_notifier_handler); > + if (ret) { > + RTE_LOG(ERR, EAL, "Fail to enable req notifier.\n"); I think we should unregister notifier and close(fd) here. > + return -1; > + } > + > + return 0; > +} > + > +/*disable notifier (only disable req now) */ Space is missing before disable. > +static int > +pci_vfio_disable_notifier(struct rte_pci_device *dev) > +{ > + int ret; > + > + ret = rte_intr_disable(&dev->req_notifier_handler); > + if (ret) { > + RTE_LOG(ERR, EAL, "fail to disable req notifier.\n"); I'd like to understand correct way handle errors here. Should we terminate here or continue and unregister handler and close FD anyway? > + return -1; > + } > + > + ret = rte_intr_callback_unregister(&dev->req_notifier_handler, > + pci_vfio_req_handler, > + (void *)&dev->device); > + if (ret) { > + RTE_LOG(ERR, EAL, > + "fail to unregister req notifier handler.\n"); > + return -1; > + } Shoudn't we close eventfd? > + return 0; > +} > + > static int > pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index) > { > @@ -430,6 +515,7 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev) > struct pci_map *maps; > > dev->intr_handle.fd = -1; > + dev->req_notifier_handler.fd = -1; > > /* store PCI address string */ > snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT, > @@ -521,6 +607,11 @@ pci_vfio_map_resource_primary(struct rte_pci_device *dev) > goto err_vfio_res; > } > > + if (pci_vfio_enable_notifier(dev, vfio_dev_fd) != 0) { > + RTE_LOG(ERR, EAL, "Error setting up notifier!\n"); > + return -1; I think we should do goto to make required cleanup. > + } > + > TAILQ_INSERT_TAIL(vfio_res_list, vfio_res, next); > > return 0; > @@ -546,6 +637,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev) > struct pci_map *maps; > > dev->intr_handle.fd = -1; > + dev->req_notifier_handler.fd = -1; > > /* store PCI address string */ > snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT, > @@ -586,6 +678,7 @@ pci_vfio_map_resource_secondary(struct rte_pci_device *dev) > > /* we need save vfio_dev_fd, so it can be used during release */ > dev->intr_handle.vfio_dev_fd = vfio_dev_fd; > + dev->req_notifier_handler.vfio_dev_fd = vfio_dev_fd; > > return 0; > err_vfio_dev_fd: > @@ -658,6 +751,8 @@ pci_vfio_unmap_resource_primary(struct rte_pci_device *dev) > snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT, > loc->domain, loc->bus, loc->devid, loc->function); > > + pci_vfio_disable_notifier(dev); > + Is it OK to ignore disable failure here? Why? It would be good to see explanations in comments. Does it leak eventfd now? > if (close(dev->intr_handle.fd) < 0) { > RTE_LOG(INFO, EAL, "Error when closing eventfd file descriptor for %s\n", > pci_addr); > diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c > index f313fe9..2a8e5e9 100644 > --- a/drivers/bus/pci/pci_common.c > +++ b/drivers/bus/pci/pci_common.c > @@ -446,6 +446,16 @@ pci_hot_unplug_handler(struct rte_device *dev) > return -1; > > switch (pdev->kdrv) { > + case RTE_KDRV_VFIO: > + /** Why is doxygen style comment is used here? > + * vfio kernel module guaranty the pci device would not be > + * deleted until the user space release the resource, so no > + * need to remap BARs resource here, just directly notify > + * the req event to the user space to handle it. > + */ > + rte_dev_event_callback_process(dev->name, > + RTE_DEV_EVENT_REMOVE); > + break; > case RTE_KDRV_IGB_UIO: > case RTE_KDRV_UIO_GENERIC: > case RTE_KDRV_NIC_UIO: