[v8,04/14] build: reformat and move Arm config and comments

Message ID 1604649795-27476-5-git-send-email-juraj.linkes@pantheon.tech (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Arm build options rework |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Juraj Linkeš Nov. 6, 2020, 8:03 a.m. UTC
  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>
---
 config/arm/arm64_armv8_linux_gcc | 21 ++++++-
 config/arm/meson.build           | 94 +++++++++++++++++++-------------
 2 files changed, 77 insertions(+), 38 deletions(-)
  

Comments

Honnappa Nagarahalli Nov. 8, 2020, 2:51 a.m. UTC | #1
<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
  
Juraj Linkeš Nov. 9, 2020, 12:48 p.m. UTC | #2
> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Sunday, November 8, 2020 3:51 AM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>; bruce.richardson@intel.com;
> Ruifeng Wang <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>;
> vcchunga@amazon.com; Dharmik Thakkar <Dharmik.Thakkar@arm.com>;
> jerinjacobk@gmail.com; hemant.agrawal@nxp.com; Ajit Khaparde
> (ajit.khaparde@broadcom.com) <ajit.khaparde@broadcom.com>;
> ferruh.yigit@intel.com; aconole@redhat.com
> Cc: dev@dpdk.org; nd <nd@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH v8 04/14] build: reformat and move Arm config and
> comments
> 
> <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.
> 

Judging from the previous explanatory comments, this file was also serving as an example cross file explaining the supported values, so I expanded that, because I like the concept of an example cross file. In my opinion, the comments actually do add a bit in that the user doesn't have to extract the supported values from code.
But, as mentioned in one of the other threads, we should move this to docs, so I'll remove the comments.

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

At first I though about the inconsistency this comment would introduce, but I think it's fine, since generic is a special build and having an explanation of this build only is welcome.

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

I added the aarch64 specifier since we're also defining armv7 build in this file, however briefly. I thought Implementer ID and part number also exist on armv7, is that not right? I wanted to make it explicit that these flags would only be used for aarch64 builds, but I guess it's implicit enough.

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

I am using the dictionary definition - use the config when no other alternative is found. We're using part_number_config_arm in four places, implementers generic, 0x41, 0x56, dpaa. For 0x41, it's the actual config, the other implementers default to it since we don't have specific config for them.

> >  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
  
Honnappa Nagarahalli Nov. 10, 2020, 12:02 a.m. UTC | #3
<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'
> > >

<snip>

> > >
> > > +# 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
> >
> 
> I added the aarch64 specifier since we're also defining armv7 build in this file,
Agree, no need to change

> however briefly. I thought Implementer ID and part number also exist on
> armv7, is that not right? I wanted to make it explicit that these flags would
> only be used for aarch64 builds, but I guess it's implicit enough.
> 
> > > +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?
> >
> 
> I am using the dictionary definition - use the config when no other alternative
> is found. We're using part_number_config_arm in four places, implementers
> generic, 0x41, 0x56, dpaa. For 0x41, it's the actual config, the other
> implementers default to it since we don't have specific config for them.
Ok

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

Patch

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
 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
+
+# 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 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]
+]
 flags_implementer_arm = [
 	['RTE_MACHINE', '"armv8a"'],
-	['RTE_MAX_LCORE', 16],
 	['RTE_USE_C11_MEM_MODEL', true],
-	['RTE_CACHE_LINE_SIZE', 64]]
+	['RTE_CACHE_LINE_SIZE', 64],
+	['RTE_MAX_LCORE', 16]
+]
 flags_implementer_cavium = [
+	['RTE_MAX_VFIO_GROUPS', 128],
 	['RTE_CACHE_LINE_SIZE', 128],
-	['RTE_MAX_NUMA_NODES', 2],
 	['RTE_MAX_LCORE', 96],
-	['RTE_MAX_VFIO_GROUPS', 128]]
+	['RTE_MAX_NUMA_NODES', 2]
+]
 flags_implementer_dpaa = [
 	['RTE_MACHINE', '"dpaa"'],
+	['RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', false],
 	['RTE_USE_C11_MEM_MODEL', true],
 	['RTE_CACHE_LINE_SIZE', 64],
-	['RTE_MAX_NUMA_NODES', 1],
 	['RTE_MAX_LCORE', 16],
-	['RTE_LIBRTE_DPAA2_USE_PHYS_IOVA', false]]
+	['RTE_MAX_NUMA_NODES', 1]
+]
 flags_implementer_emag = [
 	['RTE_MACHINE', '"emag"'],
 	['RTE_CACHE_LINE_SIZE', 64],
-	['RTE_MAX_NUMA_NODES', 1],
-	['RTE_MAX_LCORE', 32]]
+	['RTE_MAX_LCORE', 32],
+	['RTE_MAX_NUMA_NODES', 1]
+]
 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 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]
+]
 flags_part_number_thunderx2 = [
 	['RTE_MACHINE', '"thunderx2"'],
+	['RTE_ARM_FEATURE_ATOMICS', true],
+	['RTE_USE_C11_MEM_MODEL', true],
 	['RTE_CACHE_LINE_SIZE', 64],
-	['RTE_MAX_NUMA_NODES', 2],
 	['RTE_MAX_LCORE', 256],
-	['RTE_ARM_FEATURE_ATOMICS', true],
-	['RTE_USE_C11_MEM_MODEL', true]]
+	['RTE_MAX_NUMA_NODES', 2]
+]
 flags_part_number_octeontx2 = [
 	['RTE_MACHINE', '"octeontx2"'],
-	['RTE_MAX_NUMA_NODES', 1],
-	['RTE_MAX_LCORE', 36],
 	['RTE_ARM_FEATURE_ATOMICS', true],
+	['RTE_USE_C11_MEM_MODEL', true],
 	['RTE_EAL_IGB_UIO', false],
-	['RTE_USE_C11_MEM_MODEL', true]]
+	['RTE_MAX_LCORE', 36],
+	['RTE_MAX_NUMA_NODES', 1]
+]
 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
 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) != '')