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