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

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

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

> 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

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

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