[v19,1/3] build: disable/enable drivers in Arm builds
Checks
Commit Message
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@pantheon.tech>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@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(-)
Comments
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@pantheon.tech>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
I think this patch has changed since I last acked it. Further review below.
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@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
>
> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Friday, April 9, 2021 12:03 PM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> ajit.khaparde@broadcom.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> dev@dpdk.org
> Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm builds
>
> 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@pantheon.tech>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
> I think this patch has changed since I last acked it. Further review below.
>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@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]
>
This need not be true. It's added in config/arm/meson.build in the subsequent patch, which comes before drivers/meson.build, which is why I structured it this way - I don't think there's a reason to move the initialization around in the same series, but I could do that.
> 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.
>
The name can be the same. The reason I have two different types is that it struck me as more user friendly - specifying something in code is more intuitive as list as opposed to comma-delimited string, whereas it's more intuitive as comma-delimited string on cmdline. It's possible that having a comma-delimited string everywhere is actually better anyway - I'll change it.
> > +
> > +# 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 we're joining two empty strings (which happens when neither the cmdline nor the code list is specified), then we'll end up with a singular comma instead of an empty string.
Even then both of these are strings (which I'll change), we'll still need the strip, as the above scenario would still happen.
> > +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.
>
An empty string results in ['.'], not in []. I ran into problems with allowlists when ['.'] is returned - I'm assuming that the allowlist is either empty or whathever is in the allowlist means something and '.' is meaningless since it's not a driver. This was the most straightforward solution I found. For disabled drivers we don't need this, but I did the same thing for consistency.
> > +
> > +# 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.
>
Do you want to distinguish between enabling/disabling driver when cross compiling and when doing a regular build? From the previous discussion, I thought we didn't want to mix enable/disable lists no matter what the build or source is. The different scenarios in my mind are combinations of:
1. cross/regular build
2. no list, just enable list, just disable list, both lists
3. where a list is specified - cmdline or meson code (soc config or cross file)
These give us quite a number of different scenarios. When do we want to allow the mixing of enable/disable lists and when not? It's not clear to me from your description, but it seems that specifying a list on the cmdline should overwrite or supplement either an enable or disable list specified in code - we would allow just one of these in code and then augment that with cmdline.
> >
> > 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.
>
As mentioned above, this is in anticipation of the subsequent patch. The other place that makes sense is in config/meson.build. I could put it into drivers/meson.build in this commit and then move it to either the top-level meson.build or to config/meson.build. What do you think?
> > 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
> >
On Fri, Apr 09, 2021 at 02:10:08PM +0000, Juraj Linkeš wrote:
>
>
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Friday, April 9, 2021 12:03 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> > Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> > jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> > ajit.khaparde@broadcom.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> > dev@dpdk.org
> > Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm builds
> >
> > 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@pantheon.tech>
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> >
> > I think this patch has changed since I last acked it. Further review below.
> >
> > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@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]
> >
>
> This need not be true. It's added in config/arm/meson.build in the subsequent patch, which comes before drivers/meson.build, which is why I structured it this way - I don't think there's a reason to move the initialization around in the same series, but I could do that.
Ok, I did not realise that. I need to look at how they are used in that
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.
> >
>
> The name can be the same. The reason I have two different types is that it struck me as more user friendly - specifying something in code is more intuitive as list as opposed to comma-delimited string, whereas it's more intuitive as comma-delimited string on cmdline. It's possible that having a comma-delimited string everywhere is actually better anyway - I'll change it.
>
> > > +
> > > +# 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 we're joining two empty strings (which happens when neither the cmdline nor the code list is specified), then we'll end up with a singular comma instead of an empty string.
>
> Even then both of these are strings (which I'll change), we'll still need the strip, as the above scenario would still happen.
Ok, didn't realise that the trailing "," will still be present. However, I
think a better fix for this, and the issue below with "." being printed in
the empty case is to add the following to buildtools/list-dir-globs.py:
@@ -14,6 +14,8 @@
os.getenv('MESON_SUBDIR', '.'))
for path in sys.argv[1].split(','):
+ if not path:
+ continue
for p in iglob(os.path.join(root, path)):
if os.path.isdir(p):
print(os.path.relpath(p))
With that added, we don't need to worry about null strings or and trailing
commas and can just do:
disabled_drivers += ',' + get_option('disable_drivers')
>
> > > +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.
> >
>
> An empty string results in ['.'], not in []. I ran into problems with allowlists when ['.'] is returned - I'm assuming that the allowlist is either empty or whathever is in the allowlist means something and '.' is meaningless since it's not a driver. This was the most straightforward solution I found. For disabled drivers we don't need this, but I did the same thing for consistency.
>
I think the above two-line change to the globs script should fix this.
> > > +
> > > +# 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.
> >
>
> Do you want to distinguish between enabling/disabling driver when cross compiling and when doing a regular build? From the previous discussion, I thought we didn't want to mix enable/disable lists no matter what the build or source is. The different scenarios in my mind are combinations of:
> 1. cross/regular build
> 2. no list, just enable list, just disable list, both lists
> 3. where a list is specified - cmdline or meson code (soc config or cross file)
>
> These give us quite a number of different scenarios. When do we want to allow the mixing of enable/disable lists and when not? It's not clear to me from your description, but it seems that specifying a list on the cmdline should overwrite or supplement either an enable or disable list specified in code - we would allow just one of these in code and then augment that with cmdline.
>
It's got quite a complicated number of scenarios, I admit.
One thought, though not sure if it would work, is to always check
enabled_drivers and disabled_drivers. If no enable_drivers option in
cross-file or cmdline, we initialize the value to "list_dir_globs *.*",
i.e. everything enabled. Similarly, if no disable_driver options provided,
we use an empty list.
Thereafter in the main loop we just check for each driver that it is in the
enable list and not in the disabled one.
Does that seem like it would work?
/Bruce
On Fri, Apr 09, 2021 at 03:31:31PM +0100, Bruce Richardson wrote:
> On Fri, Apr 09, 2021 at 02:10:08PM +0000, Juraj Linkeš wrote:
> >
> >
> > > -----Original Message-----
> > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > Sent: Friday, April 9, 2021 12:03 PM
> > > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> > > Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> > > jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> > > ajit.khaparde@broadcom.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> > > dev@dpdk.org
> > > Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm builds
> > >
> > > 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@pantheon.tech>
> > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > >
> > > I think this patch has changed since I last acked it. Further review below.
> > >
> > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@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]
> > >
> >
> > This need not be true. It's added in config/arm/meson.build in the subsequent patch, which comes before drivers/meson.build, which is why I structured it this way - I don't think there's a reason to move the initialization around in the same series, but I could do that.
>
> Ok, I did not realise that. I need to look at how they are used in that
> file.
>
One further minor nit here is that the global array defined in the
top-level meson.build all start with "dpdk_". This seems a good policy to
follow to have consistent naming.
> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Friday, April 9, 2021 4:32 PM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> ajit.khaparde@broadcom.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> dev@dpdk.org
> Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm builds
>
> On Fri, Apr 09, 2021 at 02:10:08PM +0000, Juraj Linkeš wrote:
> >
> >
> > > -----Original Message-----
> > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > Sent: Friday, April 9, 2021 12:03 PM
> > > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> > > Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> > > jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> > > ajit.khaparde@broadcom.com; ferruh.yigit@intel.com;
> > > aboyer@pensando.io; dev@dpdk.org
> > > Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm
> > > builds
> > >
> > > 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@pantheon.tech>
> > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > >
> > > I think this patch has changed since I last acked it. Further review below.
> > >
> > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@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]
> > >
> >
> > This need not be true. It's added in config/arm/meson.build in the subsequent
> patch, which comes before drivers/meson.build, which is why I structured it this
> way - I don't think there's a reason to move the initialization around in the same
> series, but I could do that.
>
> Ok, I did not realise that. I need to look at how they are used in that 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.
> > >
> >
> > The name can be the same. The reason I have two different types is that it
> struck me as more user friendly - specifying something in code is more intuitive
> as list as opposed to comma-delimited string, whereas it's more intuitive as
> comma-delimited string on cmdline. It's possible that having a comma-delimited
> string everywhere is actually better anyway - I'll change it.
> >
> > > > +
> > > > +# 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 we're joining two empty strings (which happens when neither the cmdline
> nor the code list is specified), then we'll end up with a singular comma instead of
> an empty string.
> >
> > Even then both of these are strings (which I'll change), we'll still need the strip,
> as the above scenario would still happen.
>
> Ok, didn't realise that the trailing "," will still be present. However, I think a
> better fix for this, and the issue below with "." being printed in the empty case is
> to add the following to buildtools/list-dir-globs.py:
>
> @@ -14,6 +14,8 @@
> os.getenv('MESON_SUBDIR', '.'))
>
> for path in sys.argv[1].split(','):
> + if not path:
> + continue
> for p in iglob(os.path.join(root, path)):
> if os.path.isdir(p):
> print(os.path.relpath(p))
>
> With that added, we don't need to worry about null strings or and trailing
> commas and can just do:
> disabled_drivers += ',' + get_option('disable_drivers')
>
Great, this is the sort of review I was hoping to get. This is much more elegant.
> >
> > > > +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.
> > >
> >
> > An empty string results in ['.'], not in []. I ran into problems with allowlists
> when ['.'] is returned - I'm assuming that the allowlist is either empty or
> whathever is in the allowlist means something and '.' is meaningless since it's not
> a driver. This was the most straightforward solution I found. For disabled drivers
> we don't need this, but I did the same thing for consistency.
> >
>
> I think the above two-line change to the globs script should fix this.
>
Right, it should.
> > > > +
> > > > +# 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.
> > >
> >
> > Do you want to distinguish between enabling/disabling driver when cross
> compiling and when doing a regular build? From the previous discussion, I
> thought we didn't want to mix enable/disable lists no matter what the build or
> source is. The different scenarios in my mind are combinations of:
> > 1. cross/regular build
> > 2. no list, just enable list, just disable list, both lists 3. where a
> > list is specified - cmdline or meson code (soc config or cross file)
> >
> > These give us quite a number of different scenarios. When do we want to
> allow the mixing of enable/disable lists and when not? It's not clear to me from
> your description, but it seems that specifying a list on the cmdline should
> overwrite or supplement either an enable or disable list specified in code - we
> would allow just one of these in code and then augment that with cmdline.
> >
>
> It's got quite a complicated number of scenarios, I admit.
> One thought, though not sure if it would work, is to always check
> enabled_drivers and disabled_drivers. If no enable_drivers option in cross-file or
> cmdline, we initialize the value to "list_dir_globs *.*", i.e. everything enabled.
> Similarly, if no disable_driver options provided, we use an empty list.
>
> Thereafter in the main loop we just check for each driver that it is in the enable
> list and not in the disabled one.
>
> Does that seem like it would work?
I think it would work and it would mean that we'd allow using both enable lists and disable lists for all scenarios. Is that intentional?
>
> /Bruce
> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Friday, April 9, 2021 5:38 PM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> ajit.khaparde@broadcom.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v19 1/3] build: disable/enable drivers in Arm
> builds
>
> On Fri, Apr 09, 2021 at 03:31:31PM +0100, Bruce Richardson wrote:
> > On Fri, Apr 09, 2021 at 02:10:08PM +0000, Juraj Linkeš wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > > Sent: Friday, April 9, 2021 12:03 PM
> > > > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > > Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> > > > Phil.Yang@arm.com; vcchunga@amazon.com;
> Dharmik.Thakkar@arm.com;
> > > > jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> > > > ajit.khaparde@broadcom.com; ferruh.yigit@intel.com;
> > > > aboyer@pensando.io; dev@dpdk.org
> > > > Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm
> > > > builds
> > > >
> > > > 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@pantheon.tech>
> > > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > >
> > > > I think this patch has changed since I last acked it. Further review below.
> > > >
> > > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@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]
> > > >
> > >
> > > This need not be true. It's added in config/arm/meson.build in the
> subsequent patch, which comes before drivers/meson.build, which is why I
> structured it this way - I don't think there's a reason to move the initialization
> around in the same series, but I could do that.
> >
> > Ok, I did not realise that. I need to look at how they are used in
> > that file.
> >
> One further minor nit here is that the global array defined in the top-level
> meson.build all start with "dpdk_". This seems a good policy to follow to have
> consistent naming.
I don't really understand why the prefix is there as we're only dealing with dpdk (or not?). Or does the prefix indicate that these are "global" variables? I'll add it so it's consistent, but there's still the question of whether we want the variables in top-level meson.build or in config/meson.build. I don't think we're going to do anything in the buildtools dir with them, so config/meson.build now seems like the best place to me - it is a variable affecting config. And then the vars could be without the prefix and have the same name as the build option.
On Wed, Apr 14, 2021 at 09:02:16AM +0000, Juraj Linkeš wrote:
>
>
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Friday, April 9, 2021 4:32 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> > Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> > jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> > ajit.khaparde@broadcom.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> > dev@dpdk.org
> > Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm builds
> >
> > On Fri, Apr 09, 2021 at 02:10:08PM +0000, Juraj Linkeš wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > > Sent: Friday, April 9, 2021 12:03 PM
> > > > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > > > Cc: Ruifeng.Wang@arm.com; Honnappa.Nagarahalli@arm.com;
> > > > Phil.Yang@arm.com; vcchunga@amazon.com; Dharmik.Thakkar@arm.com;
> > > > jerinjacobk@gmail.com; hemant.agrawal@nxp.com;
> > > > ajit.khaparde@broadcom.com; ferruh.yigit@intel.com;
> > > > aboyer@pensando.io; dev@dpdk.org
> > > > Subject: Re: [PATCH v19 1/3] build: disable/enable drivers in Arm
> > > > builds
> > > >
> > > > 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@pantheon.tech>
> > > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > >
> > > > I think this patch has changed since I last acked it. Further review below.
> > > >
> > > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@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]
> > > >
> > >
> > > This need not be true. It's added in config/arm/meson.build in the subsequent
> > patch, which comes before drivers/meson.build, which is why I structured it this
> > way - I don't think there's a reason to move the initialization around in the same
> > series, but I could do that.
> >
> > Ok, I did not realise that. I need to look at how they are used in that 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.
> > > >
> > >
> > > The name can be the same. The reason I have two different types is that it
> > struck me as more user friendly - specifying something in code is more intuitive
> > as list as opposed to comma-delimited string, whereas it's more intuitive as
> > comma-delimited string on cmdline. It's possible that having a comma-delimited
> > string everywhere is actually better anyway - I'll change it.
> > >
> > > > > +
> > > > > +# 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 we're joining two empty strings (which happens when neither the cmdline
> > nor the code list is specified), then we'll end up with a singular comma instead of
> > an empty string.
> > >
> > > Even then both of these are strings (which I'll change), we'll still need the strip,
> > as the above scenario would still happen.
> >
> > Ok, didn't realise that the trailing "," will still be present. However, I think a
> > better fix for this, and the issue below with "." being printed in the empty case is
> > to add the following to buildtools/list-dir-globs.py:
> >
> > @@ -14,6 +14,8 @@
> > os.getenv('MESON_SUBDIR', '.'))
> >
> > for path in sys.argv[1].split(','):
> > + if not path:
> > + continue
> > for p in iglob(os.path.join(root, path)):
> > if os.path.isdir(p):
> > print(os.path.relpath(p))
> >
> > With that added, we don't need to worry about null strings or and trailing
> > commas and can just do:
> > disabled_drivers += ',' + get_option('disable_drivers')
> >
>
> Great, this is the sort of review I was hoping to get. This is much more elegant.
>
> > >
> > > > > +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.
> > > >
> > >
> > > An empty string results in ['.'], not in []. I ran into problems with allowlists
> > when ['.'] is returned - I'm assuming that the allowlist is either empty or
> > whathever is in the allowlist means something and '.' is meaningless since it's not
> > a driver. This was the most straightforward solution I found. For disabled drivers
> > we don't need this, but I did the same thing for consistency.
> > >
> >
> > I think the above two-line change to the globs script should fix this.
> >
>
> Right, it should.
>
> > > > > +
> > > > > +# 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.
> > > >
> > >
> > > Do you want to distinguish between enabling/disabling driver when cross
> > compiling and when doing a regular build? From the previous discussion, I
> > thought we didn't want to mix enable/disable lists no matter what the build or
> > source is. The different scenarios in my mind are combinations of:
> > > 1. cross/regular build
> > > 2. no list, just enable list, just disable list, both lists 3. where a
> > > list is specified - cmdline or meson code (soc config or cross file)
> > >
> > > These give us quite a number of different scenarios. When do we want to
> > allow the mixing of enable/disable lists and when not? It's not clear to me from
> > your description, but it seems that specifying a list on the cmdline should
> > overwrite or supplement either an enable or disable list specified in code - we
> > would allow just one of these in code and then augment that with cmdline.
> > >
> >
> > It's got quite a complicated number of scenarios, I admit.
> > One thought, though not sure if it would work, is to always check
> > enabled_drivers and disabled_drivers. If no enable_drivers option in cross-file or
> > cmdline, we initialize the value to "list_dir_globs *.*", i.e. everything enabled.
> > Similarly, if no disable_driver options provided, we use an empty list.
> >
> > Thereafter in the main loop we just check for each driver that it is in the enable
> > list and not in the disabled one.
> >
> > Does that seem like it would work?
>
> I think it would work and it would mean that we'd allow using both enable lists and disable lists for all scenarios. Is that intentional?
>
I think it's necessary to have sane behaviour myself. However, we can still
limit the use to not both together if it causes problems. My main concern
is how to manage overriding via options any values in the cross-files,
ideally without modifying the files, and this seemed a good way.
/Bruce
@@ -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]
]
@@ -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
@@ -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
+
+# 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(',')
+if disabled_drivers != ''
+ disabled_drivers = run_command(list_dir_globs,
+ disabled_drivers).stdout().split()
+else
+ disabled_drivers = []
+endif
+
+# 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
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
@@ -22,6 +22,8 @@ dpdk_drivers = []
dpdk_extra_ldflags = []
dpdk_libs_disabled = []
dpdk_drvs_disabled = []
+disabled_drivers = []
+enabled_drivers = []
abi_version_file = files('ABI_VERSION')
if host_machine.cpu_family().startswith('x86')
@@ -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,