Re: [dpdk-dev] [PATCH v8 14/14] build: add Arm SoC meson option

[Juraj Linkeš <juraj.linkes@pantheon.tech>]

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Monday, November 9, 2020 3:40 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 14/14] build: add Arm SoC meson option
> 
> <snip>
> 
> >
> > Add Arm SoC configuration to Arm meson.build and add a meson option to
> > enable those options for native builds. This is preferable to
> > specifying a cross file when doing aarch64 -> aarch64 builds, since
> > the cross file specifies the toolchain as well.
> >
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > ---
> >  config/arm/arm64_armada_linux_gcc      |   6 +-
> >  config/arm/arm64_armv8_linux_gcc       |  58 +++--------
> >  config/arm/arm64_bluefield_linux_gcc   |   6 +-
> >  config/arm/arm64_dpaa_linux_gcc        |   5 +-
> >  config/arm/arm64_emag_linux_gcc        |   5 +-
> >  config/arm/arm64_graviton2_linux_gcc   |   6 +-
> >  config/arm/arm64_n1sdp_linux_gcc       |   6 +-
> >  config/arm/arm64_octeontx2_linux_gcc   |   6 +-
> >  config/arm/arm64_stingray_linux_gcc    |   6 +-
> >  config/arm/arm64_thunderx2_linux_gcc   |   5 +-
> >  config/arm/arm64_thunderxt88_linux_gcc |   5 +-
> >  config/arm/meson.build                 | 132 ++++++++++++++++++++++++-
> >  meson_options.txt                      |   2 +
> >  13 files changed, 154 insertions(+), 94 deletions(-)
> >
> > diff --git a/config/arm/arm64_armada_linux_gcc
> > b/config/arm/arm64_armada_linux_gcc
> > index f5403f0a6..7cc40d1f4 100644
> > --- a/config/arm/arm64_armada_linux_gcc
> > +++ b/config/arm/arm64_armada_linux_gcc
> > @@ -14,8 +14,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementer_id = '0x56'
> > -max_lcores = 16
> > -max_numa_nodes = 1
> > -numa = false
> > -disabled_drivers = ['bus/dpaa', 'bus/fslmc', 'common/dpaax']
> > +soc = 'armada'
> > diff --git a/config/arm/arm64_armv8_linux_gcc
> > b/config/arm/arm64_armv8_linux_gcc
> > index 77e3d6278..d4cb6b5bf 100644
> > --- a/config/arm/arm64_armv8_linux_gcc
> > +++ b/config/arm/arm64_armv8_linux_gcc
> > @@ -13,46 +13,18 @@ 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'
> > -
> > -# Supported part_numbers for generic:
> > -# 'generic': valid for all armv8-a architectures (unoptimized
> > portable build) - part_number = 'generic'
> > -
> > -# Supported part_numbers for 0x41, 0x56, dpaa:
> > -# '0xd03':   cortex-a53
> > -# '0xd04':   cortex-a35
> > -# '0xd05':   cortex-a55
> > -# '0xd07':   cortex-a57
> > -# '0xd08':   cortex-a72
> > -# '0xd09':   cortex-a73
> > -# '0xd0a':   cortex-a75
> > -# '0xd0b':   cortex-a76
> > -# '0xd0c':   neoverse-n1
> > -
> > -# Supported part_numbers for 0x43:
> > -# '0xa1':    thunderxt88
> > -# '0xa2':    thunderxt81
> > -# '0xa3':    thunderxt83
> > -# '0xaf':    thunderx2t99
> > -# '0xb2':    octeontx2
> > -
> > -# Supported part_numbers for 0x50:
> > -# '0x0':     emag
> > -
> > -# Supported extra configuration
> > -# max_numa_nodes = n  # will set RTE_MAX_NUMA_NODES
> > -# max_lcores = n      # will set RTE_MAX_LCORE
> > -max_lcores = 256
> > -max_numa_nodes = 4
> > -
> > -# numa = false        # set to false if the target is not a NUMA system
> > -# disabled_drivers = ['bus/dpaa', 'crypto']
> > -    # add to the set of disabled libraries
> > +# Supported SoCs:
> > +# generic
> > +# armada
> > +# bluefield
> > +# dpaa
> > +# emag
> > +# graviton2
> > +# n1sdp
> > +# octeontx2
> > +# stingray
> > +# thunderx2
> > +# thunderxt88
> > +# thunderx2t99
> We can remove the comments here. This list needs to be documented.
> 

