From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f173.google.com (mail-pd0-f173.google.com [209.85.192.173]) by dpdk.org (Postfix) with ESMTP id C901F5AB5 for ; Fri, 23 Jan 2015 04:21:02 +0100 (CET) Received: by mail-pd0-f173.google.com with SMTP id fp1so5851677pdb.4 for ; Thu, 22 Jan 2015 19:21:02 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=7Sdclyn2eAm7Q0ErUzMIzPy56mHuhpnEkWwKdNCL/fQ=; b=DWzIm7YMGKzKeq1pnmRk0k3EKjv6O7+8NS45O02caYCiMMulPQoJZZDXojSZTfFRfk +QJuc21/cC6dUK+JElbftmxcRPn6nlcnfmq9k6DcROtg+qvHq3nj+cyu+Fvwz5rWgj66 u8MKDIVEx3Q3qxhfpNxQD15kD8XwtaUpY1fUfcyp98Znbx/FCa5g9EC5XeA30fpmSmg0 X9NVCZgTQq62ypkEj4ylyQZX5/BfPGmcKvH7EyhM4zOX5Nw7hlBdrMjB9+SEXv5cZgM8 +B4AdrQ2d1BZ3iGX0W7KDkT0jVybrFPJa+949U3AFc5KGSVLekPpm56eufASuiNPM/1O TEbQ== X-Gm-Message-State: ALoCoQkNtiJ7zSWwWSpwBurLdgDUiInxKqgPZ3riw/AGlyRAvgMXtiYbxfjTop6qGMY9FRYyYqbs X-Received: by 10.69.31.33 with SMTP id kj1mr7555896pbd.43.1421983261958; Thu, 22 Jan 2015 19:21:01 -0800 (PST) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id g11sm249154pat.24.2015.01.22.19.20.59 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Jan 2015 19:21:01 -0800 (PST) Message-ID: <54C1BE1A.30201@igel.co.jp> Date: Fri, 23 Jan 2015 12:20:58 +0900 From: Tetsuya Mukawa User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: "Qiu, Michael" References: <1418106629-22227-2-git-send-email-mukawa@igel.co.j> <1421664027-17971-1-git-send-email-mukawa@igel.co.jp> <1421664027-17971-7-git-send-email-mukawa@igel.co.jp> <533710CFB86FA344BFBF2D6802E60286CB7D11@SHSMSX101.ccr.corp.intel.com> <54BF78EF.7060304@igel.co.jp> <533710CFB86FA344BFBF2D6802E60286CB882E@SHSMSX101.ccr.corp.intel.com> <54C0CDDE.8030805@igel.co.jp> <533710CFB86FA344BFBF2D6802E60286CB8B2A@SHSMSX101.ccr.corp.intel.com> In-Reply-To: <533710CFB86FA344BFBF2D6802E60286CB8B2A@SHSMSX101.ccr.corp.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v4 06/11] eal/linux/pci: Add functions for unmapping igb_uio resources X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Jan 2015 03:21:03 -0000 Hi Michael, On 2015/01/23 11:54, Qiu, Michael wrote: > On 1/22/2015 6:16 PM, Tetsuya Mukawa wrote: >> Hi Michael, >> >> On 2015/01/22 17:12, Qiu, Michael wrote: >>> On 1/21/2015 6:01 PM, Tetsuya Mukawa wrote: >>>> Hi Michael, >>>> >>>> On 2015/01/20 18:23, Qiu, Michael wrote: >>>>> On 1/19/2015 6:42 PM, Tetsuya Mukawa wrote: >>>>>> The patch adds functions for unmapping igb_uio resources. The patc= h is only >>>>>> for Linux and igb_uio environment. VFIO and BSD are not supported.= >>>>>> >>>>>> v4: >>>>>> - Add paramerter checking. >>>>>> - Add header file to determine if hotplug can be enabled. >>>>>> >>>>>> Signed-off-by: Tetsuya Mukawa >>>>>> --- >>>>>> lib/librte_eal/common/Makefile | 1 + >>>>>> lib/librte_eal/common/include/rte_dev_hotplug.h | 44 ++++++++++++= +++++ >>>>>> lib/librte_eal/linuxapp/eal/eal_pci.c | 38 ++++++++++++= +++ >>>>>> lib/librte_eal/linuxapp/eal/eal_pci_init.h | 8 +++ >>>>>> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 65 ++++++++++++= +++++++++++++ >>>>>> 5 files changed, 156 insertions(+) >>>>>> create mode 100644 lib/librte_eal/common/include/rte_dev_hotplug.= h >>>>>> >>>>>> diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/commo= n/Makefile >>>>>> index 52c1a5f..db7cc93 100644 >>>>>> --- a/lib/librte_eal/common/Makefile >>>>>> +++ b/lib/librte_eal/common/Makefile >>>>>> @@ -41,6 +41,7 @@ INC +=3D rte_eal_memconfig.h rte_malloc_heap.h >>>>>> INC +=3D rte_hexdump.h rte_devargs.h rte_dev.h >>>>>> INC +=3D rte_common_vect.h >>>>>> INC +=3D rte_pci_dev_feature_defs.h rte_pci_dev_features.h >>>>>> +INC +=3D rte_dev_hotplug.h >>>>>> =20 >>>>>> ifeq ($(CONFIG_RTE_INSECURE_FUNCTION_WARNING),y) >>>>>> INC +=3D rte_warnings.h >>>>>> diff --git a/lib/librte_eal/common/include/rte_dev_hotplug.h b/lib= /librte_eal/common/include/rte_dev_hotplug.h >>>>>> new file mode 100644 >>>>>> index 0000000..b333e0f >>>>>> --- /dev/null >>>>>> +++ b/lib/librte_eal/common/include/rte_dev_hotplug.h >>>>>> @@ -0,0 +1,44 @@ >>>>>> +/*- >>>>>> + * BSD LICENSE >>>>>> + * >>>>>> + * Copyright(c) 2015 IGEL Co.,LTd. >>>>>> + * All rights reserved. >>>>>> + * >>>>>> + * Redistribution and use in source and binary forms, with or w= ithout >>>>>> + * modification, are permitted provided that the following cond= itions >>>>>> + * are met: >>>>>> + * >>>>>> + * * Redistributions of source code must retain the above cop= yright >>>>>> + * notice, this list of conditions and the following discla= imer. >>>>>> + * * Redistributions in binary form must reproduce the above = copyright >>>>>> + * notice, this list of conditions and the following discla= imer in >>>>>> + * the documentation and/or other materials provided with t= he >>>>>> + * distribution. >>>>>> + * * Neither the name of IGEL Co.,Ltd. nor the names of its >>>>>> + * contributors may be used to endorse or promote products = derived >>>>>> + * from this software without specific prior written permis= sion. >>>>>> + * >>>>>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTR= IBUTORS >>>>>> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BU= T NOT >>>>>> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FI= TNESS FOR >>>>>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE C= OPYRIGHT >>>>>> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, IN= CIDENTAL, >>>>>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT= NOT >>>>>> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOS= S OF USE, >>>>>> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED A= ND ON ANY >>>>>> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, = OR TORT >>>>>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT O= F THE USE >>>>>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH= DAMAGE. >>>>>> + */ >>>>>> + >>>>>> +#ifndef _RTE_DEV_HOTPLUG_H_ >>>>>> +#define _RTE_DEV_HOTPLUG_H_ >>>>>> + >>>>>> +/* >>>>>> + * determine if hotplug can be enabled on the system >>>>>> + */ >>>>>> +#if defined(RTE_LIBRTE_EAL_HOTPLUG) && defined(RTE_LIBRTE_EAL_LIN= UXAPP) >>>>> As you said, VFIO should not work with it, so does it need to add t= he >>>>> vfio check here? >>>> Could I have a advice of you? >>>> First I guess it's the best to include "eal_vfio.h" here, and add >>>> checking of VFIO_PRESENT macro. >>> I have a question, will your hotplug feature support freebsd ? >>> >>> If not, how about to put it in "lib/librte_eal/linuxapp/eal/" ? Also= =20 >>> include attach or detach affairs. >> I appreciate your comments. >> >> So far, FreeBSD doesn't support PCI hotplug. So I didn't implement cod= e >> for FreeBSD. >> But in the future, I want to implement it when FreeBSD supports it. >> Also my hotplug implementation depends on legacy code already >> implemented in common layer. >> Anyway, for me it's nice to implement the feature in common layer. >> >>>> But it seems I cannot reach "eal_vfio.h" from this file. >>> Yes, you can't :) >>> >>>> My second option is just checking RTE_EAL_VFIO macro. >>>> But according to "eal_vfio.h", if kernel is under 3.6.0, VFIO_PRESEN= T >>> Actually, in my opinion, whatever vfio or uio, only need be care in >>> runtime. >>> >>> DPDK to check vfio only to add support for vfio, but this does not >>> means the device will use vfio, >>> >>> So even if VFIO_PRESENT is defined, and vfio is enabled, but the devi= ce >>> is bind to igb_uio, then your hotplug still need work, but if it bin= d >>> to vfio, will not, am I right? >>> >>> If yes, I'm not sure if your hotplug has this ability, but it is >>> reasonable, I think. >> I agree with your concept. But I guess it's not only related with >> hotplug function. >> The hotplug implementation depends on legacy functions that is for >> probing device. >> To implement above concept will change not only hotplug behavior but >> also legacy device probing. >> >> Conceptually I agree with such a functionality, but legacy probing >> function doesn't have such a feature so far. >> So I guess it's nice to separate this feature from hotplug patches. >> Realistically, the hotplug patches are big, and it's a bit hard to add= >> and manage one more feature. >> If it's ok to separate the patch, it's helpful to manage patches. >> >>>> will not be defined even when RTE_EAL_VFIO is enabled. >>>> So I guess simply macro checking will not work correctly. >>>> =20 >>>> Anyway, here are my implementation choices so far. >>>> >>>> 1. Like "eal_vfio.h", check kernel version in "rte_dev_hotplug.h". >>>> In this case, if "eal_vfio.h" is changed, "rte_edv_hotplug.h" may ne= ed >>>> to be changed also. >>>> >>>> 2. Merge "eal_vfio.h" and "rte_dev_hotplug.h" definitions, and defin= e >>>> these in new rte header like "rte_settings.h". >>>> >>>> Can I have advice about it? >>> As I said, VFIO enable or not is not related for your hotplug, only t= he >>> devices managed by VFIO will affect your hotplug. >>> >>> If you agree, I think need discuss the details of it. >> Yes, I agree with your concept. >> And if you agree, I will implement it separately. >> >> To discuss how to handle VFIO and igb_uio devices in parallel, I guess= >> we need to >> think about generic uio driver for pci devices. >> I guess before applying uio generic patch, this kind of discussion wil= l >> be needed. >> I hope igb_uio (and VFIO?) be obsolete after applying uio generic. > No, igb_uio is similar to uio generic, but VFIO is another totally > different way of UIO. > > So applying uio generic has no affect for VFIO I appreciate your comments. OK, I understand the relation of VFIO and uio generic. > >> In the case, we don't need to think it. >> >> BTW, back to 'rte_dev_hotplug.h' discussion of hotplug patch. >> If VFIO_PRESENT isn't checked here, attaching port will be successful >> even if the device is under VFIO. >> (Because we already has legacy code for probing VFIO device. The hotpl= ug >> function just re-use it.) >> But we cannot detach the device, because there is no code for closing >> VFIO device. >> There is no crash when the VFIO device is detached, but some error >> messages will be displayed. >> So I guess this behavior is like your description. >> How about it? > Actually, what I'm considering is to add one flag like "int pt_mod" in > "struct rte_pci_device", and give the value in pci scan stage, then it > will reduce lots of work in EAL of VFIO checking, like PCI memory > mapping stage, hotplug and so on. > For hotplug, it can check the flag at runtime. And when hotplug support= s > VFIO, you can simply ignore this flag. > > Do you think it is valuable to do so? > > If yes I think I will make a patch for that, and send to you, you can > post your patch set with that, or if you want implement it yourself, le= t > me know. I agree with you. And thank you so much. Could you please send a patch? I will put it on the top of my patches. (If you have favorite where you want to put it on, could you please let me know?) Thanks, Tetsuya > Thanks, > Michael >> Thanks, >> Tetsuya >> >>> Thanks, >>> Michael >>>> Thanks, >>>> Tetsuya >>>> >>>>> Thanks, >>>>> Michael >>>>>> +#define ENABLE_HOTPLUG >>>>>> +#endif /* RTE_LIBRTE_EAL_HOTPLUG & RTE_LIBRTE_EAL_LINUXAPP */ >>>>>> + >>>>>> +#endif /* _RTE_DEV_HOTPLUG_H_ */ >>>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_ea= l/linuxapp/eal/eal_pci.c >>>>>> index 3d2d93c..52c464c 100644 >>>>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c >>>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c >>>>>> @@ -137,6 +137,25 @@ pci_map_resource(void *requested_addr, int fd= , off_t offset, size_t size) >>>>>> return mapaddr; >>>>>> } >>>>>> =20 >>>>>> +#ifdef ENABLE_HOTPLUG >>>>>> +/* unmap a particular resource */ >>>>>> +void >>>>>> +pci_unmap_resource(void *requested_addr, size_t size) >>>>>> +{ >>>>>> + if (requested_addr =3D=3D NULL) >>>>>> + return; >>>>>> + >>>>>> + /* Unmap the PCI memory resource of device */ >>>>>> + if (munmap(requested_addr, size)) { >>>>>> + RTE_LOG(ERR, EAL, "%s(): cannot munmap(%p, 0x%lx): %s\n", >>>>>> + __func__, requested_addr, (unsigned long)size, >>>>>> + strerror(errno)); >>>>>> + } else >>>>>> + RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", >>>>>> + requested_addr); >>>>>> +} >>>>>> +#endif /* ENABLE_HOTPLUG */ >>>>>> + >>>>>> /* parse the "resource" sysfs file */ >>>>>> #define IORESOURCE_MEM 0x00000200 >>>>>> =20 >>>>>> @@ -510,6 +529,25 @@ pci_map_device(struct rte_pci_device *dev) >>>>>> return 0; >>>>>> } >>>>>> =20 >>>>>> +#ifdef ENABLE_HOTPLUG >>>>>> +static void >>>>>> +pci_unmap_device(struct rte_pci_device *dev) >>>>>> +{ >>>>>> + if (dev =3D=3D NULL) >>>>>> + return; >>>>>> + >>>>>> + /* try unmapping the NIC resources using VFIO if it exists */ >>>>>> +#ifdef VFIO_PRESENT >>>>>> + if (pci_vfio_is_enabled()) { >>>>>> + RTE_LOG(ERR, EAL, "%s() doesn't support vfio yet.\n", >>>>>> + __func__); >>>>>> + return; >>>>>> + } >>>>>> +#endif >>>>>> + pci_uio_unmap_resource(dev); >>>>>> +} >>>>>> +#endif /* ENABLE_HOTPLUG */ >>>>>> + >>>>>> /* >>>>>> * If vendor/device ID match, call the devinit() function of the >>>>>> * driver. >>>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_init.h b/lib/libr= te_eal/linuxapp/eal/eal_pci_init.h >>>>>> index 1070eb8..5152a0b 100644 >>>>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_init.h >>>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_init.h >>>>>> @@ -34,6 +34,7 @@ >>>>>> #ifndef EAL_PCI_INIT_H_ >>>>>> #define EAL_PCI_INIT_H_ >>>>>> =20 >>>>>> +#include >>>>>> #include "eal_vfio.h" >>>>>> =20 >>>>>> struct pci_map { >>>>>> @@ -71,6 +72,13 @@ void *pci_map_resource(void *requested_addr, in= t fd, off_t offset, >>>>>> /* map IGB_UIO resource prototype */ >>>>>> int pci_uio_map_resource(struct rte_pci_device *dev); >>>>>> =20 >>>>>> +#ifdef ENABLE_HOTPLUG >>>>>> +void pci_unmap_resource(void *requested_addr, size_t size); >>>>>> + >>>>>> +/* unmap IGB_UIO resource prototype */ >>>>>> +void pci_uio_unmap_resource(struct rte_pci_device *dev); >>>>>> +#endif /* ENABLE_HOTPLUG */ >>>>>> + >>>>>> #ifdef VFIO_PRESENT >>>>>> =20 >>>>>> #define VFIO_MAX_GROUPS 64 >>>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librt= e_eal/linuxapp/eal/eal_pci_uio.c >>>>>> index 1da3507..81830d1 100644 >>>>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c >>>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c >>>>>> @@ -404,6 +404,71 @@ pci_uio_map_resource(struct rte_pci_device *d= ev) >>>>>> return 0; >>>>>> } >>>>>> =20 >>>>>> +#ifdef ENABLE_HOTPLUG >>>>>> +static void >>>>>> +pci_uio_unmap(struct mapped_pci_resource *uio_res) >>>>>> +{ >>>>>> + int i; >>>>>> + >>>>>> + if (uio_res =3D=3D NULL) >>>>>> + return; >>>>>> + >>>>>> + for (i =3D 0; i !=3D uio_res->nb_maps; i++) >>>>>> + pci_unmap_resource(uio_res->maps[i].addr, >>>>>> + (size_t)uio_res->maps[i].size); >>>>>> +} >>>>>> + >>>>>> +static struct mapped_pci_resource * >>>>>> +pci_uio_find_resource(struct rte_pci_device *dev) >>>>>> +{ >>>>>> + struct mapped_pci_resource *uio_res; >>>>>> + >>>>>> + if (dev =3D=3D NULL) >>>>>> + return NULL; >>>>>> + >>>>>> + TAILQ_FOREACH(uio_res, pci_res_list, next) { >>>>>> + >>>>>> + /* skip this element if it doesn't match our PCI address */ >>>>>> + if (!eal_compare_pci_addr(&uio_res->pci_addr, &dev->addr)) >>>>>> + return uio_res; >>>>>> + } >>>>>> + return NULL; >>>>>> +} >>>>>> + >>>>>> +/* unmap the PCI resource of a PCI device in virtual memory */ >>>>>> +void >>>>>> +pci_uio_unmap_resource(struct rte_pci_device *dev) >>>>>> +{ >>>>>> + struct mapped_pci_resource *uio_res; >>>>>> + >>>>>> + if (dev =3D=3D NULL) >>>>>> + return; >>>>>> + >>>>>> + /* find an entry for the device */ >>>>>> + uio_res =3D pci_uio_find_resource(dev); >>>>>> + if (uio_res =3D=3D NULL) >>>>>> + return; >>>>>> + >>>>>> + /* secondary processes - just free maps */ >>>>>> + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) >>>>>> + return pci_uio_unmap(uio_res); >>>>>> + >>>>>> + TAILQ_REMOVE(pci_res_list, uio_res, next); >>>>>> + >>>>>> + /* unmap all resources */ >>>>>> + pci_uio_unmap(uio_res); >>>>>> + >>>>>> + /* free uio resource */ >>>>>> + rte_free(uio_res); >>>>>> + >>>>>> + /* close fd if in primary process */ >>>>>> + close(dev->intr_handle.fd); >>>>>> + >>>>>> + dev->intr_handle.fd =3D -1; >>>>>> + dev->intr_handle.type =3D RTE_INTR_HANDLE_UNKNOWN; >>>>>> +} >>>>>> +#endif /* ENABLE_HOTPLUG */ >>>>>> + >>>>>> /* >>>>>> * parse a sysfs file containing one integer value >>>>>> * different to the eal version, as it needs to work with 64-bit = values >>