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

[Juraj Linkeš]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson at intel.com>
> Sent: Friday, April 9, 2021 5:38 PM
> To: Juraj Linkeš <juraj.linkes at pantheon.tech>
> Cc: Ruifeng.Wang at arm.com; Honnappa.Nagarahalli at arm.com;
> Phil.Yang at arm.com; vcchunga at amazon.com; Dharmik.Thakkar at arm.com;
> jerinjacobk at gmail.com; hemant.agrawal at nxp.com;
> ajit.khaparde at broadcom.com; ferruh.yigit at intel.com; aboyer at pensando.io;
> dev at 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 at intel.com>
> > > > Sent: Friday, April 9, 2021 12:03 PM
> > > > To: Juraj Linkeš <juraj.linkes at pantheon.tech>
> > > > Cc: Ruifeng.Wang at arm.com; Honnappa.Nagarahalli at arm.com;
> > > > Phil.Yang at arm.com; vcchunga at amazon.com;
> Dharmik.Thakkar at arm.com;
> > > > jerinjacobk at gmail.com; hemant.agrawal at nxp.com;
> > > > ajit.khaparde at broadcom.com; ferruh.yigit at intel.com;
> > > > aboyer at pensando.io; dev at 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 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]
> > > >
> > >
> > > 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.