From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <thomas.monjalon@6wind.com>
Received: from mail-lf0-f48.google.com (mail-lf0-f48.google.com
[209.85.215.48]) by dpdk.org (Postfix) with ESMTP id 048D62BE1
for <dev@dpdk.org>; Wed, 25 May 2016 21:39:46 +0200 (CEST)
Received: by mail-lf0-f48.google.com with SMTP id k98so22703346lfi.1
for <dev@dpdk.org>; Wed, 25 May 2016 12:39:45 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=6wind-com.20150623.gappssmtp.com; s=20150623;
h=from:to:cc:subject:date:message-id:user-agent:in-reply-to
:references:mime-version:content-transfer-encoding;
bh=OHf23d2SDnInLcVtyEYtqME+a6i9Kaw5tgzYPMkVNH8=;
b=r5l1+lomMUloDqJmctj1rvII9l5AKjR5HbBeAIkL1/8NsrbPeZ0FHM4jFpgPRAP9EL
1NRrswqsPh3jcT4DJJ7ZHZiumMbAHGyMADUQ6L+nFYUbsexx1XEF4ia05MZ+iHNVLgsh
Xc6miBPIE37qWjITGkAAc0aQmhTHiZJKo33UYNxCrSe59Mi8h17i5NJMBgBBgDEc/lH9
/37uKtlrCix0sXvtPpc0VnqB+jGHr6GUQdS3ysQCDlnqyIKvI7+WGfws5uODQxNT+42F
63+noVodRM+weJYnDj439kideMVeQPGgMlTP6GjMp5mqHrMDC6Okw+GeWIPFb6C9Ffni
uOBA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20130820;
h=x-gm-message-state:from:to:cc:subject:date:message-id:user-agent
:in-reply-to:references:mime-version:content-transfer-encoding;
bh=OHf23d2SDnInLcVtyEYtqME+a6i9Kaw5tgzYPMkVNH8=;
b=kk/v+xt78m7NOgMJ55G9onXTPnNp2g3hWbcb+fMIyX6s0ARzESyUdV5MIZPIFLYMbk
F7/DVzxcAvXeoiPjajWkIe6BIDCEn1GqJNmg2ChbMTzliW6W4e+bY0wcgp4V0+itHB6O
BIadabQMAFXpz8/ruJ9IelACLmwMVXuuGJngwYhNqNrvQ1B74flVlwMkG2BlvxRTW5X3
Roa7M7/CbFRCJK6YYXkHFaohzAOTpod/nYVphqkh8NSVrtu2l8xNQPDZYr+QXzgrGpCr
D3DeV+dZiwyPrqPAlh0KaYQyt8PuvtsfF7xn3qynL/Nr1j+/ZkTwYo/CF9yWY+R0imXo
vuEA==
X-Gm-Message-State: ALyK8tLkZreaLhJ1YuuDxjOasxQtpc6LvqSNJUQHcHCYaLtlpFBkAdtFKh86hUJYLacKNUUC
X-Received: by 10.195.9.67 with SMTP id dq3mr428819wjd.140.1464205185411;
Wed, 25 May 2016 12:39:45 -0700 (PDT)
Received: from xps13.localnet (184.203.134.77.rev.sfr.net. [77.134.203.184])
by smtp.gmail.com with ESMTPSA id dd7sm10474571wjb.22.2016.05.25.12.39.44
(version=TLSv1/SSLv3 cipher=OTHER);
Wed, 25 May 2016 12:39:44 -0700 (PDT)
From: Thomas Monjalon <thomas.monjalon@6wind.com>
To: Neil Horman <nhorman@tuxdriver.com>
Cc: dev@dpdk.org, Bruce Richardson <bruce.richardson@intel.com>,
Stephen Hemminger <stephen@networkplumber.org>,
Panu Matilainen <pmatilai@redhat.com>
Date: Wed, 25 May 2016 21:39:43 +0200
Message-ID: <1507554.Pcj3Og5Zqt@xps13>
User-Agent: KMail/4.14.10 (Linux/4.1.6-1-ARCH; KDE/4.14.11; x86_64; ; )
In-Reply-To: <20160525191345.GF14322@hmsreliant.think-freely.org>
References: <1463431287-4551-1-git-send-email-nhorman@tuxdriver.com>
<1710741.QL28xgSMua@xps13>
<20160525191345.GF14322@hmsreliant.think-freely.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Subject: Re: [dpdk-dev] [PATCHv4 1/5] pmdinfogen: Add buildtools and
pmdinfogen utility
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Wed, 25 May 2016 19:39:46 -0000
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.