DPDK patches and discussions
 help / color / mirror / Atom feed
From: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
To: Bruce Richardson <bruce.richardson@intel.com>,
	bluca@debian.org, harry.van.haaren@intel.com,
	jerin.jacob@caviumnetworks.com, herbert.guan@arm.com
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] build: add support for detecting march on ARM
Date: Wed, 10 Jan 2018 13:51:30 +0530	[thread overview]
Message-ID: <20180110082129.ag5t3qihwgqioghi@Pavan-LT> (raw)
In-Reply-To: <20180108170547.GA7960@bricha3-MOBL3.ger.corp.intel.com>

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 <pbhagavatula@caviumnetworks.com>
> > ---
> >
> >  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

> /Bruce
>

Thanks,
Pavan.

  reply	other threads:[~2018-01-10  8:21 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-30 16:37 Pavan Nikhilesh
2018-01-08  8:13 ` Jerin Jacob
2018-01-08 11:12   ` Pavan Nikhilesh
2018-01-08 17:05 ` Bruce Richardson
2018-01-10  8:21   ` Pavan Nikhilesh [this message]
2018-01-10 10:12     ` Bruce Richardson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180110082129.ag5t3qihwgqioghi@Pavan-LT \
    --to=pbhagavatula@caviumnetworks.com \
    --cc=bluca@debian.org \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=harry.van.haaren@intel.com \
    --cc=herbert.guan@arm.com \
    --cc=jerin.jacob@caviumnetworks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).