Re: [dpdk-dev] [PATCH] build: add support for detecting march on ARM

[Pavan Nikhilesh <pbhagavatula@caviumnetworks.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.