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

Juraj Linkeš juraj.linkes at pantheon.tech
Tue Nov 3 11:54:13 CET 2020



> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>
> Sent: Monday, November 2, 2020 8:32 PM
> 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
> 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 v4 2/6] build: refactor Arm build
> 
> <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'.
> > > > >
> > > >
> > > > This default means what default means, no special meaning, that is
> > > > if there isn't a more specific configuration, default to this one.
> > > > It's possible that generic is better, but now that I think about
> > > > it, using something else than default or generic might be the best
> > > > to avoid confusion. Possibly just part_number_arm, which will make
> > > > it in line with the
> > > other var names.
> > > Agree, better to call it 'part_number_arm'.
> > >
> > > >
> > > > > > += {
> > > > > > +	'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.
> > > > >
> > > >
> > > > This actually means generic part for the implementer, not generic
> > > > for everything. I understand this is here to produce binaries that
> > > > would run on everything from that impelemeter (in line of what you
> > > > mention below, this would be implementer-generic configuration, a
> > > > fourth category). In my patchset, it's also a fallback when
> > > > building for an unknown part number from the implementer.
> > > We do not need implementer-generic binaries/build. However, we will
> > > have some parameters that are common across all the part numbers
> > > from that implementer (probably we should not call it as
> > > 'implementer-generic' to avoid confusion. May be
> > > 'implementer-common-
> > flags' or 'implementer-flags-extra').
> > > Those parameters can be used for every part number.
> >
> > These are currently named flags_<implementer> such as flags_arm and
> > flags_cavium. We could rename them to implementer_flags_<implementer>.
> Ok
> 
> >
> > >
> > > If we know the implementer, we will have the part number as well (this
> > > is part of the Arm architecture definition). Unknown part number
> > > should result in an error message.
> > >
> >
> > Yes, we'll always have both. But there are still a couple of corner cases with
> > native builds, which should always work:
> I do not think these are corner cases. These are basically cases where the
> support is not added yet in the build system. We do not need to handle these
> specifically.
> I also do not think the notion that the 'native' build should always work is correct
> (at least on Arm platforms). The reason I say that is because 'native', IMO, not
> only defines the ISA, but other parameters as well in DPDK.
> 

So our stance would basically be the native build doesn't work for unsupported implementers/part numbers and then direct the user to generic build. That seems reasonable.

> > 1. We don't support the implementer (i.e. there's no variable with name
> > implementer_<implementer_id>). In this case, what do default to? The
> > generic build, which is the current behavior?
> This should result in an error. The implementer should add support in
> meson.build
> 

Ok, then we need to change the current configuration. The implementers were added in bulk with the same configuration, so that can't really be considered supporting them in this way. I think we should keep implementers which have specific implementer flags or machine configuration defined for them and remove the rest. That would leave us with:
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]
implementer_0x50 = ['Ampere Computing', flags_implementer_emag, part_number_config_emag]
implementer_0x56 = ['Marvell ARMADA', flags_implementer_armada, part_number_config_arm]
implementer_dpaa = ['NXP DPAA', flags_implementer_dpaa, part_number_config_arm]

And we would remove these (everything with the same flags/part number config as the generic build):
implementer_0x42 = ['Broadcom', flags_implementer_generic, part_number_config_arm]
implementer_0x44 = ['DEC', flags_implementer_generic, part_number_config_arm]
implementer_0x49 = ['Infineon', flags_implementer_generic, part_number_config_arm]
implementer_0x4d = ['Motorola', flags_implementer_generic, part_number_config_arm]
implementer_0x4e = ['NVIDIA', flags_implementer_generic, part_number_config_arm]
implementer_0x51 = ['Qualcomm', flags_implementer_generic, part_number_config_arm]
implementer_0x53 = ['Samsung', flags_implementer_generic, part_number_config_arm]
implementer_0x69 = ['Intel', flags_implementer_generic, part_number_config_arm]

> > 2. We support the implementer, but we don't support the part number. In
> > this case, we can just use native machine args (-march=native) and use only
> > implementer specific flags.
> Same thing, should result in error. The implementer should add support in
> meson.build.
> 

There are two different scenarios, native and soc-specific/cross builds.
For native builds, we could just use the implementer flags with native machine args. This could be still viewed as implementer intending to support it for native build - by omitting it, they'd basically be saying there aren't any extra flags for the part number. There are hidden assumptions here and it results in data that's not clear, so It's probably best to enforce even for native builds, as that we remove any ambiguity.
For soc-specific/cross builds, we don't know which machine args to use, so that's an error.

I'll make the changes so that an unknown implementer/part number results in an error for all builds.

> >
> > For generic, soc-specifc and cross builds it makes sense to error with uknown
> > implementer/part number, since we wouldn't know what to build otherwise.
> >
> > > >
> > > > Since this is not generic for everything, only for the implementer,
> > > > we're lacking the true common default machine args for everything.
> > > >
> > > > > > +	'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.
> > > >
> > > > This should be core-generic configuration, I can rename it to
> > > > flags_n1generic_extra.
> > > Agree.
> > >
> > > >
> > > > > 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
> > > > >
> > > >
> > > > The fourth category I mentioned above, implementer-generic, is used
> > > > in how
> > > We should not have this category.
> > >
> > > > the arm configuration is currently processed:
> > > > 1) default configuration
> > > > Added to/overwritten by
> > > > 2) implementer configuration (one of which is the configuration for
> > > > generic
> > > > build) Added to/overwritten by
> > > This should be just parameters that are common across all the part
> > > numbers of that implementer (for ex: RTE_CACHE_LINE_SIZE = 128), not a
> > build.
> > >
> >
> > It's not a build, just how the configation is organized and processed, as in we
> > apply the common default flags, then implementer flags, then part number
> > flags and then soc flags, overwriting flags in all of the steps.
> Ok
> 
> >
> > > > 3) part number (or core-generic) configuration Added to/overwritten
> > > > by
> > > > 4) SoC configuration (what we're adding now)
> > > >
> > > > It's not organized as cleanly - the machine args contain both 2) and 3).
> > > > Depending on what we want to do with the 'generic' part number I'll
> > > > adjust the config organization.
> > > >
> > > > >  } 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.
> > > > >
> > > >
> > > > The generic config for these makes sense if a fourth category,
> > > > implementer- generic makes sense.
> > > > For example, for part_number_config_emag this means: for
> > implementer
> > > > _0x50 (Ampere Computing) use the generic machine args if there's
> > > > nothing more specific (which there isn't - no other part number).
> > > There should be a part number for this. Not sure why it is not present
> > > here. I will find out the info.
> > >
> > > >
> > > > > >
> > > > > >  ## 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
> > > > >
> > > >
> > > > Is that because the docs mentioned here may not be the most recent?
> > > > It was useful to me in understanding where the implementer/part
> > > > number values come from.
> > > Yes, mainly because the spec gets updated. Instead you could say
> > > "Refer to MIDR in Arm Architecture Reference Manual")
> > >
> >
> > Ok, I'll make this change.
> >
> > > >
> > > > > > -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?
> > > > >
> > > >
> > > > Already addressed in the next version.
> > > >
> > > > > > +			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



More information about the dev mailing list