[RFC,v2] build: kni cross-compilation support

  The kni linux module is using a custom target for building, which
doesn't take into account any cross compilation arguments. The arguments
in question are ARCH, CROSS_COMPILE (for gcc, clang) and CC, LD (for
clang). Get those from the cross file and pass them to the custom
target.

The user supplied path may not contain the 'build' directory, such as
when using cross-compiled headers, so only append that in the default
case (when no path is supplied in native builds) and use the unmodified
path from the user otherwise. Also modify the install path accordingly.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 kernel/linux/kni/meson.build |  8 +++---
 kernel/linux/meson.build     | 56 ++++++++++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 9 deletions(-)
  

On Thu, Feb 04, 2021 at 10:51:41AM +0100, Juraj Linkeš wrote:
> The kni linux module is using a custom target for building, which
> doesn't take into account any cross compilation arguments. The arguments
> in question are ARCH, CROSS_COMPILE (for gcc, clang) and CC, LD (for
> clang). Get those from the cross file and pass them to the custom
> target.
> 
> The user supplied path may not contain the 'build' directory, such as
> when using cross-compiled headers, so only append that in the default
> case (when no path is supplied in native builds) and use the unmodified
> path from the user otherwise. Also modify the install path accordingly.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

Some comments inline below.

/Bruce

