[dpdk-dev] [PATCH v19 1/3] build: disable/enable drivers in Arm builds

[Bruce Richardson]

On Fri, Apr 09, 2021 at 10:41:17AM +0200, Juraj Linkeš wrote:
> Add support for enabling or disabling drivers for Arm cross build. Do
> not implement any enable/disable lists yet.
> 
> Enabling drivers is useful when building for an SoC where we only want
> to build a few drivers. That way the list won't be too long.
> 
> Similarly, disabling drivers is useful when we want to disable only a
> few drivers.
> 
> Both of these are advantageous mainly in aarch64 -> aarch64 (or arch ->
> same arch) builds, where the build machine may have the required driver
> dependencies, yet we don't want to build drivers for a specific SoC.
> 
> By default, build all drivers for which dependencies are found. If
> enabled_drivers is a non-empty list, build only those drivers.  If
> disabled_drivers is non-empty list, build all drivers except those in
> disabled_drivers. Error out if both are specified (i.e. do not support
> that case).
> 
> There are two drivers, bus/pci and bus/vdev, which break the build if
> not enabled. Address this by always enabling these if the user disables
> them or doesn't specify in their allowlist.
> 
> Also remove the old Makefile arm configuration options which don't do
> anything in Meson.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes at pantheon.tech>
> Acked-by: Bruce Richardson <bruce.richardson at intel.com>

I think this patch has changed since I last acked it. Further review below.

> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang at arm.com>
> ---
>  config/arm/meson.build                        |  4 --
>  .../linux_gsg/cross_build_dpdk_for_arm64.rst  |  8 +++
>  drivers/meson.build                           | 49 +++++++++++++++++--
>  meson.build                                   |  2 +
>  meson_options.txt                             |  2 +
>  5 files changed, 56 insertions(+), 9 deletions(-)
> 
> diff --git a/config/arm/meson.build b/config/arm/meson.build
> index 00bc4610a3..a241c45d13 100644
> --- a/config/arm/meson.build
> +++ b/config/arm/meson.build
> @@ -16,9 +16,6 @@ flags_common = [
>  	#	['RTE_ARM64_MEMCPY_ALIGN_MASK', 0xF],
>  	#	['RTE_ARM64_MEMCPY_STRICT_ALIGN', false],
>  
> -	['RTE_NET_FM10K', false],
> -	['RTE_NET_AVP', false],
> -
>  	['RTE_SCHED_VECTOR', false],
>  	['RTE_ARM_USE_WFE', false],
>  	['RTE_ARCH_ARM64', true],
> @@ -125,7 +122,6 @@ implementer_cavium = {
>  				['RTE_MACHINE', '"octeontx2"'],
>  				['RTE_ARM_FEATURE_ATOMICS', true],
>  				['RTE_USE_C11_MEM_MODEL', true],
> -				['RTE_EAL_IGB_UIO', false],
>  				['RTE_MAX_LCORE', 36],
>  				['RTE_MAX_NUMA_NODES', 1]
>  			]
> diff --git a/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst b/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst
> index faaf24b95b..1504dbfef0 100644
> --- a/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst
> +++ b/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst
> @@ -234,3 +234,11 @@ There are other options you may specify in a cross file to tailor the build::
>        numa = false        # set to false to force building for a non-NUMA system
>           # if not set or set to true, the build system will build for a NUMA
>           # system only if libnuma is installed
> +
> +      disabled_drivers = ['bus/dpaa', 'crypto/*']  # add disabled drivers
> +         # valid values are dir/subdirs in the drivers directory
> +         # wildcards are allowed
> +
> +      enabled_drivers = ['common/*', 'bus/*']  # build only these drivers
> +         # valid values are dir/subdirs in the drivers directory
> +         # wildcards are allowed
> diff --git a/drivers/meson.build b/drivers/meson.build
> index 9c8eded697..be5fd2df88 100644
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -19,8 +19,39 @@ subdirs = [
>  	'baseband', # depends on common and bus.
>  ]
>  
> -disabled_drivers = run_command(list_dir_globs, get_option('disable_drivers'),
> -		).stdout().split()
> +always_enabled = ['bus/pci', 'bus/vdev']
> +
> +if meson.is_cross_build()
> +	disabled_drivers += meson.get_cross_property('disabled_drivers', [])
> +	enabled_drivers += meson.get_cross_property('enabled_drivers', [])
> +endif

I don't think "+=" is correct here. This is the first use of the
disabled_drivers variable. [Sorry, correction, I see you've added a new
definition of it in the top-level meson.build. I think it's better to move
that to this file]

Also, would it not be better to have the cross-file option the same as that
used in the parameter option? Right now you have the cross-file option as
"disabled_drivers" vs cmdline option "disable_drivers" and the types being
list and string respectively too. Why not have the cross-file option be a
string called "disable_drivers" too? It would save some joining an
array/string conversion below and simplify things.

> +
> +# add cmdline disabled drivers (comma separated string)
> +# and meson disabled drivers (list)
> +# together into a comma separated string
> +disabled_drivers = ','.join([get_option('disable_drivers'), ','.join(disabled_drivers)]).strip(',')

Do we need the string at the end here? I would think that join never adds a
trailing comma? As stated above if these were both strings it might make
things shorter and easier.

> +if disabled_drivers != ''
> +	disabled_drivers = run_command(list_dir_globs,
> +		disabled_drivers).stdout().split()
> +else
> +	disabled_drivers = []
> +endif

Don't think we need the "if/else" here. The existing code works fine with
taking an empty array.

> +
> +# add cmdline enabled drivers (comma separated string)
> +# and meson enabled drivers (list)
> +# together into a comma separated string
> +enabled_drivers = ','.join([get_option('enable_drivers'), ','.join(enabled_drivers)]).strip(',')
> +if enabled_drivers != ''
> +	enabled_drivers = run_command(list_dir_globs,
> +		enabled_drivers).stdout().split()
> +else
> +	enabled_drivers = []
> +endif
> +
> +if disabled_drivers != [] and enabled_drivers != []
> +	error('Simultaneous disabled drivers and enabled drivers ' +
> +	      'configuration is not supported.')
> +endif

For use in cross-files this makes sense, but I'm not sure it's the correct
approach for when a cross-file specifies enable and the user specifies
disable on the cmdline. In that case, the enable list should just have the
disabled values removed from it. Therefore, I suggest having this check
only in the cross-build section.

>  
>  default_cflags = machine_args
>  default_cflags += ['-DALLOW_EXPERIMENTAL_API']
> @@ -49,7 +80,7 @@ foreach subpath:subdirs
>  		dpdk_driver_classes += class
>  	endif
>  	# get already enabled drivers of the same class
> -	enabled_drivers = get_variable(class + '_drivers', [])
> +	enabled_class_drivers = get_variable(class + '_drivers', [])
>  
>  	foreach drv:drivers
>  		drv_path = join_paths(class, drv)
> @@ -77,11 +108,19 @@ foreach subpath:subdirs
>  		if disabled_drivers.contains(drv_path)
>  			build = false
>  			reason = 'explicitly disabled via build config'
> +		elif enabled_drivers.length() > 0 and not enabled_drivers.contains(drv_path)
> +			build = false
> +			reason = 'not in enabled drivers build config'
>  		else
>  			# pull in driver directory which should update all the local variables
>  			subdir(drv_path)
>  		endif
>  
> +		if not build and always_enabled.contains(drv_path)
> +			build = true
> +			message('Driver @0@ cannot be disabled, enabling.'.format(drv_path))
> +		endif
> +
>  		if build
>  			# get dependency objs from strings
>  			shared_deps = ext_deps
> @@ -109,7 +148,7 @@ foreach subpath:subdirs
>  						'_disable_reason', reason)
>  			endif
>  		else
> -			enabled_drivers += name
> +			enabled_class_drivers += name
>  			lib_name = '_'.join(['rte', class, name])
>  			dpdk_conf.set(lib_name.to_upper(), 1)
>  
> @@ -214,5 +253,5 @@ foreach subpath:subdirs
>  		endif # build
>  	endforeach
>  
> -	set_variable(class + '_drivers', enabled_drivers)
> +	set_variable(class + '_drivers', enabled_class_drivers)
>  endforeach
> diff --git a/meson.build b/meson.build
> index 7778e18200..38a2bd5416 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -22,6 +22,8 @@ dpdk_drivers = []
>  dpdk_extra_ldflags = []
>  dpdk_libs_disabled = []
>  dpdk_drvs_disabled = []
> +disabled_drivers = []
> +enabled_drivers = []

These variables don't need to be declared here. They are only used in
drivers/meson.build and nowhere else.

>  abi_version_file = files('ABI_VERSION')
>  
>  if host_machine.cpu_family().startswith('x86')
> diff --git a/meson_options.txt b/meson_options.txt
> index 86bc6c88f6..d2d24a1424 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -8,6 +8,8 @@ option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-<VERSION>',
>  	description: 'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.')
>  option('enable_docs', type: 'boolean', value: false,
>  	description: 'build documentation')
> +option('enable_drivers', type: 'string', value: '',
> +	description: 'Comma-separated list of drivers to build. If unspecified, build all drivers.')
>  option('enable_driver_sdk', type: 'boolean', value: false,
>  	description: 'Install headers to build drivers.')
>  option('enable_kmods', type: 'boolean', value: false,
> -- 
> 2.20.1
>