Re: [dpdk-dev] [PATCH v4 06/11] eal/linux/pci: Add functions for unmapping igb_uio resources

["Qiu, Michael" <michael.qiu@intel.com>]

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 patch 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 <mukawa@igel.co.jp>
>>>>> ---
>>>>>  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/common/Makefile
>>>>> index 52c1a5f..db7cc93 100644
>>>>> --- a/lib/librte_eal/common/Makefile
>>>>> +++ b/lib/librte_eal/common/Makefile
>>>>> @@ -41,6 +41,7 @@ INC += rte_eal_memconfig.h rte_malloc_heap.h
>>>>>  INC += rte_hexdump.h rte_devargs.h rte_dev.h
>>>>>  INC += rte_common_vect.h
>>>>>  INC += rte_pci_dev_feature_defs.h rte_pci_dev_features.h
>>>>> +INC += rte_dev_hotplug.h
>>>>>  
>>>>>  ifeq ($(CONFIG_RTE_INSECURE_FUNCTION_WARNING),y)
>>>>>  INC += 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 without
>>>>> + *   modification, are permitted provided that the following conditions
>>>>> + *   are met:
>>>>> + *
>>>>> + *     * Redistributions of source code must retain the above copyright
>>>>> + *       notice, this list of conditions and the following disclaimer.
>>>>> + *     * Redistributions in binary form must reproduce the above copyright
>>>>> + *       notice, this list of conditions and the following disclaimer in
>>>>> + *       the documentation and/or other materials provided with the
>>>>> + *       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 permission.
>>>>> + *
>>>>> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>>>>> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>>>>> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>>>>> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>>>>> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>>>>> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>>>>> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>>>>> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>>>>> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>>>>> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF 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_LINUXAPP)
>>>> As you said, VFIO should not work with it, so does it need to add the
>>>> 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 
>> include attach or detach affairs.
> I appreciate your comments.
>
> So far, FreeBSD doesn't support PCI hotplug. So I didn't implement code
> 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_PRESENT
>> 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 device
>> is bind to igb_uio, then your hotplug still  need work, but if it bind
>> 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.
>>>  
>>> 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 need
>>> to be changed also.
>>>
>>> 2. Merge "eal_vfio.h" and "rte_dev_hotplug.h" definitions, and define
>>> 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 the
>> 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 will
> 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

> 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 hotplug
> 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 supports
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, let
me know.

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_eal/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;
>>>>>  }
>>>>>  
>>>>> +#ifdef ENABLE_HOTPLUG
>>>>> +/* unmap a particular resource */
>>>>> +void
>>>>> +pci_unmap_resource(void *requested_addr, size_t size)
>>>>> +{
>>>>> +	if (requested_addr == 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
>>>>>  
>>>>> @@ -510,6 +529,25 @@ pci_map_device(struct rte_pci_device *dev)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +#ifdef ENABLE_HOTPLUG
>>>>> +static void
>>>>> +pci_unmap_device(struct rte_pci_device *dev)
>>>>> +{
>>>>> +	if (dev == 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/librte_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_
>>>>>  
>>>>> +#include <rte_dev_hotplug.h>
>>>>>  #include "eal_vfio.h"
>>>>>  
>>>>>  struct pci_map {
>>>>> @@ -71,6 +72,13 @@ void *pci_map_resource(void *requested_addr, int fd, off_t offset,
>>>>>  /* map IGB_UIO resource prototype */
>>>>>  int pci_uio_map_resource(struct rte_pci_device *dev);
>>>>>  
>>>>> +#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
>>>>>  
>>>>>  #define VFIO_MAX_GROUPS 64
>>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_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 *dev)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +#ifdef ENABLE_HOTPLUG
>>>>> +static void
>>>>> +pci_uio_unmap(struct mapped_pci_resource *uio_res)
>>>>> +{
>>>>> +	int i;
>>>>> +
>>>>> +	if (uio_res == NULL)
>>>>> +		return;
>>>>> +
>>>>> +	for (i = 0; i != 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 == 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 == NULL)
>>>>> +		return;
>>>>> +
>>>>> +	/* find an entry for the device */
>>>>> +	uio_res = pci_uio_find_resource(dev);
>>>>> +	if (uio_res == NULL)
>>>>> +		return;
>>>>> +
>>>>> +	/* secondary processes - just free maps */
>>>>> +	if (rte_eal_process_type() != 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 = -1;
>>>>> +	dev->intr_handle.type = 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
>
>