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

["Sujith Sankar (ssujith)" <ssujith@cisco.com>]

On 24/11/14 5:47 am, "Neil Horman" <nhorman@tuxdriver.com> wrote:

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

Thanks for the comment, Neil.  I shall move this to a separate patch and
put it earlier in the series.

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


I see that vfio_cfg.vfio_container_fd is obtained and stored in
pci_vfio_enable(), and this is not modified later.
ENIC PMD needs it to add the IOMMU mapping for buffers used for
communicating with adapter firmware.  That¹s just adding an entry, and
container fd is just passed as an argument.  So the following addition in
eal_pci_vfio.c should be sufficient.  Since vfio_cfg is per process, I do
not think that any other checking is required.

int
pci_vfio_map_dma(struct vfio_iommu_type1_dma_map *dma_map)
{
	return ioctl(vfio_cfg.vfio_container_fd, VFIO_IOMMU_MAP_DMA, dma_map);
}



Does this look alright?  Do you think that I¹ve missed out anything here?

Thanks,
-Sujith


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