Yea, makes sense.

> > +
> > +soc = 'generic'
> > diff --git a/config/arm/arm64_bluefield_linux_gcc
> > b/config/arm/arm64_bluefield_linux_gcc
> > index 6bef87fbd..7b1fae8b9 100644
> > --- a/config/arm/arm64_bluefield_linux_gcc
> > +++ b/config/arm/arm64_bluefield_linux_gcc
> > @@ -13,8 +13,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementer_id = '0x41'
> > -part_number = '0xd08'
> > -max_lcores = 16
> > -max_numa_nodes = 1
> > -numa = false
> > +soc = 'bluefield'
> > diff --git a/config/arm/arm64_dpaa_linux_gcc
> > b/config/arm/arm64_dpaa_linux_gcc index 3458b9d7b..e52188842 100644
> > --- a/config/arm/arm64_dpaa_linux_gcc
> > +++ b/config/arm/arm64_dpaa_linux_gcc
> > @@ -14,7 +14,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementer_id = 'dpaa'
> > -max_lcores = 16
> > -max_numa_nodes = 1
> > -numa = false
> > +soc = 'dpaa'
> > diff --git a/config/arm/arm64_emag_linux_gcc
> > b/config/arm/arm64_emag_linux_gcc index 7cbb05510..6c24b4bca 100644
> > --- a/config/arm/arm64_emag_linux_gcc
> > +++ b/config/arm/arm64_emag_linux_gcc
> > @@ -13,7 +13,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementer_id = '0x50'
> > -part_number = '0x0'
> > -max_lcores = 32
> > -max_numa_nodes = 1
> > +soc = 'emag'
> > diff --git a/config/arm/arm64_graviton2_linux_gcc
> > b/config/arm/arm64_graviton2_linux_gcc
> > index cfe239797..bae35d6be 100644
> > --- a/config/arm/arm64_graviton2_linux_gcc
> > +++ b/config/arm/arm64_graviton2_linux_gcc
> > @@ -13,8 +13,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementor_id = '0x41'
> > -implementor_pn = '0xd0c'
> > -max_lcores = 64
> > -max_numa_nodes = 1
> > -numa = false
> > +soc = 'graviton2'
> > diff --git a/config/arm/arm64_n1sdp_linux_gcc
> > b/config/arm/arm64_n1sdp_linux_gcc
> > index b00f2d1ef..249ff4738 100644
> > --- a/config/arm/arm64_n1sdp_linux_gcc
> > +++ b/config/arm/arm64_n1sdp_linux_gcc
> > @@ -13,8 +13,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementer_id = '0x41'
> > -part_number = '0xd0c'
> > -max_lcores = 4
> > -max_numa_nodes = 1
> > -numa = false
> > +soc = 'n1sdp'
> > diff --git a/config/arm/arm64_octeontx2_linux_gcc
> > b/config/arm/arm64_octeontx2_linux_gcc
> > index 593769709..063018e8f 100644
> > --- a/config/arm/arm64_octeontx2_linux_gcc
> > +++ b/config/arm/arm64_octeontx2_linux_gcc
> > @@ -13,8 +13,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementer_id = '0x43'
> > -part_number = '0xb2'
> > -max_lcores = 36
> > -max_numa_nodes = 1
> > -numa = false
> > +soc = 'octeontx2'
> > diff --git a/config/arm/arm64_stingray_linux_gcc
> > b/config/arm/arm64_stingray_linux_gcc
> > index 6bef87fbd..1209a8c0b 100644
> > --- a/config/arm/arm64_stingray_linux_gcc
> > +++ b/config/arm/arm64_stingray_linux_gcc
> > @@ -13,8 +13,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementer_id = '0x41'
> > -part_number = '0xd08'
> > -max_lcores = 16
> > -max_numa_nodes = 1
> > -numa = false
> > +soc = 'stingray'
> > diff --git a/config/arm/arm64_thunderx2_linux_gcc
> > b/config/arm/arm64_thunderx2_linux_gcc
> > index c06dcdc2b..348650712 100644
> > --- a/config/arm/arm64_thunderx2_linux_gcc
> > +++ b/config/arm/arm64_thunderx2_linux_gcc
> > @@ -13,7 +13,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementer_id = '0x43'
> > -part_number = '0xaf'
> > -max_lcores = 256
> > -max_numa_nodes = 2
> > +soc = 'thunderx2'
> > diff --git a/config/arm/arm64_thunderxt88_linux_gcc
> > b/config/arm/arm64_thunderxt88_linux_gcc
> > index 3ba1528e4..d31d0c6d8 100644
> > --- a/config/arm/arm64_thunderxt88_linux_gcc
> > +++ b/config/arm/arm64_thunderxt88_linux_gcc
> > @@ -13,7 +13,4 @@ cpu = 'armv8-a'
> >  endian = 'little'
> >
> >  [properties]
> > -implementer_id = '0x43'
> > -part_number = '0xa1'
> > -max_lcores = 96
> > -max_numa_nodes = 1
> > +soc = 'thunderxt88'
> > diff --git a/config/arm/meson.build b/config/arm/meson.build index
> > 52274c0c5..cd270f84b 100644
> > --- a/config/arm/meson.build
> > +++ b/config/arm/meson.build
> > @@ -57,7 +57,7 @@ part_number_config_arm = {  ## Part numbers are
> > specific to Arm implementers  # implementer specific aarch64 flags
> > have middle priority
> >  #     (will overwrite common flags)
> > -# part number specific aarch64 flags have the highest priority
> > +# part number specific aarch64 flags have higher priority
> >  #     (will overwrite both common and implementer specific flags)
> >  implementers = {
> >  	'generic': {
> > @@ -164,6 +164,102 @@ implementers = {
> >  	}
> >  }
> >
> > +# soc specific aarch64 flags have the highest priority
> > +#     (will overwrite all other flags)
> > +socs = {
> Too big to maintain
> 

I don't have great alternate solutions that would work:
1. Possibly adding 'soc' under machine specific configuration. That would result in terrible code (O(N)) which would have to go through all implementer/part numbers to find the soc configuration.
2. Move the config from this dict to machine specific configuration as in 1, but also keep the backwards mapping of soc->(implementer, part_number). That way we'll have sane code which just does dict lookups (O(log N), much better).

Actually, the second option doesn't look that bad. We'll end up with extra mapping like this:

'bluefield': {'implementer': '0x41', 'part_number': '0xd08'}

This would be in the soc dict and then the extra soc configuration in part number config.

NOTE: some socs have the same flags as their corresponsing part number flags (actually all of them except n1sdp). Do we want to make soc flags optional or do we want them to be speficied even when they won't change the config?

> > +	'generic': {
> > +		'implementer': 'generic',
> > +		'part_number': 'generic',
> > +		'flags': []
> > +	},
> > +	'armada': {
> > +		'implementer': '0x56',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 16],
> > +			['RTE_MAX_NUMA_NODES', 1]
> > +		],
> > +		'numa': false,
> > +		'disabled_drivers': ['bus/dpaa', 'bus/fslmc', 'common/dpaax']
> > +	},
> > +	'bluefield': {
> > +		'implementer': '0x41',
> > +		'part_number': '0xd08',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 16],
> > +			['RTE_MAX_NUMA_NODES', 1]
> > +		],
> > +		'numa': false
> > +	},
> > +	'dpaa': {
> > +		'implementer': 'dpaa',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 16],
> > +			['RTE_MAX_NUMA_NODES', 1]
> > +		],
> > +		'numa': false
> > +	},
> > +	'emag': {
> > +		'implementer': '0x50',
> > +		'part_number': '0x0',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 32],
> > +			['RTE_MAX_NUMA_NODES', 1]
> > +		]
> > +	},
> > +	'graviton2': {
> > +		'implementer': '0x41',
> > +		'part_number': '0xd0c',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 64],
> > +			['RTE_MAX_NUMA_NODES', 1]
> > +		],
> > +		'numa': false
> > +	},
> > +	'n1sdp': {
> > +		'implementer': '0x41',
> > +		'part_number': '0xd0c',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 4],
> > +			['RTE_MAX_NUMA_NODES', 1]
> > +		],
> > +		'numa': false
> > +	},
> > +	'octeontx2': {
> > +		'implementer': '0x43',
> > +		'part_number': '0xb2',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 32],
> > +			['RTE_MAX_NUMA_NODES', 1]
> > +		],
> > +		'numa': false
> > +	},
> > +	'stingray': {
> > +		'implementer': '0x41',
> > +		'part_number': '0xd08',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 16],
> > +			['RTE_MAX_NUMA_NODES', 1]
> > +		],
> > +		'numa': false
> > +	},
> > +	'thunderx2': {
> > +		'implementer': '0x43',
> > +		'part_number': '0xaf',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 256],
> > +			['RTE_MAX_NUMA_NODES', 2]
> > +		]
> > +	},
> > +	'thunderxt88': {
> > +		'implementer': '0x43',
> > +		'part_number': '0xa1',
> > +		'flags': [
> > +			['RTE_MAX_LCORE', 96],
> > +			['RTE_MAX_NUMA_NODES', 1]
> > +		]
> > +	}
> > +}
> > +
> >  dpdk_conf.set('RTE_ARCH_ARM', 1)
> >  dpdk_conf.set('RTE_FORCE_INTRINSICS', 1)
> >
> > @@ -176,11 +272,18 @@ if dpdk_conf.get('RTE_ARCH_32')  else
> >  	# aarch64 build
> >  	use_native_machine_args = false
> > +	arm_soc = get_option('arm_soc')
> > +	soc_config = {}
> >  	if not meson.is_cross_build()
> >  		if machine == 'generic'
> >  			# generic build
> > +			if arm_soc != ''
> > +				error('Arm SoC is unsupported with generic
> > build.')
> > +			endif
> >  			implementer_id = 'generic'
> >  			part_number = 'generic'
> > +		elif arm_soc != ''
> > +			soc_config = socs.get(arm_soc, {'not_supported':
> > true})
> >  		else
> >  			# native build
> >  			# The script returns ['Implementer', 'Variant',
> 'Architecture', @@
> > -199,8 +302,27 @@ else
> >  		endif
> >  	else
> >  		# cross build
> > -		implementer_id =
> > meson.get_cross_property('implementer_id')
> > -		part_number = meson.get_cross_property('part_number')
> > +		arm_soc = meson.get_cross_property('soc', '')
> > +		if arm_soc == ''
> > +			error('Arm SoC must be specified in the cross file.')
> > +		endif
> > +		soc_config = socs.get(arm_soc, {'not_supported': true})
> > +	endif
> > +
> > +	soc_flags = []
> > +	if soc_config.has_key('not_supported')
> > +		error('SoC @0@ not supported.'.format(arm_soc))
> > +	elif soc_config != {}
> > +		implementer_id = soc_config['implementer']
> > +		implementer_config = implementers[implementer_id]
> > +		part_number = soc_config['part_number']
> > +		soc_flags = soc_config['flags']
> > +		if not soc_config.get('numa', true)
> > +			has_libnuma = 0
> > +		endif
> > +		if soc_config.has_key('disabled_drivers')
> > +			disabled_drivers += soc_config['disabled_drivers']
> > +		endif
> >  	endif
> >
> >  	if implementers.has_key(implementer_id)
> > @@ -226,8 +348,8 @@ else
> >  		      '(-Dmachine=generic) build.')
> >  	endif
> >
> > -	# use default flags with implementer flags
> > -	dpdk_flags = flags_common_default + implementer_config['flags'] +
> > part_number_config.get('flags', [])
> > +	# add flags in the proper order
> > +	dpdk_flags = flags_common_default + implementer_config['flags'] +
> > +part_number_config.get('flags', []) + soc_flags
> >
> >  	# apply supported machine args
> >  	machine_args = [] # Clear previous machine args diff --git
> > a/meson_options.txt b/meson_options.txt index e1059fb16..33b8b236c
> > 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -1,5 +1,7 @@
> >  # Please keep these options sorted alphabetically.
> >
> > +option('arm_soc', type: 'string', value: '',
> > +	description: 'Specify if you want to build for a particular Arm SoC
> > +when building on an aarch64 machine.')
> Kind of indicates that it is a cross build.

What do you mean by cross build in this context? It's not a meson cross build, since it doesn't mention cross files, so I don't think you meant that. Aside from that, how does building on an aarch64 machine for aarch64 target indicate a cross build? I guess it says Arm SoC, so it could be a aarch64 -> armv7 cross build? Then I could change it to "a particular aarch64 Arm SoC".

> How about: "Build natively for the specified Arm SoC"?
> 

I specifically didn't want to use the word native here because arm native build implies ['-march=native', '-mtune=native'], which won't be used in soc builds. Also, this wording assumes an aarch64 build machine, which is likely a safe assumption, but I wanted to avoid the assumption and spell it out.

> >  option('armv8_crypto_dir', type: 'string', value: '',
> >  	description: 'path to the armv8_crypto library installation
> > directory') option('disable_drivers', type: 'string', value: '',
> > --
> > 2.20.1