From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility Date: Wed, 25 May 2016 21:39:43 +0200 Message-ID: <1507554.Pcj3Og5Zqt@xps13> References: <1463431287-4551-1-git-send-email-nhorman@tuxdriver.com> <1710741.QL28xgSMua@xps13> <20160525191345.GF14322@hmsreliant.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org, Bruce Richardson , Stephen Hemminger , Panu Matilainen To: Neil Horman Return-path: Received: from mail-lf0-f48.google.com (mail-lf0-f48.google.com [209.85.215.48]) by dpdk.org (Postfix) with ESMTP id 048D62BE1 for ; Wed, 25 May 2016 21:39:46 +0200 (CEST) Received: by mail-lf0-f48.google.com with SMTP id k98so22703346lfi.1 for ; Wed, 25 May 2016 12:39:45 -0700 (PDT) In-Reply-To: <20160525191345.GF14322@hmsreliant.think-freely.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 2016-05-25 15:13, Neil Horman: > On Wed, May 25, 2016 at 07:39:30PM +0200, Thomas Monjalon wrote: > > 2016-05-25 13:22, Neil Horman: > > > On Wed, May 25, 2016 at 03:21:19PM +0200, Thomas Monjalon wrote: > > > > 2016-05-24 15:41, Neil Horman: > > > > > +include $(RTE_SDK)/mk/rte.buildtools.mk > > > > > > > > Why a new Makefile? Can you use rte.hostapp.mk? > > > > > > > I don't know, maybe. Nothing else currently uses rte.hostapp.mk, so I missed > > > its existance. I make the argument that, that being the case, we should stick > > > with the Makefile I just tested with, and remove the rte.hostapp.mk file > > > > No, rte.hostapp.mk has been used and tested in the history of the project. > > Please try it. > > > It works, but its really ugly (as it means that the buildtools directory gets > install to the hostapp directory under the build). I could move that of course, > but at this point, you are asking me to remove a working makefile to replace it > with another makefile that, by all rights should have been removed as part of > commit efa2084a840fb83fd9be83adca57e5f23d3fa9fe: > Author: Thomas Monjalon > Date: Tue Mar 10 17:55:25 2015 +0100 > > scripts: remove useless build tools > > test-framework.sh is an old script to check building of some dependencies. > testhost is an old app used to check HOSTCC. > > Let's clean the scripts directory. > > Here you removed the only user of rte.hostapp.mk, but neglected to remove > hostapp.mk itself. Yes. I didn't really neglect to remove it. I thought it would be used later. > I really fail to see why making me rework my current > makefile setup, that matches the purpose of the tool is a superior solution to > just getting rid of the unused makefile thats there right now. I'm just trying to avoid creating a new makefile for each tool. Is it possible to fix the directory in rte.hostapp.mk? Every apps use the same makefile rte.app.mk. I think it should be the same for host apps. > > > > > +++ b/buildtools/pmdinfogen/pmdinfogen.c > > > > [...] > > > > > + /* > > > > > + * If this returns NULL, then this is a PMD_VDEV, because > > > > > + * it has no pci table reference > > > > > + */ > > > > > > > > We can imagine physical PMD not using PCI. > > > > I think this comment should be removed. > > > We can, but currently its a true statement. we have two types of PMDs, a PDEV > > > and a VDEV, the former is a pci device, and the latter is a virtual device, so > > > you can imply the PDEV type from the presence of pci entries, and VDEV from the > > > alternative. If we were to do something, I would recommend adding a macro to > > > explicitly ennumerate each pmds type. I would prefer to wait until that was a > > > need however, as it can be done invisibly to the user. > > > > We are removing the PMD types in the EAL rework. > > So this comment will be outdated. Better to remove now. > > > Then, I'm just not going to enumerate the type of driver at all, I'll remove > that attribute entirely. OK > But I really don't like to write code for things that are 'predictive'. Not really predictive as it is an older patch. > > > > [...] > > > > > + fprintf(ofd,"\\\"type\\\" : \\\"%s\\\", ", drv->pci_tbl ? "PMD_PDEV" : "PMD_VDEV"); > > > > > > > > Please forget the naming PDEV/VDEV. > > > > > > > I don't know what you mean here, you would rather they be named PCI and Virtual, > > > or something else? > > > > Yes please. > > > No, If you're removing the types, and you're sure of that, I'm just going to > remove the description entirely. If you're unsure about exactly whats going to > happen, we should reflect the state of the build now, and make the appropriate > change when it lands. OK to remove the type description. > > > > > +++ b/buildtools/pmdinfogen/pmdinfogen.h > > > > [...] > > > > > +#define Elf_Ehdr Elf64_Ehdr > > > > > +#define Elf_Shdr Elf64_Shdr > > > > > +#define Elf_Sym Elf64_Sym > > > > > +#define Elf_Addr Elf64_Addr > > > > > +#define Elf_Sword Elf64_Sxword > > > > > +#define Elf_Section Elf64_Half > > > > > +#define ELF_ST_BIND ELF64_ST_BIND > > > > > +#define ELF_ST_TYPE ELF64_ST_TYPE > > > > > + > > > > > +#define Elf_Rel Elf64_Rel > > > > > +#define Elf_Rela Elf64_Rela > > > > > +#define ELF_R_SYM ELF64_R_SYM > > > > > +#define ELF_R_TYPE ELF64_R_TYPE > > > > > > > > Why these defines are needed? > > > > > > > Because I borrowed the code from modpost.c, which allows for both ELF32 and > > > ELF64 compilation. I wanted to keep it in place should DPDK ever target > > > different sized architectures. > > > > Maybe a comment is needed. > > Is ELF32 used on 32-bit archs like i686 or ARMv7? > It depends on exactly how its built, but that would be a common use, yes. We have such 32-bit archs in DPDK. Is pmdinfogen working for them? > > > > > +struct rte_pci_id { > > > > > + uint16_t vendor_id; /**< Vendor ID or PCI_ANY_ID. */ > > > > > + uint16_t device_id; /**< Device ID or PCI_ANY_ID. */ > > > > > + uint16_t subsystem_vendor_id; /**< Subsystem vendor ID or PCI_ANY_ID. */ > > > > > + uint16_t subsystem_device_id; /**< Subsystem device ID or PCI_ANY_ID. */ > > > > > +}; > > > > [...] > > > > > +struct pmd_driver { > > > > > + Elf_Sym *name_sym; > > > > > + const char *name; > > > > > + struct rte_pci_id *pci_tbl; > > > > > + struct pmd_driver *next; > > > > > + > > > > > + const char* opt_vals[PMD_OPT_MAX]; > > > > > +}; > > > > > > > > Are you duplicating some structures from EAL? > > > > It will be out of sync easily. > > > > > > > Only the rte_pci_id, which hasn't changed since the initial public release of > > > the DPDK. We can clean this up later if you like, but I'm really not too > > > worried about it. > > > > I would prefer an include if possible. > > rte_pci_id is changing in 16.07 ;) > > > So, we've had this discussion before :). Its really not fair to ask anyone to > write code based on predictive changes. If theres some patch out there thats > planning on making a change, we can't be expected to write with it in mind. If > you want people to use it, then get it merged. I understand thats not really > the issue here, and I'm making the change because you're right, we should avoid > duplicating the structures if we can, but please understand that its impossible > to write for change thats predicted to come at a later date. I understand your point. The rte_pci_id change has been reviewed several times already and should be applied very soon. > > > > > +++ b/mk/rte.buildtools.mk > > > > > > > > This file must be removed I think. > > > > We are going to be sick after digesting so much makefiles ;) > > > > > > > See above, given that I just tested this, and rte.hostapp.mk isn't used, I'd > > > recommend deleting the latter, rather than deleting this one and moving to the > > > old one. > > > > See above, I do not agree :) > > > Then we're not going to agree about this :). I'll re-iterate my stance. Moving to > use rte.hotapp.mk, causes alot more work for me, makes the use of the tool > somewhat uglier, and by all rights shouldn't be there at all, due to your > previously mentioned commit. It just makes more sense to use the buildtools > makefile and remove the vesitgial rte.hostapp.mk makefile.