Re: [dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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.