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

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

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?

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

> 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.
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?]

/Bruce