Re: [PATCH v2 05/14] pci: introduce PCI lib and bus
From: Shreyansh Jain <hidden>
Date: 2017-09-18 12:08:05
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:quoted
Hello Gaetan, On Monday 18 September 2017 03:01 PM, Gaetan Rivet wrote:quoted
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 <redacted> --- 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<lot of snip here...>quoted
+#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.quoted
quoted
+ 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.