[PATCH 0/2] bootconfig: Support early options in embedded config

[PATCH 0/2] bootconfig: Support early options in embedded config

[Petr Malat]

These 2 patches add a support for specifying early options in embedded
bootconfig and merging embedded and initrd bootconfig into one.

To allow handling of early options, it's necessary to eliminate allocations
from embedded bootconfig handling, which can be done by parsing the config
data in place and allocating xbc_nodes array statically.

Later, when initrd is available, it either replaces embedded data or is
appended to them. To append initrd data, it's necessary to relocate already
parsed data to a bigger memory chunk, but that's not a problem, because
xbc_node structure uses offsets and not absolute pointers.

Also, update the documentation to make users aware early options can't be
configured in the initrd.

[PATCH 1/2] bootconfig: Support appending initrd config to embedded one

[Petr Malat]

When both embedded and initrd boot configs are present, initrd config is
preferred. Introduce CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD option, which
allows appending the initrd config to the embedded one.

We handle embedded boot config in-place to avoid allocations, which will
be handy for early parameters support.

Signed-off-by: Petr Malat <oss@malat.biz>
---
 include/linux/bootconfig.h |  10 ++-
 init/Kconfig               |   9 +++
 init/main.c                |  62 ++++++++++--------
 lib/bootconfig-data.S      |   3 +-
 lib/bootconfig.c           | 129 +++++++++++++++++++++++++++----------
 5 files changed, 146 insertions(+), 67 deletions(-)

diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index ca73940e26df..88bbcffa82d5 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -281,7 +281,10 @@ static inline int __init xbc_node_compose_key(struct xbc_node *node,
 }
 
 /* XBC node initializer */
-int __init xbc_init(const char *buf, size_t size, const char **emsg, int *epos);
+int __init xbc_init(char *buf, size_t size, const char **emsg, int *epos);
+
+/* Append XBC data */
+int __init xbc_append(const char *data, size_t size, const char **emsg, int *epos);
 
 /* XBC node and size information */
 int __init xbc_get_info(int *node_size, size_t *data_size);
@@ -291,10 +294,11 @@ void __init xbc_exit(void);
 
 /* XBC embedded bootconfig data in kernel */
 #ifdef CONFIG_BOOT_CONFIG_EMBED
-const char * __init xbc_get_embedded_bootconfig(size_t *size);
+char * __init xbc_get_embedded_bootconfig(size_t *size);
 #else
-static inline const char *xbc_get_embedded_bootconfig(size_t *size)
+static inline char *xbc_get_embedded_bootconfig(size_t *size)
 {
+	*size = 0;
 	return NULL;
 }
 #endif
diff --git a/init/Kconfig b/init/Kconfig
index 6d35728b94b2..9161d2dbad0c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1339,6 +1339,15 @@ config BOOT_CONFIG_EMBED
 
 	  If unsure, say N.
 
+config BOOT_CONFIG_EMBED_APPEND_INITRD
+	bool "Append initrd bootconfig to the embedded one"
+	depends on BOOT_CONFIG_EMBED && BLK_DEV_INITRD
+	default n
+	help
+	  By default if both embedded bootconfig and initrd bootconfig are
+	  found, initrd bootconfig is preferred. If this option is set, initrd
+	  bootconfig gets appended to the embedded one.
+
 config BOOT_CONFIG_EMBED_FILE
 	string "Embedded bootconfig file path"
 	depends on BOOT_CONFIG_EMBED
diff --git a/init/main.c b/init/main.c
index 436d73261810..0cd738f7f0cf 100644
--- a/init/main.c
+++ b/init/main.c
@@ -267,6 +267,9 @@ static void * __init get_boot_config_from_initrd(size_t *_size)
 	u32 *hdr;
 	int i;
 
+	if (_size)
+		*_size = 0;
+
 	if (!initrd_end)
 		return NULL;
 
@@ -309,6 +312,8 @@ static void * __init get_boot_config_from_initrd(size_t *_size)
 #else
 static void * __init get_boot_config_from_initrd(size_t *_size)
 {
+	if (_size)
+		*_size = 0;
 	return NULL;
 }
 #endif
@@ -404,16 +409,16 @@ static int __init warn_bootconfig(char *str)
 static void __init setup_boot_config(void)
 {
 	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
-	const char *msg, *data;
+	const char *msg, *initrd_data;
 	int pos, ret;
-	size_t size;
-	char *err;
+	size_t initrd_size, embeded_size = 0;
+	char *err, *embeded_data = NULL;
 
 	/* Cut out the bootconfig data even if we have no bootconfig option */
-	data = get_boot_config_from_initrd(&size);
+	initrd_data = get_boot_config_from_initrd(&initrd_size);
 	/* If there is no bootconfig in initrd, try embedded one. */
-	if (!data)
-		data = xbc_get_embedded_bootconfig(&size);
+	if (!initrd_data || IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD))
+		embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
 
 	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
 	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
@@ -426,7 +431,7 @@ static void __init setup_boot_config(void)
 	if (err)
 		initargs_offs = err - tmp_cmdline;
 
-	if (!data) {
+	if (!initrd_data && !embeded_data) {
 		/* If user intended to use bootconfig, show an error level message */
 		if (bootconfig_found)
 			pr_err("'bootconfig' found on command line, but no bootconfig found\n");
@@ -435,28 +440,29 @@ static void __init setup_boot_config(void)
 		return;
 	}
 
-	if (size >= XBC_DATA_MAX) {
-		pr_err("bootconfig size %ld greater than max size %d\n",
-			(long)size, XBC_DATA_MAX);
-		return;
-	}
-
-	ret = xbc_init(data, size, &msg, &pos);
-	if (ret < 0) {
-		if (pos < 0)
-			pr_err("Failed to init bootconfig: %s.\n", msg);
-		else
-			pr_err("Failed to parse bootconfig: %s at %d.\n",
-				msg, pos);
-	} else {
-		xbc_get_info(&ret, NULL);
-		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 */
-		extra_init_args = xbc_make_cmdline("init");
-	}
+	ret = xbc_init(embeded_data, embeded_size, &msg, &pos);
+	if (ret < 0)
+		goto err0;
+
+	/* Call append even if no data are there as embedded bootconfig is in .init */
+	ret = xbc_append(initrd_data, initrd_size, &msg, &pos);
+	if (ret < 0)
+		goto err0;
+
+	xbc_get_info(&ret, NULL);
+	pr_info("Load bootconfig: %ld bytes %d nodes\n", (long)(embeded_size + initrd_size), ret);
+	/* keys starting with "kernel." are passed via cmdline */
+	extra_command_line = xbc_make_cmdline("kernel");
+	/* Also, "init." keys are init arguments */
+	extra_init_args = xbc_make_cmdline("init");
 	return;
+
+err0:	if (pos < 0)
+		pr_err("Failed to init bootconfig: %s.\n", msg);
+	else
+		pr_err("Failed to parse %s bootconfig: %s at %zu.\n",
+				pos < embeded_size ? "embedded" : "initrd",
+				msg, pos < embeded_size ? pos : pos - embeded_size);
 }
 
 static void __init exit_boot_config(void)
diff --git a/lib/bootconfig-data.S b/lib/bootconfig-data.S
index ef85ba1a82f4..f447e24eb8fa 100644
--- a/lib/bootconfig-data.S
+++ b/lib/bootconfig-data.S
@@ -2,9 +2,10 @@
 /*
  * Embed default bootconfig in the kernel.
  */
-	.section .init.rodata, "aw"
+	.section .init.data, "aw"
 	.global embedded_bootconfig_data
 embedded_bootconfig_data:
 	.incbin "lib/default.bconf"
+	.byte 0
 	.global embedded_bootconfig_data_end
 embedded_bootconfig_data_end:
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index c59d26068a64..841163ce5313 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -15,13 +15,13 @@
 
 #ifdef CONFIG_BOOT_CONFIG_EMBED
 /* embedded_bootconfig_data is defined in bootconfig-data.S */
-extern __visible const char embedded_bootconfig_data[];
-extern __visible const char embedded_bootconfig_data_end[];
+extern __visible char embedded_bootconfig_data[];
+extern __visible char embedded_bootconfig_data_end[];
 
-const char * __init xbc_get_embedded_bootconfig(size_t *size)
+char * __init xbc_get_embedded_bootconfig(size_t *size)
 {
 	*size = embedded_bootconfig_data_end - embedded_bootconfig_data;
-	return (*size) ? embedded_bootconfig_data : NULL;
+	return *size ? embedded_bootconfig_data : NULL;
 }
 #endif
 
@@ -48,6 +48,7 @@ const char * __init xbc_get_embedded_bootconfig(size_t *size)
 static struct xbc_node *xbc_nodes __initdata;
 static int xbc_node_num __initdata;
 static char *xbc_data __initdata;
+static bool xbc_data_allocated __initdata;
 static size_t xbc_data_size __initdata;
 static struct xbc_node *last_parent __initdata;
 static const char *xbc_err_msg __initdata;
@@ -846,13 +847,14 @@ static int __init xbc_verify_tree(void)
 }
 
 /* Need to setup xbc_data and xbc_nodes before call this. */
