Re: [dpdk-dev] [PATCH v8 04/14] build: reformat and move Arm config and comments

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> 
> Change formatting so that it's more consistent and readable, add/modify
> comments/stdout messages, move configuration options to more appropriate
> places and make the order consistent according to these rules:
> 1. First list generic configuration options, then list options that may
>    be overwritten. List SoC-specific options last.
> 2. For SoC-specific options, list number of cores before the number of
>    NUMA nodes, to make it consistent with config/meson.build.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Few nits, otherwise, looks good.
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> ---
>  config/arm/arm64_armv8_linux_gcc | 21 ++++++-
>  config/arm/meson.build           | 94 +++++++++++++++++++-------------
>  2 files changed, 77 insertions(+), 38 deletions(-)
> 
> diff --git a/config/arm/arm64_armv8_linux_gcc
> b/config/arm/arm64_armv8_linux_gcc
> index 13ee8b223..04cd82ba9 100644
> --- a/config/arm/arm64_armv8_linux_gcc
> +++ b/config/arm/arm64_armv8_linux_gcc
> @@ -13,9 +13,16 @@ cpu = 'armv8-a'
>  endian = 'little'
> 
>  [properties]
> +# Supported implementers:
> +# 'generic': Generic armv8
> +# '0x41':    Arm
> +# '0x43':    Cavium
> +# '0x50':    Ampere Computing
> +# '0x56':    Marvell ARMADA
> +# 'dpaa':    NXP DPAA
I do think the comments add much value here and they are not relevant too (as this is a cross build file for a generic build). They are captured in config/arm/meson.build already.

Instead, would be good to add something like "Generate binaries that are portable across all Armv8 machines"

>  implementer_id = 'generic'
> 
> -# Valid options for Arm's part_number:
> +# Supported part_numbers for generic, 0x41, 0x56, dpaa:
>  # 'generic': valid for all armv8-a architectures (default value)
>  # '0xd03':   cortex-a53
>  # '0xd04':   cortex-a35
> @@ -25,4 +32,16 @@ implementer_id = 'generic'
>  # '0xd09':   cortex-a73
>  # '0xd0a':   cortex-a75
>  # '0xd0b':   cortex-a76
> +# '0xd0c':   neoverse-n1
>  part_number = 'generic'
> +
> +# Supported part_numbers for 0x43:
> +# 'generic': valid for all Cavium builds
> +# '0xa1':    thunderxt88
> +# '0xa2':    thunderxt81
> +# '0xa3':    thunderxt83
> +# '0xaf':    thunderx2t99
> +# '0xb2':    octeontx2
Same here. It would be good to remove the existing comments as well.

> +
> +# Supported part_numbers for 0x50:
> +# 'generic': valid for all Ampere builds
> diff --git a/config/arm/meson.build b/config/arm/meson.build index
> 7c7059cc2..5b922ef9c 100644
> --- a/config/arm/meson.build
> +++ b/config/arm/meson.build
> @@ -5,6 +5,7 @@
> 
>  arm_force_native_march = false
> 
> +# common flags to all aarch64 builds, with lowest priority
>  flags_common_default = [
>  	# Accelarate rte_memcpy. Be sure to run unit test
> (memcpy_perf_autotest)
>  	# to determine the best threshold in code. Refer to notes in source file
> @@ -12,8 +13,8 @@ flags_common_default = [
>  	['RTE_ARCH_ARM64_MEMCPY', false],
>  	#	['RTE_ARM64_MEMCPY_ALIGNED_THRESHOLD', 2048],
>  	#	['RTE_ARM64_MEMCPY_UNALIGNED_THRESHOLD', 512],
> -	# Leave below RTE_ARM64_MEMCPY_xxx options commented out,
> unless there're
> -	# strong reasons.
> +	# Leave below RTE_ARM64_MEMCPY_xxx options commented out,
> +	# unless there are strong reasons.
>  	#	['RTE_ARM64_MEMCPY_SKIP_GCC_VER_CHECK', false],
>  	#	['RTE_ARM64_MEMCPY_ALIGN_MASK', 0xF],
>  	#	['RTE_ARM64_MEMCPY_STRICT_ALIGN', false],
> @@ -23,69 +24,86 @@ flags_common_default = [
> 
>  	['RTE_SCHED_VECTOR', false],
>  	['RTE_ARM_USE_WFE', false],
> +	['RTE_ARCH_ARM64', true],
> +	['RTE_CACHE_LINE_SIZE', 128]
>  ]
> 
> +# implementer specific aarch64 flags, with middle priority # (will
Nit                                          ^^^^^^^ can be removed

> +overwrite common flags)
>  flags_implementer_generic = [
>  	['RTE_MACHINE', '"armv8a"'],
> -	['RTE_MAX_LCORE', 256],
>  	['RTE_USE_C11_MEM_MODEL', true],
> -	['RTE_CACHE_LINE_SIZE', 128]]
> +	['RTE_CACHE_LINE_SIZE', 128],
> +	['RTE_MAX_LCORE', 256]
> +]

<snip>

