> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> Sent: Wednesday, October 28, 2020 5:59 PM
> To: Juraj Linkeš <juraj.lin...@pantheon.tech>; bruce.richard...@intel.com;
> Ruifeng Wang <ruifeng.w...@arm.com>; Phil Yang <phil.y...@arm.com>;
> vcchu...@amazon.com; Dharmik Thakkar <dharmik.thak...@arm.com>;
> jerinjac...@gmail.com; hemant.agra...@nxp.com
> Cc: dev@dpdk.org; nd <n...@arm.com>; Honnappa Nagarahalli
> <honnappa.nagaraha...@arm.com>; nd <n...@arm.com>
> Subject: RE: [PATCH v4 2/6] build: refactor Arm build
>
> <snip>
>
> >
> > * Rename variables to have names that better describe what the
> > variables store
> > * Remove unused or superfluous variables
> > * Change a list to dictionary where key lookup is needed
> > * Add informatory comments in the code
> > * Minor code restructure and reformatting
> >
> > Signed-off-by: Juraj Linkeš <juraj.lin...@pantheon.tech>
> > ---
> > config/arm/arm64_armada_linux_gcc | 2 +-
> > config/arm/arm64_armv8_linux_gcc | 8 +-
> > config/arm/arm64_bluefield_linux_gcc | 4 +-
> > config/arm/arm64_dpaa_linux_gcc | 2 +-
> > config/arm/arm64_emag_linux_gcc | 2 +-
> > config/arm/arm64_n1sdp_linux_gcc | 4 +-
> > config/arm/arm64_octeontx2_linux_gcc | 4 +-
> > config/arm/arm64_stingray_linux_gcc | 4 +-
> > config/arm/arm64_thunderx2_linux_gcc | 4 +-
> > config/arm/arm64_thunderx_linux_gcc | 2 +-
> > config/arm/meson.build | 247 +++++++++++++++------------
> > 11 files changed, 153 insertions(+), 130 deletions(-)
> >
>
> <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.
> > += {
> > + '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.
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.
> 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 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
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).
> >
> > ## 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.
> > -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
