Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h

["Gaëtan Rivet" <grive@u256.net>]

On 19/07/20 14:17 +0530, Jerin Jacob wrote:
> On Sun, Jul 19, 2020 at 1:12 AM Manish Chopra <manishc@marvell.com> wrote:
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Manish Chopra
> > > Sent: Friday, July 17, 2020 2:19 AM
> > > To: Gaëtan Rivet <grive@u256.net>; Thomas Monjalon
> > > <thomas@monjalon.net>
> > > Cc: Jerin Jacob <jerinjacobk@gmail.com>; Ferruh Yigit
> > > <ferruh.yigit@intel.com>; Igor Russkikh <irusskikh@marvell.com>; dpdk-dev
> > > <dev@dpdk.org>
> > > Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add
> > > rte_pci_regs.h
> > >
> > > > -----Original Message-----
> > > > From: Gaëtan Rivet <grive@u256.net>
> > > > Sent: Thursday, July 16, 2020 11:26 PM
> > > > To: Thomas Monjalon <thomas@monjalon.net>
> > > > Cc: Jerin Jacob <jerinjacobk@gmail.com>; Manish Chopra
> > > > <manishc@marvell.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Igor
> > > > Russkikh <irusskikh@marvell.com>; dpdk-dev <dev@dpdk.org>
> > > > Subject: [EXT] Re: [dpdk-dev] [PATCH v2 1/7] lib/librte_pci: add
> > > > rte_pci_regs.h
> > > >
> > > > External Email
> > > >
> > > > ----------------------------------------------------------------------
> > > > On 16/07/20 18:57 +0200, Thomas Monjalon wrote:
> > > > > 16/07/2020 18:43, Jerin Jacob:
> > > > > > On Thu, Jul 16, 2020 at 9:25 PM Thomas Monjalon
> > > > <thomas@monjalon.net> wrote:
> > > > > > >
> > > > > > > 16/07/2020 15:02, Jerin Jacob:
> > > > > > > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon
> > > > <thomas@monjalon.net> wrote:
> > > > > > > > >
> > > > > > > > > 16/07/2020 13:55, Jerin Jacob:
> > > > > > > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon
> > > > <thomas@monjalon.net> wrote:
> > > > > > > > > > >
> > > > > > > > > > > 16/07/2020 12:27, Jerin Jacob:
> > > > > > > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Gaëtan Rivet
> > > > <grive@u256.net> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > On 16/07/20 12:08 +0200, Gaëtan Rivet wrote:
> > > > > > > > > > > > > > Re-CCing dev@dpdk.org as it was removed from the
> > > reply.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On 13/07/20 08:13 -0700, Manish Chopra wrote:
> > > > > > > > > > > > > > > This is merely copy of latest linux/pci_regs.h
> > > > > > > > > > > > > > > in order to avoid dependency of dpdk on user headers.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I guess this dependency is an issue on non-linux
> > > > > > > > > > > > > > systems, when you must use those defines in a
> > > > > > > > > > > > > > generic implementation. Can you confirm this is
> > > > > > > > > > > > > > the
> > > > motivation here?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > If so, I think it would be clearer to state "in
> > > > > > > > > > > > > > order to avoid dependency of DPDK on linux headers".
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > To add to it, if this is actually the motivation to
> > > > > > > > > > > > > add this header, I don't think it is sufficient.
> > > > > > > > > > > > >
> > > > > > > > > > > > > You can restrict the function definition to the
> > > > > > > > > > > > > linux part of the PCI bus driver instead, using
> > > > > > > > > > > > > stubs for other
> > > > systems.
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > Signed-off-by: Manish Chopra
> > > > > > > > > > > > > > > <manishc@marvell.com>
> > > > > > > > > > > > > > > Signed-off-by: Igor Russkikh
> > > > > > > > > > > > > > > <irusskikh@marvell.com>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >  drivers/bus/pci/linux/pci_uio.c     |    2 +-
> > > > > > > > > > > > > > >  drivers/bus/pci/linux/pci_vfio.c    |    2 +-
> > > > > > > > > > > > > > >  drivers/net/bnx2x/bnx2x.h           |    2 +-
> > > > > > > > > > > > > > >  drivers/net/hns3/hns3_ethdev_vf.c   |    2 +-
> > > > > > > > > > > > > > >  drivers/vdpa/ifc/base/ifcvf_osdep.h |    2 +-
> > > > > > > > > > > > > > >  lib/librte_pci/Makefile             |    1 +
> > > > > > > > > > > > > > >  lib/librte_pci/meson.build          |    2 +-
> > > > > > > > > > > > > > >  lib/librte_pci/rte_pci_regs.h       | 1075
> > > > +++++++++++++++++++++++++++
> > > > > > > > > > > > > > >  8 files changed, 1082 insertions(+), 6
> > > > > > > > > > > > > > > deletions(-)  create mode 100644
> > > > > > > > > > > > > > > lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > [...]
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > diff --git a/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > > > b/lib/librte_pci/rte_pci_regs.h new file mode
> > > > > > > > > > > > > > > 100644 index 000000000..1d11f4de5
> > > > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > > > +++ b/lib/librte_pci/rte_pci_regs.h
> > > > > > > > > > > > > > > @@ -0,0 +1,1075 @@
> > > > > > > > > > > > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH
> > > > > > > > > > > > > > > +Linux-syscall-note */
> > > > > > > > > > > > > > > +/*
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This file is delivered alongside the PCI lib,
> > > > > > > > > > > > > > targeting
> > > > userspace.
> > > > > > > > > > > > > > This seems to be an exception to the license
> > > > > > > > > > > > > > policy described in license/README. Code shared
> > > > > > > > > > > > > > between kernel and userspace is expected to be
> > > > > > > > > > > > > > dual-licensed BSD-3
> > > > and GPL-2.0.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > As it is a copy of Linux user includes,
> > > > > > > > > > > > > > re-licensing it as BSD-3 as well is not possible.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So I think it might require a techboard +
> > > > > > > > > > > > > > governing board exception approval. Ferruh or
> > > > > > > > > > > > > > Thomas, what do you
> > > > think?
> > > > > > > > > > > >
> > > > > > > > > > > > I think, instead of importing GPL-2.0 file, We can add
> > > > > > > > > > > > the constants as need by the DPDK as symbols start
> > > > > > > > > > > > from RTE_PCI_*(It will fix up the namespace as well).
> > > > > > > > > > >
> > > > > > > > > > > If symbols can be found in /usr/include/, don't add anything.
> > > > > > > > > >
> > > > > > > > > > Not by default on all the distros. It is part of pciutils library.
> > > > > > > > > > Moreover, we need these symbols for Windows OS as well.
> > > > > > > > > > IMO, We should add absolute minimum constants that needed
> > > > > > > > > > for DPDK as RTE_PCI_*
> > > > > > > > >
> > > > > > > > > I am for mandating the dependency instead of copying it.
> > > > > > > >
> > > > > > > > You mean _pciutils_ package as a mandatory dependency to  DPDK.
> > > > > > >
> > > > > > > There is already this dependency:
> > > > > > >         #include <linux/pci_regs.h>
> > > > > >
> > > > > > I just checked in archlinux, PCI headers can be provided by
> > > > > >
> > > > > > # pacman -F /usr/include/pci/header.h usr/include/pci/header.h is
> > > > > > owned by core/pciutils 3.7.0-
> > > > > >
> > > > > > # pacman -F /usr/include/linux/pci.h usr/include/linux/pci.h is
> > > > > > owned by core/linux-api-headers 5.4.17-1
> > > > > >
> > > > > >
> > > > > > > I'm missing the real justification for this patch.
> > > > > >
> > > > > > See below.
> > > > > >
> > > > > > > Is there some missing definitions?
> > > > > > > Is there some environments where this file is missing?
> > > > > > >
> > > > > > > > > pciutils cannot be installed on Windows?
> > > > > > > > > Why do you care about Windows?
> > > > > > > > > I don't see any contribution for qede on Windows.
> > > > > > > >
> > > > > > > > You closely review the patch, it not about qede. The proposed
> > > > > > > > file comes at lib/librte_pci/rte_pci_regs.h which is common to
> > > > Windows.
> > > > > > >
> > > > > > > The series is for qede. I'm trying to understand the motivation.
> > > > > >
> > > > > > First version of qede driver sent with defined generic PCI symbols
> > > > > > and generic PCI function like pci_find_next_ext_capability() in
> > > > > > qede
> > > > driver.
> > > > >
> > > > > That's a pity the v2 is not threaded with v1, I would have found
> > > > > these explanations easily myself.
> > > > >
> > > > > > In the review, I suggested using generic rte_ function as
> > > > > > a) It is not specific to qede.
> > > > > > b) Other drivers also doing the same thing in their own driver
> > > > > > space as there is no dpdk API for the same.
> > > > > > This patches create generic API for pci_find_next_ext_capability()
> > > > > > and remove duplicate implementation from the drivers.
> > > > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patches.dpdk.o
> > > > > > rg
> > > > > >
> > > >
> > > _patch_73959_&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=bMTgx2X48QVX
> > > > yXOEL8
> > > > > > ALyI4dsWoR-m74c5n3d-
> > > > ruJI8&m=eNuzGYhB7u2Wzru3VeBTY7QDZSSb9VQ9eQXW56D4
> > > > > > 64Y&s=eatY5xyw-474yS0cBJXyG7gLyPXFo243P2LmBDDsXd8&e=
> > > > >
> > > > > I agree it's good to have an API for such thing.
> > > > >
> > > > > So far such feature is supported in drivers on Linux, requiring only
> > > > > Linux headers to be installed.
> > > > > Do we need more?
> > > > >
> > > > >
> > > >
> > > > +1 to make it generic, no question here.
> > > >
> > > > On linux, the dependency is already there (either from linux headers
> > > > or
> > > > pciutils) to have the original. So including this header in DPDK is
> > > > only useful for other OSes.
> > > >
> > > > I think right now we should only add pci_find_next_ext_capability()
> > > > full implementation within linux part of PCI bus, other systems being
> > > stubs.
> > > >
> > > > We can go with your suggestion Jerin about adding only the specific
> > > > symbols needed, prefixed with RTE_, once we decide to have windows
> > > support.
> > > > Question is whether we need it right now. Is there a driver that would
> > > > make use of it support more than linux currently?
> > > >
> > > > --
> > >
> > > I don't know if there are any drivers which will require this other than linux
> > > as of today -
> > >
> > > My only motivation of adding these symbols in dpdk via rte_pci_regs.h (new
> > > file in lib/librte_pci/) was to avoid any dependency of dpdk on
> > > /usr/include/../pci_regs.h, since I was little unsure whether in all
> > > distributions (linux/windows) supported will have the required PCI defines
> > > available in /usr/include/../pci_regs.h file or not in order to implement
> > > rte_pci_find_next_ext_capability().  (unless user to bound for updating
> > > headers by mean of installing any latest _pciutils_/packages). Moreover, for
> > > not just this API, but if going forward if we have to add any new APIs which
> > > could rely/depend on PCI defines availability under /usr/include.
> > >
> > > From the discussion so far -
> > >
> > > 1. Define the function under drivers/bus/pci/linux/pci.c only and add
> > > empty/stub implementation for windows/pci.c and bsd/pci.c ?
> > > 2.  Just relying on /usr/include/ is perfectly okay without adding any defines
> > > anywhere for now ?, it will just require <linux/pci_regs.h> inclusion in
> > >      drivers/bus/pci/linux/pci.c. OR Shall I add (may be in
> > > lib/librte_pci/rte_pci.h ?) only required PCI defines with RTE_ prefixed and
> > > use them instead ?
> > >
> >
> > Hello Gaetan/Jerin,
> >
> > Could you please comment on above - what's sufficient to be incorporated in v3 for now ?
> > I will work on the changes accordingly.
> 
> I am for introducing a handful(Not the completed list, Only add what
> is needed for DPDK) of PCI constants as RTE_PCI_ in our public API.
> And implement rte_pci_find_next_ext_capability() as common routine as
> it is not specific to Linux.(I.e reading the config registers should
> be only Linux/Windows-specific)
> 
> I leave the final decision to PCI and other maintainers.

Hi Jerin, Manish,

There is currently no PCI bus maintainer, but most people in the project
are interested in it working properly :).

I think it's a good principle to try to limit as much as possible the
amount of system-specific code. rte_pci_read_config() seems like a fair
cut-off point.

Given than the current PMD implementations were doing exactly what you
describe, I think it's better to go for it currently, even if no one is
using those symbols outside linux. So +1 from me regarding what you
propose.

-- 
Gaëtan