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.
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' ?
update the filesystem or worry about it where to store the headers on it. However, the feature is also buildable as a module in case the user desires it not being part of the kernel image. This makes it possible to load and unload the headers from memory on demand. A tracing program, or a kernel module builder can load the module, do its operations, and then unload the module to save kernel memory. The total memory needed is 3.8MB.
By having the archive available at a fixed location independent of filesystem dependencies and conventions, all debugging tools can directly refer to the fixed location for the archive, without concerning with where the headers on a typical filesystem which significantly simplifies tooling that needs kernel headers.
The code to read the headers is based on /proc/config.gz code and uses the same technique to embed the headers.
IKHD_ST and IKHD_ED markers as is to facilitate future patches that would extract the headers from a kernel or module image.
Signed-off-by: Joel Fernandes (Google) joel@joelfernandes.org
[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 that
This 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 eBPF
Now that you have dropped the ability to "build kernel modules", I'd like you to update this help message.
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.
+find $cpio_dir -type f -print0 |
Please replace two spaces after 'find' with one.
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 ?
- (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.
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.
+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)
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?
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.