On Thu, Apr 11, 2019 at 10:47:23AM +0900, Masahiro Yamada wrote:
> Hi Joel,
>
> I have no objection to this patch.
> I checked though it once again,
> please let me point out a little more.
>
> They are all nits.
>
>
>
> On Tue, Apr 9, 2019 at 6:37 AM Joel Fernandes (Google)
> <joel@xxxxxxxxxxxxxxxxx> wrote:
> >
> > Introduce in-kernel headers which are made available as an archive
> > through proc (/proc/kheaders.tar.xz file). This archive makes it
> > possible to run eBPF and other tracing programs tracing programs that
>
> Just one "tracing programs" is enough.
fixed.
> > need to extend the kernel for tracing purposes without any dependency on
> > the file system having headers.
> >
> > On Android and embedded systems, it is common to switch kernels but not
> > have kernel headers available on the file system. Further once a
> > different kernel is booted, any headers stored on the file system will
> > no longer be useful. By storing the headers as a compressed archive
> > within the kernel, we can avoid these issues that have been a hindrance
> > for a long time.
> >
> > The best way to use this feature is by building it in. Several users
> > have a need for this, when they switch debug kernels, they donot want to
>
> 'donot' -> 'do not' ?
fixed
[snip]
> >
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 4592bf7997c0..ea75bfbf7dfa 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -580,6 +580,17 @@ config IKCONFIG_PROC
> > This option enables access to the kernel configuration file
> > through /proc/config.gz.
> >
> > +config IKHEADERS_PROC
> > + tristate "Enable kernel header artifacts through /proc/kheaders.tar.xz"
> > + depends on PROC_FS
> > + help
> > + This option enables access to the kernel header and other artifacts that
>
> This line is indented by a TAB, which is correct.
>
>
> > + are generated during the build process. These can be used to build kernel
> > + modules or by other in-kernel programs such as those generated by eBPF
>
> Now that you have dropped the ability to "build kernel modules",
> I'd like you to update this help message.
Sorry, will fix.
> > + and systemtap tools. If you build the headers as a module, a module
> > + called kheaders.ko is built which can be loaded on-demand to get access
> > + to the headers.
>
> These lines are indented by 8-spaces instead of one TAB.
> Please use TAB-indentation consistently.
>
>
> [snip]
>
>
> > +rm -rf $cpio_dir
> > +mkdir $cpio_dir
> > +
> > +pushd $kroot > /dev/null
> > +for f in $src_file_list;
> > + do find "$f" ! -name "*.cmd" ! -name ".*";
> > +done | cpio --quiet -pd $cpio_dir
> > +popd > /dev/null
> > +
> > +# The second CPIO can complain if files already exist which can
> > +# happen with out of tree builds. Just silence CPIO for now.
> > +for f in $obj_file_list;
> > + do find "$f" ! -name "*.cmd" ! -name ".*";
> > +done | cpio --quiet -pd $cpio_dir >/dev/null 2>&1
> > +
>
> Could you add a simple comment about what the following code is doing?
> "Remove comments except SPDX" etc.
Ok.
> > +find $cpio_dir -type f -print0 |
>
> Please replace two spaces after 'find' with one.
fixed
> > + xargs -0 -P8 -n1 perl -pi -e 'BEGIN {undef $/;}; s/\/\*((?!SPDX).)*?\*\///smg;'
> > +
> > +tar -Jcf $tarfile -C $cpio_dir/ . > /dev/null
> > +
> > +echo "$src_files_md5" > kernel/kheaders.md5
> > +echo "$obj_files_md5" >> kernel/kheaders.md5
> > +echo "$(md5sum $tarfile | cut -d ' ' -f1)" >> kernel/kheaders.md5
> > +
> > +rm -rf $cpio_dir
> > diff --git a/kernel/kheaders.c b/kernel/kheaders.c
> > new file mode 100644
> > index 000000000000..d072a958a8f1
> > --- /dev/null
> > +++ b/kernel/kheaders.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * kernel/kheaders.c
> > + * Provide headers and artifacts needed to build kernel modules.
>
> Ditto. Could you update this comment ?
fixed.
> > + * (Borrowed code from kernel/configs.c)
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/init.h>
> > +#include <linux/uaccess.h>
> > +
> > +/*
> > + * Define kernel_headers_data and kernel_headers_data_end, within which the the
> > + * compressed kernel headers are stpred. The file is first compressed with xz.
> > + */
> > +
> > +asm (
> > +" .pushsection .rodata, \"a\" \n"
> > +" .global kernel_headers_data \n"
> > +"kernel_headers_data: \n"
> > +" .incbin \"kernel/kheaders_data.tar.xz\" \n"
> > +" .global kernel_headers_data_end \n"
> > +"kernel_headers_data_end: \n"
> > +" .popsection \n"
> > +);
>
> You mentioned "IKHD_ST and IKHD_ED markers..." in the commit description,
> but I do not see them in the code.
fixed
> If you plan to work on a tool to extract the headers,
> I think it is OK to have the markers here.
>
> Anyway, please make the code and the commit-log consistent.
yes, will do. thanks
> > +extern char kernel_headers_data;
> > +extern char kernel_headers_data_end;
> > +
> > +static ssize_t
> > +ikheaders_read_current(struct file *file, char __user *buf,
>
> Could you stretch this line ?
> It will still fit in 80-cols.
>
> (This is a coding style error in kernel/configs.c)
It takes 87 cols if I expand, so I'll leave it as is.
> Last thing, when CONFIG_IKHEADERS_PROC=y,
> I always see:
> GEN kernel/kheaders_data.tar.xz
>
> which I think misleading because
> the script is just checking the md5sum.
>
>
>
> What I like to see is:
> CHK kernel/kheaders_data.tar.xz
> for checking md5sum.
>
> And,
> GEN kernel/kheaders_data.tar.xz
> for really (re-)generating the tarball.
>
>
> How about this code?
Yes this is better, I changed it to that.
Also looking forward to your tag on the v7 posting if it looks to you now.
thanks a lot for the review!
- Joel
> index e3c581d8cde7..12399614c350 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -125,7 +125,7 @@ $(obj)/config_data.gz: $(KCONFIG_CONFIG) FORCE
>
> $(obj)/kheaders.o: $(obj)/kheaders_data.tar.xz
>
> -quiet_cmd_genikh = GEN $(obj)/kheaders_data.tar.xz
> +quiet_cmd_genikh = CHK $(obj)/kheaders_data.tar.xz
> cmd_genikh = $(srctree)/kernel/gen_ikh_data.sh $@
> $(obj)/kheaders_data.tar.xz: FORCE
> $(call cmd,genikh)
> diff --git a/kernel/gen_ikh_data.sh b/kernel/gen_ikh_data.sh
> index ef72c2740d01..613960e18691 100755
> --- a/kernel/gen_ikh_data.sh
> +++ b/kernel/gen_ikh_data.sh
> @@ -57,6 +57,10 @@ if [ -f kernel/kheaders.md5 ] &&
> exit
> fi
>
> +if [ "${quiet}" != "silent_" ]; then
> + echo " GEN $tarfile"
> +fi
> +
> rm -rf $cpio_dir
> mkdir $cpio_dir
>
>
> Thanks.
>
> --
> Best Regards
> Masahiro Yamada
