From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:user-agent:in-reply-to :references:mime-version:content-transfer-encoding; bh=OHf23d2SDnInLcVtyEYtqME+a6i9Kaw5tgzYPMkVNH8=; b=r5l1+lomMUloDqJmctj1rvII9l5AKjR5HbBeAIkL1/8NsrbPeZ0FHM4jFpgPRAP9EL 1NRrswqsPh3jcT4DJJ7ZHZiumMbAHGyMADUQ6L+nFYUbsexx1XEF4ia05MZ+iHNVLgsh Xc6miBPIE37qWjITGkAAc0aQmhTHiZJKo33UYNxCrSe59Mi8h17i5NJMBgBBgDEc/lH9 /37uKtlrCix0sXvtPpc0VnqB+jGHr6GUQdS3ysQCDlnqyIKvI7+WGfws5uODQxNT+42F 63+noVodRM+weJYnDj439kideMVeQPGgMlTP6GjMp5mqHrMDC6Okw+GeWIPFb6C9Ffni uOBA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:user-agent :in-reply-to:references:mime-version:content-transfer-encoding; bh=OHf23d2SDnInLcVtyEYtqME+a6i9Kaw5tgzYPMkVNH8=; b=kk/v+xt78m7NOgMJ55G9onXTPnNp2g3hWbcb+fMIyX6s0ARzESyUdV5MIZPIFLYMbk F7/DVzxcAvXeoiPjajWkIe6BIDCEn1GqJNmg2ChbMTzliW6W4e+bY0wcgp4V0+itHB6O BIadabQMAFXpz8/ruJ9IelACLmwMVXuuGJngwYhNqNrvQ1B74flVlwMkG2BlvxRTW5X3 Roa7M7/CbFRCJK6YYXkHFaohzAOTpod/nYVphqkh8NSVrtu2l8xNQPDZYr+QXzgrGpCr D3DeV+dZiwyPrqPAlh0KaYQyt8PuvtsfF7xn3qynL/Nr1j+/ZkTwYo/CF9yWY+R0imXo vuEA== X-Gm-Message-State: ALyK8tLkZreaLhJ1YuuDxjOasxQtpc6LvqSNJUQHcHCYaLtlpFBkAdtFKh86hUJYLacKNUUC X-Received: by 10.195.9.67 with SMTP id dq3mr428819wjd.140.1464205185411; Wed, 25 May 2016 12:39:45 -0700 (PDT) Received: from xps13.localnet (184.203.134.77.rev.sfr.net. [77.134.203.184]) by smtp.gmail.com with ESMTPSA id dd7sm10474571wjb.22.2016.05.25.12.39.44 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 25 May 2016 12:39:44 -0700 (PDT) From: Thomas Monjalon To: Neil Horman Cc: dev@dpdk.org, Bruce Richardson , Stephen Hemminger , Panu Matilainen Date: Wed, 25 May 2016 21:39:43 +0200 Message-ID: <1507554.Pcj3Og5Zqt@xps13> User-Agent: KMail/4.14.10 (Linux/4.1.6-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: <20160525191345.GF14322@hmsreliant.think-freely.org> References: <1463431287-4551-1-git-send-email-nhorman@tuxdriver.com> <1710741.QL28xgSMua@xps13> <20160525191345.GF14322@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 May 2016 19:39:46 -0000 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.