From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f45.google.com (mail-wm0-f45.google.com [74.125.82.45]) by dpdk.org (Postfix) with ESMTP id 76F0D377C for ; Wed, 25 May 2016 19:39:32 +0200 (CEST) Received: by mail-wm0-f45.google.com with SMTP id n129so194122266wmn.1 for ; Wed, 25 May 2016 10:39:32 -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=36wceFXbExD1rjgxoZl3P5HQqblgbCtPjhEs6v46gLM=; b=dh8GW9zfwxSa7V5GmXtLcC5QVVUMxMC1SL1KCb+8kM1xq67cSlutH0xrAQzXCcbjgi YMQoZ8zKPEhxxpm75Y279cr15i3OJpgoI8+5K4y3QH5YB8WrjjpcFko8co3sNWHuPhx1 x6gRdKIGUAsBK2QUnsclettfkBYFJIHNgGHbEypSUey9Eq8c8DwqNiyQ6cs6EPOkC7RC vrYNuOILDEJT6nwXC73SiTsds/N09h/BFYpCA8957Bd6HOc5ymSGmq/Co1PJ/xlCnjPn QB4trXHFXAdqraKV+cv6Td7FUiZ3xKbGoIyn6sgQ+hwP9ig7oDiQMi4+BVkKad+a2FmU 1Tkw== 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=36wceFXbExD1rjgxoZl3P5HQqblgbCtPjhEs6v46gLM=; b=ij3OGPHjvQqoW3snPHXv/dLXtQv1h35ykZV9MuBw3p8b523I+aEwr2Tt6XMmGPywwo d+u1/Nw9kmlH8kM8uSGugcfquGPCqj/h7Zk+un6h0yiROik7zqQZw06z/6xsLuPUvrdI 42uBAkSGUlZFvcUzVcGZbQx6mbQOeaBbaqX5smt8anOW+gYwRA0EEjkJsvvTO6lvWx7z FMgZ7XMJXjdhxHFBI4QKg30/guUS5mX6gxdHdhrJtAFnbT91+5czMbEa+oP7OTZFzjhp 9AY3FO500PLWT2nzGdwIp/W1KY9x82FxDv7neYWTasIIhA4OGeq+JyuSOWaRMXTfb7qi oxCQ== X-Gm-Message-State: ALyK8tLFlS//nANR0H1golPiGTjDKBB2roKSVs2P6tNk3525BlA4DA7pWD26hcHgbhCe6u5Q X-Received: by 10.28.226.84 with SMTP id z81mr5005558wmg.73.1464197972167; Wed, 25 May 2016 10:39:32 -0700 (PDT) Received: from xps13.localnet (184.203.134.77.rev.sfr.net. [77.134.203.184]) by smtp.gmail.com with ESMTPSA id 63sm26085227wmz.5.2016.05.25.10.39.31 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 25 May 2016 10:39:31 -0700 (PDT) From: Thomas Monjalon To: Neil Horman Cc: dev@dpdk.org, Bruce Richardson , Stephen Hemminger , Panu Matilainen Date: Wed, 25 May 2016 19:39:30 +0200 Message-ID: <1710741.QL28xgSMua@xps13> User-Agent: KMail/4.14.10 (Linux/4.1.6-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: <20160525172227.GB14322@hmsreliant.think-freely.org> References: <1463431287-4551-1-git-send-email-nhorman@tuxdriver.com> <12522566.xGRn81iLRB@xps13> <20160525172227.GB14322@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 17:39:32 -0000 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: > > > --- a/GNUmakefile > > > +++ b/GNUmakefile > > > -ROOTDIRS-y := lib drivers app > > > +ROOTDIRS-y := buildtools lib drivers app > > > > Why a new directory? > > It is not a script nor an end-user tool, I guess. > Dependencies. This tool has to be built prior to the rest of the dpdk, but app > already relies on dpdk libraries to be built, so you get circular dependencies. > I could have put it in scripts I guess, but its not a script. Its own directory > seemed to make the most sense, given those two points OK > > > +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. > > > +++ 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. > > [...] > > > + 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. > > [...] > > > + if (info.drivers) { > > > + output_pmd_info_string(&info, argv[2]); > > > + rc = 0; > > > + } else { > > > + fprintf(stderr, "Hmm, Appears to be a driver but no drivers registered\n"); > > > > Why it appears to be a driver? > > What means "no drivers registered" exactly? > > > It means that the tool has identified this file as a driver based on some > criteria (in this case the source code contained a use of the > PMD_REGISTER_DRIVER macro, but for whatever reason, when this tool scanned it, > it never located the pmd_driver_name symbol. It should never happen, and > serves as a indicator to the developer that they need to investigate either the > construction of the driver or the use of this tool. OK > > > +++ 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? > > > +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 ;) > > > +struct elf_info { > > > + unsigned long size; > > > + Elf_Ehdr *hdr; > > > + Elf_Shdr *sechdrs; > > > + Elf_Sym *symtab_start; > > > + Elf_Sym *symtab_stop; > > > + Elf_Section export_sec; > > > + Elf_Section export_unused_sec; > > > + Elf_Section export_gpl_sec; > > > + Elf_Section export_unused_gpl_sec; > > > + Elf_Section export_gpl_future_sec; > > > + char *strtab; > > > + char *modinfo; > > > + unsigned int modinfo_len; > > > > Why these fields? > > > Because thats how you parse an ELF file and look up the information you need to > extract the data this tool then exports. I don't mean to sound short, but your > question doesn't really make any sense. The members of this structure are > needed to extract the info from object files to build the export strings that > pmdinfo.py needs later. Sorry, I haven't parse the whole code but some fields are unused. And modinfo looks wrong. > > > +++ 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 :)