[dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and pmdinfogen utility
Thomas Monjalon
thomas.monjalon at 6wind.com
Wed May 25 15:21:19 CEST 2016
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.
More information about the dev
mailing list