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 15:21:19 +0200 Message-ID: <12522566.xGRn81iLRB@xps13> 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-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-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) In-Reply-To: <1464118912-19658-2-git-send-email-nhorman@tuxdriver.com> 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-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.