[dpdk-dev] [PATCH v4 2/6] build: refactor Arm build

[Honnappa Nagarahalli]

<snip>

> 
> * Rename variables to have names that better describe what the variables
> store
> * Remove unused or superfluous variables
> * Change a list to dictionary where key lookup is needed
> * Add informatory comments in the code
> * Minor code restructure and reformatting
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes at pantheon.tech>
> ---
>  config/arm/arm64_armada_linux_gcc    |   2 +-
>  config/arm/arm64_armv8_linux_gcc     |   8 +-
>  config/arm/arm64_bluefield_linux_gcc |   4 +-
>  config/arm/arm64_dpaa_linux_gcc      |   2 +-
>  config/arm/arm64_emag_linux_gcc      |   2 +-
>  config/arm/arm64_n1sdp_linux_gcc     |   4 +-
>  config/arm/arm64_octeontx2_linux_gcc |   4 +-
>  config/arm/arm64_stingray_linux_gcc  |   4 +-
>  config/arm/arm64_thunderx2_linux_gcc |   4 +-
>  config/arm/arm64_thunderx_linux_gcc  |   2 +-
>  config/arm/meson.build               | 247 +++++++++++++++------------
>  11 files changed, 153 insertions(+), 130 deletions(-)
> 

<snip>

> diff --git a/config/arm/meson.build b/config/arm/meson.build index
> 491842cad..6c31ab167 100644
> --- a/config/arm/meson.build
> +++ b/config/arm/meson.build
> @@ -3,12 +3,12 @@
>  # Copyright(c) 2017 Cavium, Inc
>  # Copyright(c) 2020 PANTHEON.tech s.r.o.
> 
> -# for checking defines we need to use the correct compiler flags -march_opt
> = '-march=@0@'.format(machine)
> -
> +# set arm_force_native_march if you want to use machine args below #
> +instead of discovered values; only works when doing an actual native
> +build
>  arm_force_native_march = false
> -arm_force_generic_march = (machine == 'generic')
> +native_machine_args = ['-march=native', '-mtune=native']
> 

[...]

> -
> -machine_args_default = [
> -	['default', ['-march=armv8-a+crc', '-moutline-atomics']],
> -	['native', ['-march=native']],
> -	['0xd03', ['-mcpu=cortex-a53']],
> -	['0xd04', ['-mcpu=cortex-a35']],
> -	['0xd07', ['-mcpu=cortex-a57']],
> -	['0xd08', ['-mcpu=cortex-a72']],
> -	['0xd09', ['-mcpu=cortex-a73']],
> -	['0xd0a', ['-mcpu=cortex-a75']],
> -	['0xd0b', ['-mcpu=cortex-a76']],
> -	['0xd0c', ['-march=armv8.2-a+crc+crypto', '-mcpu=neoverse-n1'],
> flags_n1sdp_extra]]
> -
> -machine_args_cavium = [
> -	['default', ['-march=armv8-a+crc+crypto','-mcpu=thunderx']],
> -	['native', ['-march=native']],
> -	['0xa1', ['-mcpu=thunderxt88'], flags_thunderx_extra],
> -	['0xa2', ['-mcpu=thunderxt81'], flags_thunderx_extra],
> -	['0xa3', ['-mcpu=thunderxt83'], flags_thunderx_extra],
> -	['0xaf', ['-march=armv8.1-a+crc+crypto','-mcpu=thunderx2t99'],
> flags_thunderx2_extra],
> -	['0xb2', ['-march=armv8.2-a+crc+crypto+lse','-mcpu=octeontx2'],
> flags_octeontx2_extra]]
> -
> -machine_args_emag = [
> -	['default', ['-march=armv8-a+crc+crypto', '-mtune=emag']],
> -	['native', ['-march=native']]]
> +	['RTE_USE_C11_MEM_MODEL', true]
> +]
> +# arm config (implementer 0x41) is the default config pn_config_default
What does it mean by 'default' here? I am somewhat confused between 'default' and 'generic'. We should look to remove 'default' as much as possible and stick with 'generic'.

