linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot
@ 2022-03-28 14:29 Masami Hiramatsu
  2022-03-28 14:30 ` [PATCH v5 1/3] bootconfig: Check the checksum before removing the bootconfig from initrd Masami Hiramatsu
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2022-03-28 14:29 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Padmanabha Srinivasaiah, LKML, Jonathan Corbet,
	linux-doc, Randy Dunlap, Nick Desaulniers, Sami Tolvanen,
	Nathan Chancellor, llvm, Masahiro Yamada,
	Linux Kbuild mailing list

Hi,

Here are the 5th version of the patchset to enable kernel embedded bootconfig
for non-initrd kernel boot environment. This version fixes to sort .gitignore
and fixes lib/Makefile to cleanup default.bconf correctly and to allow user
to specify a relative path to CONFIG_EMBED_BOOT_CONFIG_FILE. (Thanks Masahiro!)
Also this update the document patch about the relative path.
Here is the previous thread [1].

[1] https://lore.kernel.org/all/164833878595.2575750.1483106296151574233.stgit@devnote2/T/#u

You can embed a bootconfig file into the kernel as a default bootconfig,
which will be used if there is no initrd or no bootconfig is attached to initrd. 

This needs 2 options: CONFIG_EMBED_BOOT_CONFIG=y and set the file
path to CONFIG_EMBED_BOOT_CONFIG_FILE. Even if you embed the bootconfig file
to the kernel, it will not be enabled unless you pass "bootconfig" kernel
command line option at boot. Moreover, since this is just a "default"
bootconfig, you can override it with a new bootconfig if you attach another
bootconfig to the initrd (if possible).
CONFIG_EMBED_BOOT_CONFIG_FILE can take both absolute and relative path, but
to simplify and make it independent from the build environment, I recommend
you to use an absolute path for that.

This is requested by Padmanabha at the below thread[2];

[2] https://lore.kernel.org/all/20220307184011.GA2570@pswork/T/#u


KNOWN ISSUE:

According to the report from Padmanabha[3] and my analysis [4], the embedded
bootconfig data may not be updated if you do incremental build the kernel
with CONFIG_LTO_CLANG_THIN.

[3] https://lore.kernel.org/all/20220321183500.GA4065@pswork/T/#u
[4] https://lore.kernel.org/all/20220327115526.cc4b0ff55fc53c97683c3e4d@kernel.org/

This seems like clang's LTO Thin mode issue. It may not detect the inline
asm depends on external files.

I think the possible workaround is to split the inline asm which includes
'.incbin' directive into an asm file. But this should be done in another
seires because there are other features which uses '.incbin'. (e.g.
/proc/config.gz)

Thank you,

---

Masami Hiramatsu (3):
      bootconfig: Check the checksum before removing the bootconfig from initrd
      bootconfig: Support embedding a bootconfig file in kernel
      docs: bootconfig: Add how to embed the bootconfig into kernel


 Documentation/admin-guide/bootconfig.rst |   31 +++++++++++++++++++++++++++---
 include/linux/bootconfig.h               |   10 ++++++++++
 init/Kconfig                             |   21 ++++++++++++++++++++
 init/main.c                              |   31 +++++++++++++++---------------
 lib/.gitignore                           |    1 +
 lib/Makefile                             |    9 +++++++++
 lib/bootconfig.c                         |   23 ++++++++++++++++++++++
 7 files changed, 108 insertions(+), 18 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v5 1/3] bootconfig: Check the checksum before removing the bootconfig from initrd
  2022-03-28 14:29 [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot Masami Hiramatsu
@ 2022-03-28 14:30 ` Masami Hiramatsu
  2022-03-28 14:30 ` [PATCH v5 2/3] bootconfig: Support embedding a bootconfig file in kernel Masami Hiramatsu
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2022-03-28 14:30 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Padmanabha Srinivasaiah, LKML, Jonathan Corbet,
	linux-doc, Randy Dunlap, Nick Desaulniers, Sami Tolvanen,
	Nathan Chancellor, llvm, Masahiro Yamada,
	Linux Kbuild mailing list

Check the bootconfig's checksum before removing the bootcinfig data
from initrd to avoid modifying initrd by mistake.
This will also simplifies the get_boot_config_from_initrd() interface.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 init/main.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/init/main.c b/init/main.c
index 98182c3c2c4b..266d61bc67b0 100644
--- a/init/main.c
+++ b/init/main.c
@@ -266,7 +266,7 @@ static int __init loglevel(char *str)
 early_param("loglevel", loglevel);
 
 #ifdef CONFIG_BLK_DEV_INITRD
-static void * __init get_boot_config_from_initrd(u32 *_size, u32 *_csum)
+static void * __init get_boot_config_from_initrd(u32 *_size)
 {
 	u32 size, csum;
 	char *data;
@@ -300,17 +300,20 @@ static void * __init get_boot_config_from_initrd(u32 *_size, u32 *_csum)
 		return NULL;
 	}
 
+	if (xbc_calc_checksum(data, size) != csum) {
+		pr_err("bootconfig checksum failed\n");
+		return NULL;
+	}
+
 	/* Remove bootconfig from initramfs/initrd */
 	initrd_end = (unsigned long)data;
 	if (_size)
 		*_size = size;
-	if (_csum)
-		*_csum = csum;
 
 	return data;
 }
 #else
-static void * __init get_boot_config_from_initrd(u32 *_size, u32 *_csum)
+static void * __init get_boot_config_from_initrd(u32 *_size)
 {
 	return NULL;
 }
@@ -409,12 +412,12 @@ static void __init setup_boot_config(void)
 	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
 	const char *msg;
 	int pos;
-	u32 size, csum;
+	u32 size;
 	char *data, *err;
 	int ret;
 
 	/* Cut out the bootconfig data even if we have no bootconfig option */
-	data = get_boot_config_from_initrd(&size, &csum);
+	data = get_boot_config_from_initrd(&size);
 
 	strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
 	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
@@ -438,11 +441,6 @@ static void __init setup_boot_config(void)
 		return;
 	}
 
-	if (xbc_calc_checksum(data, size) != csum) {
-		pr_err("bootconfig checksum failed\n");
-		return;
-	}
-
 	ret = xbc_init(data, size, &msg, &pos);
 	if (ret < 0) {
 		if (pos < 0)
@@ -471,7 +469,7 @@ static void __init exit_boot_config(void)
 static void __init setup_boot_config(void)
 {
 	/* Remove bootconfig data from initrd */
-	get_boot_config_from_initrd(NULL, NULL);
+	get_boot_config_from_initrd(NULL);
 }
 
 static int __init warn_bootconfig(char *str)


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v5 2/3] bootconfig: Support embedding a bootconfig file in kernel
  2022-03-28 14:29 [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot Masami Hiramatsu
  2022-03-28 14:30 ` [PATCH v5 1/3] bootconfig: Check the checksum before removing the bootconfig from initrd Masami Hiramatsu
