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

Juraj Linkeš juraj.linkes at pantheon.tech
Mon Nov 9 13:48:02 CET 2020



> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>
> Sent: Sunday, November 8, 2020 3:51 AM
> To: Juraj Linkeš <juraj.linkes at pantheon.tech>; bruce.richardson at intel.com;
> Ruifeng Wang <Ruifeng.Wang at arm.com>; Phil Yang <Phil.Yang at arm.com>;
> vcchunga at amazon.com; Dharmik Thakkar <Dharmik.Thakkar at arm.com>;
> jerinjacobk at gmail.com; hemant.agrawal at nxp.com; Ajit Khaparde
> (ajit.khaparde at broadcom.com) <ajit.khaparde at broadcom.com>;
> ferruh.yigit at intel.com; aconole at redhat.com
> Cc: dev at dpdk.org; nd <nd at arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli at arm.com>; nd <nd at 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 at pantheon.tech>
> Few nits, otherwise, looks good.
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli at 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



More information about the dev mailing list