-static int __init xbc_parse_tree(void)
+static int __init xbc_parse_tree(int offset)
 {
 	char *p, *q;
 	int ret = 0, c;
 
-	last_parent = NULL;
-	p = xbc_data;
+	if (!offset)
+		last_parent = NULL;
+	p = xbc_data + offset;
 	do {
 		q = strpbrk(p, "{}=+;:\n#");
 		if (!q) {
@@ -906,18 +908,42 @@ static int __init xbc_parse_tree(void)
  */
 void __init xbc_exit(void)
 {
-	xbc_free_mem(xbc_data, xbc_data_size);
+	if (xbc_data_allocated)
+		xbc_free_mem(xbc_data, xbc_data_size);
 	xbc_data = NULL;
 	xbc_data_size = 0;
+	xbc_data_allocated = 0;
 	xbc_node_num = 0;
 	xbc_free_mem(xbc_nodes, sizeof(struct xbc_node) * XBC_NODE_MAX);
 	xbc_nodes = NULL;
 	brace_index = 0;
 }
 
+static int xbc_parse_and_verify_tree(int offset, int *epos, const char **emsg)
+{
+	int ret;
+
+	ret = xbc_parse_tree(offset);
+	if (!ret) {
+		ret = xbc_verify_tree();
+		if (!ret)
+			return xbc_node_num;
+	}
+
+	if (epos)
+		*epos = xbc_err_pos;
+	if (emsg)
+		*emsg = xbc_err_msg;
+
+	xbc_exit();
+	return ret;
+}
+
 /**
  * xbc_init() - Parse given XBC file and build XBC internal tree
- * @data: The boot config text original data
+ * @data: Null terminated boot config data, that can be directly
+ *        modified by the parser and will exist till xbc_exit()
+ *        or xbc_append() is called.
  * @size: The size of @data
  * @emsg: A pointer of const char * to store the error message
  * @epos: A pointer of int to store the error position
@@ -930,10 +956,8 @@ void __init xbc_exit(void)
  * @epos will be updated with the error position which is the byte offset
  * of @buf. If the error is not a parser error, @epos will be -1.
  */
-int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
+int __init xbc_init(char *data, size_t size, const char **emsg, int *epos)
 {
-	int ret;
-
 	if (epos)
 		*epos = -1;
 
@@ -942,44 +966,79 @@ int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
 			*emsg = "Bootconfig is already initialized";
 		return -EBUSY;
 	}
-	if (size > XBC_DATA_MAX || size == 0) {
+	if (size > XBC_DATA_MAX || (size == 0 && data != NULL)) {
 		if (emsg)
 			*emsg = size ? "Config data is too big" :
 				"Config data is empty";
 		return -ERANGE;
 	}
 
-	xbc_data = xbc_alloc_mem(size + 1);
-	if (!xbc_data) {
-		if (emsg)
-			*emsg = "Failed to allocate bootconfig data";
-		return -ENOMEM;
-	}
-	memcpy(xbc_data, data, size);
-	xbc_data[size] = '\0';
-	xbc_data_size = size + 1;
-
 	xbc_nodes = xbc_alloc_mem(sizeof(struct xbc_node) * XBC_NODE_MAX);
 	if (!xbc_nodes) {
 		if (emsg)
 			*emsg = "Failed to allocate bootconfig nodes";
-		xbc_exit();
 		return -ENOMEM;
 	}
 	memset(xbc_nodes, 0, sizeof(struct xbc_node) * XBC_NODE_MAX);
 
-	ret = xbc_parse_tree();
-	if (!ret)
-		ret = xbc_verify_tree();
+	if (!data)
+		return 0;
+	xbc_data = data;
+	xbc_data_size = size;
+	return xbc_parse_and_verify_tree(0, epos, emsg);
+}
 
-	if (ret < 0) {
-		if (epos)
-			*epos = xbc_err_pos;
+/**
+ * xbc_append() - Append data to already existing XBC tree
+ * @data: Boot config data, which are copied by the function.
+ * @size: The size of @data
+ * @emsg: A pointer of const char * to store the error message
+ * @epos: A pointer of int to store the error position
+ */
+int __init xbc_append(const char *data, size_t size, const char **emsg, int *epos)
+{
+	size_t new_size, parse_start;
+	char *new_data;
+
+	new_size = xbc_data_size + size;
+	if (new_size > XBC_DATA_MAX) {
 		if (emsg)
-			*emsg = xbc_err_msg;
-		xbc_exit();
-	} else
-		ret = xbc_node_num;
+			*emsg = "Merged config data is too big";
+		return -ERANGE;
+	}
+	if (new_size == 0) {
+		if (data) {
+			if (emsg)
+				*emsg = "Appended data is empty";
+			return -ERANGE;
+		}
+		return 0;
+	}
 
-	return ret;
+	new_data = xbc_alloc_mem(new_size);
+	if (!new_data) {
+		if (emsg)
+			*emsg = "Failed to allocate bootconfig data";
+		return -ENOMEM;
+	}
+
+	if (xbc_data_size) {
+		memcpy(new_data, xbc_data, xbc_data_size - 1);
+		new_data[xbc_data_size - 1] = '\n';
+		parse_start = xbc_data_size - 1;
+	} else {
+		parse_start = 0;
+	}
+	memcpy(new_data + xbc_data_size, data, size);
+	new_data[new_size - 1] = 0;
+	if (xbc_data_allocated)
+		xbc_free_mem(xbc_data, xbc_data_size);
+	xbc_data_allocated = 1;
+	xbc_data = new_data;
+	xbc_data_size = new_size;
+
+	if (!data)
+		return 0;
+
+	return xbc_parse_and_verify_tree(parse_start, epos, emsg);
 }
-- 
2.30.2

[PATCH 2/2] bootconfig: Apply early options from embedded config

[Petr Malat]

Eliminate all allocations in embedded config handling to allow calling
it from arch_setup and applying early options.

Config stored in initrd can't be used for early options, because initrd
is set up after early options are processed.

Add this information to the documentation and also to the option
description.

Signed-off-by: Petr Malat <oss@malat.biz>
---
 Documentation/admin-guide/bootconfig.rst |   3 +
 init/Kconfig                             |   4 +-
 init/main.c                              | 141 ++++++++++++++++++-----
 lib/bootconfig.c                         |  20 +++-
 4 files changed, 132 insertions(+), 36 deletions(-)

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index 91339efdcb54..fb085f696f9b 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
 There are two options to boot the kernel with bootconfig: attaching the
 bootconfig to the initrd image or embedding it in the kernel itself.
 
+Early options may be specified only in the embedded bootconfig, because
+they are processed before the initrd.
+
 Attaching a Boot Config to Initrd
 ---------------------------------
 
diff --git a/init/Kconfig b/init/Kconfig
index 9161d2dbad0c..04de756c935e 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1310,7 +1310,9 @@ config BOOT_CONFIG
 	  Extra boot config allows system admin to pass a config file as
 	  complemental extension of kernel cmdline when booting.
 	  The boot config file must be attached at the end of initramfs
-	  with checksum, size and magic word.
+	  with checksum, size and magic word. Note that early options may
+	  be specified in the embedded bootconfig only. Early options
+	  specified in initrd bootconfig will not be applied.
 	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
 
 	  If unsure, say Y.
diff --git a/init/main.c b/init/main.c
index 0cd738f7f0cf..9aac59673a3a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -158,6 +158,9 @@ static size_t initargs_offs;
 static char *execute_command;
 static char *ramdisk_execute_command = "/init";
 
+static int __init do_early_param(char *param, char *val,
+				 const char *unused, void *arg);
+
 /*
  * Used to generate warnings if static_key manipulation functions are used
  * before jump_label_init is called.
@@ -406,63 +409,134 @@ static int __init warn_bootconfig(char *str)
 	return 0;
 }
 
-static void __init setup_boot_config(void)
+static void __init boot_config_pr_err(const char *msg, int pos, const char *src)
+{
+	if (pos < 0)
+		pr_err("Failed to init bootconfig: %s.\n", msg);
+	else
+		pr_err("Failed to parse %s bootconfig: %s at %d.\n",
+				src, msg, pos);
+}
+
+static int __init setup_boot_config_early(void)
 {
 	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
-	const char *msg, *initrd_data;
-	int pos, ret;
-	size_t initrd_size, embeded_size = 0;
-	char *err, *embeded_data = NULL;
+	static int prev_rtn __initdata;
+	struct xbc_node *root, *knode, *vnode;
+	char *embeded_data, *err;
+	const char *val, *msg;
+	size_t embeded_size;
+	int ret, pos;
 
-	/* Cut out the bootconfig data even if we have no bootconfig option */
-	initrd_data = get_boot_config_from_initrd(&initrd_size);
-	/* If there is no bootconfig in initrd, try embedded one. */
-	if (!initrd_data || IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD))
-		embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
+	if (prev_rtn)
+		return prev_rtn;
 
+	embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
 	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
 	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
 			 bootconfig_params);
-
-	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE)))
-		return;
-
+	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE))) {
+		prev_rtn = embeded_data ? -ENOMSG : -ENODATA;
+		return prev_rtn;
+	}
 	/* parse_args() stops at the next param of '--' and returns an address */
 	if (err)
 		initargs_offs = err - tmp_cmdline;
 
-	if (!initrd_data && !embeded_data) {
-		/* If user intended to use bootconfig, show an error level message */
-		if (bootconfig_found)
-			pr_err("'bootconfig' found on command line, but no bootconfig found\n");
-		else
-			pr_info("No bootconfig data provided, so skipping bootconfig");
-		return;
+	if (!embeded_data) {
+		prev_rtn = -ENOPROTOOPT;
+		return prev_rtn;
 	}
 
 	ret = xbc_init(embeded_data, embeded_size, &msg, &pos);
-	if (ret < 0)
-		goto err0;
+	if (ret < 0) {
+		boot_config_pr_err(msg, pos, "embedded");
+		prev_rtn = ret;
+		return prev_rtn;
+	}
+	prev_rtn = 1;
+
+	/* Process early options */
+	root = xbc_find_node("kernel");
+	if (!root)
+		goto out;
+
+	xbc_node_for_each_key_value(root, knode, val) {
+		ret = xbc_node_compose_key_after(root, knode,
+				xbc_namebuf, XBC_KEYLEN_MAX);
+		if (ret < 0)
+			continue;
+
+		vnode = xbc_node_get_child(knode);
+		if (!vnode) {
+			do_early_param(xbc_namebuf, NULL, NULL, NULL);
+			continue;
+		}
+
+		xbc_array_for_each_value(vnode, val) {
+			if (strscpy(tmp_cmdline, val, sizeof(tmp_cmdline)) < 1) {
+				pr_err("Value for '%s' too long\n", xbc_namebuf);
+				break;
+			}
+			do_early_param(xbc_namebuf, tmp_cmdline, NULL, NULL);
+		}
+	}
+
+out:	return embeded_data ? 1 : 0;
+}
+
+static void __init setup_boot_config(void)
+{
+	const char *msg, *initrd_data;
+	int pos, ret;
+	size_t initrd_size, s;
+
+	/* Cut out the bootconfig data even if we have no bootconfig option */
+	initrd_data = get_boot_config_from_initrd(&initrd_size);
+
+	ret = setup_boot_config_early();
+	if (ret == -ENOMSG || (ret == -ENODATA && initrd_data)) {
+		pr_info("Bootconfig data present, but handling is disabled\n");
+		return;
+	} else if (ret == -ENODATA) {
+		/* Bootconfig disabled and bootconfig data are not present */
+		return;
+	} else if (ret == -ENOPROTOOPT) {
+		/* Embedded bootconfig not found */
+		if (!initrd_data) {
+			pr_err("'bootconfig' found on command line, but no bootconfig data found\n");
+			return;
+		}
+		ret = xbc_init(NULL, 0, &msg, &pos);
+		if (ret)
+			goto err0;
+	} else if (ret < 0) {
+		/* Other error, should be logged already */
+		return;
+	} else if (initrd_data && !IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD)) {
+		/* Embedded bootconfig handled, but we should not append to it */
+		xbc_get_info(&ret, &s);
+		pr_info("Replacing embedded bootconfig of %d nodes and %zu bytes.\n", ret, s);
+		xbc_exit();
+		ret = xbc_init(NULL, 0, &msg, &pos);
+		if (ret)
+			goto err0;
+	}
 
 	/* Call append even if no data are there as embedded bootconfig is in .init */
 	ret = xbc_append(initrd_data, initrd_size, &msg, &pos);
 	if (ret < 0)
 		goto err0;
 
-	xbc_get_info(&ret, NULL);
-	pr_info("Load bootconfig: %ld bytes %d nodes\n", (long)(embeded_size + initrd_size), ret);
+	xbc_get_info(&ret, &s);
+	pr_info("Load bootconfig: %d nodes %zu bytes.\n", ret, s);
 	/* keys starting with "kernel." are passed via cmdline */
 	extra_command_line = xbc_make_cmdline("kernel");
 	/* Also, "init." keys are init arguments */
 	extra_init_args = xbc_make_cmdline("init");
 	return;
 
-err0:	if (pos < 0)
-		pr_err("Failed to init bootconfig: %s.\n", msg);
-	else
-		pr_err("Failed to parse %s bootconfig: %s at %zu.\n",
-				pos < embeded_size ? "embedded" : "initrd",
-				msg, pos < embeded_size ? pos : pos - embeded_size);
+err0:	boot_config_pr_err(msg, pos, "initrd");
 }
 
 static void __init exit_boot_config(void)
@@ -766,6 +840,11 @@ void __init parse_early_param(void)
 	if (done)
 		return;
 
+#ifdef CONFIG_BOOT_CONFIG_EMBED
+	/* Process early options from boot config */
+	setup_boot_config_early();
+#endif
+
 	/* All fall through to do_early_param. */
 	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
 	parse_early_options(tmp_cmdline);
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 841163ce5313..4048057e3e23 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -45,7 +45,11 @@ char * __init xbc_get_embedded_bootconfig(size_t *size)
  * node (for array).
  */
 
