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

[Bruce Richardson <bruce.richardson@intel.com>]

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 <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
> 

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