From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id BC93EA0518; Sun, 19 Jul 2020 10:47:55 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EE90D1C034; Sun, 19 Jul 2020 10:47:53 +0200 (CEST) Received: from mail-io1-f67.google.com (mail-io1-f67.google.com [209.85.166.67]) by dpdk.org (Postfix) with ESMTP id 26D151BF92 for ; Sun, 19 Jul 2020 10:47:52 +0200 (CEST) Received: by mail-io1-f67.google.com with SMTP id e64so14547987iof.12 for ; Sun, 19 Jul 2020 01:47:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=8Xz7zR9O32U7ehyTO/M+aFF/8jwRSGJ3CKykPD5a14g=; b=lBEDEp7PheYN3R5y5mseNcQd0Yim1pSq39NkgyA/JB/OQpawIGx30LFImkC7bVMIBm 4kPtaWV4bOtQ1wIjkqoYI+VpwjtP0wNIbFSvUrLSO+A4YCC6KtwKVnTnv+12X4AcqQNx UVIhnpCIFeuIVpE0UkRfSEJYKC93DWSv/qXTbXuOPlBG+Jubtz7v2ci+J9CxjJQy6GNs H8oJVtg2aQw2S5NUd7UDDvHMhnBQUqK2JXH9ET7S9Tu5zUzQmobViq7v2OehEPgfPHRv 9HJNHOgyfCyf9IfXr7XpfRgD9nxv5iFe7jAijsxBWv0EiomRTP3ApjB+uG8/aq6mwnt1 pk8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=8Xz7zR9O32U7ehyTO/M+aFF/8jwRSGJ3CKykPD5a14g=; b=Nys+/Xky3/9My1VIB9uVww/mz0OJk5dz4X7d5vXE2zk3J3aGDY1bGYqRzb/DD1vBQD MAwDymxnWmtQiw/tos7Dfm82hyd+u9L8FD9Z5O+hemInh3i3U4/DlVhoTPancjXty+fJ y5Lo/iYcQK4elGtcK54eqVBST1g/38CjSVxPuFQtfvpD+9PaNJfN8dIdviMIEDKHQJ+W pfUqvA9LnXn0Q/hhbMFSOgXQnAAVl98U4f5g1rb0ynZzMcRnH32I6utYFQq4msiTU5J6 tf1E0ORyXPfifBEJfF4EbL6Y8qki/V4Rmw3YrJpL6tnHpusmlsn001KNAJ0g+ZL1w5QK svlA== X-Gm-Message-State: AOAM533/udJlH0vX/DTsu5h7a/jrJPXJsgROx7aHVmwxngUX4Epo98hG W/oO7vW1uhpopdRlyGvsWOilBgryd2b/4Zb/4FE= X-Google-Smtp-Source: ABdhPJxEDUfeU/RMr6bNPWecbJmURl9lT1XHvxWAcV+lQ1CC/1oSZ89e2665yCf05CP8PzT1mAUrm14/heDaTjCBL4Q= X-Received: by 2002:a05:6602:21c7:: with SMTP id c7mr17723892ioc.1.1595148471223; Sun, 19 Jul 2020 01:47:51 -0700 (PDT) MIME-Version: 1.0 References: <20200713151319.17547-2-manishc@marvell.com> <72498440.BHRkrRebfW@thomas> <33229780.9rCRSYNQsn@thomas> <20200716175600.no5svwut34lb2wyr@u256.net> In-Reply-To: From: Jerin Jacob Date: Sun, 19 Jul 2020 14:17:34 +0530 Message-ID: To: Manish Chopra Cc: =?UTF-8?Q?Ga=C3=ABtan_Rivet?= , Ferruh Yigit , Igor Russkikh , dpdk-dev , Thomas Monjalon Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add rte_pci_regs.h X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list 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 Sun, Jul 19, 2020 at 1:12 AM Manish Chopra wrote: > > > -----Original Message----- > > From: dev On Behalf Of Manish Chopra > > Sent: Friday, July 17, 2020 2:19 AM > > To: Ga=C3=ABtan Rivet ; Thomas Monjalon > > > > Cc: Jerin Jacob ; Ferruh Yigit > > ; Igor Russkikh ; dpdk-d= ev > > > > Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/7] lib/librte_pci: add > > rte_pci_regs.h > > > > > -----Original Message----- > > > From: Ga=C3=ABtan Rivet > > > Sent: Thursday, July 16, 2020 11:26 PM > > > To: Thomas Monjalon > > > Cc: Jerin Jacob ; Manish Chopra > > > ; Ferruh Yigit ; Igor > > > Russkikh ; dpdk-dev > > > 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 > > > wrote: > > > > > > > > > > > > 16/07/2020 15:02, Jerin Jacob: > > > > > > > On Thu, Jul 16, 2020 at 6:20 PM Thomas Monjalon > > > wrote: > > > > > > > > > > > > > > > > 16/07/2020 13:55, Jerin Jacob: > > > > > > > > > On Thu, Jul 16, 2020 at 4:57 PM Thomas Monjalon > > > wrote: > > > > > > > > > > > > > > > > > > > > 16/07/2020 12:27, Jerin Jacob: > > > > > > > > > > > On Thu, Jul 16, 2020 at 3:48 PM Ga=C3=ABtan Rivet > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 16/07/20 12:08 +0200, Ga=C3=ABtan 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 he= aders. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 header= s". > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Igor Russkikh > > > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > 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 ad= d > > > > > > > > > > > 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 any= thing. > > > > > > > > > > > > > > > > > > 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 DPD= K. > > > > > > > > > > > > There is already this dependency: > > > > > > #include > > > > > > > > > > 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 t= o > > > Windows. > > > > > > > > > > > > The series is for qede. I'm trying to understand the motivation= . > > > > > > > > > > First version of qede driver sent with defined generic PCI symbol= s > > > > > 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=3Dhttp-3A__patches.dpd= k.o > > > > > rg > > > > > > > > > > _patch_73959_&d=3DDwIDaQ&c=3DnKjWec2b6R0mOyPaz7xtfQ&r=3DbMTgx2X48QVX > > > yXOEL8 > > > > > ALyI4dsWoR-m74c5n3d- > > > ruJI8&m=3DeNuzGYhB7u2Wzru3VeBTY7QDZSSb9VQ9eQXW56D4 > > > > > 64Y&s=3DeatY5xyw-474yS0cBJXyG7gLyPXFo243P2LmBDDsXd8&e=3D > > > > > > > > I agree it's good to have an API for such thing. > > > > > > > > So far such feature is supported in drivers on Linux, requiring onl= y > > > > 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 woul= d > > > make use of it support more than linux currently? > > > > > > -- > > > > I don't know if there are any drivers which will require this other tha= n 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 defi= nes > > available in /usr/include/../pci_regs.h file or not in order to impleme= nt > > 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 inclusion i= n > > 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_ prefixe= d 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. > > Thanks, > Manish