+#ifdef CONFIG_BOOT_CONFIG_EMBED
+static struct xbc_node xbc_nodes[XBC_NODE_MAX];
+#else
 static struct xbc_node *xbc_nodes __initdata;
+#endif
 static int xbc_node_num __initdata;
 static char *xbc_data __initdata;
 static bool xbc_data_allocated __initdata;
@@ -914,8 +918,12 @@ void __init xbc_exit(void)
 	xbc_data_size = 0;
 	xbc_data_allocated = 0;
 	xbc_node_num = 0;
+#ifdef CONFIG_BOOT_CONFIG_EMBED
+	memset(xbc_nodes, 0, sizeof(xbc_nodes));
+#else
 	xbc_free_mem(xbc_nodes, sizeof(struct xbc_node) * XBC_NODE_MAX);
 	xbc_nodes = NULL;
+#endif
 	brace_index = 0;
 }
 
@@ -973,6 +981,7 @@ int __init xbc_init(char *data, size_t size, const char **emsg, int *epos)
 		return -ERANGE;
 	}
 
+#ifndef CONFIG_BOOT_CONFIG_EMBED
 	xbc_nodes = xbc_alloc_mem(sizeof(struct xbc_node) * XBC_NODE_MAX);
 	if (!xbc_nodes) {
 		if (emsg)
@@ -980,7 +989,7 @@ int __init xbc_init(char *data, size_t size, const char **emsg, int *epos)
 		return -ENOMEM;
 	}
 	memset(xbc_nodes, 0, sizeof(struct xbc_node) * XBC_NODE_MAX);
-
+#endif
 	if (!data)
 		return 0;
 	xbc_data = data;
