From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v2 05/14] pci: introduce PCI lib and bus Date: Mon, 18 Sep 2017 17:48:59 +0530 Message-ID: <1ed11883-d6bb-05c1-aa14-92046bf8bd49@nxp.com> References: <618e47fbcfbc9ead4c009f0456faab563f22a426.1505726803.git.gaetan.rivet@6wind.com> <88d7466f-8c93-b97c-48d9-fb7bab7e7f77@nxp.com> <20170918115151.GS21444@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Cc: To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= Return-path: Received: from NAM02-CY1-obe.outbound.protection.outlook.com (mail-cys01nam02on0060.outbound.protection.outlook.com [104.47.37.60]) by dpdk.org (Postfix) with ESMTP id D6455592B for ; Mon, 18 Sep 2017 14:08:05 +0200 (CEST) In-Reply-To: <20170918115151.GS21444@bidouze.vm.6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Monday 18 September 2017 05:21 PM, Gaëtan Rivet wrote: > Hey, > > On Mon, Sep 18, 2017 at 05:23:23PM +0530, Shreyansh Jain wrote: >> Hello Gaetan, >> >> On Monday 18 September 2017 03:01 PM, Gaetan Rivet wrote: >>> The PCI lib defines the types and methods allowing to use PCI elements. >>> >>> The PCI bus implements a bus driver for PCI devices by constructing >>> rte_bus elements using the PCI lib. >>> >>> Move the relevant code out of the EAL to their expected place. >>> >>> Signed-off-by: Gaetan Rivet >>> --- >>> config/common_base | 10 + >>> drivers/bus/Makefile | 2 + >>> drivers/bus/pci/Makefile | 59 ++ >>> drivers/bus/pci/bsd/Makefile | 32 ++ >>> drivers/bus/pci/bsd/rte_pci.c | 670 ++++++++++++++++++++++ >>> drivers/bus/pci/include/rte_bus_pci.h | 387 +++++++++++++ >>> drivers/bus/pci/linux/Makefile | 37 ++ >>> drivers/bus/pci/linux/rte_pci.c | 722 ++++++++++++++++++++++++ >>> drivers/bus/pci/linux/rte_pci_init.h | 97 ++++ >>> drivers/bus/pci/linux/rte_pci_uio.c | 567 +++++++++++++++++++ >>> drivers/bus/pci/linux/rte_pci_vfio.c | 674 ++++++++++++++++++++++ >>> drivers/bus/pci/linux/rte_vfio_mp_sync.c | 424 ++++++++++++++ >>> drivers/bus/pci/private.h | 173 ++++++ >>> drivers/bus/pci/rte_bus_pci_version.map | 21 + >>> drivers/bus/pci/rte_pci_common.c | 542 ++++++++++++++++++ >>> drivers/bus/pci/rte_pci_common_uio.c | 234 ++++++++ >>> lib/Makefile | 2 + >>> lib/librte_eal/bsdapp/eal/Makefile | 3 - >>> lib/librte_eal/bsdapp/eal/eal_pci.c | 670 ---------------------- >>> lib/librte_eal/bsdapp/eal/rte_eal_version.map | 15 - >>> lib/librte_eal/common/Makefile | 2 +- >>> lib/librte_eal/common/eal_common_pci.c | 580 ------------------- >>> lib/librte_eal/common/eal_common_pci_uio.c | 233 -------- >>> lib/librte_eal/common/include/rte_pci.h | 598 -------------------- >>> lib/librte_eal/linuxapp/eal/Makefile | 10 - >>> lib/librte_eal/linuxapp/eal/eal_pci.c | 722 ------------------------ >>> lib/librte_eal/linuxapp/eal/eal_pci_init.h | 97 ---- >>> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 567 ------------------- >>> lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 674 ---------------------- >>> lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c | 424 -------------- >>> lib/librte_eal/linuxapp/eal/rte_eal_version.map | 15 - >>> lib/librte_ether/rte_ethdev.h | 2 - >>> lib/librte_pci/Makefile | 48 ++ >>> lib/librte_pci/include/rte_pci.h | 279 +++++++++ >>> lib/librte_pci/rte_pci.c | 92 +++ >>> lib/librte_pci/rte_pci_version.map | 8 + >>> mk/rte.app.mk | 3 + >>> 37 files changed, 5084 insertions(+), 4611 deletions(-) >>> create mode 100644 drivers/bus/pci/Makefile >>> create mode 100644 drivers/bus/pci/bsd/Makefile >>> create mode 100644 drivers/bus/pci/bsd/rte_pci.c >>> create mode 100644 drivers/bus/pci/include/rte_bus_pci.h >>> create mode 100644 drivers/bus/pci/linux/Makefile >>> create mode 100644 drivers/bus/pci/linux/rte_pci.c >>> create mode 100644 drivers/bus/pci/linux/rte_pci_init.h >>> create mode 100644 drivers/bus/pci/linux/rte_pci_uio.c >>> create mode 100644 drivers/bus/pci/linux/rte_pci_vfio.c >>> create mode 100644 drivers/bus/pci/linux/rte_vfio_mp_sync.c >>> create mode 100644 drivers/bus/pci/private.h >>> create mode 100644 drivers/bus/pci/rte_bus_pci_version.map >>> create mode 100644 drivers/bus/pci/rte_pci_common.c >>> create mode 100644 drivers/bus/pci/rte_pci_common_uio.c >>> delete mode 100644 lib/librte_eal/bsdapp/eal/eal_pci.c >>> delete mode 100644 lib/librte_eal/common/eal_common_pci.c >>> delete mode 100644 lib/librte_eal/common/eal_common_pci_uio.c >>> delete mode 100644 lib/librte_eal/common/include/rte_pci.h >>> delete mode 100644 lib/librte_eal/linuxapp/eal/eal_pci.c >>> delete mode 100644 lib/librte_eal/linuxapp/eal/eal_pci_init.h >>> delete mode 100644 lib/librte_eal/linuxapp/eal/eal_pci_uio.c >>> delete mode 100644 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c >>> delete mode 100644 lib/librte_eal/linuxapp/eal/eal_vfio_mp_sync.c >>> create mode 100644 lib/librte_pci/Makefile >>> create mode 100644 lib/librte_pci/include/rte_pci.h >>> create mode 100644 lib/librte_pci/rte_pci.c >>> create mode 100644 lib/librte_pci/rte_pci_version.map >>> >> >> >> >>> +#endif /* _PCI_PRIVATE_H_ */ >>> diff --git a/drivers/bus/pci/rte_bus_pci_version.map b/drivers/bus/pci/rte_bus_pci_version.map >>> new file mode 100644 >>> index 0000000..eca49e9 >>> --- /dev/null >>> +++ b/drivers/bus/pci/rte_bus_pci_version.map >>> @@ -0,0 +1,21 @@ >>> +DPDK_17.08 { >> >> You might want to bump this to 17.11. >> > > Thanks, fixing this. > >>> + global: >>> + >>> + rte_pci_detach; >>> + rte_pci_dump; >>> + rte_pci_ioport_map; >>> + rte_pci_ioport_read; >>> + rte_pci_ioport_unmap; >>> + rte_pci_ioport_write; >>> + rte_pci_map_device; >>> + rte_pci_probe; >>> + rte_pci_probe_one; >>> + rte_pci_read_config; >>> + rte_pci_register; >>> + rte_pci_scan; >>> + rte_pci_unmap_device; >>> + rte_pci_unregister; >>> + rte_pci_write_config; >>> + >>> + local: *; >>> +}; >> >> This is huuuge patch :( and I am not yet through it (most of it is movement >> so I doubt anything major would be problem here). >> Just the above comment in case you are spinning a new series. >> >> > > Thanks for reading the patch. > > Yes, most of it is moving the code as-is to a new location. > I tried to reduce it, but at some point it does not really make sense > anymore. Agree. It is difficult to manage such large movement with a compile-able patch series. I think compilation success takes priority over size in such cases so as not to break bisect. Here, I found that "eal: remove references to PCI" onwards, the compilation was breaking. You have already mentioned about this in the cover letter, it seems. > > I think the important thing to look for here is the build system, > dependency graph and the division of the PCI API between the lib and the > bus driver. Agree. I am trying to look through this. > > I divided it along the lines of the rte_pci_device being defined or not. > Anything using an rte_pci_device going to the bus and everything > else going to the lib. Yes, I agree with this in principle. rte_xxx_device and rte_xxx_driver originate from the xxx_bus. Bus should be responsible for defining (scan) xxx_devices and attaching (probe) to xxx_driver. Which in turn means that all APIs dealing with xxx_device and xxx_driver should be pivoted in the Bus layer. Though, I think here the complexity is also of crypto devices. You have already reached out to Declan through cover letter - probably his (and other crypto experts) opinion would matter here. My opinion would be to have > > I'm mostly worried about this divide. Having the rte_pci_device defined > seems mostly of the responsibility of the bus driver, in my opinion. I'd > like to hear others'. > +1 from side for this principle for divide.