From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id 9005C2BE0 for ; Wed, 25 May 2016 15:21:21 +0200 (CEST) Received: by mail-wm0-f42.google.com with SMTP id n129so62648932wmn.1 for ; Wed, 25 May 2016 06:21:21 -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=da5Wb44Th5KncTvNYGS+hJq/Woc74m90k9PSlrK7U2Y=; b=DB9Dy2G2aRmMOYV9wE03MJKfb9P3oC6QHAPQHn+tggAJHX+HdRZwfNoWb28SP/QSst s5/nkG/+L76TFy/i8rsAtqui9GIHp9yMNm58i/RopddIY193G0VMEYh86UHx/nQ8ejtU ffsNjTrZPDlCyoCnCclGImgIgYTe2TNr7sSQUW/JKUr49cFKbPuDSYdwg0/0kGMbnHqN 3Kwm7J9vOqX5e3hHm/E9nI+8+Cg+X/wgOvqhViuGvRBQgb0auwyxHi9SvihtlmUOFnXm YBBuGaqKL2KgFOAD68r4Qc3X+6No7Ac44CDBSFcGrclvBjTpikycr50q6ZujuLFaj6GC XQdA== 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=da5Wb44Th5KncTvNYGS+hJq/Woc74m90k9PSlrK7U2Y=; b=X33tYiV/OE/lMr736FgMwKDn/KcQgVorB3HAwC03aBinJqYnnBwqcSScwt7/vKvN5g WK+ucirm0idNq0rhCuHT0/mrg8+6nPHbiYD+7SA4BkcK+HBKCoCH1Fy9+PH2WfQDt/qV dLvILfHnoy7XaKkgWDyLd2M2OBMVJiwDIORGHnKvhTlJ8CuyggObQ7soS1Culq1qigSD 2mmLoVUiIYkJERzaSMY9e5Ab3RXM+oubv/Is9Evrw+GgwJSgy2FcmTmuX8tDyW1TbM+d eHPhVdUuPpSWQU0N5xXHoQtM0ZkzkystOKTTf1Amsgbfe6AsufxWYuCD4e2mK2A/nt/K rURQ== X-Gm-Message-State: ALyK8tIDC3XssoRkbEQS9bTi2iZ63D/OidBDI85H23332iubE7h84Urt6kfJ/ysCP8FZpa6t X-Received: by 10.194.10.69 with SMTP id g5mr3837731wjb.7.1464182481250; Wed, 25 May 2016 06:21:21 -0700 (PDT) Received: from xps13.localnet (184.203.134.77.rev.sfr.net. [77.134.203.184]) by smtp.gmail.com with ESMTPSA id jq1sm8765731wjc.28.2016.05.25.06.21.19 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 25 May 2016 06:21:20 -0700 (PDT) From: Thomas Monjalon To: Neil Horman Cc: dev@dpdk.org, Bruce Richardson , Stephen Hemminger , Panu Matilainen Date: Wed, 25 May 2016 15:21:19 +0200 Message-ID: <12522566.xGRn81iLRB@xps13> User-Agent: KMail/4.14.10 (Linux/4.1.6-1-ARCH; KDE/4.14.11; x86_64; ; ) In-Reply-To: <1464118912-19658-2-git-send-email-nhorman@tuxdriver.com> References: <1463431287-4551-1-git-send-email-nhorman@tuxdriver.com> <1464118912-19658-1-git-send-email-nhorman@tuxdriver.com> <1464118912-19658-2-git-send-email-nhorman@tuxdriver.com> 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 13:21:21 -0000 2016-05-24 15:41, Neil Horman: > pmdinfogen is a tool used to parse object files and build json strings for use in > later determining hardware support in a dso or application binary. pmdinfo > looks for the non-exported symbol names this_pmd_name and this_pmd_tbl > (where n is a integer counter). It records the name of each of these tuples, > using the later to find the symbolic name of the pci_table for physical devices > that the object supports. With this information, it outputs a C file with a > single line of the form: > > static char *_driver_info[] __attribute__((used)) = " \ > PMD_DRIVER_INFO="; > > Where is the arbitrary name of the pmd, and is the json > encoded string that hold relevant pmd information, including the pmd name, type > and optional array of pci device/vendor ids that the driver supports. > > This c file is suitable for compiling to object code, then relocatably linking > into the parent file from which the C was generated. This creates an entry in > the string table of the object that can inform a later tool about hardware > support. This description is helpful and should be in the doc: doc/guides/prog_guide/dev_kit_build_system.rst > --- 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. I feel strange to build an app for the build system. For information, do you know some Python lib to do this kind of tool? > +++ b/buildtools/pmdinfogen/Makefile > +#CFLAGS += $(WERROR_FLAGS) -g > +CFLAGS += -g Please choose one line or the other or none of them. > +include $(RTE_SDK)/mk/rte.buildtools.mk Why a new Makefile? Can you use rte.hostapp.mk? > +++ 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. > + if (!tmpsym) { > + drv->pci_tbl = NULL; > + return 0; > + } [...] > + > + > + return 0; > + > +} That's a lot of blank lines ;) [...] > + fprintf(ofd,"\\\"type\\\" : \\\"%s\\\", ", drv->pci_tbl ? "PMD_PDEV" : "PMD_VDEV"); Please forget the naming PDEV/VDEV. [...] > + 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? > +++ 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? > +#define TO_NATIVE(x) (x) Nice :) Why? > +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. > +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? > +++ b/mk/rte.buildtools.mk This file must be removed I think. We are going to be sick after digesting so much makefiles ;) Last comment, The MAINTAINERS file must be updated for this tool. Thanks for taking care of tooling.