@@ -999,6 +1008,7 @@ int __init xbc_append(const char *data, size_t size, const char **emsg, int *epo
 {
 	size_t new_size, parse_start;
 	char *new_data;
+	int ret;
 
 	new_size = xbc_data_size + size;
 	if (new_size > XBC_DATA_MAX) {
@@ -1024,8 +1034,8 @@ int __init xbc_append(const char *data, size_t size, const char **emsg, int *epo
 
 	if (xbc_data_size) {
 		memcpy(new_data, xbc_data, xbc_data_size - 1);
-		new_data[xbc_data_size - 1] = '\n';
 		parse_start = xbc_data_size - 1;
+		new_data[parse_start] = '\n';
 	} else {
 		parse_start = 0;
 	}
@@ -1039,6 +1049,8 @@ int __init xbc_append(const char *data, size_t size, const char **emsg, int *epo
 
 	if (!data)
 		return 0;
-
-	return xbc_parse_and_verify_tree(parse_start, epos, emsg);
+	ret = xbc_parse_and_verify_tree(parse_start, epos, emsg);
+	if (ret && epos)
+		*epos -= parse_start;
+	return ret;
 }
-- 
2.30.2

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Randy Dunlap]

Hi--

On 11/21/23 15:13, Petr Malat wrote:
> Eliminate all allocations in embedded config handling to allow calling
> it from arch_setup and applying early options.
> 
> Config stored in initrd can't be used for early options, because initrd
> is set up after early options are processed.
> 
> Add this information to the documentation and also to the option
> description.
> 
> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  Documentation/admin-guide/bootconfig.rst |   3 +
>  init/Kconfig                             |   4 +-
>  init/main.c                              | 141 ++++++++++++++++++-----
>  lib/bootconfig.c                         |  20 +++-
>  4 files changed, 132 insertions(+), 36 deletions(-)
> 
> diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> index 91339efdcb54..fb085f696f9b 100644
> --- a/Documentation/admin-guide/bootconfig.rst
> +++ b/Documentation/admin-guide/bootconfig.rst
> @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
>  There are two options to boot the kernel with bootconfig: attaching the
>  bootconfig to the initrd image or embedding it in the kernel itself.
>  
> +Early options may be specified only in the embedded bootconfig, because
> +they are processed before the initrd.
> +

I'm confused by which options are early options. Are they specified or
described somewhere?
How does a user know which options are "early" options?


>  Attaching a Boot Config to Initrd
>  ---------------------------------
>  
> diff --git a/init/Kconfig b/init/Kconfig
> index 9161d2dbad0c..04de756c935e 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
>  	  Extra boot config allows system admin to pass a config file as
>  	  complemental extension of kernel cmdline when booting.
>  	  The boot config file must be attached at the end of initramfs
> -	  with checksum, size and magic word.
> +	  with checksum, size and magic word. Note that early options may
> +	  be specified in the embedded bootconfig only. Early options
> +	  specified in initrd bootconfig will not be applied.
>  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
>  
>  	  If unsure, say Y.
> diff --git a/init/main.c b/init/main.c
> index 0cd738f7f0cf..9aac59673a3a 100644
> --- a/init/main.c
> +++ b/init/main.c

[]

> +
> +static int __init setup_boot_config_early(void)
>  {
>  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> -	const char *msg, *initrd_data;
> -	int pos, ret;
> -	size_t initrd_size, embeded_size = 0;
> -	char *err, *embeded_data = NULL;
> +	static int prev_rtn __initdata;
> +	struct xbc_node *root, *knode, *vnode;
> +	char *embeded_data, *err;
> +	const char *val, *msg;
> +	size_t embeded_size;
> +	int ret, pos;

It hurts my eyes to see "embeded" here.

Thanks.
-- 
~Randy

Re: [PATCH 0/2] bootconfig: Support early options in embedded config

[Paul E. McKenney]

On Wed, Nov 22, 2023 at 12:13:40AM +0100, Petr Malat wrote:
> These 2 patches add a support for specifying early options in embedded
> bootconfig and merging embedded and initrd bootconfig into one.
> 
> To allow handling of early options, it's necessary to eliminate allocations
> from embedded bootconfig handling, which can be done by parsing the config
> data in place and allocating xbc_nodes array statically.
> 
> Later, when initrd is available, it either replaces embedded data or is
> appended to them. To append initrd data, it's necessary to relocate already
> parsed data to a bigger memory chunk, but that's not a problem, because
> xbc_node structure uses offsets and not absolute pointers.
> 
> Also, update the documentation to make users aware early options can't be
> configured in the initrd.

Nice!!!

For the series:

Tested-by: Paul E. McKenney <paulmck@kernel.org>

(My setup isn't friendly with initrd bootconfig, so I tested this only for
the embedded bootconfig parameters.)

							Thanx, Paul

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Paul E. McKenney]

On Wed, Nov 22, 2023 at 02:47:30PM -0800, Randy Dunlap wrote:
> Hi--
> 
> On 11/21/23 15:13, Petr Malat wrote:
> > Eliminate all allocations in embedded config handling to allow calling
> > it from arch_setup and applying early options.
> > 
> > Config stored in initrd can't be used for early options, because initrd
> > is set up after early options are processed.
> > 
> > Add this information to the documentation and also to the option
> > description.
> > 
> > Signed-off-by: Petr Malat <oss@malat.biz>
> > ---
> >  Documentation/admin-guide/bootconfig.rst |   3 +
> >  init/Kconfig                             |   4 +-
> >  init/main.c                              | 141 ++++++++++++++++++-----
> >  lib/bootconfig.c                         |  20 +++-
> >  4 files changed, 132 insertions(+), 36 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> > index 91339efdcb54..fb085f696f9b 100644
> > --- a/Documentation/admin-guide/bootconfig.rst
> > +++ b/Documentation/admin-guide/bootconfig.rst
> > @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
> >  There are two options to boot the kernel with bootconfig: attaching the
> >  bootconfig to the initrd image or embedding it in the kernel itself.
> >  
> > +Early options may be specified only in the embedded bootconfig, because
> > +they are processed before the initrd.
> > +
> 
> I'm confused by which options are early options. Are they specified or
> described somewhere?
> How does a user know which options are "early" options?

I don't claim to know the full answer to this question, but those
declared with early_param() are ones that I have run into.  There are
a few hundred of these.  I believe that there are at least a few more
where the parsing is done manually directly out of the buffer, and
some of those might well also qualify as "early".

Would it make sense to add an "EARLY" to the long list of restrictions
in Documentation/admin-guide/kernel-parameters.rst?

							Thanx, Paul

> >  Attaching a Boot Config to Initrd
> >  ---------------------------------
> >  
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 9161d2dbad0c..04de756c935e 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
> >  	  Extra boot config allows system admin to pass a config file as
> >  	  complemental extension of kernel cmdline when booting.
> >  	  The boot config file must be attached at the end of initramfs
> > -	  with checksum, size and magic word.
> > +	  with checksum, size and magic word. Note that early options may
> > +	  be specified in the embedded bootconfig only. Early options
> > +	  specified in initrd bootconfig will not be applied.
> >  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
> >  
> >  	  If unsure, say Y.
> > diff --git a/init/main.c b/init/main.c
> > index 0cd738f7f0cf..9aac59673a3a 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> 
> []
> 
> > +
> > +static int __init setup_boot_config_early(void)
> >  {
> >  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> > -	const char *msg, *initrd_data;
> > -	int pos, ret;
> > -	size_t initrd_size, embeded_size = 0;
> > -	char *err, *embeded_data = NULL;
> > +	static int prev_rtn __initdata;
> > +	struct xbc_node *root, *knode, *vnode;
> > +	char *embeded_data, *err;
> > +	const char *val, *msg;
> > +	size_t embeded_size;
> > +	int ret, pos;
> 
> It hurts my eyes to see "embeded" here.
> 
> Thanks.
> -- 
> ~Randy

Re: [PATCH 0/2] bootconfig: Support early options in embedded config

[Masami Hiramatsu]

Hi Petr,

Thanks for the patch.

On Wed, 22 Nov 2023 00:13:40 +0100
Petr Malat <oss@malat.biz> wrote:

> These 2 patches add a support for specifying early options in embedded
> bootconfig and merging embedded and initrd bootconfig into one.
> 
> To allow handling of early options, it's necessary to eliminate allocations
> from embedded bootconfig handling, which can be done by parsing the config
> data in place and allocating xbc_nodes array statically.

Hm, my concern is that this can introduce some sort of overhead to parse
the bootconfig.

> 
> Later, when initrd is available, it either replaces embedded data or is
> appended to them. To append initrd data, it's necessary to relocate already
> parsed data to a bigger memory chunk, but that's not a problem, because
> xbc_node structure uses offsets and not absolute pointers.

However, as you did on this series, it is OK that it does an additional parse
for the initrd bootconfig (do not parse twice)

Let me comment your patch.

Thanks!

> 
> Also, update the documentation to make users aware early options can't be
> configured in the initrd.
> 
> 


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

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Masami Hiramatsu]

On Wed, 22 Nov 2023 15:38:03 -0800
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> On Wed, Nov 22, 2023 at 02:47:30PM -0800, Randy Dunlap wrote:
> > Hi--
> > 
> > On 11/21/23 15:13, Petr Malat wrote:
> > > Eliminate all allocations in embedded config handling to allow calling
> > > it from arch_setup and applying early options.
> > > 
> > > Config stored in initrd can't be used for early options, because initrd
> > > is set up after early options are processed.
> > > 
> > > Add this information to the documentation and also to the option
> > > description.
> > > 
> > > Signed-off-by: Petr Malat <oss@malat.biz>
> > > ---
> > >  Documentation/admin-guide/bootconfig.rst |   3 +
> > >  init/Kconfig                             |   4 +-
> > >  init/main.c                              | 141 ++++++++++++++++++-----
> > >  lib/bootconfig.c                         |  20 +++-
> > >  4 files changed, 132 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> > > index 91339efdcb54..fb085f696f9b 100644
> > > --- a/Documentation/admin-guide/bootconfig.rst
> > > +++ b/Documentation/admin-guide/bootconfig.rst
> > > @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
> > >  There are two options to boot the kernel with bootconfig: attaching the
> > >  bootconfig to the initrd image or embedding it in the kernel itself.
> > >  
> > > +Early options may be specified only in the embedded bootconfig, because
> > > +they are processed before the initrd.
> > > +
> > 
> > I'm confused by which options are early options. Are they specified or
> > described somewhere?
> > How does a user know which options are "early" options?
> 
> I don't claim to know the full answer to this question, but those
> declared with early_param() are ones that I have run into.  There are
> a few hundred of these.  I believe that there are at least a few more
> where the parsing is done manually directly out of the buffer, and
> some of those might well also qualify as "early".
> 
> Would it make sense to add an "EARLY" to the long list of restrictions
> in Documentation/admin-guide/kernel-parameters.rst?

Agreed. For the bootconfig, we need to do this...

BTW, we also need to make a block-list for some early params. some of those
MUST be passed from the bootloader. E.g. initrd address and size will be
passed from the bootloader via commandline. Thus such params in the embedded
bootconfig should be filtered at the build time.

Thank you,

> 
> 							Thanx, Paul
> 
> > >  Attaching a Boot Config to Initrd
> > >  ---------------------------------
> > >  
> > > diff --git a/init/Kconfig b/init/Kconfig
> > > index 9161d2dbad0c..04de756c935e 100644
> > > --- a/init/Kconfig
> > > +++ b/init/Kconfig
> > > @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
> > >  	  Extra boot config allows system admin to pass a config file as
> > >  	  complemental extension of kernel cmdline when booting.
> > >  	  The boot config file must be attached at the end of initramfs
> > > -	  with checksum, size and magic word.
> > > +	  with checksum, size and magic word. Note that early options may
> > > +	  be specified in the embedded bootconfig only. Early options
> > > +	  specified in initrd bootconfig will not be applied.
> > >  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
> > >  
> > >  	  If unsure, say Y.
> > > diff --git a/init/main.c b/init/main.c
> > > index 0cd738f7f0cf..9aac59673a3a 100644
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > 
> > []
> > 
> > > +
> > > +static int __init setup_boot_config_early(void)
> > >  {
> > >  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> > > -	const char *msg, *initrd_data;
> > > -	int pos, ret;
> > > -	size_t initrd_size, embeded_size = 0;
> > > -	char *err, *embeded_data = NULL;
> > > +	static int prev_rtn __initdata;
> > > +	struct xbc_node *root, *knode, *vnode;
> > > +	char *embeded_data, *err;
> > > +	const char *val, *msg;
> > > +	size_t embeded_size;
> > > +	int ret, pos;
> > 
> > It hurts my eyes to see "embeded" here.
> > 
> > Thanks.
> > -- 
> > ~Randy


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

Re: [PATCH 1/2] bootconfig: Support appending initrd config to embedded one

[Masami Hiramatsu]

On Wed, 22 Nov 2023 00:13:41 +0100
Petr Malat <oss@malat.biz> wrote:

> When both embedded and initrd boot configs are present, initrd config is
> preferred. Introduce CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD option, which
> allows appending the initrd config to the embedded one.
> 
> We handle embedded boot config in-place to avoid allocations, which will
> be handy for early parameters support.
> 
> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  include/linux/bootconfig.h |  10 ++-
>  init/Kconfig               |   9 +++
>  init/main.c                |  62 ++++++++++--------
>  lib/bootconfig-data.S      |   3 +-
>  lib/bootconfig.c           | 129 +++++++++++++++++++++++++++----------
>  5 files changed, 146 insertions(+), 67 deletions(-)
> 
> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
> index ca73940e26df..88bbcffa82d5 100644
> --- a/include/linux/bootconfig.h
> +++ b/include/linux/bootconfig.h
> @@ -281,7 +281,10 @@ static inline int __init xbc_node_compose_key(struct xbc_node *node,
>  }
>  
>  /* XBC node initializer */
> -int __init xbc_init(const char *buf, size_t size, const char **emsg, int *epos);
> +int __init xbc_init(char *buf, size_t size, const char **emsg, int *epos);
> +
> +/* Append XBC data */
> +int __init xbc_append(const char *data, size_t size, const char **emsg, int *epos);
>  
>  /* XBC node and size information */
>  int __init xbc_get_info(int *node_size, size_t *data_size);
> @@ -291,10 +294,11 @@ void __init xbc_exit(void);
>  
>  /* XBC embedded bootconfig data in kernel */
>  #ifdef CONFIG_BOOT_CONFIG_EMBED
> -const char * __init xbc_get_embedded_bootconfig(size_t *size);
> +char * __init xbc_get_embedded_bootconfig(size_t *size);
>  #else
> -static inline const char *xbc_get_embedded_bootconfig(size_t *size)
> +static inline char *xbc_get_embedded_bootconfig(size_t *size)
>  {
> +	*size = 0;
>  	return NULL;
>  }
>  #endif
> diff --git a/init/Kconfig b/init/Kconfig
> index 6d35728b94b2..9161d2dbad0c 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1339,6 +1339,15 @@ config BOOT_CONFIG_EMBED
>  
>  	  If unsure, say N.
>  
> +config BOOT_CONFIG_EMBED_APPEND_INITRD

With the early param handling feature, this must be enabled always because
without this feature, the early params are set by embedded bootconfig, but
that will be replaced by initrd bootconfig.


> +	bool "Append initrd bootconfig to the embedded one"
> +	depends on BOOT_CONFIG_EMBED && BLK_DEV_INITRD
> +	default n
> +	help
> +	  By default if both embedded bootconfig and initrd bootconfig are
> +	  found, initrd bootconfig is preferred. If this option is set, initrd
> +	  bootconfig gets appended to the embedded one.
> +
>  config BOOT_CONFIG_EMBED_FILE
>  	string "Embedded bootconfig file path"
>  	depends on BOOT_CONFIG_EMBED
> diff --git a/init/main.c b/init/main.c
> index 436d73261810..0cd738f7f0cf 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -267,6 +267,9 @@ static void * __init get_boot_config_from_initrd(size_t *_size)
>  	u32 *hdr;
>  	int i;
>  
> +	if (_size)
> +		*_size = 0;
> +
>  	if (!initrd_end)
>  		return NULL;
>  
> @@ -309,6 +312,8 @@ static void * __init get_boot_config_from_initrd(size_t *_size)
>  #else
>  static void * __init get_boot_config_from_initrd(size_t *_size)
>  {
> +	if (_size)
> +		*_size = 0;
>  	return NULL;
>  }
>  #endif
> @@ -404,16 +409,16 @@ static int __init warn_bootconfig(char *str)
>  static void __init setup_boot_config(void)
>  {
>  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> -	const char *msg, *data;
> +	const char *msg, *initrd_data;
>  	int pos, ret;
> -	size_t size;
> -	char *err;
> +	size_t initrd_size, embeded_size = 0;
> +	char *err, *embeded_data = NULL;
>  
>  	/* Cut out the bootconfig data even if we have no bootconfig option */
> -	data = get_boot_config_from_initrd(&size);
> +	initrd_data = get_boot_config_from_initrd(&initrd_size);
>  	/* If there is no bootconfig in initrd, try embedded one. */
> -	if (!data)
> -		data = xbc_get_embedded_bootconfig(&size);
> +	if (!initrd_data || IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD))
> +		embeded_data = xbc_get_embedded_bootconfig(&embeded_size);

Thus, this part can be simply get both initrd and embedded one.

Thank you,

>  
>  	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>  	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
> @@ -426,7 +431,7 @@ static void __init setup_boot_config(void)
>  	if (err)
>  		initargs_offs = err - tmp_cmdline;
>  
> -	if (!data) {
> +	if (!initrd_data && !embeded_data) {
>  		/* If user intended to use bootconfig, show an error level message */
>  		if (bootconfig_found)
>  			pr_err("'bootconfig' found on command line, but no bootconfig found\n");
> @@ -435,28 +440,29 @@ static void __init setup_boot_config(void)
>  		return;
>  	}
>  
> -	if (size >= XBC_DATA_MAX) {
> -		pr_err("bootconfig size %ld greater than max size %d\n",
> -			(long)size, XBC_DATA_MAX);
> -		return;
> -	}
> -
> -	ret = xbc_init(data, size, &msg, &pos);
> -	if (ret < 0) {
> -		if (pos < 0)
> -			pr_err("Failed to init bootconfig: %s.\n", msg);
> -		else
> -			pr_err("Failed to parse bootconfig: %s at %d.\n",
> -				msg, pos);
> -	} else {
> -		xbc_get_info(&ret, NULL);
> -		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 */
> -		extra_init_args = xbc_make_cmdline("init");
> -	}
> +	ret = xbc_init(embeded_data, embeded_size, &msg, &pos);
> +	if (ret < 0)
> +		goto err0;
> +
> +	/* Call append even if no data are there as embedded bootconfig is in .init */
> +	ret = xbc_append(initrd_data, initrd_size, &msg, &pos);
> +	if (ret < 0)
> +		goto err0;
> +
> +	xbc_get_info(&ret, NULL);
> +	pr_info("Load bootconfig: %ld bytes %d nodes\n", (long)(embeded_size + initrd_size), ret);
> +	/* keys starting with "kernel." are passed via cmdline */
> +	extra_command_line = xbc_make_cmdline("kernel");
> +	/* Also, "init." keys are init arguments */
> +	extra_init_args = xbc_make_cmdline("init");
>  	return;
> +
> +err0:	if (pos < 0)
> +		pr_err("Failed to init bootconfig: %s.\n", msg);
> +	else
> +		pr_err("Failed to parse %s bootconfig: %s at %zu.\n",
> +				pos < embeded_size ? "embedded" : "initrd",
> +				msg, pos < embeded_size ? pos : pos - embeded_size);
>  }
>  
>  static void __init exit_boot_config(void)
> diff --git a/lib/bootconfig-data.S b/lib/bootconfig-data.S
> index ef85ba1a82f4..f447e24eb8fa 100644
> --- a/lib/bootconfig-data.S
> +++ b/lib/bootconfig-data.S
> @@ -2,9 +2,10 @@
>  /*
>   * Embed default bootconfig in the kernel.
>   */
> -	.section .init.rodata, "aw"
> +	.section .init.data, "aw"
>  	.global embedded_bootconfig_data
>  embedded_bootconfig_data:
>  	.incbin "lib/default.bconf"
> +	.byte 0
>  	.global embedded_bootconfig_data_end
>  embedded_bootconfig_data_end:
> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> index c59d26068a64..841163ce5313 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -15,13 +15,13 @@
>  
>  #ifdef CONFIG_BOOT_CONFIG_EMBED
>  /* embedded_bootconfig_data is defined in bootconfig-data.S */
> -extern __visible const char embedded_bootconfig_data[];
> -extern __visible const char embedded_bootconfig_data_end[];
> +extern __visible char embedded_bootconfig_data[];
> +extern __visible char embedded_bootconfig_data_end[];
>  
> -const char * __init xbc_get_embedded_bootconfig(size_t *size)
> +char * __init xbc_get_embedded_bootconfig(size_t *size)
>  {
>  	*size = embedded_bootconfig_data_end - embedded_bootconfig_data;
> -	return (*size) ? embedded_bootconfig_data : NULL;
> +	return *size ? embedded_bootconfig_data : NULL;
>  }
>  #endif
>  
> @@ -48,6 +48,7 @@ const char * __init xbc_get_embedded_bootconfig(size_t *size)
>  static struct xbc_node *xbc_nodes __initdata;
>  static int xbc_node_num __initdata;
>  static char *xbc_data __initdata;
> +static bool xbc_data_allocated __initdata;
>  static size_t xbc_data_size __initdata;
>  static struct xbc_node *last_parent __initdata;
>  static const char *xbc_err_msg __initdata;
> @@ -846,13 +847,14 @@ static int __init xbc_verify_tree(void)
>  }
>  
>  /* Need to setup xbc_data and xbc_nodes before call this. */
> -static int __init xbc_parse_tree(void)
> +static int __init xbc_parse_tree(int offset)
>  {
>  	char *p, *q;
>  	int ret = 0, c;
>  
> -	last_parent = NULL;
> -	p = xbc_data;
> +	if (!offset)
> +		last_parent = NULL;
> +	p = xbc_data + offset;
>  	do {
>  		q = strpbrk(p, "{}=+;:\n#");
>  		if (!q) {
> @@ -906,18 +908,42 @@ static int __init xbc_parse_tree(void)
>   */
>  void __init xbc_exit(void)
>  {
> -	xbc_free_mem(xbc_data, xbc_data_size);
> +	if (xbc_data_allocated)
> +		xbc_free_mem(xbc_data, xbc_data_size);
>  	xbc_data = NULL;
>  	xbc_data_size = 0;
> +	xbc_data_allocated = 0;
>  	xbc_node_num = 0;
>  	xbc_free_mem(xbc_nodes, sizeof(struct xbc_node) * XBC_NODE_MAX);
>  	xbc_nodes = NULL;
>  	brace_index = 0;
>  }
>  
> +static int xbc_parse_and_verify_tree(int offset, int *epos, const char **emsg)
> +{
> +	int ret;
> +
> +	ret = xbc_parse_tree(offset);
> +	if (!ret) {
> +		ret = xbc_verify_tree();
> +		if (!ret)
> +			return xbc_node_num;
> +	}
> +
> +	if (epos)
> +		*epos = xbc_err_pos;
> +	if (emsg)
> +		*emsg = xbc_err_msg;
> +
> +	xbc_exit();
> +	return ret;
> +}
> +
>  /**
>   * xbc_init() - Parse given XBC file and build XBC internal tree
> - * @data: The boot config text original data
> + * @data: Null terminated boot config data, that can be directly
> + *        modified by the parser and will exist till xbc_exit()
> + *        or xbc_append() is called.
>   * @size: The size of @data
>   * @emsg: A pointer of const char * to store the error message
>   * @epos: A pointer of int to store the error position
> @@ -930,10 +956,8 @@ void __init xbc_exit(void)
>   * @epos will be updated with the error position which is the byte offset
>   * of @buf. If the error is not a parser error, @epos will be -1.
>   */
> -int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
> +int __init xbc_init(char *data, size_t size, const char **emsg, int *epos)
>  {
> -	int ret;
> -
>  	if (epos)
>  		*epos = -1;
>  
> @@ -942,44 +966,79 @@ int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
>  			*emsg = "Bootconfig is already initialized";
>  		return -EBUSY;
>  	}
> -	if (size > XBC_DATA_MAX || size == 0) {
> +	if (size > XBC_DATA_MAX || (size == 0 && data != NULL)) {
>  		if (emsg)
>  			*emsg = size ? "Config data is too big" :
>  				"Config data is empty";
>  		return -ERANGE;
>  	}
>  
> -	xbc_data = xbc_alloc_mem(size + 1);
> -	if (!xbc_data) {
> -		if (emsg)
> -			*emsg = "Failed to allocate bootconfig data";
> -		return -ENOMEM;
> -	}
> -	memcpy(xbc_data, data, size);
> -	xbc_data[size] = '\0';
> -	xbc_data_size = size + 1;
> -
>  	xbc_nodes = xbc_alloc_mem(sizeof(struct xbc_node) * XBC_NODE_MAX);
>  	if (!xbc_nodes) {
>  		if (emsg)
>  			*emsg = "Failed to allocate bootconfig nodes";
> -		xbc_exit();
>  		return -ENOMEM;
>  	}
>  	memset(xbc_nodes, 0, sizeof(struct xbc_node) * XBC_NODE_MAX);
>  
> -	ret = xbc_parse_tree();
> -	if (!ret)
> -		ret = xbc_verify_tree();
> +	if (!data)
> +		return 0;
> +	xbc_data = data;
> +	xbc_data_size = size;
> +	return xbc_parse_and_verify_tree(0, epos, emsg);
> +}
>  
> -	if (ret < 0) {
> -		if (epos)
> -			*epos = xbc_err_pos;
> +/**
> + * xbc_append() - Append data to already existing XBC tree
> + * @data: Boot config data, which are copied by the function.
> + * @size: The size of @data
> + * @emsg: A pointer of const char * to store the error message
> + * @epos: A pointer of int to store the error position
> + */
> +int __init xbc_append(const char *data, size_t size, const char **emsg, int *epos)
> +{
> +	size_t new_size, parse_start;
> +	char *new_data;
> +
> +	new_size = xbc_data_size + size;
> +	if (new_size > XBC_DATA_MAX) {
>  		if (emsg)
> -			*emsg = xbc_err_msg;
> -		xbc_exit();
> -	} else
> -		ret = xbc_node_num;
> +			*emsg = "Merged config data is too big";
> +		return -ERANGE;
> +	}
> +	if (new_size == 0) {
> +		if (data) {
> +			if (emsg)
> +				*emsg = "Appended data is empty";
> +			return -ERANGE;
> +		}
> +		return 0;
> +	}
>  
> -	return ret;
> +	new_data = xbc_alloc_mem(new_size);
> +	if (!new_data) {
> +		if (emsg)
> +			*emsg = "Failed to allocate bootconfig data";
> +		return -ENOMEM;
> +	}
> +
> +	if (xbc_data_size) {
> +		memcpy(new_data, xbc_data, xbc_data_size - 1);
> +		new_data[xbc_data_size - 1] = '\n';
> +		parse_start = xbc_data_size - 1;
> +	} else {
> +		parse_start = 0;
> +	}
> +	memcpy(new_data + xbc_data_size, data, size);
> +	new_data[new_size - 1] = 0;
> +	if (xbc_data_allocated)
> +		xbc_free_mem(xbc_data, xbc_data_size);
> +	xbc_data_allocated = 1;
> +	xbc_data = new_data;
> +	xbc_data_size = new_size;
> +
> +	if (!data)
> +		return 0;
> +
> +	return xbc_parse_and_verify_tree(parse_start, epos, emsg);
>  }
> -- 
> 2.30.2
> 


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

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Petr Malat]

Hi!

On Wed, Nov 22, 2023 at 02:47:30PM -0800, Randy Dunlap wrote:
> > +Early options may be specified only in the embedded bootconfig, because
> > +they are processed before the initrd.
> > +
> 
> I'm confused by which options are early options. Are they specified or
> described somewhere?
> How does a user know which options are "early" options?
This is not user friendly at all, I was thinking about emitting a warning
when early option is being configured from initrd, but that would require
one iteration over .init.setup section for every option present in initrd
just to show the warning.

Better idea would be to write a script, which extracts this info from
vmlinux.o and adds it to bootconfig userspace utility. Of course this makes
the utility tied to particular kernel version and configuration
combination, but for distributions it's reasonably safe with no overhead
during boot.

> > +	size_t embeded_size;
> > +	int ret, pos;
> 
> It hurts my eyes to see "embeded" here.
Thanks, I will fix it in the next version.
  Petr

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Petr Malat]

Hi!

On Thu, Nov 23, 2023 at 11:22:07AM +0900, Masami Hiramatsu wrote:
> BTW, we also need to make a block-list for some early params. some of those
> MUST be passed from the bootloader. E.g. initrd address and size will be
> passed from the bootloader via commandline. Thus such params in the embedded
> bootconfig should be filtered at the build time.

It's ok to configure these in the embedded bootconfig - in a case they are
provided by the bootloader, the bootconfig value is overridden, if not, the
value from bootconfig is used, so it works as expected.
  Petr

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Masami Hiramatsu]

On Wed, 22 Nov 2023 00:13:42 +0100
Petr Malat <oss@malat.biz> wrote:

> Eliminate all allocations in embedded config handling to allow calling
> it from arch_setup and applying early options.
> 
> Config stored in initrd can't be used for early options, because initrd
> is set up after early options are processed.
> 
> Add this information to the documentation and also to the option
> description.
> 
> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  Documentation/admin-guide/bootconfig.rst |   3 +
>  init/Kconfig                             |   4 +-
>  init/main.c                              | 141 ++++++++++++++++++-----
>  lib/bootconfig.c                         |  20 +++-
>  4 files changed, 132 insertions(+), 36 deletions(-)
> 
> diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> index 91339efdcb54..fb085f696f9b 100644
> --- a/Documentation/admin-guide/bootconfig.rst
> +++ b/Documentation/admin-guide/bootconfig.rst
> @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
>  There are two options to boot the kernel with bootconfig: attaching the
>  bootconfig to the initrd image or embedding it in the kernel itself.
>  
> +Early options may be specified only in the embedded bootconfig, because
> +they are processed before the initrd.
> +
>  Attaching a Boot Config to Initrd
>  ---------------------------------
>  
> diff --git a/init/Kconfig b/init/Kconfig
> index 9161d2dbad0c..04de756c935e 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
>  	  Extra boot config allows system admin to pass a config file as
>  	  complemental extension of kernel cmdline when booting.
>  	  The boot config file must be attached at the end of initramfs
> -	  with checksum, size and magic word.
> +	  with checksum, size and magic word. Note that early options may
> +	  be specified in the embedded bootconfig only. Early options
> +	  specified in initrd bootconfig will not be applied.
>  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
>  
>  	  If unsure, say Y.
> diff --git a/init/main.c b/init/main.c
> index 0cd738f7f0cf..9aac59673a3a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -158,6 +158,9 @@ static size_t initargs_offs;
>  static char *execute_command;
>  static char *ramdisk_execute_command = "/init";
>  
> +static int __init do_early_param(char *param, char *val,
> +				 const char *unused, void *arg);
> +
>  /*
>   * Used to generate warnings if static_key manipulation functions are used
>   * before jump_label_init is called.
> @@ -406,63 +409,134 @@ static int __init warn_bootconfig(char *str)
>  	return 0;
>  }
>  
> -static void __init setup_boot_config(void)
> +static void __init boot_config_pr_err(const char *msg, int pos, const char *src)
> +{
> +	if (pos < 0)
> +		pr_err("Failed to init bootconfig: %s.\n", msg);
> +	else
> +		pr_err("Failed to parse %s bootconfig: %s at %d.\n",
> +				src, msg, pos);
> +}
> +
> +static int __init setup_boot_config_early(void)
>  {
>  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> -	const char *msg, *initrd_data;
> -	int pos, ret;
> -	size_t initrd_size, embeded_size = 0;
> -	char *err, *embeded_data = NULL;
> +	static int prev_rtn __initdata;

I would rather like 'done_retval' instead of 'prev_rtn'.

> +	struct xbc_node *root, *knode, *vnode;
> +	char *embeded_data, *err;
> +	const char *val, *msg;
> +	size_t embeded_size;
> +	int ret, pos;
>  
> -	/* Cut out the bootconfig data even if we have no bootconfig option */
> -	initrd_data = get_boot_config_from_initrd(&initrd_size);
> -	/* If there is no bootconfig in initrd, try embedded one. */
> -	if (!initrd_data || IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD))
> -		embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> +	if (prev_rtn)
> +		return prev_rtn;
>  
> +	embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
>  	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>  	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
>  			 bootconfig_params);

This copy & check should be skipped if IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE) because
this only checks "bootconfig" is in the cmdline.
Can you introduce following flag and initialize it here?

