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

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

2016-05-25 15:13, Neil Horman:
> On Wed, May 25, 2016 at 07:39:30PM +0200, Thomas Monjalon wrote:
> > 2016-05-25 13:22, Neil Horman:
> > > On Wed, May 25, 2016 at 03:21:19PM +0200, Thomas Monjalon wrote:
> > > > 2016-05-24 15:41, Neil Horman:
> > > > > +include $(RTE_SDK)/mk/rte.buildtools.mk
> > > > 
> > > > Why a new Makefile? Can you use rte.hostapp.mk?
> > > > 
> > > I don't know, maybe.  Nothing else currently uses rte.hostapp.mk, so I missed
> > > its existance.  I make the argument that, that being the case, we should stick
> > > with the Makefile I just tested with, and remove the rte.hostapp.mk file
> > 
> > No, rte.hostapp.mk has been used and tested in the history of the project.
> > Please try it.
> > 
> It works, but its really ugly (as it means that the buildtools directory gets
> install to the hostapp directory under the build).  I could move that of course,
> but at this point, you are asking me to remove a working makefile to replace it
> with another makefile that, by all rights should have been removed as part of
> commit efa2084a840fb83fd9be83adca57e5f23d3fa9fe:
> Author: Thomas Monjalon <thomas.monjalon@6wind.com>
> Date:   Tue Mar 10 17:55:25 2015 +0100
> 
>     scripts: remove useless build tools
>     
>     test-framework.sh is an old script to check building of some dependencies.
>     testhost is an old app used to check HOSTCC.
>     
>     Let's clean the scripts directory.
> 
> Here you removed the only user of rte.hostapp.mk, but neglected to remove
> hostapp.mk itself.

Yes. I didn't really neglect to remove it. I thought it would be used later.

> I really fail to see why making me rework my current
> makefile setup, that matches the purpose of the tool is a superior solution to
> just getting rid of the unused makefile thats there right now.

I'm just trying to avoid creating a new makefile for each tool.
Is it possible to fix the directory in rte.hostapp.mk?
Every apps use the same makefile rte.app.mk. I think it should be the same
for host apps.

> > > > > +++ 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.
> > > We can, but currently its a true statement.  we have two types of PMDs, a PDEV
> > > and a VDEV, the former is a pci device, and the latter is a virtual device, so
> > > you can imply the PDEV type from the presence of pci entries, and VDEV from the
> > > alternative.  If we were to do something, I would recommend adding a macro to
> > > explicitly ennumerate each pmds type.  I would prefer to wait until that was a
> > > need however, as it can be done invisibly to the user.
> > 
> > We are removing the PMD types in the EAL rework.
> > So this comment will be outdated. Better to remove now.
> > 
> Then, I'm just not going to enumerate the type of driver at all, I'll remove
> that attribute entirely.

OK

> But I really don't like to write code for things that are 'predictive'.

Not really predictive as it is an older patch.

> > > > [...]
> > > > > +		fprintf(ofd,"\\\"type\\\" : \\\"%s\\\", ", drv->pci_tbl ? "PMD_PDEV" : "PMD_VDEV");
> > > > 
> > > > Please forget the naming PDEV/VDEV.
> > > > 
> > > I don't know what you mean here, you would rather they be named PCI and Virtual,
> > > or something else?
> > 
> > Yes please.
> > 
> No, If you're removing the types, and you're sure of that, I'm just going to
> remove the description entirely.  If you're unsure about exactly whats going to
> happen, we should reflect the state of the build now, and make the appropriate
> change when it lands.

OK to remove the type description.

> > > > > +++ 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?
> > > > 
> > > Because I borrowed the code from modpost.c, which allows for both ELF32 and
> > > ELF64 compilation.  I wanted to keep it in place should DPDK ever target
> > > different sized architectures.
> > 
> > Maybe a comment is needed.
> > Is ELF32 used on 32-bit archs like i686 or ARMv7?
> It depends on exactly how its built, but that would be a common use, yes.

We have such 32-bit archs in DPDK. Is pmdinfogen working for them?

> > > > > +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.
> > > > 
> > > Only the rte_pci_id, which hasn't changed since the initial public release of
> > > the DPDK.  We can clean this up later if you like, but I'm really not too
> > > worried about it.
> > 
> > I would prefer an include if possible.
> > rte_pci_id is changing in 16.07 ;)
> > 
> So, we've had this discussion before :).  Its really not fair to ask anyone to
> write code based on predictive changes.  If theres some patch out there thats
> planning on making a change, we can't be expected to write with it in mind.  If
> you want people to use it, then get it merged.  I understand thats not really
> the issue here, and I'm making the change because you're right, we should avoid
> duplicating the structures if we can, but please understand that its impossible
> to write for change thats predicted to come at a later date.

I understand your point.
The rte_pci_id change has been reviewed several times already and should be
applied very soon.

> > > > > +++ b/mk/rte.buildtools.mk
> > > > 
> > > > This file must be removed I think.
> > > > We are going to be sick after digesting so much makefiles ;)
> > > > 
> > > See above, given that I just tested this, and rte.hostapp.mk isn't used, I'd
> > > recommend deleting the latter, rather than deleting this one and moving to the
> > > old one.
> > 
> > See above, I do not agree :)
> > 
> Then we're not going to agree about this :).  I'll re-iterate my stance.  Moving to
> use rte.hotapp.mk, causes alot more work for me, makes the use of the tool
> somewhat uglier, and by all rights shouldn't be there at all, due to your
> previously mentioned commit. It just makes more sense to use the buildtools
> makefile and remove the vesitgial rte.hostapp.mk makefile.