From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 806C65AB9 for ; Fri, 23 Jan 2015 03:54:25 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 22 Jan 2015 18:48:35 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,453,1418112000"; d="scan'208";a="655248699" Received: from pgsmsx102.gar.corp.intel.com ([10.221.44.80]) by fmsmga001.fm.intel.com with ESMTP; 22 Jan 2015 18:54:22 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.110.15) by PGSMSX102.gar.corp.intel.com (10.221.44.80) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 23 Jan 2015 10:54:03 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.64]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.231]) with mapi id 14.03.0195.001; Fri, 23 Jan 2015 10:54:02 +0800 From: "Qiu, Michael" To: Tetsuya Mukawa Thread-Topic: [dpdk-dev] [PATCH v4 06/11] eal/linux/pci: Add functions for unmapping igb_uio resources Thread-Index: AQHQNWFAXN6+QKM0HkSGXiCxS7ko7w== Date: Fri, 23 Jan 2015 02:54:01 +0000 Message-ID: <533710CFB86FA344BFBF2D6802E60286CB8B2A@SHSMSX101.ccr.corp.intel.com> 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> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 02:54:26 -0000 On 1/22/2015 6:16 PM, Tetsuya Mukawa wrote:=0A= > Hi Michael,=0A= >=0A= > On 2015/01/22 17:12, Qiu, Michael wrote:=0A= >> On 1/21/2015 6:01 PM, Tetsuya Mukawa wrote:=0A= >>> Hi Michael,=0A= >>>=0A= >>> On 2015/01/20 18:23, Qiu, Michael wrote:=0A= >>>> On 1/19/2015 6:42 PM, Tetsuya Mukawa wrote:=0A= >>>>> The patch adds functions for unmapping igb_uio resources. The patch i= s only=0A= >>>>> for Linux and igb_uio environment. VFIO and BSD are not supported.=0A= >>>>>=0A= >>>>> v4:=0A= >>>>> - Add paramerter checking.=0A= >>>>> - Add header file to determine if hotplug can be enabled.=0A= >>>>>=0A= >>>>> Signed-off-by: Tetsuya Mukawa =0A= >>>>> ---=0A= >>>>> lib/librte_eal/common/Makefile | 1 +=0A= >>>>> lib/librte_eal/common/include/rte_dev_hotplug.h | 44 +++++++++++++++= ++=0A= >>>>> lib/librte_eal/linuxapp/eal/eal_pci.c | 38 +++++++++++++++= =0A= >>>>> lib/librte_eal/linuxapp/eal/eal_pci_init.h | 8 +++=0A= >>>>> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 65 +++++++++++++++= ++++++++++=0A= >>>>> 5 files changed, 156 insertions(+)=0A= >>>>> create mode 100644 lib/librte_eal/common/include/rte_dev_hotplug.h= =0A= >>>>>=0A= >>>>> diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/M= akefile=0A= >>>>> index 52c1a5f..db7cc93 100644=0A= >>>>> --- a/lib/librte_eal/common/Makefile=0A= >>>>> +++ b/lib/librte_eal/common/Makefile=0A= >>>>> @@ -41,6 +41,7 @@ INC +=3D rte_eal_memconfig.h rte_malloc_heap.h=0A= >>>>> INC +=3D rte_hexdump.h rte_devargs.h rte_dev.h=0A= >>>>> INC +=3D rte_common_vect.h=0A= >>>>> INC +=3D rte_pci_dev_feature_defs.h rte_pci_dev_features.h=0A= >>>>> +INC +=3D rte_dev_hotplug.h=0A= >>>>> =0A= >>>>> ifeq ($(CONFIG_RTE_INSECURE_FUNCTION_WARNING),y)=0A= >>>>> INC +=3D rte_warnings.h=0A= >>>>> diff --git a/lib/librte_eal/common/include/rte_dev_hotplug.h b/lib/li= brte_eal/common/include/rte_dev_hotplug.h=0A= >>>>> new file mode 100644=0A= >>>>> index 0000000..b333e0f=0A= >>>>> --- /dev/null=0A= >>>>> +++ b/lib/librte_eal/common/include/rte_dev_hotplug.h=0A= >>>>> @@ -0,0 +1,44 @@=0A= >>>>> +/*-=0A= >>>>> + * BSD LICENSE=0A= >>>>> + *=0A= >>>>> + * Copyright(c) 2015 IGEL Co.,LTd.=0A= >>>>> + * All rights reserved.=0A= >>>>> + *=0A= >>>>> + * Redistribution and use in source and binary forms, with or with= out=0A= >>>>> + * modification, are permitted provided that the following conditi= ons=0A= >>>>> + * are met:=0A= >>>>> + *=0A= >>>>> + * * Redistributions of source code must retain the above copyri= ght=0A= >>>>> + * notice, this list of conditions and the following disclaime= r.=0A= >>>>> + * * Redistributions in binary form must reproduce the above cop= yright=0A= >>>>> + * notice, this list of conditions and the following disclaime= r in=0A= >>>>> + * the documentation and/or other materials provided with the= =0A= >>>>> + * distribution.=0A= >>>>> + * * Neither the name of IGEL Co.,Ltd. nor the names of its=0A= >>>>> + * contributors may be used to endorse or promote products der= ived=0A= >>>>> + * from this software without specific prior written permissio= n.=0A= >>>>> + *=0A= >>>>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBU= TORS=0A= >>>>> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT N= OT=0A= >>>>> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNE= SS FOR=0A= >>>>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPY= RIGHT=0A= >>>>> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCID= ENTAL,=0A= >>>>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NO= T=0A= >>>>> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS O= F USE,=0A= >>>>> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND = ON ANY=0A= >>>>> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR = TORT=0A= >>>>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF T= HE USE=0A= >>>>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DA= MAGE.=0A= >>>>> + */=0A= >>>>> +=0A= >>>>> +#ifndef _RTE_DEV_HOTPLUG_H_=0A= >>>>> +#define _RTE_DEV_HOTPLUG_H_=0A= >>>>> +=0A= >>>>> +/*=0A= >>>>> + * determine if hotplug can be enabled on the system=0A= >>>>> + */=0A= >>>>> +#if defined(RTE_LIBRTE_EAL_HOTPLUG) && defined(RTE_LIBRTE_EAL_LINUXA= PP)=0A= >>>> As you said, VFIO should not work with it, so does it need to add the= =0A= >>>> vfio check here?=0A= >>> Could I have a advice of you?=0A= >>> First I guess it's the best to include "eal_vfio.h" here, and add=0A= >>> checking of VFIO_PRESENT macro.=0A= >> I have a question, will your hotplug feature support freebsd ?=0A= >>=0A= >> If not, how about to put it in "lib/librte_eal/linuxapp/eal/" ? Also = =0A= >> include attach or detach affairs.=0A= > I appreciate your comments.=0A= >=0A= > So far, FreeBSD doesn't support PCI hotplug. So I didn't implement code= =0A= > for FreeBSD.=0A= > But in the future, I want to implement it when FreeBSD supports it.=0A= > Also my hotplug implementation depends on legacy code already=0A= > implemented in common layer.=0A= > Anyway, for me it's nice to implement the feature in common layer.=0A= >=0A= >>> But it seems I cannot reach "eal_vfio.h" from this file.=0A= >> Yes, you can't :)=0A= >>=0A= >>> My second option is just checking RTE_EAL_VFIO macro.=0A= >>> But according to "eal_vfio.h", if kernel is under 3.6.0, VFIO_PRESENT= =0A= >> Actually, in my opinion, whatever vfio or uio, only need be care in=0A= >> runtime.=0A= >>=0A= >> DPDK to check vfio only to add support for vfio, but this does not=0A= >> means the device will use vfio,=0A= >>=0A= >> So even if VFIO_PRESENT is defined, and vfio is enabled, but the device= =0A= >> is bind to igb_uio, then your hotplug still need work, but if it bind= =0A= >> to vfio, will not, am I right?=0A= >>=0A= >> If yes, I'm not sure if your hotplug has this ability, but it is=0A= >> reasonable, I think.=0A= > I agree with your concept. But I guess it's not only related with=0A= > hotplug function.=0A= > The hotplug implementation depends on legacy functions that is for=0A= > probing device.=0A= > To implement above concept will change not only hotplug behavior but=0A= > also legacy device probing.=0A= >=0A= > Conceptually I agree with such a functionality, but legacy probing=0A= > function doesn't have such a feature so far.=0A= > So I guess it's nice to separate this feature from hotplug patches.=0A= > Realistically, the hotplug patches are big, and it's a bit hard to add=0A= > and manage one more feature.=0A= > If it's ok to separate the patch, it's helpful to manage patches.=0A= >=0A= >>> will not be defined even when RTE_EAL_VFIO is enabled.=0A= >>> So I guess simply macro checking will not work correctly.=0A= >>> =0A= >>> Anyway, here are my implementation choices so far.=0A= >>>=0A= >>> 1. Like "eal_vfio.h", check kernel version in "rte_dev_hotplug.h".=0A= >>> In this case, if "eal_vfio.h" is changed, "rte_edv_hotplug.h" may need= =0A= >>> to be changed also.=0A= >>>=0A= >>> 2. Merge "eal_vfio.h" and "rte_dev_hotplug.h" definitions, and define= =0A= >>> these in new rte header like "rte_settings.h".=0A= >>>=0A= >>> Can I have advice about it?=0A= >> As I said, VFIO enable or not is not related for your hotplug, only the= =0A= >> devices managed by VFIO will affect your hotplug.=0A= >>=0A= >> If you agree, I think need discuss the details of it.=0A= > Yes, I agree with your concept.=0A= > And if you agree, I will implement it separately.=0A= >=0A= > To discuss how to handle VFIO and igb_uio devices in parallel, I guess=0A= > we need to=0A= > think about generic uio driver for pci devices.=0A= > I guess before applying uio generic patch, this kind of discussion will= =0A= > be needed.=0A= > I hope igb_uio (and VFIO?) be obsolete after applying uio generic.=0A= =0A= No, igb_uio is similar to uio generic, but VFIO is another totally=0A= different way of UIO.=0A= =0A= So applying uio generic has no affect for VFIO=0A= =0A= > In the case, we don't need to think it.=0A= >=0A= > BTW, back to 'rte_dev_hotplug.h' discussion of hotplug patch.=0A= > If VFIO_PRESENT isn't checked here, attaching port will be successful=0A= > even if the device is under VFIO.=0A= > (Because we already has legacy code for probing VFIO device. The hotplug= =0A= > function just re-use it.)=0A= > But we cannot detach the device, because there is no code for closing=0A= > VFIO device.=0A= > There is no crash when the VFIO device is detached, but some error=0A= > messages will be displayed.=0A= > So I guess this behavior is like your description.=0A= > How about it?=0A= =0A= Actually, what I'm considering is to add one flag like "int pt_mod" in=0A= "struct rte_pci_device", and give the value in pci scan stage, then it=0A= will reduce lots of work in EAL of VFIO checking, like PCI memory=0A= mapping stage, hotplug and so on.=0A= For hotplug, it can check the flag at runtime. And when hotplug supports=0A= VFIO, you can simply ignore this flag.=0A= =0A= Do you think it is valuable to do so?=0A= =0A= If yes I think I will make a patch for that, and send to you, you can=0A= post your patch set with that, or if you want implement it yourself, let=0A= me know.=0A= =0A= Thanks,=0A= Michael=0A= > Thanks,=0A= > Tetsuya=0A= >=0A= >> Thanks,=0A= >> Michael=0A= >>> Thanks,=0A= >>> Tetsuya=0A= >>>=0A= >>>> Thanks,=0A= >>>> Michael=0A= >>>>> +#define ENABLE_HOTPLUG=0A= >>>>> +#endif /* RTE_LIBRTE_EAL_HOTPLUG & RTE_LIBRTE_EAL_LINUXAPP */=0A= >>>>> +=0A= >>>>> +#endif /* _RTE_DEV_HOTPLUG_H_ */=0A= >>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/l= inuxapp/eal/eal_pci.c=0A= >>>>> index 3d2d93c..52c464c 100644=0A= >>>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c=0A= >>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c=0A= >>>>> @@ -137,6 +137,25 @@ pci_map_resource(void *requested_addr, int fd, o= ff_t offset, size_t size)=0A= >>>>> return mapaddr;=0A= >>>>> }=0A= >>>>> =0A= >>>>> +#ifdef ENABLE_HOTPLUG=0A= >>>>> +/* unmap a particular resource */=0A= >>>>> +void=0A= >>>>> +pci_unmap_resource(void *requested_addr, size_t size)=0A= >>>>> +{=0A= >>>>> + if (requested_addr =3D=3D NULL)=0A= >>>>> + return;=0A= >>>>> +=0A= >>>>> + /* Unmap the PCI memory resource of device */=0A= >>>>> + if (munmap(requested_addr, size)) {=0A= >>>>> + RTE_LOG(ERR, EAL, "%s(): cannot munmap(%p, 0x%lx): %s\n",=0A= >>>>> + __func__, requested_addr, (unsigned long)size,=0A= >>>>> + strerror(errno));=0A= >>>>> + } else=0A= >>>>> + RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n",=0A= >>>>> + requested_addr);=0A= >>>>> +}=0A= >>>>> +#endif /* ENABLE_HOTPLUG */=0A= >>>>> +=0A= >>>>> /* parse the "resource" sysfs file */=0A= >>>>> #define IORESOURCE_MEM 0x00000200=0A= >>>>> =0A= >>>>> @@ -510,6 +529,25 @@ pci_map_device(struct rte_pci_device *dev)=0A= >>>>> return 0;=0A= >>>>> }=0A= >>>>> =0A= >>>>> +#ifdef ENABLE_HOTPLUG=0A= >>>>> +static void=0A= >>>>> +pci_unmap_device(struct rte_pci_device *dev)=0A= >>>>> +{=0A= >>>>> + if (dev =3D=3D NULL)=0A= >>>>> + return;=0A= >>>>> +=0A= >>>>> + /* try unmapping the NIC resources using VFIO if it exists */=0A= >>>>> +#ifdef VFIO_PRESENT=0A= >>>>> + if (pci_vfio_is_enabled()) {=0A= >>>>> + RTE_LOG(ERR, EAL, "%s() doesn't support vfio yet.\n",=0A= >>>>> + __func__);=0A= >>>>> + return;=0A= >>>>> + }=0A= >>>>> +#endif=0A= >>>>> + pci_uio_unmap_resource(dev);=0A= >>>>> +}=0A= >>>>> +#endif /* ENABLE_HOTPLUG */=0A= >>>>> +=0A= >>>>> /*=0A= >>>>> * If vendor/device ID match, call the devinit() function of the=0A= >>>>> * driver.=0A= >>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_init.h b/lib/librte_= eal/linuxapp/eal/eal_pci_init.h=0A= >>>>> index 1070eb8..5152a0b 100644=0A= >>>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_init.h=0A= >>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_init.h=0A= >>>>> @@ -34,6 +34,7 @@=0A= >>>>> #ifndef EAL_PCI_INIT_H_=0A= >>>>> #define EAL_PCI_INIT_H_=0A= >>>>> =0A= >>>>> +#include =0A= >>>>> #include "eal_vfio.h"=0A= >>>>> =0A= >>>>> struct pci_map {=0A= >>>>> @@ -71,6 +72,13 @@ void *pci_map_resource(void *requested_addr, int f= d, off_t offset,=0A= >>>>> /* map IGB_UIO resource prototype */=0A= >>>>> int pci_uio_map_resource(struct rte_pci_device *dev);=0A= >>>>> =0A= >>>>> +#ifdef ENABLE_HOTPLUG=0A= >>>>> +void pci_unmap_resource(void *requested_addr, size_t size);=0A= >>>>> +=0A= >>>>> +/* unmap IGB_UIO resource prototype */=0A= >>>>> +void pci_uio_unmap_resource(struct rte_pci_device *dev);=0A= >>>>> +#endif /* ENABLE_HOTPLUG */=0A= >>>>> +=0A= >>>>> #ifdef VFIO_PRESENT=0A= >>>>> =0A= >>>>> #define VFIO_MAX_GROUPS 64=0A= >>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_e= al/linuxapp/eal/eal_pci_uio.c=0A= >>>>> index 1da3507..81830d1 100644=0A= >>>>> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c=0A= >>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c=0A= >>>>> @@ -404,6 +404,71 @@ pci_uio_map_resource(struct rte_pci_device *dev)= =0A= >>>>> return 0;=0A= >>>>> }=0A= >>>>> =0A= >>>>> +#ifdef ENABLE_HOTPLUG=0A= >>>>> +static void=0A= >>>>> +pci_uio_unmap(struct mapped_pci_resource *uio_res)=0A= >>>>> +{=0A= >>>>> + int i;=0A= >>>>> +=0A= >>>>> + if (uio_res =3D=3D NULL)=0A= >>>>> + return;=0A= >>>>> +=0A= >>>>> + for (i =3D 0; i !=3D uio_res->nb_maps; i++)=0A= >>>>> + pci_unmap_resource(uio_res->maps[i].addr,=0A= >>>>> + (size_t)uio_res->maps[i].size);=0A= >>>>> +}=0A= >>>>> +=0A= >>>>> +static struct mapped_pci_resource *=0A= >>>>> +pci_uio_find_resource(struct rte_pci_device *dev)=0A= >>>>> +{=0A= >>>>> + struct mapped_pci_resource *uio_res;=0A= >>>>> +=0A= >>>>> + if (dev =3D=3D NULL)=0A= >>>>> + return NULL;=0A= >>>>> +=0A= >>>>> + TAILQ_FOREACH(uio_res, pci_res_list, next) {=0A= >>>>> +=0A= >>>>> + /* skip this element if it doesn't match our PCI address */=0A= >>>>> + if (!eal_compare_pci_addr(&uio_res->pci_addr, &dev->addr))=0A= >>>>> + return uio_res;=0A= >>>>> + }=0A= >>>>> + return NULL;=0A= >>>>> +}=0A= >>>>> +=0A= >>>>> +/* unmap the PCI resource of a PCI device in virtual memory */=0A= >>>>> +void=0A= >>>>> +pci_uio_unmap_resource(struct rte_pci_device *dev)=0A= >>>>> +{=0A= >>>>> + struct mapped_pci_resource *uio_res;=0A= >>>>> +=0A= >>>>> + if (dev =3D=3D NULL)=0A= >>>>> + return;=0A= >>>>> +=0A= >>>>> + /* find an entry for the device */=0A= >>>>> + uio_res =3D pci_uio_find_resource(dev);=0A= >>>>> + if (uio_res =3D=3D NULL)=0A= >>>>> + return;=0A= >>>>> +=0A= >>>>> + /* secondary processes - just free maps */=0A= >>>>> + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY)=0A= >>>>> + return pci_uio_unmap(uio_res);=0A= >>>>> +=0A= >>>>> + TAILQ_REMOVE(pci_res_list, uio_res, next);=0A= >>>>> +=0A= >>>>> + /* unmap all resources */=0A= >>>>> + pci_uio_unmap(uio_res);=0A= >>>>> +=0A= >>>>> + /* free uio resource */=0A= >>>>> + rte_free(uio_res);=0A= >>>>> +=0A= >>>>> + /* close fd if in primary process */=0A= >>>>> + close(dev->intr_handle.fd);=0A= >>>>> +=0A= >>>>> + dev->intr_handle.fd =3D -1;=0A= >>>>> + dev->intr_handle.type =3D RTE_INTR_HANDLE_UNKNOWN;=0A= >>>>> +}=0A= >>>>> +#endif /* ENABLE_HOTPLUG */=0A= >>>>> +=0A= >>>>> /*=0A= >>>>> * parse a sysfs file containing one integer value=0A= >>>>> * different to the eal version, as it needs to work with 64-bit val= ues=0A= >=0A= >=0A= =0A=