Re: [dpdk-dev] [PATCH v3 6/6] DPDK changes for accommodating ENIC PMD

[Neil Horman <nhorman@tuxdriver.com>]

On Sun, Nov 23, 2014 at 09:38:19PM +0530, Sujith Sankar wrote:
> Signed-off-by: Sujith Sankar <ssujith@cisco.com>
> ---
>  config/common_linuxapp                             | 5 +++++
>  lib/Makefile                                       | 1 +
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c         | 7 +++++++
>  lib/librte_eal/linuxapp/eal/include/eal_pci_init.h | 1 +
>  mk/rte.app.mk                                      | 4 ++++
>  5 files changed, 18 insertions(+)
> 
> diff --git a/config/common_linuxapp b/config/common_linuxapp
> index 57b61c9..3c091e7 100644
> --- a/config/common_linuxapp
> +++ b/config/common_linuxapp
> @@ -210,6 +210,11 @@ CONFIG_RTE_LIBRTE_I40E_QUEUE_NUM_PER_VM=4
>  CONFIG_RTE_LIBRTE_I40E_ITR_INTERVAL=-1
>  
>  #
> +# Compile burst-oriented Cisco ENIC PMD driver
> +#
> +CONFIG_RTE_LIBRTE_ENIC_PMD=y
> +
> +#
>  # Compile burst-oriented VIRTIO PMD driver
>  #
>  CONFIG_RTE_LIBRTE_VIRTIO_PMD=y
> diff --git a/lib/Makefile b/lib/Makefile
> index e3237ff..1911790 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -43,6 +43,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += librte_cmdline
>  DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ether
>  DIRS-$(CONFIG_RTE_LIBRTE_E1000_PMD) += librte_pmd_e1000
>  DIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += librte_pmd_ixgbe
> +DIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += librte_pmd_enic
>  DIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += librte_pmd_i40e
>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_BOND) += librte_pmd_bond
>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_RING) += librte_pmd_ring
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index c776ddc..6bf8f2e 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -736,6 +736,7 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
>  		maps[i].offset = reg.offset;
>  		maps[i].size = reg.size;
>  		dev->mem_resource[i].addr = bar_addr;
> +		dev->mem_resource[i].len = reg.size;
>  	}
>  
>  	/* if secondary process, do not set up interrupts */
> @@ -791,4 +792,10 @@ pci_vfio_is_enabled(void)
>  {
>  	return vfio_cfg.vfio_enabled;
>  }
> +
> +int
> +pci_vfio_container_fd(void)
> +{
> +	return vfio_cfg.vfio_container_fd;
> +}
You should move this function definition to a separate patch and put it earlier
in the series, as you call this function two patches back.

Also, this gives me pause, as it seems like you're working around the VFIO api.
>From what I see, you just use this to get an fd that you can use in an ioctl to
set some DMA settings.  First off, theres already a function called
pci_vfio_get_container_fd, which does exactly what you are doing here, with
additional safety checking.  

However, even though there is an existing function to do what you want, I would
recommend that you not use it for your purposes.  Whenever you expose something
like a file descriptor, you run the risk of multiple accessors racing trying to
set/unset features and preform operations.  It would be better if you could add
apropriate api calls to vfio interface to set what you want.  That way the
library can add appropriate locking if/when needed

Neil

>  #endif
> diff --git a/lib/librte_eal/linuxapp/eal/include/eal_pci_init.h b/lib/librte_eal/linuxapp/eal/include/eal_pci_init.h
> index d758bee..c9e9e40 100644
> --- a/lib/librte_eal/linuxapp/eal/include/eal_pci_init.h
> +++ b/lib/librte_eal/linuxapp/eal/include/eal_pci_init.h
> @@ -71,6 +71,7 @@ int pci_uio_map_resource(struct rte_pci_device *dev);
>  
>  int pci_vfio_enable(void);
>  int pci_vfio_is_enabled(void);
> +int pci_vfio_container_fd(void);
>  int pci_vfio_mp_sync_setup(void);
>  
>  /* map VFIO resource prototype */
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index 285b65c..95c652f 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -186,6 +186,10 @@ ifeq ($(CONFIG_RTE_LIBRTE_VMXNET3_PMD),y)
>  LDLIBS += -lrte_pmd_vmxnet3_uio
>  endif
>  
> +ifeq ($(CONFIG_RTE_LIBRTE_ENIC_PMD),y)
> +LDLIBS += -lrte_pmd_enic
> +endif
> +
>  ifeq ($(CONFIG_RTE_LIBRTE_VIRTIO_PMD),y)
>  LDLIBS += -lrte_pmd_virtio_uio
>  endif
> -- 
> 1.9.1
> 
>