From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 4A9771B1E5 for ; Wed, 10 Jan 2018 11:12:13 +0100 (CET) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Jan 2018 02:12:11 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,339,1511856000"; d="scan'208";a="192321664" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.77]) by orsmga005.jf.intel.com with SMTP; 10 Jan 2018 02:12:09 -0800 Received: by (sSMTP sendmail emulation); Wed, 10 Jan 2018 10:12:08 +0000 Date: Wed, 10 Jan 2018 10:12:08 +0000 From: Bruce Richardson To: Pavan Nikhilesh Cc: bluca@debian.org, harry.van.haaren@intel.com, jerin.jacob@caviumnetworks.com, herbert.guan@arm.com, dev@dpdk.org Message-ID: <20180110101207.GA6388@bricha3-MOBL3.ger.corp.intel.com> References: <20171230163754.20356-1-pbhagavatula@caviumnetworks.com> <20180108170547.GA7960@bricha3-MOBL3.ger.corp.intel.com> <20180110082129.ag5t3qihwgqioghi@Pavan-LT> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180110082129.ag5t3qihwgqioghi@Pavan-LT> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.1 (2017-09-22) Subject: Re: [dpdk-dev] [PATCH] build: add support for detecting march on ARM X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Jan 2018 10:12:14 -0000 On Wed, Jan 10, 2018 at 01:51:30PM +0530, Pavan Nikhilesh wrote: > On Mon, Jan 08, 2018 at 05:05:47PM +0000, Bruce Richardson wrote: > > On Sat, Dec 30, 2017 at 10:07:54PM +0530, Pavan Nikhilesh wrote: > > > Added support for detecting march and mcpu by reading midr_el1 register. > > > The implementer, primary part number values read can be used to figure > > > out the underlying arm cpu. > > > > > > Signed-off-by: Pavan Nikhilesh > > > --- > > > > > > The current method used for reading MIDR_EL1 form userspace might not be > > > reliable and can be easily modified by updating config/arm/machine.py. > > > > > > More info on midr_el1 can be found at > > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500g/BABFEABI.html > > > > > > This patch depends on http://dpdk.org/dev/patchwork/patch/32410/ > > > > > > > I had intended that patch to just be a prototype to start adding ARM > > support - have you considered taking that and rolling in these changes > > into that as a part of a set? > > Agreed, I will merge the patches as a patchset in v2. > > > > > > config/arm/machine.py | 18 ++++++++++++++++++ > > > config/arm/meson.build | 20 ++++++++++++++++++++ > > > config/meson.build | 3 ++- > > > drivers/meson.build | 2 +- > > > examples/meson.build | 2 +- > > > lib/meson.build | 2 +- > > > meson.build | 2 +- > > > 7 files changed, 44 insertions(+), 5 deletions(-) > > > create mode 100755 config/arm/machine.py > > > > > > diff --git a/config/arm/machine.py b/config/arm/machine.py > > > new file mode 100755 > > > index 000000000..3c6e7b6a7 > > > --- /dev/null > > > +++ b/config/arm/machine.py > > > @@ -0,0 +1,18 @@ > > > +#!/usr/bin/python > > > +import pprint > > > +pp = pprint > > > + > > > +ident = [] > > > +fname = '/sys/devices/system/cpu/cpu0/regs/identification/midr_el1' > > > +with open(fname) as f: > > > + content = f.read() > > > + > > > +midr_el1 = (int(content.rstrip('\n'), 16)) > > > + > > > +ident.append(hex((midr_el1 >> 24) & 0xFF)) # Implementer > > > +ident.append(hex((midr_el1 >> 20) & 0xF)) # Variant > > > +ident.append(hex((midr_el1 >> 16) & 0XF)) # Architecture > > > +ident.append(hex((midr_el1 >> 4) & 0xFFF)) # Primary Part number > > > +ident.append(hex(midr_el1 & 0xF)) # Revision > > > + > > > +print(' '.join(ident)) > > > diff --git a/config/arm/meson.build b/config/arm/meson.build > > > index 250958415..f6ae69c21 100644 > > > --- a/config/arm/meson.build > > > +++ b/config/arm/meson.build > > > @@ -41,3 +41,23 @@ else > > > endif > > > dpdk_conf.set('RTE_CACHE_LINE_SIZE', 128) > > > dpdk_conf.set('RTE_FORCE_INTRINSICS', 1) > > > + > > > +detect_vendor = find_program(join_paths(meson.current_source_dir(), > > > + 'machine.py')) > > > +cmd = run_command(detect_vendor.path()) > > > +if cmd.returncode() != 0 > > > + message('Unable to read midr_el1') > > > +else > > > + cmd_output = cmd.stdout().strip().split(' ') > > > + message('midr_el1 output: \n' + 'Implementor ' + cmd_output[0] + > > > + ' Variant ' + cmd_output[1] + ' Architecture ' + > > > + cmd_output[2] + ' Primary Part number ' + cmd_output[3] > > > + + ' Revision ' + cmd_output[4]) > > > + if cmd_output[0] == '0x43' > > > + message('Implementor : Cavium') > > > + dpdk_conf.set('RTE_MACHINE', 'thunderx') > > > + machine_arg = [] > > > + machine_arg += '-march=' + 'armv8-a+crc+crypto' > > > + machine_arg += '-mcpu=' + 'thunderx' > > > + endif > > > +endif > > > > Should the call to the script and return code not be dependent on the > > current value of machine i.e. only if it's "native"? If the user has > > specified a "machine" type as part of the meson configuration > > parameters, you should not override it. Similarly in the cross-build > > case, the machine type is taken from the cross-build file itself. > > > > Yes, only in native build scenario the script and result should be used I will > modify that in v2. > > > > diff --git a/config/meson.build b/config/meson.build > > > index 86e978fb1..fe8104676 100644 > > > --- a/config/meson.build > > > +++ b/config/meson.build > > > @@ -8,7 +8,8 @@ else > > > machine = get_option('machine') > > > endif > > > dpdk_conf.set('RTE_MACHINE', machine) > > > -machine_arg = '-march=' + machine > > > +machine_arg = [] > > > +machine_arg += '-march=' + machine > > > > > > > I was confused initially as to why this change, but now I realise it's > > due to the fact that for the thunderx build you need both an -march and > > an -mcpu flag. I think it might be better to make this change as a > > separate patch and rename the variable from "machine_arg" to > > "machine_args" to make it clearer it's an array. > > I was thinking of having a dpdk_machine_args in base meson.build parallel to > dpdk_extra_ldflags and friends. > > > Alternatively, we can have separate variables for march flag and mcpu > > flag. [Does cpu-type need to be a configuration parameter for ARM > > platforms, in the non-cross-build case?] > > > > Initially while experimenting with meson I did try out splitting the march and > mcpu flags but that resulted in extra overhead i.e. filtering out mcpu when > it's unused in case of x86 builds. > > In case of x86 the mcpu flag is deprecated where as in arm it provides extra > hint to the compiler to do silicon specific optimizations as the implementation > of core is vendor specific. In case of gcc7[1] the mcpu can be furthur tuned to > be core specific by basing it around the primary part number > (thunderxt81(0xA2), thunderxt83(0xA3), thunderxt88(0xA1)) detected via reading > midr_el1. > > [1] https://gcc.gnu.org/gcc-7/changes.html > Ok, go with your original idea then of making machine_args an array, it seems to make most sense, and also allows further expandability in future too in case some architectures also want to use a -mtune option. [Just add an "s" to the name! :-)] Thanks, /Bruce