>  flags_implementer_armada = [
>  	['RTE_MACHINE', '"armv8a"'],
>  	['RTE_CACHE_LINE_SIZE', 64],
> -	['RTE_MAX_NUMA_NODES', 1],
> -	['RTE_MAX_LCORE', 16]]
> +	['RTE_MAX_LCORE', 16],
> +	['RTE_MAX_NUMA_NODES', 1]
> +]
> 
> +# part number specific aarch64 flags, with highest priority # (will
Nit                                         ^^^^^^^ can be removed

> +overwrite both common and implementer specific flags)
>  flags_part_number_thunderx = [
>  	['RTE_MACHINE', '"thunderx"'],
> -	['RTE_USE_C11_MEM_MODEL', false]]
> +	['RTE_USE_C11_MEM_MODEL', false]
> +]

<snip>

>  flags_part_number_n1generic = [
>  	['RTE_MACHINE', '"neoverse-n1"'],
> -	['RTE_MAX_LCORE', 64],
> -	['RTE_CACHE_LINE_SIZE', 64],
>  	['RTE_ARM_FEATURE_ATOMICS', true],
>  	['RTE_USE_C11_MEM_MODEL', true],
> -	['RTE_MAX_MEM_MB', 1048576],
> -	['RTE_MAX_NUMA_NODES', 1],
>  	['RTE_EAL_NUMA_AWARE_HUGEPAGES', false],
> -	['RTE_LIBRTE_VHOST_NUMA', false]]
> +	['RTE_LIBRTE_VHOST_NUMA', false],
> +	['RTE_MAX_MEM_MB', 1048576],
> +	['RTE_CACHE_LINE_SIZE', 64],
> +	['RTE_MAX_LCORE', 64],
> +	['RTE_MAX_NUMA_NODES', 1]
> +]
> 
> +# arm config (implementer 0x41) is the default config
I do not understand this comment. What does 'default' config mean? Are you referring to 'generic' config?

>  part_number_config_arm = [
>  	['generic', ['-march=armv8-a+crc', '-moutline-atomics']],
>  	['native', ['-march=native']],
> @@ -96,8 +114,8 @@ part_number_config_arm = [
>  	['0xd09', ['-mcpu=cortex-a73']],
>  	['0xd0a', ['-mcpu=cortex-a75']],
>  	['0xd0b', ['-mcpu=cortex-a76']],
> -	['0xd0c', ['-march=armv8.2-a+crypto', '-mcpu=neoverse-n1'],
> flags_part_number_n1generic]]
> -
> +	['0xd0c', ['-march=armv8.2-a+crypto', '-mcpu=neoverse-n1'],
> +flags_part_number_n1generic] ]
>  part_number_config_cavium = [
>  	['generic', ['-march=armv8-a+crc+crypto','-mcpu=thunderx']],
>  	['native', ['-march=native']],
> @@ -105,13 +123,14 @@ part_number_config_cavium = [
>  	['0xa2', ['-mcpu=thunderxt81'], flags_part_number_thunderx],
>  	['0xa3', ['-mcpu=thunderxt83'], flags_part_number_thunderx],
>  	['0xaf', ['-march=armv8.1-a+crc+crypto','-mcpu=thunderx2t99'],
> flags_part_number_thunderx2],
> -	['0xb2', ['-march=armv8.2-a+crc+crypto+lse','-mcpu=octeontx2'],
> flags_part_number_octeontx2]]
> -
> +	['0xb2', ['-march=armv8.2-a+crc+crypto+lse','-mcpu=octeontx2'],
> +flags_part_number_octeontx2] ]
>  part_number_config_emag = [
>  	['generic', ['-march=armv8-a+crc+crypto', '-mtune=emag']],
> -	['native', ['-march=native']]]
> +	['native', ['-march=native']]
> +]
> 
> -## Arm implementer ID (ARM DDI 0487C.a, Section G7.2.106, Page G7-5321)
> +## Arm implementer ID (MIDR in Arm Architecture Reference Manual)
>  implementer_generic = ['Generic armv8', flags_implementer_generic,
> part_number_config_arm]
>  implementer_0x41 = ['Arm', flags_implementer_arm,
> part_number_config_arm]
>  implementer_0x43 = ['Cavium', flags_implementer_cavium,
> part_number_config_cavium] @@ -123,21 +142,21 @@
> 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
>  	implementer_id = 'generic'
>  	machine_args = [] # Clear previous machine args
>  	if machine == 'generic' and not meson.is_cross_build()
> +		# generic build
>  		implementer_config = implementer_generic
>  		part_number = 'generic'
>  	elif not meson.is_cross_build()
> +		# native build
>  		# The script returns ['Implementer', 'Variant', 'Architecture',
>  		# 'Primary Part number', 'Revision']
>  		detect_vendor = find_program(join_paths( @@ -158,6 +177,7
> @@ else
>  			part_number = 'native'
>  		endif
>  	else
> +		# cross build
>  		implementer_id =
> meson.get_cross_property('implementer_id', 'generic')
>  		part_number = meson.get_cross_property('part_number',
> 'generic')
>  		implementer_config = get_variable('implementer_' +
> implementer_id) @@ -194,7 +214,7 @@ else
>  		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