> += {
> +	'generic': [['-march=armv8-a+crc', '-moutline-atomics']],
I like that we have taken out 'native' from this list. Would it be possible to take out 'generic' from this and others below. This is because the binary built with 'generic' build should run on any Arm platform. There is no dependency on any underlying platform.

> +	'0xd03': [['-mcpu=cortex-a53']],
> +	'0xd04': [['-mcpu=cortex-a35']],
> +	'0xd07': [['-mcpu=cortex-a57']],
> +	'0xd08': [['-mcpu=cortex-a72']],
> +	'0xd09': [['-mcpu=cortex-a73']],
> +	'0xd0a': [['-mcpu=cortex-a75']],
> +	'0xd0b': [['-mcpu=cortex-a76']],
> +	'0xd0c': [['-march=armv8.2-a+crc+crypto', '-mcpu=neoverse-n1'],
> +flags_n1sdp_extra]
'flags_n1sdp_extra' does not fit here. For the part number '0xd0c' there could be multiple SoCs (N1SDP is one of them). So, if the SoC is not known, but if we know that the CPU is N1, then we should build a N1-Generic build.
So, from my perspective, there are 3 kinds of binaries:
1) generic - best portability -  (possibly) least optimized for a given platform
2) Core-Generic (for ex: N1-generic) - Portable on all N1 based SoCs only - Optimized for N1 cores
3) SoC specific - (possibly) Not portable - Most optimized for the SoC

 } pn_config_cavium = {
> +	'generic': [['-march=armv8-a+crc+crypto', '-mcpu=thunderx']],
'generic' does not make sense here.

> +	'0xa1': [['-mcpu=thunderxt88'], flags_thunderx_extra],
> +	'0xa2': [['-mcpu=thunderxt81'], flags_thunderx_extra],
> +	'0xa3': [['-mcpu=thunderxt83'], flags_thunderx_extra],
> +	'0xaf': [['-march=armv8.1-a+crc+crypto','-mcpu=thunderx2t99'],
> flags_thunderx2_extra],
> +	'0xb2': [['-march=armv8.2-a+crc+crypto+lse','-mcpu=octeontx2'],
> +flags_octeontx2_extra], } pn_config_emag = {
> +	'generic': [['-march=armv8-a+crc+crypto', '-mtune=emag']] }
Same here.
I understand that this is coming from the existing code. But, I think we should try to set it right.

> 
>  ## Arm implementer ID (ARM DDI 0487C.a, Section G7.2.106, Page G7-5321)
Nit, Would be good to remove the reference to the doc

> -impl_generic = ['Generic armv8', flags_generic, machine_args_default]
> -impl_0x41 = ['Arm', flags_arm, machine_args_default]
> -impl_0x42 = ['Broadcom', flags_generic, machine_args_default]
> -impl_0x43 = ['Cavium', flags_cavium, machine_args_cavium]
> -impl_0x44 = ['DEC', flags_generic, machine_args_default]
> -impl_0x49 = ['Infineon', flags_generic, machine_args_default] -impl_0x4d =
> ['Motorola', flags_generic, machine_args_default] -impl_0x4e = ['NVIDIA',
> flags_generic, machine_args_default]
> -impl_0x50 = ['Ampere Computing', flags_emag, machine_args_emag]
> -impl_0x51 = ['Qualcomm', flags_generic, machine_args_default]
> -impl_0x53 = ['Samsung', flags_generic, machine_args_default]
> -impl_0x56 = ['Marvell ARMADA', flags_armada, machine_args_default]
> -impl_0x69 = ['Intel', flags_generic, machine_args_default] -impl_dpaa =
> ['NXP DPAA', flags_dpaa, machine_args_default]
> +impl_generic = ['Generic armv8', flags_generic, pn_config_default]
> +impl_0x41 = ['Arm', flags_arm, pn_config_default]
> +impl_0x42 = ['Broadcom', flags_generic, pn_config_default]
> +impl_0x43 = ['Cavium', flags_cavium, pn_config_cavium]
> +impl_0x44 = ['DEC', flags_generic, pn_config_default]
> +impl_0x49 = ['Infineon', flags_generic, pn_config_default] impl_0x4d =
> +['Motorola', flags_generic, pn_config_default] impl_0x4e = ['NVIDIA',
> +flags_generic, pn_config_default]
> +impl_0x50 = ['Ampere Computing', flags_emag, pn_config_emag]
> +impl_0x51 = ['Qualcomm', flags_generic, pn_config_default]
> +impl_0x53 = ['Samsung', flags_generic, pn_config_default]
> +impl_0x56 = ['Marvell ARMADA', flags_armada, pn_config_default]
> +impl_0x69 = ['Intel', flags_generic, pn_config_default] impl_dpaa =
> +['NXP DPAA', flags_dpaa, pn_config_default]
> 
>  dpdk_conf.set('RTE_ARCH_ARM', 1)
>  dpdk_conf.set('RTE_FORCE_INTRINSICS', 1)
> 
>  if dpdk_conf.get('RTE_ARCH_32')
> +	# armv7 build
>  	dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
>  	dpdk_conf.set('RTE_ARCH_ARMv7', 1)
>  	# the minimum architecture supported, armv7-a, needs the
> following,
>  	# mk/machine/armv7a/rte.vars.mk sets it too
>  	machine_args += '-mfpu=neon'
>  else
> -	dpdk_conf.set('RTE_CACHE_LINE_SIZE', 128)
> -	dpdk_conf.set('RTE_ARCH_ARM64', 1)
> +	# aarch64 build
> +	if not meson.is_cross_build()
> +		if machine == 'generic'
> +			# default build
                                              ^^^^^^^^^^^ Generic build?