#ifdef CONFIG_BOOT_CONFIG_FORCE
#define bootconfig_enabled	(true)
#else
static bool bootconfig_enabled __initdata;
#endif

> -
> -	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE)))
> -		return;
> -
> +	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE))) {
> +		prev_rtn = embeded_data ? -ENOMSG : -ENODATA;
> +		return prev_rtn;

Than we don't need to set a strange error value...

> +	}
>  	/* parse_args() stops at the next param of '--' and returns an address */
>  	if (err)
>  		initargs_offs = err - tmp_cmdline;
>  
> -	if (!initrd_data && !embeded_data) {
> -		/* If user intended to use bootconfig, show an error level message */
> -		if (bootconfig_found)
> -			pr_err("'bootconfig' found on command line, but no bootconfig found\n");
> -		else
> -			pr_info("No bootconfig data provided, so skipping bootconfig");
> -		return;
> +	if (!embeded_data) {
> +		prev_rtn = -ENOPROTOOPT;

Also, we can recheck xbc_get_embedded_bootconfig(&embeded_size) later instead of
using this error code.

> +		return prev_rtn;
>  	}
>  
>  	ret = xbc_init(embeded_data, embeded_size, &msg, &pos);
> -	if (ret < 0)
> -		goto err0;
> +	if (ret < 0) {
> +		boot_config_pr_err(msg, pos, "embedded");
> +		prev_rtn = ret;
> +		return prev_rtn;
> +	}
> +	prev_rtn = 1;

This function should be splitted into init_embedded_boot_config() and
apply_boot_config_early(). The latter one should not be called twice.

> +
> +	/* Process early options */
> +	root = xbc_find_node("kernel");
> +	if (!root)
> +		goto out;
> +
> +	xbc_node_for_each_key_value(root, knode, val) {
> +		ret = xbc_node_compose_key_after(root, knode,
> +				xbc_namebuf, XBC_KEYLEN_MAX);
> +		if (ret < 0)
> +			continue;
> +
> +		vnode = xbc_node_get_child(knode);
> +		if (!vnode) {
> +			do_early_param(xbc_namebuf, NULL, NULL, NULL);
> +			continue;
> +		}
> +
> +		xbc_array_for_each_value(vnode, val) {
> +			if (strscpy(tmp_cmdline, val, sizeof(tmp_cmdline)) < 1) {
> +				pr_err("Value for '%s' too long\n", xbc_namebuf);
> +				break;
> +			}
> +			do_early_param(xbc_namebuf, tmp_cmdline, NULL, NULL);
> +		}
> +	}
> +
> +out:	return embeded_data ? 1 : 0;
> +}
> +
> +static void __init setup_boot_config(void)
> +{
> +	const char *msg, *initrd_data;
> +	int pos, ret;
> +	size_t initrd_size, s;
> +
> +	/* Cut out the bootconfig data even if we have no bootconfig option */
> +	initrd_data = get_boot_config_from_initrd(&initrd_size);
> +
> +	ret = setup_boot_config_early();

Because you should not apply early_params here, you need to call only
init_embedded_boot_config() here.

> +	if (ret == -ENOMSG || (ret == -ENODATA && initrd_data)) {

Also, this can be
	if (!bootconfig_enabled) {
		if (initrd_data || xbc_get_embedded_bootconfig(&s))

> +		pr_info("Bootconfig data present, but handling is disabled\n");
> +		return;


> +	} else if (ret == -ENODATA) {
> +		/* Bootconfig disabled and bootconfig data are not present */
> +		return;

this can be removed.

> +	} else if (ret == -ENOPROTOOPT) {

This should be

	} else {

> +		/* Embedded bootconfig not found */
> +		if (!initrd_data) {
> +			pr_err("'bootconfig' found on command line, but no bootconfig data found\n");
> +			return;
> +		}
> +		ret = xbc_init(NULL, 0, &msg, &pos);
> +		if (ret)
> +			goto err0;

> +	} else if (ret < 0) {
> +		/* Other error, should be logged already */
> +		return;

So this is checked at first.

> +	} else if (initrd_data && !IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD)) {

And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
should be removed.

> +		/* Embedded bootconfig handled, but we should not append to it */
> +		xbc_get_info(&ret, &s);
> +		pr_info("Replacing embedded bootconfig of %d nodes and %zu bytes.\n", ret, s);
> +		xbc_exit();
> +		ret = xbc_init(NULL, 0, &msg, &pos);
> +		if (ret)
> +			goto err0;
> +	}

Thank you,

>  
>  	/* Call append even if no data are there as embedded bootconfig is in .init */
>  	ret = xbc_append(initrd_data, initrd_size, &msg, &pos);
>  	if (ret < 0)
>  		goto err0;
>  
> -	xbc_get_info(&ret, NULL);
> -	pr_info("Load bootconfig: %ld bytes %d nodes\n", (long)(embeded_size + initrd_size), ret);
> +	xbc_get_info(&ret, &s);
> +	pr_info("Load bootconfig: %d nodes %zu bytes.\n", ret, s);
>  	/* keys starting with "kernel." are passed via cmdline */
>  	extra_command_line = xbc_make_cmdline("kernel");
>  	/* Also, "init." keys are init arguments */
>  	extra_init_args = xbc_make_cmdline("init");
>  	return;
>  
> -err0:	if (pos < 0)
> -		pr_err("Failed to init bootconfig: %s.\n", msg);
> -	else
> -		pr_err("Failed to parse %s bootconfig: %s at %zu.\n",
> -				pos < embeded_size ? "embedded" : "initrd",
> -				msg, pos < embeded_size ? pos : pos - embeded_size);
> +err0:	boot_config_pr_err(msg, pos, "initrd");
>  }
>  
>  static void __init exit_boot_config(void)
> @@ -766,6 +840,11 @@ void __init parse_early_param(void)
>  	if (done)
>  		return;
>  
> +#ifdef CONFIG_BOOT_CONFIG_EMBED
> +	/* Process early options from boot config */
> +	setup_boot_config_early();
> +#endif
> +
>  	/* All fall through to do_early_param. */
>  	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>  	parse_early_options(tmp_cmdline);
> diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> index 841163ce5313..4048057e3e23 100644
> --- a/lib/bootconfig.c
> +++ b/lib/bootconfig.c
> @@ -45,7 +45,11 @@ char * __init xbc_get_embedded_bootconfig(size_t *size)
>   * node (for array).
>   */
>  
> +#ifdef CONFIG_BOOT_CONFIG_EMBED
> +static struct xbc_node xbc_nodes[XBC_NODE_MAX];
> +#else
>  static struct xbc_node *xbc_nodes __initdata;
> +#endif
>  static int xbc_node_num __initdata;
>  static char *xbc_data __initdata;
>  static bool xbc_data_allocated __initdata;
> @@ -914,8 +918,12 @@ void __init xbc_exit(void)
>  	xbc_data_size = 0;
>  	xbc_data_allocated = 0;
>  	xbc_node_num = 0;
> +#ifdef CONFIG_BOOT_CONFIG_EMBED
> +	memset(xbc_nodes, 0, sizeof(xbc_nodes));
> +#else
>  	xbc_free_mem(xbc_nodes, sizeof(struct xbc_node) * XBC_NODE_MAX);
>  	xbc_nodes = NULL;
> +#endif
>  	brace_index = 0;
>  }
>  
> @@ -973,6 +981,7 @@ int __init xbc_init(char *data, size_t size, const char **emsg, int *epos)
>  		return -ERANGE;
>  	}
>  
> +#ifndef CONFIG_BOOT_CONFIG_EMBED
>  	xbc_nodes = xbc_alloc_mem(sizeof(struct xbc_node) * XBC_NODE_MAX);
>  	if (!xbc_nodes) {
>  		if (emsg)
> @@ -980,7 +989,7 @@ int __init xbc_init(char *data, size_t size, const char **emsg, int *epos)
>  		return -ENOMEM;
>  	}
>  	memset(xbc_nodes, 0, sizeof(struct xbc_node) * XBC_NODE_MAX);
> -
> +#endif
>  	if (!data)
>  		return 0;
>  	xbc_data = data;
> @@ -999,6 +1008,7 @@ int __init xbc_append(const char *data, size_t size, const char **emsg, int *epo
>  {
>  	size_t new_size, parse_start;
>  	char *new_data;
> +	int ret;
>  
>  	new_size = xbc_data_size + size;
>  	if (new_size > XBC_DATA_MAX) {
> @@ -1024,8 +1034,8 @@ int __init xbc_append(const char *data, size_t size, const char **emsg, int *epo
>  
>  	if (xbc_data_size) {
>  		memcpy(new_data, xbc_data, xbc_data_size - 1);
> -		new_data[xbc_data_size - 1] = '\n';
>  		parse_start = xbc_data_size - 1;
> +		new_data[parse_start] = '\n';
>  	} else {
>  		parse_start = 0;
>  	}
> @@ -1039,6 +1049,8 @@ int __init xbc_append(const char *data, size_t size, const char **emsg, int *epo
>  
>  	if (!data)
>  		return 0;
> -
> -	return xbc_parse_and_verify_tree(parse_start, epos, emsg);
> +	ret = xbc_parse_and_verify_tree(parse_start, epos, emsg);
> +	if (ret && epos)
> +		*epos -= parse_start;
> +	return ret;
>  }
> -- 
> 2.30.2
> 


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

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Masami Hiramatsu]

On Thu, 23 Nov 2023 11:04:00 +0100
Petr Malat <oss@malat.biz> wrote:

> Hi!
> 
> On Thu, Nov 23, 2023 at 11:22:07AM +0900, Masami Hiramatsu wrote:
> > BTW, we also need to make a block-list for some early params. some of those
> > MUST be passed from the bootloader. E.g. initrd address and size will be
> > passed from the bootloader via commandline. Thus such params in the embedded
> > bootconfig should be filtered at the build time.
> 
> It's ok to configure these in the embedded bootconfig - in a case they are
> provided by the bootloader, the bootconfig value is overridden, if not, the
> value from bootconfig is used, so it works as expected.

I meant some params only bootloader knows, like where the initrd is loaded.
Anyway, if user sets such value, it will break the kernel boot as expected :P.

Thank you,

>   Petr


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

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Paul E. McKenney]

On Thu, Nov 23, 2023 at 11:22:07AM +0900, Masami Hiramatsu wrote:
> On Wed, 22 Nov 2023 15:38:03 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > On Wed, Nov 22, 2023 at 02:47:30PM -0800, Randy Dunlap wrote:
> > > Hi--
> > > 
> > > On 11/21/23 15:13, Petr Malat wrote:
> > > > Eliminate all allocations in embedded config handling to allow calling
> > > > it from arch_setup and applying early options.
> > > > 
> > > > Config stored in initrd can't be used for early options, because initrd
> > > > is set up after early options are processed.
> > > > 
> > > > Add this information to the documentation and also to the option
> > > > description.
> > > > 
> > > > Signed-off-by: Petr Malat <oss@malat.biz>
> > > > ---
> > > >  Documentation/admin-guide/bootconfig.rst |   3 +
> > > >  init/Kconfig                             |   4 +-
> > > >  init/main.c                              | 141 ++++++++++++++++++-----
> > > >  lib/bootconfig.c                         |  20 +++-
> > > >  4 files changed, 132 insertions(+), 36 deletions(-)
> > > > 
> > > > diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> > > > index 91339efdcb54..fb085f696f9b 100644
> > > > --- a/Documentation/admin-guide/bootconfig.rst
> > > > +++ b/Documentation/admin-guide/bootconfig.rst
> > > > @@ -161,6 +161,9 @@ Boot Kernel With a Boot Config
> > > >  There are two options to boot the kernel with bootconfig: attaching the
> > > >  bootconfig to the initrd image or embedding it in the kernel itself.
> > > >  
> > > > +Early options may be specified only in the embedded bootconfig, because
> > > > +they are processed before the initrd.
> > > > +
> > > 
> > > I'm confused by which options are early options. Are they specified or
> > > described somewhere?
> > > How does a user know which options are "early" options?
> > 
> > I don't claim to know the full answer to this question, but those
> > declared with early_param() are ones that I have run into.  There are
> > a few hundred of these.  I believe that there are at least a few more
> > where the parsing is done manually directly out of the buffer, and
> > some of those might well also qualify as "early".
> > 
> > Would it make sense to add an "EARLY" to the long list of restrictions
> > in Documentation/admin-guide/kernel-parameters.rst?
> 
> Agreed. For the bootconfig, we need to do this...

Very good, I will put something together by early next week.

There will likely be early_setup() parameters that are not documented
in kernel-parameters.txt.  I propose leaving those undocumented, but
upgrading the header comment on early_param() to explain the situation.

Seem reasonable, or would something else work better?

> BTW, we also need to make a block-list for some early params. some of those
> MUST be passed from the bootloader. E.g. initrd address and size will be
> passed from the bootloader via commandline. Thus such params in the embedded
> bootconfig should be filtered at the build time.

Given a list of them, I would be happy to generate the patches to the
documentation.

							Thanx, Paul

> > > >  Attaching a Boot Config to Initrd
> > > >  ---------------------------------
> > > >  
> > > > diff --git a/init/Kconfig b/init/Kconfig
> > > > index 9161d2dbad0c..04de756c935e 100644
> > > > --- a/init/Kconfig
> > > > +++ b/init/Kconfig
> > > > @@ -1310,7 +1310,9 @@ config BOOT_CONFIG
> > > >  	  Extra boot config allows system admin to pass a config file as
> > > >  	  complemental extension of kernel cmdline when booting.
> > > >  	  The boot config file must be attached at the end of initramfs
> > > > -	  with checksum, size and magic word.
> > > > +	  with checksum, size and magic word. Note that early options may
> > > > +	  be specified in the embedded bootconfig only. Early options
> > > > +	  specified in initrd bootconfig will not be applied.
> > > >  	  See <file:Documentation/admin-guide/bootconfig.rst> for details.
> > > >  
> > > >  	  If unsure, say Y.
> > > > diff --git a/init/main.c b/init/main.c
> > > > index 0cd738f7f0cf..9aac59673a3a 100644
> > > > --- a/init/main.c
> > > > +++ b/init/main.c
> > > 
> > > []
> > > 
> > > > +
> > > > +static int __init setup_boot_config_early(void)
> > > >  {
> > > >  	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
> > > > -	const char *msg, *initrd_data;
> > > > -	int pos, ret;
> > > > -	size_t initrd_size, embeded_size = 0;
> > > > -	char *err, *embeded_data = NULL;
> > > > +	static int prev_rtn __initdata;
> > > > +	struct xbc_node *root, *knode, *vnode;
> > > > +	char *embeded_data, *err;
> > > > +	const char *val, *msg;
> > > > +	size_t embeded_size;
> > > > +	int ret, pos;
> > > 
> > > It hurts my eyes to see "embeded" here.
> > > 
> > > Thanks.
> > > -- 
> > > ~Randy
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Petr Malat]

On Thu, Nov 23, 2023 at 07:41:06PM +0900, Masami Hiramatsu wrote:
> On Wed, 22 Nov 2023 00:13:42 +0100
> Petr Malat <oss@malat.biz> wrote:
> > +	static int prev_rtn __initdata;
> 
> I would rather like 'done_retval' instead of 'prev_rtn'.
 
OK, I will rename it.

> > -	/* Cut out the bootconfig data even if we have no bootconfig option */
> > -	initrd_data = get_boot_config_from_initrd(&initrd_size);
> > -	/* If there is no bootconfig in initrd, try embedded one. */
> > -	if (!initrd_data || IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD))
> > -		embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> > +	if (prev_rtn)
> > +		return prev_rtn;
> >  
> > +	embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> >  	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> >  	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
> >  			 bootconfig_params);
> 
> This copy & check should be skipped if IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE) because
> this only checks "bootconfig" is in the cmdline.
> Can you introduce following flag and initialize it here?
> 
> #ifdef CONFIG_BOOT_CONFIG_FORCE
> #define bootconfig_enabled	(true)
> #else
> static bool bootconfig_enabled __initdata;
> #endif

