From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH v2 05/14] pci: introduce PCI lib and bus Date: Mon, 18 Sep 2017 13:51:51 +0200 Message-ID: <20170918115151.GS21444@bidouze.vm.6wind.com> References: <618e47fbcfbc9ead4c009f0456faab563f22a426.1505726803.git.gaetan.rivet@6wind.com> <88d7466f-8c93-b97c-48d9-fb7bab7e7f77@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org To: Shreyansh Jain Return-path: Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com [74.125.82.54]) by dpdk.org (Postfix) with ESMTP id D0281592B for ; Mon, 18 Sep 2017 13:52:02 +0200 (CEST) Received: by mail-wm0-f54.google.com with SMTP id 13so1897152wmq.2 for ; Mon, 18 Sep 2017 04:52:02 -0700 (PDT) Content-Disposition: inline In-Reply-To: <88d7466f-8c93-b97c-48d9-fb7bab7e7f77@nxp.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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. 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. 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. 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'. -- Gaëtan Rivet 6WIND