> +			impl_config = impl_generic
> +			part_number = 'generic'
> +		else
> +			# native build
> +			# The script returns ['Implementer', 'Variant',
> 'Architecture',
> +			# 'Primary Part number', 'Revision']
> +			detect_vendor = find_program(join_paths(
> +					meson.current_source_dir(),
> 'armv8_machine.py'))
> +			cmd = run_command(detect_vendor.path())
> +			if cmd.returncode() == 0
> +				cmd_output =
> cmd.stdout().to_lower().strip().split(' ')
> +			endif
> +			if arm_force_native_march == true
> +				part_number = 'native'
> +			else
> +				part_number = cmd_output[3]
> +			endif
> +			# Set to generic implementer if implementer is not
> found
> +			impl_config = get_variable('impl_' + cmd_output[0],
> 'impl_generic')
> +		endif
> +	else
> +		# cross build
> +		impl_id = meson.get_cross_property('implementer_id',
> 'generic')
> +		part_number = meson.get_cross_property('part_number',
> 'generic')
> +		impl_config = get_variable('impl_' + impl_id)
> +	endif
> 
> -	machine = []
> -	cmd_generic = ['generic', '', '', 'default', '']
> -	cmd_output = cmd_generic # Set generic by default
> -	machine_args = [] # Clear previous machine args
> -	if arm_force_generic_march and not meson.is_cross_build()
> -		machine = impl_generic
> -		impl_pn = 'default'
> +	message('Arm implementer: ' + impl_config[0])
> +	message('Arm part number: ' + part_number)
> +
> +	implementer_flags = impl_config[1]
> +	part_number_config = impl_config[2]
> +
> +	if part_number_config.has_key(part_number)
> +		# use the specified part_number machine args if found
> +		part_number_config = part_number_config[part_number]
> +	elif part_number == 'native'
> +		# use native machine args
> +		part_number_config = [[native_machine_args]]
>  	elif not meson.is_cross_build()
> -		# The script returns ['Implementer', 'Variant', 'Architecture',
> -		# 'Primary Part number', 'Revision']
> -		detect_vendor = find_program(join_paths(
> -				meson.current_source_dir(),
> 'armv8_machine.py'))
> -		cmd = run_command(detect_vendor.path())
> -		if cmd.returncode() == 0
> -			cmd_output = cmd.stdout().to_lower().strip().split('
> ')
> -		endif
> -		# Set to generic if variable is not found
> -		machine = get_variable('impl_' + cmd_output[0], ['generic'])
> -		if machine[0] == 'generic'
> -			machine = impl_generic
> -			cmd_output = cmd_generic
> -		endif
> -		impl_pn = cmd_output[3]
> -		if arm_force_native_march == true
> -			impl_pn = 'native'
> -		endif
> +		# default to generic machine args if part_number is not found
> +		# and not forcing native machine args
> +		# but don't default in cross-builds; if part_number is specified
> +		# incorrectly in a cross-file, it needs to be fixed there
> +		part_number_config = part_number_config['generic']
>  	else
> -		impl_id = meson.get_cross_property('implementor_id',
> 'generic')
> -		impl_pn = meson.get_cross_property('implementor_pn',
> 'default')
> -		machine = get_variable('impl_' + impl_id)
> +		# cross build and part number is not in part_number_config
> +		error('Cross build part number 0 at 0 not
> found.'.format(part_number))
>  	endif
> 
> -	# Apply Common Defaults. These settings may be overwritten by
> machine
> -	# settings later.
> -	foreach flag: flags_common_default
> -		if flag.length() > 0
> -			dpdk_conf.set(flag[0], flag[1])
> +	dpdk_flags = flags_common_default + implementer_flags
> +
> +	if part_number_config.length() > 1
> +		dpdk_flags += part_number_config[1]
> +	endif
> +
> +	machine_args = [] # Clear previous machine args
> +	foreach flag: part_number_config[0]
> +		if cc.has_argument(flag)
> +			machine_args += flag
>  		endif
>  	endforeach
> 
> -	message('Implementer : ' + machine[0])
> -	foreach flag: machine[1]
> +	foreach flag: dpdk_flags
>  		if flag.length() > 0
>  			dpdk_conf.set(flag[0], flag[1])
>  		endif
>  	endforeach
> -
> -	foreach marg: machine[2]
> -		if marg[0] == impl_pn
> -			foreach flag: marg[1]
> -				if cc.has_argument(flag)
> -					machine_args += flag
> -				endif
> -			endforeach
> -			# Apply any extra machine specific flags.
> -			foreach flag: marg.get(2, flags_default_extra)
> -				if flag.length() > 0
> -					dpdk_conf.set(flag[0], flag[1])
> -				endif
> -			endforeach
> -		endif
> -	endforeach
>  endif
> -message(machine_args)
> +
> +message('Using machine args: @0@'.format(machine_args))
> 
>  if (cc.get_define('__ARM_NEON', args: machine_args) != '' or
>      cc.get_define('__aarch64__', args: machine_args) != '')
> --
> 2.20.1