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

[Tetsuya Mukawa <mukawa@igel.co.jp>]

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 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

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 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.

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_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
>>