> ---
>  kernel/linux/kni/meson.build |  8 +++---
>  kernel/linux/meson.build     | 56 ++++++++++++++++++++++++++++++++----
>  2 files changed, 55 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/linux/kni/meson.build b/kernel/linux/kni/meson.build
> index 07e0c9dae7..bb7123354f 100644
> --- a/kernel/linux/kni/meson.build
> +++ b/kernel/linux/kni/meson.build
> @@ -13,7 +13,7 @@ kni_sources = files(
>  custom_target('rte_kni',
>  	input: kni_sources,
>  	output: 'rte_kni.ko',
> -	command: ['make', '-j4', '-C', kernel_dir + '/build',
> +	command: ['make', '-j4', '-C', kernel_dir,
>  		'M=' + meson.current_build_dir(),
>  		'src=' + meson.current_source_dir(),
>  		'MODULE_CFLAGS=-include ' + meson.source_root() + '/config/rte_config.h' +
> @@ -21,8 +21,8 @@ custom_target('rte_kni',
>  		' -I' + meson.source_root() + '/lib/librte_kni' +
>  		' -I' + meson.build_root() +
>  		' -I' + meson.current_source_dir(),
> -		'modules'],
> +		'modules'] + cross_args,
>  	depends: kni_mkfile,
> -	install: true,
> -	install_dir: kernel_dir + '/extra/dpdk',
> +	install: install,
> +	install_dir: install_dir,
>  	build_by_default: get_option('enable_kmods'))

rather than "kernel_dir" and "install_dir" can we rename these to
"kernel_build_dir" and "kernel_install_dir" for clarity.

> diff --git a/kernel/linux/meson.build b/kernel/linux/meson.build
> index 5c864a4653..74097299bb 100644
> --- a/kernel/linux/meson.build
> +++ b/kernel/linux/meson.build
> @@ -3,20 +3,66 @@
>  
>  subdirs = ['kni']
>  
> +kernel_version = run_command('uname', '-r').stdout().strip()

Rename to "host_kernel_version" and probably should be only queried in the
native build case.

> +cross_args = []
>  # if we are cross-compiling we need kernel_dir specified
> -if get_option('kernel_dir') == '' and meson.is_cross_build()
> -	error('Need "kernel_dir" option for kmod compilation when cross-compiling')
> +if meson.is_cross_build()
> +	if get_option('kernel_dir') == ''
> +		error('Need "kernel_dir" option for kmod compilation when cross-compiling')
> +	else
> +		install_dir = ''
> +		install = false

I think these should be defined and initialized further up the file,
outside the conditional block.

> +		cross_compiler = find_program('c').path()
> +		if cross_compiler.endswith('gcc')
> +			cross_prefix = run_command([py3, '-c', 'print("' + cross_compiler + '"[:-3])']).stdout().strip()
> +		elif cross_compiler.endswith('clang')
> +			cross_prefix = ''
> +			found_target = false
> +			# search for '-target' and use the arg that follows
> +			# (i.e. the value of '-target') as cross_prefix
> +			foreach cross_c_arg : meson.get_cross_property('c_args')
> +				if found_target and cross_prefix == ''
> +					cross_prefix = cross_c_arg
> +				endif
> +				if cross_c_arg == '-target'
> +					found_target = true
> +				endif
> +			endforeach
> +			if cross_prefix == ''
> +				error('Didn\'t find -target and its value in' +
> +				      ' c_args in input cross-file.')
> +			endif
> +			linker = 'lld'
> +			foreach cross_c_link_arg : meson.get_cross_property('c_link_args')
> +				if cross_c_link_arg.startswith('-fuse-ld')
> +					linker = cross_c_link_arg.split('=')[1]
> +				endif
> +			endforeach
> +			cross_args += ['CC=@0@'.format(cross_compiler), 'LD=ld.@0@'.format(linker)]
> +		else
> +			error('Unsupported cross compiler: @0@'.format(cross_compiler))
> +		endif
> +		if host_machine.cpu_family() == 'aarch64'
> +			cross_arch = 'arm64'
> +		else
> +			cross_arch = build_machine.cpu_family()
> +		endif
> +		cross_args += ['ARCH=@0@'.format(cross_arch),
> +			'CROSS_COMPILE=@0@'.format(cross_prefix)]
> +	endif
> +else
> +	install_dir = '/lib/modules/' + kernel_version + '/extra/dpdk'
> +	install = true
>  endif
>  

The block for cross-compiling is fairly large and complex, so I'm wondering
how we can simplify things a bit. If we had multiple kernel modules I'd
suggest splitting thing up into a native and cross-build subdirectories to
get the build info, but that seems like overkill here. Instead I wonder if
it would work better to handle all the native case initially in a hopefully
simpler "if block" and then do subdir_done(), leaving everything else to be
the cross-compilation recipe and saving at least one level of indentation.

Also, throughout the block, anywhere you have an "error()" call you can use
"endif" instead of "else" and save more indentation space.

>  kernel_dir = get_option('kernel_dir')
>  if kernel_dir == ''
>  	# use default path for native builds
> -	kernel_version = run_command('uname', '-r').stdout().strip()
> -	kernel_dir = '/lib/modules/' + kernel_version
> +	kernel_dir = '/lib/modules/' + kernel_version + '/build'
>  endif
>  
>  # test running make in kernel directory, using "make kernelversion"
> -make_returncode = run_command('make', '-sC', kernel_dir + '/build',
> +make_returncode = run_command('make', '-sC', kernel_dir,
>  		'kernelversion').returncode()
>  if make_returncode != 0
>  	error('Cannot compile kernel modules as requested - are kernel headers installed?')
> -- 
> 2.20.1
>
  

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Thursday, February 4, 2021 6:34 PM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Cc: thomas@monjalon.net; Ruifeng.Wang@arm.com;
> Honnappa.Nagarahalli@arm.com; jerinjacobk@gmail.com;
> hemant.agrawal@nxp.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> dev@dpdk.org
> Subject: Re: [RFC PATCH v2] build: kni cross-compilation support
> 
> On Thu, Feb 04, 2021 at 10:51:41AM +0100, Juraj Linkeš wrote:
> > The kni linux module is using a custom target for building, which
> > doesn't take into account any cross compilation arguments. The
> > arguments in question are ARCH, CROSS_COMPILE (for gcc, clang) and CC,
> > LD (for clang). Get those from the cross file and pass them to the
> > custom target.
> >
> > The user supplied path may not contain the 'build' directory, such as
> > when using cross-compiled headers, so only append that in the default
> > case (when no path is supplied in native builds) and use the
> > unmodified path from the user otherwise. Also modify the install path
> accordingly.
> >
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> 
> Some comments inline below.
> 

Thanks, these are very helpful.

> /Bruce
> 
> > ---
> >  kernel/linux/kni/meson.build |  8 +++---
> >  kernel/linux/meson.build     | 56 ++++++++++++++++++++++++++++++++----
> >  2 files changed, 55 insertions(+), 9 deletions(-)
> >
> > diff --git a/kernel/linux/kni/meson.build
> > b/kernel/linux/kni/meson.build index 07e0c9dae7..bb7123354f 100644
> > --- a/kernel/linux/kni/meson.build
> > +++ b/kernel/linux/kni/meson.build
> > @@ -13,7 +13,7 @@ kni_sources = files(  custom_target('rte_kni',
> >  	input: kni_sources,
> >  	output: 'rte_kni.ko',
> > -	command: ['make', '-j4', '-C', kernel_dir + '/build',
> > +	command: ['make', '-j4', '-C', kernel_dir,
> >  		'M=' + meson.current_build_dir(),
> >  		'src=' + meson.current_source_dir(),
> >  		'MODULE_CFLAGS=-include ' + meson.source_root() +
> > '/config/rte_config.h' + @@ -21,8 +21,8 @@ custom_target('rte_kni',
> >  		' -I' + meson.source_root() + '/lib/librte_kni' +
> >  		' -I' + meson.build_root() +
> >  		' -I' + meson.current_source_dir(),
> > -		'modules'],
> > +		'modules'] + cross_args,
> >  	depends: kni_mkfile,
> > -	install: true,
> > -	install_dir: kernel_dir + '/extra/dpdk',
> > +	install: install,
> > +	install_dir: install_dir,
> >  	build_by_default: get_option('enable_kmods'))
> 
> rather than "kernel_dir" and "install_dir" can we rename these to
> "kernel_build_dir" and "kernel_install_dir" for clarity.
> 

Makes sense, I'll do that.

> > diff --git a/kernel/linux/meson.build b/kernel/linux/meson.build index
> > 5c864a4653..74097299bb 100644
> > --- a/kernel/linux/meson.build
> > +++ b/kernel/linux/meson.build
> > @@ -3,20 +3,66 @@
> >
> >  subdirs = ['kni']
> >
> > +kernel_version = run_command('uname', '-r').stdout().strip()
> 
> Rename to "host_kernel_version" and probably should be only queried in the
> native build case.
> 

In meson vernicular the host machine is where the binaries will be running, i.e. what we're building for, so this may be a bit confusing - we could name it build_machine_kernel_version.
In any case, I'll move to the the native build if branch. But then maybe we don't need to rename it?

> > +cross_args = []
> >  # if we are cross-compiling we need kernel_dir specified -if
> > get_option('kernel_dir') == '' and meson.is_cross_build()
> > -	error('Need "kernel_dir" option for kmod compilation when cross-
> compiling')
> > +if meson.is_cross_build()
> > +	if get_option('kernel_dir') == ''
> > +		error('Need "kernel_dir" option for kmod compilation when
> cross-compiling')
> > +	else
> > +		install_dir = ''
> > +		install = false
> 
> I think these should be defined and initialized further up the file, outside the
> conditional block.
> 

Ok, I'll make the default true and set it to false in the cross compilation case.

> > +		cross_compiler = find_program('c').path()
> > +		if cross_compiler.endswith('gcc')
> > +			cross_prefix = run_command([py3, '-c', 'print("' +
> cross_compiler + '"[:-3])']).stdout().strip()
> > +		elif cross_compiler.endswith('clang')
> > +			cross_prefix = ''
> > +			found_target = false
> > +			# search for '-target' and use the arg that follows
> > +			# (i.e. the value of '-target') as cross_prefix
> > +			foreach cross_c_arg :
> meson.get_cross_property('c_args')
> > +				if found_target and cross_prefix == ''
> > +					cross_prefix = cross_c_arg
> > +				endif
> > +				if cross_c_arg == '-target'
> > +					found_target = true
> > +				endif
> > +			endforeach
> > +			if cross_prefix == ''
> > +				error('Didn\'t find -target and its value in' +
> > +				      ' c_args in input cross-file.')
> > +			endif
> > +			linker = 'lld'
> > +			foreach cross_c_link_arg :
> meson.get_cross_property('c_link_args')
> > +				if cross_c_link_arg.startswith('-fuse-ld')
> > +					linker = cross_c_link_arg.split('=')[1]
> > +				endif
> > +			endforeach
> > +			cross_args += ['CC=@0@'.format(cross_compiler),
> 'LD=ld.@0@'.format(linker)]
> > +		else
> > +			error('Unsupported cross compiler:
> @0@'.format(cross_compiler))
> > +		endif
> > +		if host_machine.cpu_family() == 'aarch64'
> > +			cross_arch = 'arm64'
> > +		else
> > +			cross_arch = build_machine.cpu_family()
> > +		endif
> > +		cross_args += ['ARCH=@0@'.format(cross_arch),
> > +			'CROSS_COMPILE=@0@'.format(cross_prefix)]
> > +	endif
> > +else
> > +	install_dir = '/lib/modules/' + kernel_version + '/extra/dpdk'
> > +	install = true
> >  endif
> >
> 
> The block for cross-compiling is fairly large and complex, so I'm wondering how
> we can simplify things a bit. If we had multiple kernel modules I'd suggest
> splitting thing up into a native and cross-build subdirectories to get the build
> info, but that seems like overkill here.

This configuration would be the same for all kernel modules (right?), so I'm not sure how the number of kernel modules is relevant here.
If we split it, what would the dir structure look like? Something like this?
kernel/linux/
├── aarch64
├── native
├── kni
├── <other_mods>

> Instead I wonder if it would work better to
> handle all the native case initially in a hopefully simpler "if block" and then do
> subdir_done(), leaving everything else to be the cross-compilation recipe and
> saving at least one level of indentation.
> 

Wouldn't we need to duplicate the code that does make kernelversion and subdirs the actaul modules?

> Also, throughout the block, anywhere you have an "error()" call you can use
> "endif" instead of "else" and save more indentation space.
> 

Good point, I'll make the change.

> >  kernel_dir = get_option('kernel_dir')  if kernel_dir == ''
> >  	# use default path for native builds
> > -	kernel_version = run_command('uname', '-r').stdout().strip()
> > -	kernel_dir = '/lib/modules/' + kernel_version
> > +	kernel_dir = '/lib/modules/' + kernel_version + '/build'
> >  endif
> >
> >  # test running make in kernel directory, using "make kernelversion"
> > -make_returncode = run_command('make', '-sC', kernel_dir + '/build',
> > +make_returncode = run_command('make', '-sC', kernel_dir,
> >  		'kernelversion').returncode()
> >  if make_returncode != 0
> >  	error('Cannot compile kernel modules as requested - are kernel
> > headers installed?')
> > --
> > 2.20.1
> >
  

On Fri, Feb 05, 2021 at 09:26:05AM +0000, Juraj Linkeš wrote:
> 
> 
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Thursday, February 4, 2021 6:34 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: thomas@monjalon.net; Ruifeng.Wang@arm.com;
> > Honnappa.Nagarahalli@arm.com; jerinjacobk@gmail.com;
> > hemant.agrawal@nxp.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> > dev@dpdk.org
> > Subject: Re: [RFC PATCH v2] build: kni cross-compilation support
> > 
> > On Thu, Feb 04, 2021 at 10:51:41AM +0100, Juraj Linkeš wrote:
> > > The kni linux module is using a custom target for building, which
> > > doesn't take into account any cross compilation arguments. The
> > > arguments in question are ARCH, CROSS_COMPILE (for gcc, clang) and CC,
> > > LD (for clang). Get those from the cross file and pass them to the
> > > custom target.
> > >
> > > The user supplied path may not contain the 'build' directory, such as
> > > when using cross-compiled headers, so only append that in the default
> > > case (when no path is supplied in native builds) and use the
> > > unmodified path from the user otherwise. Also modify the install path
> > accordingly.
> > >
> > > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > 
> > Some comments inline below.
> > 
> 
> Thanks, these are very helpful.
> 
<snip>
> > >
> > > +kernel_version = run_command('uname', '-r').stdout().strip()
> > 
> > Rename to "host_kernel_version" and probably should be only queried in the
> > native build case.
> > 
> 
> In meson vernicular the host machine is where the binaries will be running, i.e. what we're building for, so this may be a bit confusing - we could name it build_machine_kernel_version.
> In any case, I'll move to the the native build if branch. But then maybe we don't need to rename it?
> 
Agreed.

> > > +cross_args = []
> > >  # if we are cross-compiling we need kernel_dir specified -if
> > > get_option('kernel_dir') == '' and meson.is_cross_build()
> > > -	error('Need "kernel_dir" option for kmod compilation when cross-
> > compiling')
> > > +if meson.is_cross_build()
<snip>
> > >  endif
> > >
> > 
> > The block for cross-compiling is fairly large and complex, so I'm wondering how
> > we can simplify things a bit. If we had multiple kernel modules I'd suggest
> > splitting thing up into a native and cross-build subdirectories to get the build
> > info, but that seems like overkill here.
> 
> This configuration would be the same for all kernel modules (right?), so I'm not sure how the number of kernel modules is relevant here.
> If we split it, what would the dir structure look like? Something like this?
> kernel/linux/
> ├── aarch64
> ├── native
> ├── kni
> ├── <other_mods>
> 

Yep, that would be the structure - though perhaps with "cross" rather than
"aarch64" being the alternative to "native".  The reason I felt that the
number of kmods was relevant is that it would be really weird to have 2
subdirectories for infrastructure for a single directory containing one
module - a 200% percent overhead ratio. :-) Therefore, I thinking keeping
it all in one file is best, but we'll see after the next revision how it
looks.

> > Instead I wonder if it would work better to
> > handle all the native case initially in a hopefully simpler "if block" and then do
> > subdir_done(), leaving everything else to be the cross-compilation recipe and
> > saving at least one level of indentation.
> > 
> 
> Wouldn't we need to duplicate the code that does make kernelversion and subdirs the actaul modules?
> 

Definitely yes for the subdir call, but that is only one line now. The
"make kernelversion" call - is that needed in the cross-compile case since
one explicitly has to pass in the kernel directories? My original thinking
was to use it to check for the sources more in the native case where we are
picking up paths automatically. Again, it's only a line or two duplicated.
Alternatively, that check could be moved to the "kni" directory meson.build
file, which would make it common.
  

On Fri, Feb 05, 2021 at 09:26:05AM +0000, Juraj Linkeš wrote:
> 
> 
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Thursday, February 4, 2021 6:34 PM
> > To: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > Cc: thomas@monjalon.net; Ruifeng.Wang@arm.com;
> > Honnappa.Nagarahalli@arm.com; jerinjacobk@gmail.com;
> > hemant.agrawal@nxp.com; ferruh.yigit@intel.com; aboyer@pensando.io;
> > dev@dpdk.org
> > Subject: Re: [RFC PATCH v2] build: kni cross-compilation support
> > 
> > On Thu, Feb 04, 2021 at 10:51:41AM +0100, Juraj Linkeš wrote:
> > > The kni linux module is using a custom target for building, which
> > > doesn't take into account any cross compilation arguments. The
> > > arguments in question are ARCH, CROSS_COMPILE (for gcc, clang) and CC,
> > > LD (for clang). Get those from the cross file and pass them to the
> > > custom target.
> > >
> > > The user supplied path may not contain the 'build' directory, such as
> > > when using cross-compiled headers, so only append that in the default
> > > case (when no path is supplied in native builds) and use the
> > > unmodified path from the user otherwise. Also modify the install path
> > accordingly.
> > >
> > > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > 
> > Some comments inline below.
> > 
> 
> Thanks, these are very helpful.
> 
<snip>
> > > +		install_dir = ''
> > > +		install = false
> > 
> > I think these should be defined and initialized further up the file, outside the
> > conditional block.
> > 
> 
> Ok, I'll make the default true and set it to false in the cross compilation case.
> 
Can you just initialize the value to !meson.is_cross_build() and leave it
at that?
  

05/02/2021 10:38, Bruce Richardson:
> On Fri, Feb 05, 2021 at 09:26:05AM +0000, Juraj Linkeš wrote:
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > > The block for cross-compiling is fairly large and complex, so I'm wondering how
> > > we can simplify things a bit. If we had multiple kernel modules I'd suggest
> > > splitting thing up into a native and cross-build subdirectories to get the build
> > > info, but that seems like overkill here.
> > 
> > This configuration would be the same for all kernel modules (right?), so I'm not sure how the number of kernel modules is relevant here.
> > If we split it, what would the dir structure look like? Something like this?
> > kernel/linux/
> > ├── aarch64
> > ├── native
> > ├── kni
> > ├── <other_mods>
> > 
> 
> Yep, that would be the structure - though perhaps with "cross" rather than
> "aarch64" being the alternative to "native".  The reason I felt that the
> number of kmods was relevant is that it would be really weird to have 2
> subdirectories for infrastructure for a single directory containing one
> module - a 200% percent overhead ratio. :-) Therefore, I thinking keeping
> it all in one file is best, but we'll see after the next revision how it
> looks.

Going forward, we should not have any Linux kernel module in this DPDK repository.
We must encourage upstream developments.
That's why I am against adding more directories in kernel/linux/.
KNI is still there (could move later), but please handle it in kni/ directory.