From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH v3 6/6] DPDK changes for accommodating ENIC PMD Date: Mon, 24 Nov 2014 06:33:15 -0500 Message-ID: <20141124113315.GA6038@hmsreliant.think-freely.org> References: <1416758899-1351-1-git-send-email-ssujith@cisco.com> <1416758899-1351-7-git-send-email-ssujith@cisco.com> <20141124001744.GA7751@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: "dev-VfR2kkLFssw@public.gmane.org" , "Prasad Rao \(prrao\)" To: "Sujith Sankar (ssujith)" Return-path: Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" On Mon, Nov 24, 2014 at 05:45:54AM +0000, Sujith Sankar (ssujith) wrote: >=20 >=20 > On 24/11/14 5:47 am, "Neil Horman" wrote: >=20 > >On Sun, Nov 23, 2014 at 09:38:19PM +0530, Sujith Sankar wrote: > >> Signed-off-by: Sujith Sankar > >> --- > >> 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(+) > >>=20 > >> 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=3D4 > >> CONFIG_RTE_LIBRTE_I40E_ITR_INTERVAL=3D-1 > >> =20 > >> # > >> +# Compile burst-oriented Cisco ENIC PMD driver > >> +# > >> +CONFIG_RTE_LIBRTE_ENIC_PMD=3Dy > >> + > >> +# > >> # Compile burst-oriented VIRTIO PMD driver > >> # > >> CONFIG_RTE_LIBRTE_VIRTIO_PMD=3Dy > >> 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) +=3D librte_cmdl= ine > >> DIRS-$(CONFIG_RTE_LIBRTE_ETHER) +=3D librte_ether > >> DIRS-$(CONFIG_RTE_LIBRTE_E1000_PMD) +=3D librte_pmd_e1000 > >> DIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) +=3D librte_pmd_ixgbe > >> +DIRS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) +=3D librte_pmd_enic > >> DIRS-$(CONFIG_RTE_LIBRTE_I40E_PMD) +=3D librte_pmd_i40e > >> DIRS-$(CONFIG_RTE_LIBRTE_PMD_BOND) +=3D librte_pmd_bond > >> DIRS-$(CONFIG_RTE_LIBRTE_PMD_RING) +=3D 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 =3D reg.offset; > >> maps[i].size =3D reg.size; > >> dev->mem_resource[i].addr =3D bar_addr; > >> + dev->mem_resource[i].len =3D reg.size; > >> } > >> =20 > >> /* 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 i= t > >earlier > >in the series, as you call this function two patches back. >=20 > Thanks for the comment, Neil. I shall move this to a separate patch an= d > put it earlier in the series. >=20 > > > >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 >=20 >=20 > 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=B9s just adding an entry, an= d > 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. >=20 > 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); > } >=20 >=20 >=20 > Does this look alright? Do you think that I=B9ve missed out anything h= ere? >=20 It looks better yes, but I'm still confused as to why its necessecary. L= ooking back at your use of the fd, you're adding an iov entry for a buffer alloc= ated via rte_memzone_reserve, which should come out of the dpdk's configured m= emory. In pci_vfio_setup_dma_maps, which is part of the pci probe path in eal li= brary, all of the DPDK's physically available memory is already mapped into the = iommu in a 1:1 fashion. So why do you need to do this again? Neil