Even when CONFIG_BOOT_CONFIG_FORCE is set, we must call parse_args to find
the location of -- to know where init options should be added. It's done the
same way in the current code.

 
> > -
> > -	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE)))
> > -		return;
> > -
> > +	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE))) {
> > +		prev_rtn = embeded_data ? -ENOMSG : -ENODATA;
> > +		return prev_rtn;
> 
> Than we don't need to set a strange error value...

It's used for error logging as the current code emits a different
messages in these situations, but I will try to refactor it.


> > +	}
> >  	/* parse_args() stops at the next param of '--' and returns an address */
> >  	if (err)
> >  		initargs_offs = err - tmp_cmdline;
> >  
> > -	if (!initrd_data && !embeded_data) {
> > -		/* If user intended to use bootconfig, show an error level message */
> > -		if (bootconfig_found)
> > -			pr_err("'bootconfig' found on command line, but no bootconfig found\n");
> > -		else
> > -			pr_info("No bootconfig data provided, so skipping bootconfig");
> > -		return;
> > +	if (!embeded_data) {
> > +		prev_rtn = -ENOPROTOOPT;
> 
> Also, we can recheck xbc_get_embedded_bootconfig(&embeded_size) later instead of
> using this error code.

ok, I will try to refactor the error logging. Calling
xbc_get_embedded_bootconfig is cheap.

> > +		return prev_rtn;
> >  	}
> >  
> >  	ret = xbc_init(embeded_data, embeded_size, &msg, &pos);
> > -	if (ret < 0)
> > -		goto err0;
> > +	if (ret < 0) {
> > +		boot_config_pr_err(msg, pos, "embedded");
> > +		prev_rtn = ret;
> > +		return prev_rtn;
> > +	}
> > +	prev_rtn = 1;
> 
> This function should be splitted into init_embedded_boot_config() and
> apply_boot_config_early(). The latter one should not be called twice.
>
> > .....
> > +
> > +static void __init setup_boot_config(void)
> > +{
> > +	const char *msg, *initrd_data;
> > +	int pos, ret;
> > +	size_t initrd_size, s;
> > +
> > +	/* Cut out the bootconfig data even if we have no bootconfig option */
> > +	initrd_data = get_boot_config_from_initrd(&initrd_size);
> > +
> > +	ret = setup_boot_config_early();
> 
> Because you should not apply early_params here, you need to call only
> init_embedded_boot_config() here.

setup_boot_config_early() must be called from 2 places, because there is
no guarantee the architecture specific code calls parse_early_param() - it's
not mandatory. If it's not called by architecture, it's called quite late
by start_kernel(), later than setup_boot_config().
I want to avoid different behavior on different architectures, so I always
process early options in the embedded config only, although on some
architectures even these from initrd could be used, but it could cause
issues in the future if the architecture would need to switch.


> > +	if (ret == -ENOMSG || (ret == -ENODATA && initrd_data)) {
> 
> Also, this can be
> 	if (!bootconfig_enabled) {
> 		if (initrd_data || xbc_get_embedded_bootconfig(&s))
> 
> > +		pr_info("Bootconfig data present, but handling is disabled\n");
> > +		return;
> 
> 
> > +	} else if (ret == -ENODATA) {
> > +		/* Bootconfig disabled and bootconfig data are not present */
> > +		return;
> 
> this can be removed.
> 
> > +	} else if (ret == -ENOPROTOOPT) {
> 
> This should be
> 
> 	} else {
> 
> > +		/* Embedded bootconfig not found */
> > +		if (!initrd_data) {
> > +			pr_err("'bootconfig' found on command line, but no bootconfig data found\n");
> > +			return;
> > +		}
> > +		ret = xbc_init(NULL, 0, &msg, &pos);
> > +		if (ret)
> > +			goto err0;
> 
> > +	} else if (ret < 0) {
> > +		/* Other error, should be logged already */
> > +		return;
> 
> So this is checked at first.
> 
> > +	} else if (initrd_data && !IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD)) {
> 
> And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> should be removed.

I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
compatible change and I didn't want to risk breaking current use cases.
My change tries to get early options working without affecting how
other options are handled, but I think appending the config is more
reasonable behavior and if you do not see it as a problem to not be
backward compatible here, I will delete the "replace" behavior.

I will try to refactor the error handling.
  Petr

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Petr Malat]

Hi!
On Thu, Nov 23, 2023 at 11:18:05PM +0900, Masami Hiramatsu wrote:
> On Thu, 23 Nov 2023 11:04:00 +0100
> > On Thu, Nov 23, 2023 at 11:22:07AM +0900, Masami Hiramatsu wrote:
> > > BTW, we also need to make a block-list for some early params. some of those
> > > MUST be passed from the bootloader. E.g. initrd address and size will be
> > > passed from the bootloader via commandline. Thus such params in the embedded
> > > bootconfig should be filtered at the build time.
> > 
> > It's ok to configure these in the embedded bootconfig - in a case they are
> > provided by the bootloader, the bootconfig value is overridden, if not, the
> > value from bootconfig is used, so it works as expected.
> 
> I meant some params only bootloader knows, like where the initrd is loaded.
> Anyway, if user sets such value, it will break the kernel boot as expected :P.

If somebody sets these by hand, he probably knows what is he doing. I
remember myself hardcoding initrd size in Linux to workaround broken
bootloader.

I find it more important to warn the user when he tries to set early
options in initrd (assuming my change gets merged), as that's what
burned me.
  Petr

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Masami Hiramatsu]

Hi Petr,

On Fri, 24 Nov 2023 02:58:01 +0100
Petr Malat <oss@malat.biz> wrote:

> On Thu, Nov 23, 2023 at 07:41:06PM +0900, Masami Hiramatsu wrote:
> > On Wed, 22 Nov 2023 00:13:42 +0100
> > Petr Malat <oss@malat.biz> wrote:
> > > +	static int prev_rtn __initdata;
> > 
> > I would rather like 'done_retval' instead of 'prev_rtn'.
>  
> OK, I will rename it.
> 
> > > -	/* Cut out the bootconfig data even if we have no bootconfig option */
> > > -	initrd_data = get_boot_config_from_initrd(&initrd_size);
> > > -	/* If there is no bootconfig in initrd, try embedded one. */
> > > -	if (!initrd_data || IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD))
> > > -		embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> > > +	if (prev_rtn)
> > > +		return prev_rtn;
> > >  
> > > +	embeded_data = xbc_get_embedded_bootconfig(&embeded_size);
> > >  	strscpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
> > >  	err = parse_args("bootconfig", tmp_cmdline, NULL, 0, 0, 0, NULL,
> > >  			 bootconfig_params);
> > 
> > This copy & check should be skipped if IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE) because
> > this only checks "bootconfig" is in the cmdline.
> > Can you introduce following flag and initialize it here?
> > 
> > #ifdef CONFIG_BOOT_CONFIG_FORCE
> > #define bootconfig_enabled	(true)
> > #else
> > static bool bootconfig_enabled __initdata;
> > #endif
> 
> Even when CONFIG_BOOT_CONFIG_FORCE is set, we must call parse_args to find
> the location of -- to know where init options should be added. It's done the
> same way in the current code.