@ 2022-03-28 14:30 ` Masami Hiramatsu
  2022-03-28 14:30 ` [PATCH v5 3/3] docs: bootconfig: Add how to embed the bootconfig into kernel Masami Hiramatsu
  2022-03-30 18:04 ` [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot Nick Desaulniers
  3 siblings, 0 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2022-03-28 14:30 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Padmanabha Srinivasaiah, LKML, Jonathan Corbet,
	linux-doc, Randy Dunlap, Nick Desaulniers, Sami Tolvanen,
	Nathan Chancellor, llvm, Masahiro Yamada,
	Linux Kbuild mailing list

This allows kernel developer to embed a default bootconfig file in
the kernel instead of embedding it in the initrd. This will be good
for who are using the kernel without initrd, or who needs a default
bootconfigs.
This needs to set two kconfigs: CONFIG_EMBED_BOOT_CONFIG=y and set
the file path to CONFIG_EMBED_BOOT_CONFIG_FILE.

Note that you still need 'bootconfig' command line option to load the
embedded bootconfig. Also if you boot using an initrd with a different
bootconfig, the kernel will use the bootconfig in the initrd, instead
of the default bootconfig.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v5:
  - Fix .gitignore to be sorted alphabetically.
  - Make default.bconf is cleaned up correctly.
  - Allow user to specify relative path to CONFIG_EMBED_BOOT_CONFIG_FILE.
    (Thanks Masahiro!)
 Changes in v4:
  - Avoid updating the default.bconf if the file is not changed.
---
 include/linux/bootconfig.h |   10 ++++++++++
 init/Kconfig               |   21 +++++++++++++++++++++
 init/main.c                |   13 ++++++++-----
 lib/.gitignore             |    1 +
 lib/Makefile               |    9 +++++++++
 lib/bootconfig.c           |   23 +++++++++++++++++++++++
 6 files changed, 72 insertions(+), 5 deletions(-)

diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index a4665c7ab07c..5dbda5e3e9bb 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -289,4 +289,14 @@ int __init xbc_get_info(int *node_size, size_t *data_size);
 /* XBC cleanup data structures */
 void __init xbc_exit(void);
 
+/* XBC embedded bootconfig data in kernel */
+#ifdef CONFIG_EMBED_BOOT_CONFIG
+char * __init xbc_get_embedded_bootconfig(size_t *size);
+#else
+static inline char *xbc_get_embedded_bootconfig(size_t *size)
+{
+	return NULL;
+}
+#endif
+
 #endif
diff --git a/init/Kconfig b/init/Kconfig
index 97463a33baa7..f5d14a78cfce 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1361,6 +1361,27 @@ config BOOT_CONFIG
 
 	  If unsure, say Y.
 
+config EMBED_BOOT_CONFIG
+	bool "Embed bootconfig file in the kernel"
+	depends on BOOT_CONFIG
+	default n
+	help
+	  Embed a bootconfig file given by EMBED_BOOT_CONFIG_FILE in the
+	  kernel. Usually, the bootconfig file is loaded with the initrd
+	  image. But if the system doesn't support initrd, this option will
+	  help you by embedding a bootconfig file while building the kernel.
+
+	  If unsure, say N.
+
+config EMBED_BOOT_CONFIG_FILE
+	string "Embedded bootconfig file path"
+	default ""
+	depends on EMBED_BOOT_CONFIG
+	help
+	  Specify a bootconfig file which will be embedded to the kernel.
+	  This bootconfig will be used if there is no initrd or no other
+	  bootconfig in the initrd.
+
 choice
 	prompt "Compiler optimization level"
 	default CC_OPTIMIZE_FOR_PERFORMANCE
diff --git a/init/main.c b/init/main.c
index 266d61bc67b0..a5db3e36b809 100644
--- a/init/main.c
+++ b/init/main.c
@@ -266,7 +266,7 @@ static int __init loglevel(char *str)
 early_param("loglevel", loglevel);
 
 #ifdef CONFIG_BLK_DEV_INITRD
-static void * __init get_boot_config_from_initrd(u32 *_size)
+static void * __init get_boot_config_from_initrd(size_t *_size)
 {
 	u32 size, csum;
 	char *data;
@@ -412,12 +412,15 @@ static void __init setup_boot_config(void)
 	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
 	const char *msg;
 	int pos;
-	u32 size;
+	size_t size;
 	char *data, *err;
 	int ret;
 
 	/* Cut out the bootconfig data even if we have no bootconfig option */
 	data = get_boot_config_from_initrd(&size);
+	/* If there is no bootconfig in initrd, try embedded one. */
+	if (!data)
+		data = xbc_get_embedded_bootconfig(&size);
 
 	strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
 	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
@@ -436,8 +439,8 @@ static void __init setup_boot_config(void)
 	}
 
 	if (size >= XBC_DATA_MAX) {
-		pr_err("bootconfig size %d greater than max size %d\n",
-			size, XBC_DATA_MAX);
+		pr_err("bootconfig size %ld greater than max size %d\n",
+			(long)size, XBC_DATA_MAX);
 		return;
 	}
 
@@ -450,7 +453,7 @@ static void __init setup_boot_config(void)
 				msg, pos);
 	} else {
 		xbc_get_info(&ret, NULL);
-		pr_info("Load bootconfig: %d bytes %d nodes\n", size, ret);
+		pr_info("Load bootconfig: %ld bytes %d nodes\n", (long)size, ret);
 		/* keys starting with "kernel." are passed via cmdline */
 		extra_command_line = xbc_make_cmdline("kernel");
 		/* Also, "init." keys are init arguments */
diff --git a/lib/.gitignore b/lib/.gitignore
index e5e217b8307b..54596b634ecb 100644
--- a/lib/.gitignore
+++ b/lib/.gitignore
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 /crc32table.h
 /crc64table.h
+/default.bconf
 /gen_crc32table
 /gen_crc64table
 /oid_registry_data.c
diff --git a/lib/Makefile b/lib/Makefile
index 6b9ffc1bd1ee..8b6ba55e2c9f 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -279,6 +279,15 @@ $(foreach file, $(libfdt_files), \
 	$(eval CFLAGS_$(file) = -I $(srctree)/scripts/dtc/libfdt))
 lib-$(CONFIG_LIBFDT) += $(libfdt_files)
 
+ifeq ($(CONFIG_EMBED_BOOT_CONFIG),y)
+$(obj)/bootconfig.o: $(obj)/default.bconf
+endif
+
+targets += default.bconf
+filechk_defbconf = cat $(or $(real-prereqs), /dev/null)
+$(obj)/default.bconf: $(CONFIG_EMBED_BOOT_CONFIG_FILE) FORCE
+	$(call filechk,defbconf)
+
 lib-$(CONFIG_BOOT_CONFIG) += bootconfig.o
 
 obj-$(CONFIG_RBTREE_TEST) += rbtree_test.o
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 74f3201ab8e5..3a3bf3a208e3 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -12,6 +12,29 @@
 #include <linux/kernel.h>
 #include <linux/memblock.h>
 #include <linux/string.h>
+
+#ifdef CONFIG_EMBED_BOOT_CONFIG
+asm (
+"	.pushsection .init.data, \"aw\"			\n"
+"	.global embedded_bootconfig_data		\n"
+"embedded_bootconfig_data:				\n"
+"	.incbin \"lib/default.bconf\"			\n"
+"	.global embedded_bootconfig_data_end		\n"
+"embedded_bootconfig_data_end:				\n"
+"	.popsection					\n"
+);
+
+extern __visible char embedded_bootconfig_data[];
+extern __visible char embedded_bootconfig_data_end[];
+
+char * __init xbc_get_embedded_bootconfig(size_t *size)
+{
+	*size = embedded_bootconfig_data_end - embedded_bootconfig_data;
+	return (*size) ? embedded_bootconfig_data : NULL;
+}
+
+#endif
+
 #else /* !__KERNEL__ */
 /*
  * NOTE: This is only for tools/bootconfig, because tools/bootconfig will


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v5 3/3] docs: bootconfig: Add how to embed the bootconfig into kernel
  2022-03-28 14:29 [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot Masami Hiramatsu
  2022-03-28 14:30 ` [PATCH v5 1/3] bootconfig: Check the checksum before removing the bootconfig from initrd Masami Hiramatsu
  2022-03-28 14:30 ` [PATCH v5 2/3] bootconfig: Support embedding a bootconfig file in kernel Masami Hiramatsu
@ 2022-03-28 14:30 ` Masami Hiramatsu
  2022-03-30 18:04 ` [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot Nick Desaulniers
  3 siblings, 0 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2022-03-28 14:30 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Padmanabha Srinivasaiah, LKML, Jonathan Corbet,
	linux-doc, Randy Dunlap, Nick Desaulniers, Sami Tolvanen,
	Nathan Chancellor, llvm, Masahiro Yamada,
	Linux Kbuild mailing list

Add a description how to embed the bootconfig file into kernel.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v5:
  - Update CONFIG_EMBED_BOOT_CONFIG_FILE which can accept relative path.
 Changes in v3:
  - Fix typos.
 Changes in v2:
  - Corrected the text accoding to Randy's suggestion.
  - Do not reccomend to use relative path for CONFIG_EMBED_BOOT_CONFIG_FILE.
---
 Documentation/admin-guide/bootconfig.rst |   31 +++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index a1860fc0ca88..83a2a0cc9afa 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -158,9 +158,15 @@ Each key-value pair is shown in each line with following style::
 Boot Kernel With a Boot Config
 ==============================
 
-Since the boot configuration file is loaded with initrd, it will be added
-to the end of the initrd (initramfs) image file with padding, size,
-checksum and 12-byte magic word as below.
+There are two options to boot the kernel with bootconfig: attaching the
+bootconfig to the initrd image or embedding it in the kernel itself.
+
+Attaching a Boot Config to Initrd
+---------------------------------
+
+Since the boot configuration file is loaded with initrd by default,
+it will be added to the end of the initrd (initramfs) image file with
+padding, size, checksum and 12-byte magic word as below.
 
 [initrd][bootconfig][padding][size(le32)][checksum(le32)][#BOOTCONFIG\n]
 
@@ -196,6 +202,25 @@ To remove the config from the image, you can use -d option as below::
 Then add "bootconfig" on the normal kernel command line to tell the
 kernel to look for the bootconfig at the end of the initrd file.
 
+Embedding a Boot Config into Kernel
+-----------------------------------
+
+If you can not use initrd, you can also embed the bootconfig file in the
+kernel by Kconfig options. In this case, you need to recompile the kernel
+with the following configs::
+
+ CONFIG_EMBED_BOOT_CONFIG=y
+ CONFIG_EMBED_BOOT_CONFIG_FILE="/PATH/TO/BOOTCONFIG/FILE"
+
+``CONFIG_EMBED_BOOT_CONFIG_FILE`` requires an absolute path or a relative
+path to the bootconfig file from source tree or object tree.
+The kernel will embed it as the default bootconfig.
+
+Just as when attaching the bootconfig to the initrd, you need ``bootconfig``
+option on the kernel command line to enable the embedded bootconfig.
+
+Note that even if you set this option, you can override the embedded
+bootconfig by another bootconfig which attached to the initrd.
 
 Kernel parameters via Boot Config
 =================================


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot
  2022-03-28 14:29 [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot Masami Hiramatsu
                   ` (2 preceding siblings ...)
  2022-03-28 14:30 ` [PATCH v5 3/3] docs: bootconfig: Add how to embed the bootconfig into kernel Masami Hiramatsu
@ 2022-03-30 18:04 ` Nick Desaulniers
  2022-03-31  1:45   ` Masami Hiramatsu
  3 siblings, 1 reply; 8+ messages in thread
From: Nick Desaulniers @ 2022-03-30 18:04 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Padmanabha Srinivasaiah, LKML, Jonathan Corbet,
	linux-doc, Randy Dunlap, Sami Tolvanen, Nathan Chancellor, llvm,
	Masahiro Yamada, Linux Kbuild mailing list

On Mon, Mar 28, 2022 at 7:29 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> KNOWN ISSUE:
>
> According to the report from Padmanabha[3] and my analysis [4], the embedded
> bootconfig data may not be updated if you do incremental build the kernel
> with CONFIG_LTO_CLANG_THIN.
>
> [3] https://lore.kernel.org/all/20220321183500.GA4065@pswork/T/#u
> [4] https://lore.kernel.org/all/20220327115526.cc4b0ff55fc53c97683c3e4d@kernel.org/
>
> This seems like clang's LTO Thin mode issue. It may not detect the inline
> asm depends on external files.
>
> I think the possible workaround is to split the inline asm which includes
> '.incbin' directive into an asm file. But this should be done in another
> seires because there are other features which uses '.incbin'. (e.g.
> /proc/config.gz)

Hi Masami,
I saw Padmanabha's report (thanks for the report); sorry for not
responding sooner, I've been traveling recently for a funeral.

Any chance we can use

CFLAGS_REMOVE_<file>.o := $(CC_FLAGS_LTO)

a la
commit d2dcd3e37475 ("x86, cpu: disable LTO for cpu.c")

with a comment linking to
https://github.com/ClangBuiltLinux/linux/issues/1618

for the Translation Units using .incbin, until we have had more time
to triage+fix?

-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot
  2022-03-30 18:04 ` [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot Nick Desaulniers
@ 2022-03-31  1:45   ` Masami Hiramatsu
  2022-03-31  2:13     ` Masahiro Yamada
  2022-03-31 17:31     ` Nick Desaulniers
  0 siblings, 2 replies; 8+ messages in thread
From: Masami Hiramatsu @ 2022-03-31  1:45 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Steven Rostedt, Padmanabha Srinivasaiah, LKML, Jonathan Corbet,
	linux-doc, Randy Dunlap, Sami Tolvanen, Nathan Chancellor, llvm,
	Masahiro Yamada, Linux Kbuild mailing list

Hi Nick,

On Wed, 30 Mar 2022 11:04:50 -0700
Nick Desaulniers <ndesaulniers@google.com> wrote:

> On Mon, Mar 28, 2022 at 7:29 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > KNOWN ISSUE:
> >
> > According to the report from Padmanabha[3] and my analysis [4], the embedded
> > bootconfig data may not be updated if you do incremental build the kernel
> > with CONFIG_LTO_CLANG_THIN.
> >
> > [3] https://lore.kernel.org/all/20220321183500.GA4065@pswork/T/#u
> > [4] https://lore.kernel.org/all/20220327115526.cc4b0ff55fc53c97683c3e4d@kernel.org/
> >
> > This seems like clang's LTO Thin mode issue. It may not detect the inline
> > asm depends on external files.
> >
> > I think the possible workaround is to split the inline asm which includes
> > '.incbin' directive into an asm file. But this should be done in another
> > seires because there are other features which uses '.incbin'. (e.g.
> > /proc/config.gz)
> 
> Hi Masami,
> I saw Padmanabha's report (thanks for the report); sorry for not
> responding sooner, I've been traveling recently for a funeral.

Oh, sorry about that. I think this is not so hurry. 

> 
> Any chance we can use
> 
> CFLAGS_REMOVE_<file>.o := $(CC_FLAGS_LTO)
> 
> a la
> commit d2dcd3e37475 ("x86, cpu: disable LTO for cpu.c")

Hm, this looks good to me. Let me confirm that works.
(Does this mean the bootconfig.o will be compiled to elf binary?)

> 
> with a comment linking to
> https://github.com/ClangBuiltLinux/linux/issues/1618

Thanks for reporting!

> 
> for the Translation Units using .incbin, until we have had more time
> to triage+fix?

Yes. For this series, I can update with above one, but it doesn't cover the
other parts. And since this issue is independent from the bootconfig, 
I think we need a wider patch series to mitigate this issue on config.gz
(and other .incbin users) too.

Thank you,

> 
> -- 
> Thanks,
> ~Nick Desaulniers


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot
  2022-03-31  1:45   ` Masami Hiramatsu
@ 2022-03-31  2:13     ` Masahiro Yamada
  2022-03-31 17:31     ` Nick Desaulniers
  1 sibling, 0 replies; 8+ messages in thread
From: Masahiro Yamada @ 2022-03-31  2:13 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Nick Desaulniers, Steven Rostedt, Padmanabha Srinivasaiah, LKML,
	Jonathan Corbet, open list:DOCUMENTATION, Randy Dunlap,
	Sami Tolvanen, Nathan Chancellor, llvm,
	Linux Kbuild mailing list

On Thu, Mar 31, 2022 at 10:45 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Nick,
>
> On Wed, 30 Mar 2022 11:04:50 -0700
> Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> > On Mon, Mar 28, 2022 at 7:29 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > KNOWN ISSUE:
> > >
> > > According to the report from Padmanabha[3] and my analysis [4], the embedded
> > > bootconfig data may not be updated if you do incremental build the kernel
> > > with CONFIG_LTO_CLANG_THIN.
> > >
> > > [3] https://lore.kernel.org/all/20220321183500.GA4065@pswork/T/#u
> > > [4] https://lore.kernel.org/all/20220327115526.cc4b0ff55fc53c97683c3e4d@kernel.org/
> > >
> > > This seems like clang's LTO Thin mode issue. It may not detect the inline
> > > asm depends on external files.
> > >
> > > I think the possible workaround is to split the inline asm which includes
> > > '.incbin' directive into an asm file. But this should be done in another
> > > seires because there are other features which uses '.incbin'. (e.g.
> > > /proc/config.gz)
> >
> > Hi Masami,
> > I saw Padmanabha's report (thanks for the report); sorry for not
> > responding sooner, I've been traveling recently for a funeral.
>
> Oh, sorry about that. I think this is not so hurry.
>
> >
> > Any chance we can use
> >
> > CFLAGS_REMOVE_<file>.o := $(CC_FLAGS_LTO)
> >
> > a la
> > commit d2dcd3e37475 ("x86, cpu: disable LTO for cpu.c")
>
> Hm, this looks good to me. Let me confirm that works.
> (Does this mean the bootconfig.o will be compiled to elf binary?)


Why are you using  ".incbin" in the C file in the first place?


You might have mimicked  kernel/configs.c
but there was a reason;  kernel/configs.c can be a module,
but we cannot put MODULE_LICENSE() in *.S  file.
(commit 13610aa908dcfce77135bb799c0a10d0172da6ba)


In this case, CONFIG_EMBED_BOOT_CONFIG is a bool
option.

Why don't you simply move the asm() part to a separate
bootconfig-data.S ?

Clang lto flags are only passed to *.c files,
so we do not need to be worried about  .incbin in *.S files.


Then, Makefile will be even cleaner (no ifeq-block)


lib-$(CONFIG_BOOT_CONFIG) += bootconfig.o
obj-$(CONFIG_EMBED_BOOT_CONFIG) += bootconfig-data.o

$(obj)/bootconfig-data.o: $(obj)/default.bconf

targets += default.bconf
filechk_defbconf = cat $(or $(real-prereqs), /dev/null)
$(obj)/default.bconf: $(CONFIG_EMBED_BOOT_CONFIG_FILE) FORCE
        $(call filechk,defbconf)





BTW, why lib-$(CONFIG_BOOT_CONFIG),
instead of obj-$(CONFIG_BOOT_CONFIG)  ?



>
> >
> > with a comment linking to
> > https://github.com/ClangBuiltLinux/linux/issues/1618
>
> Thanks for reporting!
>
> >
> > for the Translation Units using .incbin, until we have had more time
> > to triage+fix?
>
> Yes. For this series, I can update with above one, but it doesn't cover the
> other parts. And since this issue is independent from the bootconfig,
> I think we need a wider patch series to mitigate this issue on config.gz
> (and other .incbin users) too.
>
> Thank you,
>
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>




--
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot
  2022-03-31  1:45   ` Masami Hiramatsu
  2022-03-31  2:13     ` Masahiro Yamada
@ 2022-03-31 17:31     ` Nick Desaulniers
  1 sibling, 0 replies; 8+ messages in thread
From: Nick Desaulniers @ 2022-03-31 17:31 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Padmanabha Srinivasaiah, LKML, Jonathan Corbet,
	linux-doc, Randy Dunlap, Sami Tolvanen, Nathan Chancellor, llvm,
	Masahiro Yamada, Linux Kbuild mailing list

On Wed, Mar 30, 2022 at 6:45 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Nick,
>
> On Wed, 30 Mar 2022 11:04:50 -0700
> Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> >
> > Any chance we can use
> >
> > CFLAGS_REMOVE_<file>.o := $(CC_FLAGS_LTO)
> >
> > a la
> > commit d2dcd3e37475 ("x86, cpu: disable LTO for cpu.c")
>
> Hm, this looks good to me. Let me confirm that works.
> (Does this mean the bootconfig.o will be compiled to elf binary?)

I know we went with Masahiro's suggestion, which is clever and better,
but to answer this question; yes, under LTO, the linker can link
together inputs that are a mix of ELF object files (basically, no LTO
optimizations) with LLVM IR (w/ LTO optimizations between such files).
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-03-31 17:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-28 14:29 [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot Masami Hiramatsu
2022-03-28 14:30 ` [PATCH v5 1/3] bootconfig: Check the checksum before removing the bootconfig from initrd Masami Hiramatsu
2022-03-28 14:30 ` [PATCH v5 2/3] bootconfig: Support embedding a bootconfig file in kernel Masami Hiramatsu
2022-03-28 14:30 ` [PATCH v5 3/3] docs: bootconfig: Add how to embed the bootconfig into kernel Masami Hiramatsu
2022-03-30 18:04 ` [PATCH v5 0/3] bootconfig: Support embedding a bootconfig in kernel for non initrd boot Nick Desaulniers
2022-03-31  1:45   ` Masami Hiramatsu
2022-03-31  2:13     ` Masahiro Yamada
2022-03-31 17:31     ` Nick Desaulniers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).