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<n> and this_pmd_tbl<n> > (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 *<pmd_name>_driver_info[] __attribute__((used)) = " \ > PMD_DRIVER_INFO=<json string>"; > > Where <pmd_name> is the arbitrary name of the pmd, and <json_string> 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.