Ah, got it.

> 
>  
> > > -
> > > -	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE)))
> > > -		return;
> > > -
> > > +	if (IS_ERR(err) || !(bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE))) {
> > > +		prev_rtn = embeded_data ? -ENOMSG : -ENODATA;
> > > +		return prev_rtn;
> > 
> > Than we don't need to set a strange error value...
> 
> It's used for error logging as the current code emits a different
> messages in these situations, but I will try to refactor it.

Yeah, I recommend you to set a global flag if initializing something.

> > > +	}
> > >  	/* parse_args() stops at the next param of '--' and returns an address */
> > >  	if (err)
> > >  		initargs_offs = err - tmp_cmdline;
> > >  
> > > -	if (!initrd_data && !embeded_data) {
> > > -		/* If user intended to use bootconfig, show an error level message */
> > > -		if (bootconfig_found)
> > > -			pr_err("'bootconfig' found on command line, but no bootconfig found\n");
> > > -		else
> > > -			pr_info("No bootconfig data provided, so skipping bootconfig");
> > > -		return;
> > > +	if (!embeded_data) {
> > > +		prev_rtn = -ENOPROTOOPT;
> > 
> > Also, we can recheck xbc_get_embedded_bootconfig(&embeded_size) later instead of
> > using this error code.
> 
> ok, I will try to refactor the error logging. Calling
> xbc_get_embedded_bootconfig is cheap.

Yes.

> 
> > > +		return prev_rtn;
> > >  	}
> > >  
> > >  	ret = xbc_init(embeded_data, embeded_size, &msg, &pos);
> > > -	if (ret < 0)
> > > -		goto err0;
> > > +	if (ret < 0) {
> > > +		boot_config_pr_err(msg, pos, "embedded");
> > > +		prev_rtn = ret;
> > > +		return prev_rtn;
> > > +	}
> > > +	prev_rtn = 1;
> > 
> > This function should be splitted into init_embedded_boot_config() and
> > apply_boot_config_early(). The latter one should not be called twice.
> >
> > > .....
> > > +
> > > +static void __init setup_boot_config(void)
> > > +{
> > > +	const char *msg, *initrd_data;
> > > +	int pos, ret;
> > > +	size_t initrd_size, s;
> > > +
> > > +	/* Cut out the bootconfig data even if we have no bootconfig option */
> > > +	initrd_data = get_boot_config_from_initrd(&initrd_size);
> > > +
> > > +	ret = setup_boot_config_early();
> > 
> > Because you should not apply early_params here, you need to call only
> > init_embedded_boot_config() here.
> 
> setup_boot_config_early() must be called from 2 places, because there is
> no guarantee the architecture specific code calls parse_early_param() - it's
> not mandatory. If it's not called by architecture, it's called quite late
> by start_kernel(), later than setup_boot_config().

Right. I meant that you can skip the second one if the first one is
called.

> I want to avoid different behavior on different architectures, so I always
> process early options in the embedded config only, although on some
> architectures even these from initrd could be used, but it could cause
> issues in the future if the architecture would need to switch.

Ah, I got it. There are 2 cases.

- If setup_arch() calls parse_early_param(), the 2nd setup_boot_config_early()
  in the setup_boot_config() will do nothing.

- If setup_arch() does not call parse_early_param(), the 1st
  setup_boot_config_early() in the setup_boot_config() will apply early params
  but the 2nd setup_boot_config_early() in the parse_early_param() will do nothing.
  
OK. And can you write a comment it?

> 
> 
> > > +	if (ret == -ENOMSG || (ret == -ENODATA && initrd_data)) {
> > 
> > Also, this can be
> > 	if (!bootconfig_enabled) {
> > 		if (initrd_data || xbc_get_embedded_bootconfig(&s))
> > 
> > > +		pr_info("Bootconfig data present, but handling is disabled\n");
> > > +		return;
> > 
> > 
> > > +	} else if (ret == -ENODATA) {
> > > +		/* Bootconfig disabled and bootconfig data are not present */
> > > +		return;
> > 
> > this can be removed.
> > 
> > > +	} else if (ret == -ENOPROTOOPT) {
> > 
> > This should be
> > 
> > 	} else {
> > 
> > > +		/* Embedded bootconfig not found */
> > > +		if (!initrd_data) {
> > > +			pr_err("'bootconfig' found on command line, but no bootconfig data found\n");
> > > +			return;
> > > +		}
> > > +		ret = xbc_init(NULL, 0, &msg, &pos);
> > > +		if (ret)
> > > +			goto err0;
> > 
> > > +	} else if (ret < 0) {
> > > +		/* Other error, should be logged already */
> > > +		return;
> > 
> > So this is checked at first.
> > 
> > > +	} else if (initrd_data && !IS_ENABLED(CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD)) {
> > 
> > And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> > should be removed.
> 
> I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
> compatible change and I didn't want to risk breaking current use cases.
> My change tries to get early options working without affecting how
> other options are handled, but I think appending the config is more
> reasonable behavior and if you do not see it as a problem to not be
> backward compatible here, I will delete the "replace" behavior.

That's a good point. OK if disabling CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD,
it must skip setting early_params to avoid "hidden setting" from the
embedded bootconfig.

Thank you,

> 
> I will try to refactor the error handling.
>   Petr


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

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Petr Malat]

Hi Masami,
On Mon, Nov 27, 2023 at 07:46:30AM +0900, Masami Hiramatsu wrote:

Shortened the mail as this seems to be the last open point

> > > And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> > > should be removed.
> > 
> > I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
> > compatible change and I didn't want to risk breaking current use cases.
> > My change tries to get early options working without affecting how
> > other options are handled, but I think appending the config is more
> > reasonable behavior and if you do not see it as a problem to not be
> > backward compatible here, I will delete the "replace" behavior.
> 
> That's a good point. OK if disabling CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD,
> it must skip setting early_params to avoid "hidden setting" from the
> embedded bootconfig.
 
That's not a good idea because then disabling BOOT_CONFIG_EMBED_APPEND_INITRD
would disable early options handling even if the user doesn't use initrd at
all, which we do not want.

I suggest logging a KERN_NOTICE message if any early option was applied and
at the same time embedded bootconfig was replaced.
  Petr

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Masami Hiramatsu]

On Mon, 27 Nov 2023 16:02:22 +0100
Petr Malat <oss@malat.biz> wrote:

> Hi Masami,
> On Mon, Nov 27, 2023 at 07:46:30AM +0900, Masami Hiramatsu wrote:
> 
> Shortened the mail as this seems to be the last open point
> 
> > > > And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> > > > should be removed.
> > > 
> > > I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
> > > compatible change and I didn't want to risk breaking current use cases.
> > > My change tries to get early options working without affecting how
> > > other options are handled, but I think appending the config is more
> > > reasonable behavior and if you do not see it as a problem to not be
> > > backward compatible here, I will delete the "replace" behavior.
> > 
> > That's a good point. OK if disabling CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD,
> > it must skip setting early_params to avoid "hidden setting" from the
> > embedded bootconfig.
>  
> That's not a good idea because then disabling BOOT_CONFIG_EMBED_APPEND_INITRD
> would disable early options handling even if the user doesn't use initrd at
> all, which we do not want.

Yes, so the config name and comment needs to be updated. The problematic point
is that we can not know where there is an initrd (and bootconfig on initrd)
until parsing/applying the early params. And we have to avoid setting "hidden"
parameter.

> 
> I suggest logging a KERN_NOTICE message if any early option was applied and
> at the same time embedded bootconfig was replaced.

No, that's no good because kernel log can be cleared.
Usually user will check /proc/cmdline to check what parameters are set, so at
least it needs to be updated, but also, we need to keep the original cmdline
for make sure the user can reuse it for kexec. To solve this issue without
contradiction, we need to ask user to set BOOT_CONFIG_EMBED_APPEND_INITRD=y
to support early params in bootconfig. (Thus it will be something like
"BOOT_CONFIG_EMBED_EARLY_PARAM")

Thank you,

>   Petr


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

Re: [PATCH 2/2] bootconfig: Apply early options from embedded config

[Petr Malat]

Hi!
Sorry for late reply, I didn't have much time to work on this the last week.

On Tue, Nov 28, 2023 at 07:13:55AM +0900, Masami Hiramatsu wrote:
> On Mon, 27 Nov 2023 16:02:22 +0100
> Petr Malat <oss@malat.biz> wrote:
> 
> > Hi Masami,
> > On Mon, Nov 27, 2023 at 07:46:30AM +0900, Masami Hiramatsu wrote:
> > 
> > Shortened the mail as this seems to be the last open point
> > 
> > > > > And as I pointed, we can remove CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD so this case
> > > > > should be removed.
> > > > 
> > > > I have added BOOT_CONFIG_EMBED_APPEND_INITRD, because it's not backward
> > > > compatible change and I didn't want to risk breaking current use cases.
> > > > My change tries to get early options working without affecting how
> > > > other options are handled, but I think appending the config is more
> > > > reasonable behavior and if you do not see it as a problem to not be
> > > > backward compatible here, I will delete the "replace" behavior.
> > > 
> > > That's a good point. OK if disabling CONFIG_BOOT_CONFIG_EMBED_APPEND_INITRD,
> > > it must skip setting early_params to avoid "hidden setting" from the
> > > embedded bootconfig.
> >  
> > That's not a good idea because then disabling BOOT_CONFIG_EMBED_APPEND_INITRD
> > would disable early options handling even if the user doesn't use initrd at
> > all, which we do not want.
> 
> Yes, so the config name and comment needs to be updated. The problematic point
> is that we can not know where there is an initrd (and bootconfig on initrd)
> until parsing/applying the early params. And we have to avoid setting "hidden"
> parameter.
> 
> > 
> > I suggest logging a KERN_NOTICE message if any early option was applied and
> > at the same time embedded bootconfig was replaced.
> 
> No, that's no good because kernel log can be cleared.
> Usually user will check /proc/cmdline to check what parameters are set, so at
> least it needs to be updated, but also, we need to keep the original cmdline
> for make sure the user can reuse it for kexec. To solve this issue without
> contradiction, we need to ask user to set BOOT_CONFIG_EMBED_APPEND_INITRD=y
> to support early params in bootconfig. (Thus it will be something like
> "BOOT_CONFIG_EMBED_EARLY_PARAM")

I made a bit of prototyping and I can add applied early options from embedded
config to /proc/cmdline even when the embedded config is replaced. I don't like
applying these options conditionally, because that leads to confusing behavior
of seeing these options in /proc/cmdline and /proc/bootconfig without actually
applying them. I find that worst than the opposite behavior as if user sets
options in the config and then sees them in both cmdline and bootconfig, he
assumes they have been applied.

We can also introduce BOOT_CONFIG_EMBED_REPLACE_INITRD for backward compatibility
and write that this option will be removed in the future. If nobody complains
for a while, we drop it. Then we are left with the appending only.
  Petr