Re: [dpdk-dev] [RFC PATCH 1/2] build: rework Arm build options

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

<snip>

> >
> > >
> > > The current system can identify only the implementer and part number
> > > of the arm device we're targeting, which is enough to identify the target
> CPU.
> > > However, even the full MIDR information is not enough to identify
> > > the SoC we're targeting.
> > >
> > > Expand the "machine" meson variable to allow specifying the target arm
> SoC.
> > > The SoC identification implies the CPU, so drop the current MIDR
> > > based CPU identification in favor of user input, which is the only
> > > way to identify the target SoC. This implies that native builds are
> > > not possible. Instead do a default (non-optimized, but executable on
> > > any
> > > aarch64 device) build.
> > >
> > > Also use the machine custom property in cross files for cross builds.
> > >
> > > An example of where the current system is insufficient is the cortex-a72
> CPU.
> > > These cores are used in a variety of SoC from Broadcomm, NXP, Huawei
> > > and others. The MIDR information would only identify the core, but
> > > there's not way to identify the SoC to fine tune the build to it.
> > >
> > > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > ---
> > >  config/arm/arm64_armv8_linux_gcc              |  28 ----
> > >  config/arm/default/arm64_armv8_linux_gcc      |  23 +++
> > >  config/arm/default/meson.build                |   8 +
> > >  config/arm/meson.build                        | 139 +++++++++++-------
> > >  .../{ => thunderx}/arm64_thunderx_linux_gcc   |   2 +-
> > >  config/arm/thunderx/meson.build               |  18 +++
> > >  .../thunderxt88/arm64_thunderxt88_linux_gcc   |  16 ++
> > >  config/arm/thunderxt88/meson.build            |  18 +++
> > >  config/meson.build                            |   9 +-
> > >  9 files changed, 177 insertions(+), 84 deletions(-)  delete mode
> > > 100644 config/arm/arm64_armv8_linux_gcc  create mode 100644
> > > config/arm/default/arm64_armv8_linux_gcc
> > >  create mode 100644 config/arm/default/meson.build  rename
> > > config/arm/{ => thunderx}/arm64_thunderx_linux_gcc (92%)  create
> > > mode
> > > 100644 config/arm/thunderx/meson.build  create mode 100644
> > > config/arm/thunderxt88/arm64_thunderxt88_linux_gcc
> > >  create mode 100644 config/arm/thunderxt88/meson.build
> > >
> > > diff --git a/config/arm/arm64_armv8_linux_gcc
> > > b/config/arm/arm64_armv8_linux_gcc
> > > deleted file mode 100644
> > > index 88f0ff9da..000000000
> > > --- a/config/arm/arm64_armv8_linux_gcc
> > > +++ /dev/null
> > > @@ -1,28 +0,0 @@
> > > -[binaries]
> > > -c = 'aarch64-linux-gnu-gcc'
> > > -cpp = 'aarch64-linux-gnu-cpp'
> > > -ar = 'aarch64-linux-gnu-gcc-ar'
> > > -strip = 'aarch64-linux-gnu-strip'
> > > -pkgconfig = 'aarch64-linux-gnu-pkg-config'
> > > -pcap-config = ''
> > > -
> > > -[host_machine]
> > > -system = 'linux'
> > > -cpu_family = 'aarch64'
> > > -cpu = 'armv8-a'
> > > -endian = 'little'
> > > -
> > > -[properties]
> > > -implementor_id = 'generic'
> > > -
> > > -# Valid options for Arm's implementor_pn:
> > > -# 'default': valid for all armv8-a architectures (default value)
> > > -# '0xd03':   cortex-a53
> > > -# '0xd04':   cortex-a35
> > > -# '0xd05':   cortex-a55
> > > -# '0xd07':   cortex-a57
> > > -# '0xd08':   cortex-a72
> > > -# '0xd09':   cortex-a73
> > > -# '0xd0a':   cortex-a75
> > > -# '0xd0b':   cortex-a76
> > > -implementor_pn = 'default'
> > > diff --git a/config/arm/default/arm64_armv8_linux_gcc
> > > b/config/arm/default/arm64_armv8_linux_gcc
> > > new file mode 100644
> > > index 000000000..56efd7a05
> > > --- /dev/null
> > > +++ b/config/arm/default/arm64_armv8_linux_gcc
> > > @@ -0,0 +1,23 @@
> > > +[binaries]
> > > +c = 'aarch64-linux-gnu-gcc'
> > > +cpp = 'aarch64-linux-gnu-cpp'
> > > +ar = 'aarch64-linux-gnu-gcc-ar'
> > > +strip = 'aarch64-linux-gnu-strip'
> > > +pkgconfig = 'aarch64-linux-gnu-pkg-config'
> > > +pcap-config = ''
> > > +
> > > +[host_machine]
> > > +system = 'linux'
> > > +cpu_family = 'aarch64'
> > > +cpu = 'armv8-a'
> > > +endian = 'little'
> > > +
> > > +[properties]
> > > +# Valid values for the machine property are the soc names
> > > +identified by # directories in config/arm # Possible suffixes for socs:
> > > +# -mn-kc
> > > +# where m is the number of numa nodes and k is the number of lcores
> > > +# e.g. for thunderx soc the possible values are "thunderx" or
> > > +"thunderx-1n-
> > > 48c"
> > Is it required to talk about the suffixes? I think the statement
> > "Valid values for the machine property are the soc names identified by
> directories in config/arm"
> > should be sufficient.
> >
> 
> Yes, this is not the right place for this, so a shorted explanation is enough.
> 
> > > +# the suffixes themselves need to be supported in
> > > +config/arm/<soc_dir>/meson.build machine = 'default'
> > IMO, "default" does not convey much information. Is it possible to
> > change it without a deprecation notice? "generic" or "armv8" conveys more
> information.
> > Any opinions from others?
> >
> 
> Maybe we can have multiple labels mean the same thing - default, generic,
> armv8 or even generic-armv8 (that's the most descriptive name IMO) could
> all mean the generic arm build executable on all aarch64 SoCs. That many
> names is probably confusing, but sticking to two (default and one more)
> seems feasible.
> 
> What does everyone else think?
'armv8' makes lot more sense to me and it conveys what the build does. We can keep 'default' to be backward compatible, we can skip others.

> 
> > > diff --git a/config/arm/default/meson.build
> > > b/config/arm/default/meson.build new file mode 100644 index
> > > 000000000..5714d0084
> > > --- /dev/null
> > > +++ b/config/arm/default/meson.build
> > > @@ -0,0 +1,8 @@
> > > +flags = [
> > > +	['RTE_MACHINE', '"armv8a"'],
> > > +	['RTE_MAX_LCORE', 256],
> > > +	['RTE_USE_C11_MEM_MODEL', true],
> > > +	['RTE_CACHE_LINE_SIZE', 128]
> > > +]
> > > +
> > > +machine_args = ['-march=armv8-a+crc']
> > > diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > > 8728051d5..05b790f40 100644
> > > --- a/config/arm/meson.build
> > > +++ b/config/arm/meson.build
> > > @@ -2,11 +2,11 @@
> > >  # Copyright(c) 2017 Intel Corporation.
> > >  # Copyright(c) 2017 Cavium, Inc
> > >
> > > -# for checking defines we need to use the correct compiler flags
> > > -march_opt = '-march=@0@'.format(machine)
> > > -
> > > -arm_force_native_march = false
> > > -arm_force_default_march = (machine == 'default')
> > > +if machine == 'native'
> > > +	# arm doesn't support a native build - user input of soc is required
> > > +	# default to the portable build instead of native
> > > +	machine = 'default'
> > Do we need a message here to the user?
> >
> 
> Better to have one, good catch.
> 
> > > +endif
> > >
> > >  flags_common_default = [
> > >  	# Accelarate rte_memcpy. Be sure to run unit test
> > > (memcpy_perf_autotest) @@ -131,6 +131,7 @@ impl_dpaa = ['NXP
> DPAA',
> > > flags_dpaa, machine_args_generic]
> > > dpdk_conf.set('RTE_FORCE_INTRINSICS',
> > > 1)
> > >
> > >  if not dpdk_conf.get('RTE_ARCH_64')
> > > +	# armv7 build
> > >  	dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
> > >  	dpdk_conf.set('RTE_ARCH_ARM', 1)
> > >  	dpdk_conf.set('RTE_ARCH_ARMv7', 1) @@ -138,41 +139,9 @@ if not
> > > dpdk_conf.get('RTE_ARCH_64')
> > >  	# mk/machine/armv7a/rte.vars.mk sets it too
> > >  	machine_args += '-mfpu=neon'
> > >  else
> > > +	# aarch64 build
> > >  	dpdk_conf.set('RTE_CACHE_LINE_SIZE', 128)
> > >  	dpdk_conf.set('RTE_ARCH_ARM64', 1)
> > > -
> > > -	machine = []
> > > -	cmd_generic = ['generic', '', '', 'default', '']
> > > -	cmd_output = cmd_generic # Set generic by default
> > > -	machine_args = [] # Clear previous machine args
> > > -	if arm_force_default_march and not meson.is_cross_build()
> > > -		machine = impl_generic
> > > -		impl_pn = 'default'
> > > -	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
> > > -	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)
> > > -	endif
> > > -
> > >  	# Apply Common Defaults. These settings may be overwritten by
> > > machine
> > >  	# settings later.
> > >  	foreach flag: flags_common_default @@ -181,30 +150,89 @@ else
> > >  		endif
> > >  	endforeach
> > >
> > > -	message('Implementer : ' + machine[0])
> > > -	foreach flag: machine[1]
> > > +	soc = machine.split('-')[0]
> > > +	# TODO make sure the subdir exists
> > > +	subdir(soc)
> > > +
> > > +	if soc != 'default'
> > > +		# combine common and soc specific flags
> > > +		# TODO check that soc is in the dict
> > > +		flags = flags['common'] + flags[soc]
> > > +	endif
> > > +
> > > +	foreach flag: 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
> > > +	verified_machine_args = []
> > > +
> > > +	foreach flag: machine_args
> > > +		if cc.has_argument(flag)
> > > +			verified_machine_args += flag
> > > +		else
> > > +			message('Unsupported machine flag "@0@",
> > > ignoring'.format(flag))
> > >  		endif
> > >  	endforeach
> > > +	machine_args = verified_machine_args
> > > +
> > > +#	machine = []
> > > +#	cmd_generic = ['generic', '', '', 'default', '']
> > > +#	cmd_output = cmd_generic # Set generic by default
> > > +#	machine_args = [] # Clear previous machine args
> > > +#	if arm_force_default_march and not meson.is_cross_build()
> > > +#		machine = impl_generic
> > > +#		impl_pn = 'default'
> > > +#	elif not meson.is_cross_build()
> > Isn't this for native build? Do we still need this?
> >
> 
> Yes, I should've removed this instead of commenting it out.
> 
> > > +#		# 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
> > > +#	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)
> > > +#	endif
> > > +#
> > > +#	message('Implementer : ' + machine[0])
> > > +#	foreach flag: machine[1]
> > > +#		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) != '') @@
> > > -225,3
> > > +253,6 @@ if cc.get_define('__ARM_FEATURE_CRYPTO', args:
> > > +machine_args)
> > != ''
> > >  	compile_time_cpuflags += ['RTE_CPUFLAG_AES',
> > 'RTE_CPUFLAG_PMULL',
> > >  	'RTE_CPUFLAG_SHA1', 'RTE_CPUFLAG_SHA2']  endif
> > > +
> > > +message('dpdk conf options after arm stuff: @0@'.format(dpdk_conf))
> > > +
> > > diff --git a/config/arm/arm64_thunderx_linux_gcc
> > > b/config/arm/thunderx/arm64_thunderx_linux_gcc
> > > similarity index 92%
> > > rename from config/arm/arm64_thunderx_linux_gcc
> > > rename to config/arm/thunderx/arm64_thunderx_linux_gcc
> > > index 6572ab615..e53a86e8f 100644
> > > --- a/config/arm/arm64_thunderx_linux_gcc
> > > +++ b/config/arm/thunderx/arm64_thunderx_linux_gcc
> > > @@ -13,4 +13,4 @@ cpu = 'armv8-a'
> > >  endian = 'little'
> > >
> > >  [properties]
> > > -implementor_id = '0x43'
> > > +machine = 'thunderx'
> > > diff --git a/config/arm/thunderx/meson.build
> > > b/config/arm/thunderx/meson.build new file mode 100644 index
> > > 000000000..873d79a64
> > > --- /dev/null
> > > +++ b/config/arm/thunderx/meson.build
> > > @@ -0,0 +1,18 @@
> > > +flags = {
> > > +	'common': [
> > > +		['RTE_CACHE_LINE_SIZE', 128],
> > > +		['RTE_MAX_VFIO_GROUPS', 128],
> > > +		['RTE_MACHINE', '"thunderx"'],
> > > +		['RTE_USE_C11_MEM_MODEL', false]
> > > +	],
> > > +	'thunderx': [
> > > +		['RTE_MAX_NUMA_NODES', 2],
> > > +		['RTE_MAX_LCORE', 96]
> > > +	],
> > > +	'thunderx-1n-48c': [
> > > +		['RTE_MAX_NUMA_NODES', 1],
> > > +		['RTE_MAX_LCORE', 48]
> > > +	]
> > > +}
> > > +
> > > +machine_args = ['-march=armv8-a+crc+crypto','-mcpu=thunderx']
> > > diff --git a/config/arm/thunderxt88/arm64_thunderxt88_linux_gcc
> > > b/config/arm/thunderxt88/arm64_thunderxt88_linux_gcc
> > > new file mode 100644
> > > index 000000000..9a622786c
> > > --- /dev/null
> > > +++ b/config/arm/thunderxt88/arm64_thunderxt88_linux_gcc
> > > @@ -0,0 +1,16 @@
> > > +[binaries]
> > > +c = 'aarch64-linux-gnu-gcc'
> > > +cpp = 'aarch64-linux-gnu-cpp'
> > > +ar = 'aarch64-linux-gnu-gcc-ar'
> > > +strip = 'aarch64-linux-gnu-strip'
> > > +pkgconfig = 'aarch64-linux-gnu-pkg-config'
> > > +pcap-config = ''
> > > +
> > > +[host_machine]
> > > +system = 'linux'
> > > +cpu_family = 'aarch64'
> > > +cpu = 'armv8-a'
> > > +endian = 'little'
> > > +
> > > +[properties]
> > > +machine = 'thunderxt88'
> > > diff --git a/config/arm/thunderxt88/meson.build
> > > b/config/arm/thunderxt88/meson.build
> > > new file mode 100644
> > > index 000000000..2aa5d2cdd
> > > --- /dev/null
> > > +++ b/config/arm/thunderxt88/meson.build
> > > @@ -0,0 +1,18 @@
> > > +flags = {
> > > +	'common': [
> > > +		['RTE_CACHE_LINE_SIZE', 128],
> > > +		['RTE_MAX_VFIO_GROUPS', 128],
> > > +		['RTE_MACHINE', '"thunderx"'],
> > > +		['RTE_USE_C11_MEM_MODEL', false]
> > > +	],
> > > +	'thunderxt88': [
> > > +		['RTE_MAX_NUMA_NODES', 2],
> > > +		['RTE_MAX_LCORE', 96]
> > > +	],
> > > +	'thunderxt88-1n-48c': [
> > > +		['RTE_MAX_NUMA_NODES', 1],
> > > +		['RTE_MAX_LCORE', 48]
> > > +	]
> > > +}
> > > +
> > > +machine_args = ['-mcpu=thunderxt88']
> > > diff --git a/config/meson.build b/config/meson.build index
> > > 69f2aeb60..177cbd49c 100644
> > > --- a/config/meson.build
> > > +++ b/config/meson.build
> > > @@ -63,7 +63,14 @@ meson.add_install_script('../buildtools/symlink-
> > > drivers-solibs.sh',
> > >
> > >  # set the machine type and cflags for it  if meson.is_cross_build()
> > > -	machine = host_machine.cpu()
> > > +	if not host_machine.cpu_family().startswith('aarch')
> > > +		# don't change the machine config for aarch32/64 builds
> > > +		# that config is set in the cross file to identify the soc
> > > +		# we're building for
> > > +		machine = host_machine.cpu()
> > > +	else
> > > +		machine = meson.get_cross_property('machine', 'default')
> > > +	endif
> > >  else
> > >  	machine = get_option('machine')
> > >  endif
> > > --
> > > 2.20.1