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@joelfernandes.org 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_FShelpThis option enables access to the kernel header and other artifacts thatThis line is indented by a TAB, which is correct.
are generated during the build process. These can be used to build kernelmodules or by other in-kernel programs such as those generated by eBPFNow 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 modulecalled kheaders.ko is built which can be loaded on-demand to get accessto 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