> -----Original Message-----
> From: Bruce Richardson <bruce.richard...@intel.com>
> Sent: Friday, April 9, 2021 4:32 PM
> To: Juraj Linkeš <juraj.lin...@pantheon.tech>
> Cc: ruifeng.w...@arm.com; honnappa.nagaraha...@arm.com;
> phil.y...@arm.com; vcchu...@amazon.com; dharmik.thak...@arm.com;
> jerinjac...@gmail.com; hemant.agra...@nxp.com;
> ajit.khapa...@broadcom.com; ferruh.yi...@intel.com; abo...@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.richard...@intel.com>
> > > Sent: Friday, April 9, 2021 12:03 PM
> > > To: Juraj Linkeš <juraj.lin...@pantheon.tech>
> > > Cc: ruifeng.w...@arm.com; honnappa.nagaraha...@arm.com;
> > > phil.y...@arm.com; vcchu...@amazon.com; dharmik.thak...@arm.com;
> > > jerinjac...@gmail.com; hemant.agra...@nxp.com;
> > > ajit.khapa...@broadcom.com; ferruh.yi...@intel.com;
> > > abo...@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.lin...@pantheon.tech>
> > > > Acked-by: Bruce Richardson <bruce.richard...@intel.com>
> > >
> > > I think this patch has changed since I last acked it. Further review
> > > below.
> > >
> > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > > > Reviewed-by: Ruifeng Wang <ruifeng.w...@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
