[PATCH 00/11] bootm: Support substitions in bootargs and add tests

[PATCH 00/11] bootm: Support substitions in bootargs and add tests

[Simon Glass]

This series adds tests to the fixup_silent-linux() function and extends
the 'zimage' command to use it.

It also adds a new string-substition feature to allow bootargs to be a
template, rather than having to build it up piece by piece with
information obtained in a build script.

It also updates zimage to use the same command-line processing.

With these additions it is possible to boot Chrome OS from a U-Boot script
on most Chromebooks.


Simon Glass (11):
  env: Allow returning errors from hdelete_r()
  bootm: Add tests for fixup_silent_linux()
  bootm: Update fixup_silent_linux() to return an error
  bootm: Rename fixup_silent_linux()
  bootm: Add a bool parameter to bootm_process_cmdline_env()
  bootm: Use size rather than length for CONSOLE_ARG
  bootm: Split out bootargs environment reading / writing
  bootm: Update bootm_process_cmdline_env() to use flags
  bootm: Allow updating the bootargs in a buffer
  x86: zimage: Add silent-console processing
  bootm: Support string substitution in bootargs

 README                |  16 +++
 arch/Kconfig          |   2 +
 arch/x86/lib/zimage.c |  14 +++
 cmd/nvedit.c          |   6 +-
 common/Kconfig.boot   |  20 ++++
 common/bootm.c        | 206 +++++++++++++++++++++++++++++------
 include/bootm.h       |  40 +++++++
 include/search.h      |  11 +-
 include/test/suites.h |   1 +
 lib/hashtable.c       |  12 +--
 test/Makefile         |   1 +
 test/bootm.c          | 243 ++++++++++++++++++++++++++++++++++++++++++
 test/cmd_ut.c         |   1 +
 test/env/hashtable.c  |   2 +-
 14 files changed, 534 insertions(+), 41 deletions(-)
 create mode 100644 test/bootm.c

-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 01/11] env: Allow returning errors from hdelete_r()

[Simon Glass]

At present this function returns 1 on success and 0 on failure. But in
the latter case it provides no indication of what went wrong.

If an attempt is made to delete a non-existent variable, the caller may
want to ignore this error. This happens when setting a non-existent
variable to "", for example.

Update the function to return 0 on success and a useful error code on
failure. Add a function comment too.

Make sure that env_set() does not return an error if it is deleting a
variable that doesn't exist. We could update env_set() to return useful
error numbers also, but that is beyond the scope of this change.

Signed-off-by: Simon Glass <sjg@chromium.org>

wip

---

 cmd/nvedit.c         |  6 ++++--
 include/search.h     | 11 ++++++++++-
 lib/hashtable.c      | 12 ++++++------
 test/env/hashtable.c |  2 +-
 4 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 7fce723800d..d0d2eca9047 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -266,7 +266,9 @@ static int _do_env_set(int flag, int argc, char *const argv[], int env_flag)
 	/* Delete only ? */
 	if (argc < 3 || argv[2] == NULL) {
 		int rc = hdelete_r(name, &env_htab, env_flag);
-		return !rc;
+
+		/* If the variable didn't exist, don't report an error */
+		return rc && rc != -ENOENT ? 1 : 0;
 	}
 
 	/*
@@ -895,7 +897,7 @@ static int do_env_delete(struct cmd_tbl *cmdtp, int flag,
 	while (--argc > 0) {
 		char *name = *++argv;
 
-		if (!hdelete_r(name, &env_htab, env_flag))
+		if (hdelete_r(name, &env_htab, env_flag))
 			ret = 1;
 	}
 
diff --git a/include/search.h b/include/search.h
index e56843c26fd..d0bb44388e1 100644
--- a/include/search.h
+++ b/include/search.h
@@ -80,7 +80,16 @@ int hsearch_r(struct env_entry item, enum env_action action,
 int hmatch_r(const char *match, int last_idx, struct env_entry **retval,
 	     struct hsearch_data *htab);
 
-/* Search and delete entry matching "key" in internal hash table. */
+/**
+ * hdelete_r() - Search and delete entry in internal hash table
+ *
+ * @key: Name of entry to delete
+ * @htab: Hash table
+ * @flag: Flags to use (H_...)
+ * @return 0 on success, -ENOENT if not found, -EPERM if the hash table callback
+ *	rejected changing the variable, -EINVAL if the hash table refused to
+ *	delete the variable
+ */
 int hdelete_r(const char *key, struct hsearch_data *htab, int flag);
 
 ssize_t hexport_r(struct hsearch_data *htab, const char sep, int flag,
diff --git a/lib/hashtable.c b/lib/hashtable.c
index 7c08f5c8055..ff5ff726394 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -472,7 +472,7 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
 	idx = hsearch_r(e, ENV_FIND, &ep, htab, 0);
 	if (idx == 0) {
 		__set_errno(ESRCH);
-		return 0;	/* not found */
+		return -ENOENT;	/* not found */
 	}
 
 	/* Check for permission */
@@ -481,7 +481,7 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
 		debug("change_ok() rejected deleting variable "
 			"%s, skipping it!\n", key);
 		__set_errno(EPERM);
-		return 0;
+		return -EPERM;
 	}
 
 	/* If there is a callback, call it */
@@ -490,12 +490,12 @@ int hdelete_r(const char *key, struct hsearch_data *htab, int flag)
 		debug("callback() rejected deleting variable "
 			"%s, skipping it!\n", key);
 		__set_errno(EINVAL);
-		return 0;
+		return -EINVAL;
 	}
 
 	_hdelete(key, htab, ep, idx);
 
-	return 1;
+	return 0;
 }
 
 #if !(defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_SAVEENV))
@@ -917,7 +917,7 @@ int himport_r(struct hsearch_data *htab,
 			if (!drop_var_from_set(name, nvars, localvars))
 				continue;
 
-			if (hdelete_r(name, htab, flag) == 0)
+			if (hdelete_r(name, htab, flag))
 				debug("DELETE ERROR ##############################\n");
 
 			continue;
@@ -979,7 +979,7 @@ int himport_r(struct hsearch_data *htab,
 		 * b) if the variable was not present in current env, we notify
 		 *    it might be a typo
 		 */
-		if (hdelete_r(localvars[i], htab, flag) == 0)
+		if (hdelete_r(localvars[i], htab, flag))
 			printf("WARNING: '%s' neither in running nor in imported env!\n", localvars[i]);
 		else
 			printf("WARNING: '%s' not in imported env, deleting it!\n", localvars[i]);
diff --git a/test/env/hashtable.c b/test/env/hashtable.c
index 339cc19ba14..70102f9121c 100644
--- a/test/env/hashtable.c
+++ b/test/env/hashtable.c
@@ -80,7 +80,7 @@ static int htab_create_delete(struct unit_test_state *uts,
 		ut_asserteq_str(key, ritem->key);
 		ut_asserteq_str(key, ritem->data);
 
-		ut_asserteq(1, hdelete_r(key, htab, 0));
+		ut_asserteq(0, hdelete_r(key, htab, 0));
 	}
 
 	return 0;
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 02/11] bootm: Add tests for fixup_silent_linux()

[Simon Glass]

This function currently has no tests. Export it so that we can implement
a simple test on sandbox. Use IS_ENABLED() to remove the unused code,
instead #ifdef.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/Kconfig          |  1 +
 common/bootm.c        | 14 +++++-----
 include/bootm.h       |  3 +++
 include/test/suites.h |  1 +
 test/Makefile         |  1 +
 test/bootm.c          | 59 +++++++++++++++++++++++++++++++++++++++++++
 test/cmd_ut.c         |  1 +
 7 files changed, 73 insertions(+), 7 deletions(-)
 create mode 100644 test/bootm.c

diff --git a/arch/Kconfig b/arch/Kconfig
index 683e3843190..6caf2338bcf 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -140,6 +140,7 @@ config SANDBOX
 	imply ACPI_PMC_SANDBOX
 	imply CMD_PMC
 	imply CMD_CLONE
+	imply SILENT_CONSOLE
 
 config SH
 	bool "SuperH architecture"
diff --git a/common/bootm.c b/common/bootm.c
index b3377490b3e..8e1e5337036 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -463,18 +463,21 @@ ulong bootm_disable_interrupts(void)
 	return iflag;
 }
 
-#if defined(CONFIG_SILENT_CONSOLE) && !defined(CONFIG_SILENT_U_BOOT_ONLY)
-
 #define CONSOLE_ARG     "console="
 #define CONSOLE_ARG_LEN (sizeof(CONSOLE_ARG) - 1)
 
-static void fixup_silent_linux(void)
+void fixup_silent_linux(void)
 {
 	char *buf;
 	const char *env_val;
-	char *cmdline = env_get("bootargs");
+	char *cmdline;
 	int want_silent;
 
+	if (!IS_ENABLED(CONFIG_SILENT_CONSOLE) &&
+	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY))
+		return;
+	cmdline = env_get("bootargs");
+
 	/*
 	 * Only fix cmdline when requested. The environment variable can be:
 	 *
@@ -521,7 +524,6 @@ static void fixup_silent_linux(void)
 	debug("after silent fix-up: %s\n", env_val);
 	free(buf);
 }
-#endif /* CONFIG_SILENT_CONSOLE */
 
 /**
  * Execute selected states of the bootm command.
@@ -625,10 +627,8 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (!ret && (states & BOOTM_STATE_OS_BD_T))
 		ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images);
 	if (!ret && (states & BOOTM_STATE_OS_PREP)) {
-#if defined(CONFIG_SILENT_CONSOLE) && !defined(CONFIG_SILENT_U_BOOT_ONLY)
 		if (images->os.os == IH_OS_LINUX)
 			fixup_silent_linux();
-#endif
 		ret = boot_fn(BOOTM_STATE_OS_PREP, argc, argv, images);
 	}
 
diff --git a/include/bootm.h b/include/bootm.h
index a812a6bf24f..6d675e64559 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -85,4 +85,7 @@ void arch_preboot_os(void);
  */
 void board_preboot_os(void);
 
+/* Adjust the 'bootargs' to ensure that Linux boots silently, if required */
+void fixup_silent_linux(void);
+
 #endif
diff --git a/include/test/suites.h b/include/test/suites.h
index ab7b3bd9cad..c1d8fa9a650 100644
--- a/include/test/suites.h
+++ b/include/test/suites.h
@@ -26,6 +26,7 @@ int cmd_ut_category(const char *name, const char *prefix,
 		    struct unit_test *tests, int n_ents,
 		    int argc, char *const argv[]);
 
+int do_ut_bootm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_bloblist(struct cmd_tbl *cmdtp, int flag, int argc,
 		   char *const argv[]);
 int do_ut_compression(struct cmd_tbl *cmdtp, int flag, int argc,
diff --git a/test/Makefile b/test/Makefile
index 7c4039964e1..099f855aefe 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -3,6 +3,7 @@
 # (C) Copyright 2012 The Chromium Authors
 
 obj-$(CONFIG_SANDBOX) += bloblist.o
+obj-$(CONFIG_SANDBOX) += bootm.o
 obj-$(CONFIG_CMDLINE) += cmd/
 obj-$(CONFIG_UNIT_TEST) += cmd_ut.o
 obj-$(CONFIG_UNIT_TEST) += ut.o
diff --git a/test/bootm.c b/test/bootm.c
new file mode 100644
index 00000000000..59d16cb3df6
--- /dev/null
+++ b/test/bootm.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Tests for bootm routines
+ *
+ * Copyright 2020 Google LLC
+ */
+
+#include <common.h>
+#include <bootm.h>
+#include <test/suites.h>
+#include <test/test.h>
+#include <test/ut.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define BOOTM_TEST(_name, _flags)	UNIT_TEST(_name, _flags, bootm_test)
+
+#define CONSOLE_STR	"console=/dev/ttyS0"
+
+/* Test silent processing in the bootargs variable */
+static int bootm_test_silent_var(struct unit_test_state *uts)
+{
+	/* 'silent_linux' not set should do nothing */
+	env_set("silent_linux", NULL);
+	env_set("bootargs", CONSOLE_STR);
+	fixup_silent_linux();
+	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
+
+	env_set("bootargs", NULL);
+	fixup_silent_linux();
+	ut_assertnull(env_get("bootargs"));
+
+	ut_assertok(env_set("silent_linux", "no"));
+	env_set("bootargs", CONSOLE_STR);
+	fixup_silent_linux();
+	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
+
+	ut_assertok(env_set("silent_linux", "yes"));
+	env_set("bootargs", CONSOLE_STR);
+	fixup_silent_linux();
+	ut_asserteq_str("console=", env_get("bootargs"));
+
+	/* Empty buffer should still add the string */
+	env_set("bootargs", NULL);
+	fixup_silent_linux();
+	ut_asserteq_str("console=", env_get("bootargs"));
+
+	return 0;
+}
+BOOTM_TEST(bootm_test_silent_var, 0);
+
+int do_ut_bootm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
+{
+	struct unit_test *tests = ll_entry_start(struct unit_test, bootm_test);
+	const int n_ents = ll_entry_count(struct unit_test, bootm_test);
+
+	return cmd_ut_category("bootm", "bootm_test_", tests, n_ents,
+			       argc, argv);
+}
diff --git a/test/cmd_ut.c b/test/cmd_ut.c
index 8f0bc688a22..8d5c28b4319 100644
--- a/test/cmd_ut.c
+++ b/test/cmd_ut.c
@@ -86,6 +86,7 @@ static struct cmd_tbl cmd_ut_sub[] = {
 			 "", ""),
 	U_BOOT_CMD_MKENT(bloblist, CONFIG_SYS_MAXARGS, 1, do_ut_bloblist,
 			 "", ""),
+	U_BOOT_CMD_MKENT(bootm, CONFIG_SYS_MAXARGS, 1, do_ut_bootm, "", ""),
 	U_BOOT_CMD_MKENT(str, CONFIG_SYS_MAXARGS, 1, do_ut_str,
 			 "", ""),
 #endif
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 03/11] bootm: Update fixup_silent_linux() to return an error

[Simon Glass]

At present this function fails silently on error. Update it to produce
an error code. Report this error to the user and abort the boot, since it
likely will prevent a successful start.

No tests are added at this stage, since additional refactoring is taking
place in subsequent patches.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/bootm.c  | 22 +++++++++++++++-------
 include/bootm.h | 11 +++++++++--
 test/bootm.c    | 10 +++++-----
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index 8e1e5337036..0ca66188c2b 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -466,7 +466,7 @@ ulong bootm_disable_interrupts(void)
 #define CONSOLE_ARG     "console="
 #define CONSOLE_ARG_LEN (sizeof(CONSOLE_ARG) - 1)
 
-void fixup_silent_linux(void)
+int fixup_silent_linux(void)
 {
 	char *buf;
 	const char *env_val;
@@ -475,7 +475,7 @@ void fixup_silent_linux(void)
 
 	if (!IS_ENABLED(CONFIG_SILENT_CONSOLE) &&
 	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY))
-		return;
+		return 0;
 	cmdline = env_get("bootargs");
 
 	/*
@@ -487,9 +487,9 @@ void fixup_silent_linux(void)
 	 */
 	want_silent = env_get_yesno("silent_linux");
 	if (want_silent == 0)
-		return;
+		return 0;
 	else if (want_silent == -1 && !(gd->flags & GD_FLG_SILENT))
-		return;
+		return 0;
 
 	debug("before silent fix-up: %s\n", cmdline);
 	if (cmdline && (cmdline[0] != '\0')) {
@@ -499,7 +499,7 @@ void fixup_silent_linux(void)
 		buf = malloc(strlen(cmdline) + 1 + CONSOLE_ARG_LEN + 1);
 		if (!buf) {
 			debug("%s: out of memory\n", __func__);
-			return;
+			return -ENOSPC;
 		}
 
 		if (start) {
@@ -523,6 +523,8 @@ void fixup_silent_linux(void)
 	env_set("bootargs", env_val);
 	debug("after silent fix-up: %s\n", env_val);
 	free(buf);
+
+	return 0;
 }
 
 /**
@@ -627,8 +629,14 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (!ret && (states & BOOTM_STATE_OS_BD_T))
 		ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images);
 	if (!ret && (states & BOOTM_STATE_OS_PREP)) {
-		if (images->os.os == IH_OS_LINUX)
-			fixup_silent_linux();
+		if (images->os.os == IH_OS_LINUX) {
+			ret = fixup_silent_linux();
+			if (ret) {
+				printf("Cmdline setup failed (err=%d)\n", ret);
+				ret = CMD_RET_FAILURE;
+				goto err;
+			}
+		}
 		ret = boot_fn(BOOTM_STATE_OS_PREP, argc, argv, images);
 	}
 
diff --git a/include/bootm.h b/include/bootm.h
index 6d675e64559..438829af0fe 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -85,7 +85,14 @@ void arch_preboot_os(void);
  */
 void board_preboot_os(void);
 
-/* Adjust the 'bootargs' to ensure that Linux boots silently, if required */
-void fixup_silent_linux(void);
+/*
+ * fixup_silent_linux() - Process fix-ups for the command line
+ *
+ * Updates the 'bootargs' envvar as required. This handles making Linux boot
+ * silently if requested ('silent_linux' envvar)
+ *
+ * @return 0 if OK, -ENOMEM if out of memory
+ */
+int fixup_silent_linux(void);
 
 #endif
diff --git a/test/bootm.c b/test/bootm.c
index 59d16cb3df6..ab1711609ba 100644
--- a/test/bootm.c
+++ b/test/bootm.c
@@ -23,26 +23,26 @@ static int bootm_test_silent_var(struct unit_test_state *uts)
 	/* 'silent_linux' not set should do nothing */
 	env_set("silent_linux", NULL);
 	env_set("bootargs", CONSOLE_STR);
-	fixup_silent_linux();
+	ut_assertok(fixup_silent_linux());
 	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
 
 	env_set("bootargs", NULL);
-	fixup_silent_linux();
+	ut_assertok(fixup_silent_linux());
 	ut_assertnull(env_get("bootargs"));
 
 	ut_assertok(env_set("silent_linux", "no"));
 	env_set("bootargs", CONSOLE_STR);
-	fixup_silent_linux();
+	ut_assertok(fixup_silent_linux());
 	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
 
 	ut_assertok(env_set("silent_linux", "yes"));
 	env_set("bootargs", CONSOLE_STR);
-	fixup_silent_linux();
+	ut_assertok(fixup_silent_linux());
 	ut_asserteq_str("console=", env_get("bootargs"));
 
 	/* Empty buffer should still add the string */
 	env_set("bootargs", NULL);
-	fixup_silent_linux();
+	ut_assertok(fixup_silent_linux());
 	ut_asserteq_str("console=", env_get("bootargs"));
 
 	return 0;
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 04/11] bootm: Rename fixup_silent_linux()

[Simon Glass]

We want to add more processing to this function. Before doing so, rename
it to bootm_process_cmdline_env(), which is more generic.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/bootm.c  |  4 ++--
 include/bootm.h |  4 ++--
 test/bootm.c    | 10 +++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index 0ca66188c2b..ace5771d53f 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -466,7 +466,7 @@ ulong bootm_disable_interrupts(void)
 #define CONSOLE_ARG     "console="
 #define CONSOLE_ARG_LEN (sizeof(CONSOLE_ARG) - 1)
 
-int fixup_silent_linux(void)
+int bootm_process_cmdline_env(void)
 {
 	char *buf;
 	const char *env_val;
@@ -630,7 +630,7 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
 		ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images);
 	if (!ret && (states & BOOTM_STATE_OS_PREP)) {
 		if (images->os.os == IH_OS_LINUX) {
-			ret = fixup_silent_linux();
+			ret = bootm_process_cmdline_env();
 			if (ret) {
 				printf("Cmdline setup failed (err=%d)\n", ret);
 				ret = CMD_RET_FAILURE;
diff --git a/include/bootm.h b/include/bootm.h
index 438829af0fe..35c27ab9609 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -86,13 +86,13 @@ void arch_preboot_os(void);
 void board_preboot_os(void);
 
 /*
- * fixup_silent_linux() - Process fix-ups for the command line
+ * bootm_process_cmdline_env() - Process fix-ups for the command line
  *
  * Updates the 'bootargs' envvar as required. This handles making Linux boot
  * silently if requested ('silent_linux' envvar)
  *
  * @return 0 if OK, -ENOMEM if out of memory
  */
-int fixup_silent_linux(void);
+int bootm_process_cmdline_env(void);
 
 #endif
diff --git a/test/bootm.c b/test/bootm.c
index ab1711609ba..b69bfad4f67 100644
--- a/test/bootm.c
+++ b/test/bootm.c
@@ -23,26 +23,26 @@ static int bootm_test_silent_var(struct unit_test_state *uts)
 	/* 'silent_linux' not set should do nothing */
 	env_set("silent_linux", NULL);
 	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(fixup_silent_linux());
+	ut_assertok(bootm_process_cmdline_env());
 	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
 
 	env_set("bootargs", NULL);
-	ut_assertok(fixup_silent_linux());
+	ut_assertok(bootm_process_cmdline_env());
 	ut_assertnull(env_get("bootargs"));
 
 	ut_assertok(env_set("silent_linux", "no"));
 	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(fixup_silent_linux());
+	ut_assertok(bootm_process_cmdline_env());
 	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
 
 	ut_assertok(env_set("silent_linux", "yes"));
 	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(fixup_silent_linux());
+	ut_assertok(bootm_process_cmdline_env());
 	ut_asserteq_str("console=", env_get("bootargs"));
 
 	/* Empty buffer should still add the string */
 	env_set("bootargs", NULL);
-	ut_assertok(fixup_silent_linux());
+	ut_assertok(bootm_process_cmdline_env());
 	ut_asserteq_str("console=", env_get("bootargs"));
 
 	return 0;
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 05/11] bootm: Add a bool parameter to bootm_process_cmdline_env()

[Simon Glass]

This function will soon do more than just handle the 'silent linux'
feature. As a first step, update it to take a boolean parameter,
indicating whether or not the processing is required.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/bootm.c  | 20 ++++++++++----------
 include/bootm.h |  3 ++-
 test/bootm.c    | 10 +++++-----
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index ace5771d53f..fa2aecdbc68 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -466,15 +466,17 @@ ulong bootm_disable_interrupts(void)
 #define CONSOLE_ARG     "console="
 #define CONSOLE_ARG_LEN (sizeof(CONSOLE_ARG) - 1)
 
-int bootm_process_cmdline_env(void)
+int bootm_process_cmdline_env(bool do_silent)
 {
 	char *buf;
 	const char *env_val;
 	char *cmdline;
 	int want_silent;
 
-	if (!IS_ENABLED(CONFIG_SILENT_CONSOLE) &&
-	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY))
+	/* First check if any action is needed */
+	do_silent = IS_ENABLED(CONFIG_SILENT_CONSOLE) &&
+	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY) && do_silent;
+	if (!do_silent)
 		return 0;
 	cmdline = env_get("bootargs");
 
@@ -629,13 +631,11 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (!ret && (states & BOOTM_STATE_OS_BD_T))
 		ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images);
 	if (!ret && (states & BOOTM_STATE_OS_PREP)) {
-		if (images->os.os == IH_OS_LINUX) {
-			ret = bootm_process_cmdline_env();
-			if (ret) {
-				printf("Cmdline setup failed (err=%d)\n", ret);
-				ret = CMD_RET_FAILURE;
-				goto err;
-			}
+		ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX);
+		if (ret) {
+			printf("Cmdline setup failed (err=%d)\n", ret);
+			ret = CMD_RET_FAILURE;
+			goto err;
 		}
 		ret = boot_fn(BOOTM_STATE_OS_PREP, argc, argv, images);
 	}
diff --git a/include/bootm.h b/include/bootm.h
index 35c27ab9609..f12ee2b3cb3 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -91,8 +91,9 @@ void board_preboot_os(void);
  * Updates the 'bootargs' envvar as required. This handles making Linux boot
  * silently if requested ('silent_linux' envvar)
  *
+ * @do_silent: Process bootargs for silent console
  * @return 0 if OK, -ENOMEM if out of memory
  */
-int bootm_process_cmdline_env(void);
+int bootm_process_cmdline_env(bool do_silent);
 
 #endif
diff --git a/test/bootm.c b/test/bootm.c
index b69bfad4f67..c203f0acd60 100644
--- a/test/bootm.c
+++ b/test/bootm.c
@@ -23,26 +23,26 @@ static int bootm_test_silent_var(struct unit_test_state *uts)
 	/* 'silent_linux' not set should do nothing */
 	env_set("silent_linux", NULL);
 	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(bootm_process_cmdline_env());
+	ut_assertok(bootm_process_cmdline_env(true));
 	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
 
 	env_set("bootargs", NULL);
-	ut_assertok(bootm_process_cmdline_env());
+	ut_assertok(bootm_process_cmdline_env(true));
 	ut_assertnull(env_get("bootargs"));
 
 	ut_assertok(env_set("silent_linux", "no"));
 	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(bootm_process_cmdline_env());
+	ut_assertok(bootm_process_cmdline_env(true));
 	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
 
 	ut_assertok(env_set("silent_linux", "yes"));
 	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(bootm_process_cmdline_env());
+	ut_assertok(bootm_process_cmdline_env(true));
 	ut_asserteq_str("console=", env_get("bootargs"));
 
 	/* Empty buffer should still add the string */
 	env_set("bootargs", NULL);
-	ut_assertok(bootm_process_cmdline_env());
+	ut_assertok(bootm_process_cmdline_env(true));
 	ut_asserteq_str("console=", env_get("bootargs"));
 
 	return 0;
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 06/11] bootm: Use size rather than length for CONSOLE_ARG

[Simon Glass]

Use the size (including terminator) for in this function, rather than
the length. This is arguably easier to follow, with the coming
refactor.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/bootm.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index fa2aecdbc68..bbe59752609 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -463,8 +463,8 @@ ulong bootm_disable_interrupts(void)
 	return iflag;
 }
 
-#define CONSOLE_ARG     "console="
-#define CONSOLE_ARG_LEN (sizeof(CONSOLE_ARG) - 1)
+#define CONSOLE_ARG		"console="
+#define CONSOLE_ARG_SIZE	sizeof(CONSOLE_ARG)
 
 int bootm_process_cmdline_env(bool do_silent)
 {
@@ -498,7 +498,7 @@ int bootm_process_cmdline_env(bool do_silent)
 		char *start = strstr(cmdline, CONSOLE_ARG);
 
 		/* Allocate space for maximum possible new command line */
-		buf = malloc(strlen(cmdline) + 1 + CONSOLE_ARG_LEN + 1);
+		buf = malloc(strlen(cmdline) + 1 + CONSOLE_ARG_SIZE);
 		if (!buf) {
 			debug("%s: out of memory\n", __func__);
 			return -ENOSPC;
@@ -506,13 +506,14 @@ int bootm_process_cmdline_env(bool do_silent)
 
 		if (start) {
 			char *end = strchr(start, ' ');
-			int num_start_bytes = start - cmdline + CONSOLE_ARG_LEN;
+			int start_bytes;
 
-			strncpy(buf, cmdline, num_start_bytes);
+			start_bytes = start - cmdline + CONSOLE_ARG_SIZE - 1;
+			strncpy(buf, cmdline, start_bytes);
 			if (end)
-				strcpy(buf + num_start_bytes, end);
+				strcpy(buf + start_bytes, end);
 			else
-				buf[num_start_bytes] = '\0';
+				buf[start_bytes] = '\0';
 		} else {
 			sprintf(buf, "%s %s", cmdline, CONSOLE_ARG);
 		}
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 07/11] bootm: Split out bootargs environment reading / writing

[Simon Glass]

At present bootm_process_cmdline_env() reads the 'bootargs' variable and
then writes it back afterwards. This is painful for tests, which would
rather use a simple buffer.

It is also useful for zimage to use a buffer, since it does not actually
put the Linux command line in the bootargs variable.

Refactor the existing code into two pieces. One handles reading and
writing the environment variable, as well as allocating a buffer for use
by the rest of the code, which now operates on a buffer.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/bootm.c | 95 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 73 insertions(+), 22 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index bbe59752609..41965fa304c 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -19,6 +19,7 @@
 #include <net.h>
 #include <asm/cache.h>
 #include <asm/io.h>
+#include <linux/sizes.h>
 #if defined(CONFIG_CMD_USB)
 #include <usb.h>
 #endif
@@ -35,6 +36,8 @@
 #define CONFIG_SYS_BOOTM_LEN	0x800000
 #endif
 
+#define MAX_CMDLINE_SIZE	SZ_4K
+
 #define IH_INITRD_ARCH IH_ARCH_DEFAULT
 
 #ifndef USE_HOSTCC
@@ -466,20 +469,31 @@ ulong bootm_disable_interrupts(void)
 #define CONSOLE_ARG		"console="
 #define CONSOLE_ARG_SIZE	sizeof(CONSOLE_ARG)
 
-int bootm_process_cmdline_env(bool do_silent)
+/**
+ * fixup_silent_linux() - Handle silencing the linux boot if required
+ *
+ * This uses the silent_linux envvar to control whether to add/set a "console="
+ * parameter to the command line
+ *
+ * @buf: Buffer containing the string to process
+ * @maxlen: Maximum length of buffer
+ * @return 0 if OK, -ENOSPC if @maxlen is too small
+ */
+static int fixup_silent_linux(char *buf, int maxlen)
 {
-	char *buf;
-	const char *env_val;
-	char *cmdline;
 	int want_silent;
+	char *cmdline;
+	int size;
 
-	/* First check if any action is needed */
-	do_silent = IS_ENABLED(CONFIG_SILENT_CONSOLE) &&
-	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY) && do_silent;
-	if (!do_silent)
-		return 0;
-	cmdline = env_get("bootargs");
-
+	/*
+	 * Move the input string to the end of buffer. The output string will be
+	 * built up at the start.
+	 */
+	size = strlen(buf) + 1;
+	if (size * 2 > maxlen)
+		return -ENOSPC;
+	cmdline = buf + maxlen - size;
+	memmove(cmdline, buf, size);
 	/*
 	 * Only fix cmdline when requested. The environment variable can be:
 	 *
@@ -494,15 +508,12 @@ int bootm_process_cmdline_env(bool do_silent)
 		return 0;
 
 	debug("before silent fix-up: %s\n", cmdline);
-	if (cmdline && (cmdline[0] != '\0')) {
+	if (*cmdline) {
 		char *start = strstr(cmdline, CONSOLE_ARG);
 
-		/* Allocate space for maximum possible new command line */
-		buf = malloc(strlen(cmdline) + 1 + CONSOLE_ARG_SIZE);
-		if (!buf) {
-			debug("%s: out of memory\n", __func__);
+		/* Check space for maximum possible new command line */
+		if (size + CONSOLE_ARG_SIZE > maxlen)
 			return -ENOSPC;
-		}
 
 		if (start) {
 			char *end = strchr(start, ' ');
@@ -517,15 +528,55 @@ int bootm_process_cmdline_env(bool do_silent)
 		} else {
 			sprintf(buf, "%s %s", cmdline, CONSOLE_ARG);
 		}
-		env_val = buf;
+		if (buf + strlen(buf) >= cmdline)
+			return -ENOSPC;
 	} else {
-		buf = NULL;
-		env_val = CONSOLE_ARG;
+		if (maxlen < sizeof(CONSOLE_ARG))
+			return -ENOSPC;
+		strcpy(buf, CONSOLE_ARG);
 	}
+	debug("after silent fix-up: %s\n", buf);
 
-	env_set("bootargs", env_val);
-	debug("after silent fix-up: %s\n", env_val);
+	return 0;
+}
+
+int bootm_process_cmdline_env(bool do_silent)
+{
+	const int maxlen = MAX_CMDLINE_SIZE;
+	const char *env;
+	char *buf;
+	int ret;
+
+	/* First check if any action is needed */
+	do_silent = IS_ENABLED(CONFIG_SILENT_CONSOLE) &&
+	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY) && do_silent;
+	if (!do_silent)
+		return 0;
+
+	env = env_get("bootargs");
+	if (env && strlen(env) >= maxlen)
+		return -E2BIG;
+	buf = malloc(maxlen);
+	if (!buf)
+		return -ENOMEM;
+	if (env)
+		strcpy(buf, env);
+	else
+		*buf = '\0';
+	ret = fixup_silent_linux(buf, maxlen);
+	if (!ret) {
+		ret = env_set("bootargs", buf);
+
+		/*
+		 * If buf is "" and bootargs does not exist, this will produce
+		 * an error trying to delete bootargs. Ignore it
+		 */
+		if (ret == -ENOENT)
+			ret = 0;
+	}
 	free(buf);
+	if (ret)
+		return log_msg_ret("env", ret);
 
 	return 0;
 }
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 08/11] bootm: Update bootm_process_cmdline_env() to use flags

[Simon Glass]

At present only one transformation is supported: making the Linux console
silent. To prepare for adding more, convert the boolean parameter into a
flag value.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/bootm.c  |  8 +++++---
 include/bootm.h | 11 +++++++++--
 test/bootm.c    | 10 +++++-----
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index 41965fa304c..5a5b79c5cd0 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -540,16 +540,17 @@ static int fixup_silent_linux(char *buf, int maxlen)
 	return 0;
 }
 
-int bootm_process_cmdline_env(bool do_silent)
+int bootm_process_cmdline_env(int flags)
 {
 	const int maxlen = MAX_CMDLINE_SIZE;
+	bool do_silent;
 	const char *env;
 	char *buf;
 	int ret;
 
 	/* First check if any action is needed */
 	do_silent = IS_ENABLED(CONFIG_SILENT_CONSOLE) &&
-	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY) && do_silent;
+	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY) && (flags & BOOTM_CL_SILENT);
 	if (!do_silent)
 		return 0;
 
@@ -683,7 +684,8 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (!ret && (states & BOOTM_STATE_OS_BD_T))
 		ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images);
 	if (!ret && (states & BOOTM_STATE_OS_PREP)) {
-		ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX);
+		ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ?
+						BOOTM_CL_SILENT : 0);
 		if (ret) {
 			printf("Cmdline setup failed (err=%d)\n", ret);
 			ret = CMD_RET_FAILURE;
diff --git a/include/bootm.h b/include/bootm.h
index f12ee2b3cb3..4876d7b2882 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -75,6 +75,13 @@ void board_quiesce_devices(void);
  */
 void switch_to_non_secure_mode(void);
 
+/* Flags to control bootm_process_cmdline() */
+enum bootm_cmdline_t {
+	BOOTM_CL_SILENT	= 1 << 0,	/* Do silent console processing */
+
+	BOOTM_CL_ALL	= 1,		/* All substitutions */
+};
+
 /**
  * arch_preboot_os() - arch specific configuration before booting
  */
@@ -91,9 +98,9 @@ void board_preboot_os(void);
  * Updates the 'bootargs' envvar as required. This handles making Linux boot
  * silently if requested ('silent_linux' envvar)
  *
- * @do_silent: Process bootargs for silent console
+ * @flags: Flags to control what happens (see bootm_cmdline_t)
  * @return 0 if OK, -ENOMEM if out of memory
  */
-int bootm_process_cmdline_env(bool do_silent);
+int bootm_process_cmdline_env(int flags);
 
 #endif
diff --git a/test/bootm.c b/test/bootm.c
index c203f0acd60..ba08920bb17 100644
--- a/test/bootm.c
+++ b/test/bootm.c
@@ -23,26 +23,26 @@ static int bootm_test_silent_var(struct unit_test_state *uts)
 	/* 'silent_linux' not set should do nothing */
 	env_set("silent_linux", NULL);
 	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(bootm_process_cmdline_env(true));
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
 	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
 
 	env_set("bootargs", NULL);
-	ut_assertok(bootm_process_cmdline_env(true));
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
 	ut_assertnull(env_get("bootargs"));
 
 	ut_assertok(env_set("silent_linux", "no"));
 	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(bootm_process_cmdline_env(true));
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
 	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
 
 	ut_assertok(env_set("silent_linux", "yes"));
 	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(bootm_process_cmdline_env(true));
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
 	ut_asserteq_str("console=", env_get("bootargs"));
 
 	/* Empty buffer should still add the string */
 	env_set("bootargs", NULL);
-	ut_assertok(bootm_process_cmdline_env(true));
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
 	ut_asserteq_str("console=", env_get("bootargs"));
 
 	return 0;
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 09/11] bootm: Allow updating the bootargs in a buffer

[Simon Glass]

At present we only support updating the 'bootargs' environment
variable. Add another function to update a buffer instead. This will
allow zimage to use this feature.

Also add a lot more tests to cover various cases.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/bootm.c  |  18 +++++++-
 include/bootm.h |  16 +++++++
 test/bootm.c    | 114 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 132 insertions(+), 16 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index 5a5b79c5cd0..5484c2be900 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -540,6 +540,22 @@ static int fixup_silent_linux(char *buf, int maxlen)
 	return 0;
 }
 
+int bootm_process_cmdline(char *buf, int maxlen, int flags)
+{
+	int ret;
+
+	/* Check config first to enable compiler to eliminate code */
+	if (IS_ENABLED(CONFIG_SILENT_CONSOLE) &&
+	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY) &&
+	    (flags & BOOTM_CL_SILENT)) {
+		ret = fixup_silent_linux(buf, maxlen);
+		if (ret)
+			return log_msg_ret("silent", ret);
+	}
+
+	return 0;
+}
+
 int bootm_process_cmdline_env(int flags)
 {
 	const int maxlen = MAX_CMDLINE_SIZE;
@@ -564,7 +580,7 @@ int bootm_process_cmdline_env(int flags)
 		strcpy(buf, env);
 	else
 		*buf = '\0';
-	ret = fixup_silent_linux(buf, maxlen);
+	ret = bootm_process_cmdline(buf, maxlen, flags);
 	if (!ret) {
 		ret = env_set("bootargs", buf);
 
diff --git a/include/bootm.h b/include/bootm.h
index 4876d7b2882..8d95fb2a90a 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -93,6 +93,22 @@ void arch_preboot_os(void);
 void board_preboot_os(void);
 
 /*
+ * bootm_process_cmdline() - Process fix-ups for the command line
+ *
+ * This handles: making Linux boot silently if requested ('silent_linux' envvar)
+ *
+ * @maxlen must provide enough space for the string being processed plus the
+ * resulting string
+ *
+ * @buf: buffer holding commandline string to adjust
+ * @maxlen: Maximum length of buffer at @buf (including \0)
+ * @flags: Flags to control what happens (see bootm_cmdline_t)
+ * @return 0 if OK, -ENOMEM if out of memory, -ENOSPC if the commandline is too
+ *	long
+ */
+int bootm_process_cmdline(char *buf, int maxlen, int flags);
+
+/**
  * bootm_process_cmdline_env() - Process fix-ups for the command line
  *
  * Updates the 'bootargs' envvar as required. This handles making Linux boot
diff --git a/test/bootm.c b/test/bootm.c
index ba08920bb17..d0b29441d65 100644
--- a/test/bootm.c
+++ b/test/bootm.c
@@ -15,33 +15,117 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #define BOOTM_TEST(_name, _flags)	UNIT_TEST(_name, _flags, bootm_test)
 
+enum {
+	BUF_SIZE	= 1024,
+};
+
 #define CONSOLE_STR	"console=/dev/ttyS0"
 
-/* Test silent processing in the bootargs variable */
-static int bootm_test_silent_var(struct unit_test_state *uts)
+/* Test cmdline processing where nothing happens */
+static int bootm_test_nop(struct unit_test_state *uts)
+{
+	char buf[BUF_SIZE];
+
+	*buf = '\0';
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, true));
+	ut_asserteq_str("", buf);
+
+	strcpy(buf, "test");
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, true));
+	ut_asserteq_str("test", buf);
+
+	return 0;
+}
+BOOTM_TEST(bootm_test_nop, 0);
+
+/* Test cmdline processing when out of space */
+static int bootm_test_nospace(struct unit_test_state *uts)
+{
+	char buf[BUF_SIZE];
+
+	/* Zero buffer size */
+	*buf = '\0';
+	ut_asserteq(-ENOSPC, bootm_process_cmdline(buf, 0, true));
+
+	/* Buffer string not terminated */
+	memset(buf, 'a', BUF_SIZE);
+	ut_asserteq(-ENOSPC, bootm_process_cmdline(buf, BUF_SIZE, true));
+
+	/* Not enough space to copy string */
+	memset(buf, '\0', BUF_SIZE);
+	memset(buf, 'a', BUF_SIZE / 2);
+	ut_asserteq(-ENOSPC, bootm_process_cmdline(buf, BUF_SIZE, true));
+
+	/* Just enough space */
+	memset(buf, '\0', BUF_SIZE);
+	memset(buf, 'a', BUF_SIZE / 2 - 1);
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, true));
+
+	return 0;
+}
+BOOTM_TEST(bootm_test_nospace, 0);
+
+/* Test silent processing */
+static int bootm_test_silent(struct unit_test_state *uts)
 {
+	char buf[BUF_SIZE];
+
 	/* 'silent_linux' not set should do nothing */
 	env_set("silent_linux", NULL);
-	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
-	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
-
-	env_set("bootargs", NULL);
-	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
-	ut_assertnull(env_get("bootargs"));
+	strcpy(buf, CONSOLE_STR);
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SILENT));
+	ut_asserteq_str(CONSOLE_STR, buf);
 
 	ut_assertok(env_set("silent_linux", "no"));
-	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
-	ut_asserteq_str(CONSOLE_STR, env_get("bootargs"));
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SILENT));
+	ut_asserteq_str(CONSOLE_STR, buf);
 
 	ut_assertok(env_set("silent_linux", "yes"));
-	env_set("bootargs", CONSOLE_STR);
-	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
-	ut_asserteq_str("console=", env_get("bootargs"));
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SILENT));
+	ut_asserteq_str("console=", buf);
 
 	/* Empty buffer should still add the string */
+	*buf = '\0';
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SILENT));
+	ut_asserteq_str("console=", buf);
+
+	/* Check nothing happens when do_silent is false */
+	*buf = '\0';
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, 0));
+	ut_asserteq_str("", buf);
+
+	/* Not enough space */
+	*buf = '\0';
+	ut_asserteq(-ENOSPC, bootm_process_cmdline(buf, 8, BOOTM_CL_SILENT));
+
+	/* Just enough space */
+	*buf = '\0';
+	ut_assertok(bootm_process_cmdline(buf, 9, BOOTM_CL_SILENT));
+
+	/* add at end */
+	strcpy(buf, "something");
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SILENT));
+	ut_asserteq_str("something console=", buf);
+
+	/* change at start */
+	strcpy(buf, CONSOLE_STR " something");
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SILENT));
+	ut_asserteq_str("console= something", buf);
+
+	return 0;
+}
+BOOTM_TEST(bootm_test_silent, 0);
+
+/* Test silent processing in the bootargs variable */
+static int bootm_test_silent_var(struct unit_test_state *uts)
+{
 	env_set("bootargs", NULL);
+	env_set("silent_linux", NULL);
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
+	ut_assertnull(env_get("bootargs"));
+
+	env_set("bootargs", CONSOLE_STR);
+	env_set("silent_linux", "yes");
 	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
 	ut_asserteq_str("console=", env_get("bootargs"));
 
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 10/11] x86: zimage: Add silent-console processing

[Simon Glass]

At present zimage does its own command-line processing and does not
support the 'silent console' feature. There doesn't seem to be any good
reason for this.

Add support for silent console to zimage.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/lib/zimage.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c
index a00964cc8d9..8e1e8017bd8 100644
--- a/arch/x86/lib/zimage.c
+++ b/arch/x86/lib/zimage.c
@@ -13,6 +13,7 @@
  */
 
 #include <common.h>
+#include <bootm.h>
 #include <command.h>
 #include <env.h>
 #include <irq_func.h>
@@ -317,6 +318,11 @@ int setup_zimage(struct boot_params *setup_base, char *cmd_line, int auto_boot,
 	}
 
 	if (cmd_line) {
+		int max_size = 0xff;
+		int ret;
+
+		if (bootproto >= 0x0206)
+			max_size = hdr->cmdline_size;
 		if (bootproto >= 0x0202) {
 			hdr->cmd_line_ptr = (uintptr_t)cmd_line;
 		} else if (bootproto >= 0x0200) {
@@ -332,6 +338,14 @@ int setup_zimage(struct boot_params *setup_base, char *cmd_line, int auto_boot,
 			strcpy(cmd_line, (char *)cmdline_force);
 		else
 			build_command_line(cmd_line, auto_boot);
+		ret = bootm_process_cmdline(cmd_line, max_size, BOOTM_CL_ALL);
+		if (ret) {
+			printf("Cmdline setup failed (err=%d)\n", ret);
+			return ret;
+		}
+		printf("Kernel command line: \"");
+		puts(cmd_line);
+		printf("\"\n");
 	}
 
 	if (IS_ENABLED(CONFIG_INTEL_MID) && bootproto >= 0x0207)
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 11/11] bootm: Support string substitution in bootargs

[Simon Glass]

In some cases it is necessary to pass parameters to Linux so that it will
boot correctly. For example, the rootdev parameter is often used to
specify the root device. However the root device may change depending on
whence U-Boot loads the kernel. At present it is necessary to build up
the command line by adding device strings to it one by one.

It is often more convenient to provide a template for bootargs, with
U-Boot doing the substitution from other environment variables.

Add a way to substitute strings in the bootargs variable. This allows
things like "rootdev=%U" to be used in bootargs, with the %U substitution
providing the UUID of the root device.

For example, to substitute the GUID of the kernel partition:

  setenv bootargs "console=/dev/ttyS0 rootdev=%U/PARTNROFF=1 kern_guid=%U"
  part uuid mmc 2:2 uuid
  setenv bootargs_U $uuid
  bootm

This is particularly useful when the command line from another place. For
example, Chrome OS stores the command line next to the kernel itself. It
depends on the kernel version being used as well as the hardware features,
so it is extremely difficult to devise a U-Boot script that works on all
boards and kernel versions. With this feature, the command line can be
read from disk and used directly, with a few substitutions set up.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 README              |  16 +++++++
 arch/Kconfig        |   1 +
 common/Kconfig.boot |  20 ++++++++
 common/bootm.c      |  72 +++++++++++++++++++++++++++--
 include/bootm.h     |  14 ++++--
 test/bootm.c        | 110 ++++++++++++++++++++++++++++++++++++++++++--
 6 files changed, 221 insertions(+), 12 deletions(-)

diff --git a/README b/README
index 91c5a1a8fa3..263e31ab7f6 100644
--- a/README
+++ b/README
@@ -3229,6 +3229,22 @@ List of environment variables (most likely not complete):
 
   bootargs	- Boot arguments when booting an RTOS image
 
+  bootargs_subst - Substitution parameters for bootargs. These are applied after
+		  the commandline has been built. The format is:
+
+		    <key>=<value>[!<key>=<value>...]
+
+		  where
+		    <key> is a string to replace
+		    <value> is the value to replace it with (either a simple
+		       string or an environment variable starting with $
+
+		  One use for this is to insert the root-disk UUID into the
+		  command line where bootargs contains "root=%U"
+
+		    part uuid mmc 2:2 uuid
+		    setenv cmdline_repl %U=$uuid
+
   bootfile	- Name of the image to load with TFTP
 
   bootm_low	- Memory range available for image processing in the bootm
diff --git a/arch/Kconfig b/arch/Kconfig
index 6caf2338bcf..421ea9a9b51 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -141,6 +141,7 @@ config SANDBOX
 	imply CMD_PMC
 	imply CMD_CLONE
 	imply SILENT_CONSOLE
+	imply BOOTARGS_SUBST
 
 config SH
 	bool "SuperH architecture"
diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 4191bfb34df..c5668cf2931 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -848,6 +848,26 @@ config BOOTARGS
 	  CONFIG_BOOTARGS goes into the environment value "bootargs". Note that
 	  this value will also override the "chosen" node in FDT blob.
 
+config BOOTARGS_SUBST
+	bool "Support substituting strings in boot arguments"
+	help
+	  This allows substituting string values in the boot arguments. These
+	  are applied after the commandline has been built. Set the
+	  'bootargs_subst' envvar to control this. The format is:
+
+		<key>=<value>[!<key>=<value>...]
+
+		  where
+		    <key> is a string to replace
+		    <value> is the value to replace it with (either a simple
+		       string or an environment variable starting with $
+
+		  One use for this is to insert the root-disk UUID into the
+		  command line where bootargs contains "root=%U"
+
+		    part uuid mmc 2:2 uuid
+		    setenv cmdline_repl %U=$uuid
+
 config USE_BOOTCOMMAND
 	bool "Enable a default value for bootcmd"
 	help
diff --git a/common/bootm.c b/common/bootm.c
index 5484c2be900..0a89a72a660 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -540,6 +540,68 @@ static int fixup_silent_linux(char *buf, int maxlen)
 	return 0;
 }
 
+/**
+ * process_subst() - Handle substitution of %x fields in the environment
+ *
+ * For every "%x" found, it is replaced with the value of envvar "bootargs_x"
+ * where x is a case-sensitive alphanumeric character. If that environment
+ * variable does not exist, no substitution is made and the %x remains, to
+ * allow for plain % characters in the string.
+ *
+ * @buf: Buffer containing the string to process
+ * @maxlen: Maximum length of buffer
+ * @return 0 if OK, -ENOSPC if @maxlen is too small
+ */
+static int process_subst(char *buf, int maxlen)
+{
+	const char *in;
+	char *cmdline, *out;
+	bool percent;
+	int size;
+
+	/* Move to end of buffer */
+	size = strlen(buf) + 1;
+	cmdline = buf + maxlen - size;
+	if (buf + size > cmdline)
+		return -ENOSPC;
+	memmove(cmdline, buf, size);
+
+	/* Replace %c with value of envvar 'bootargs_%c' */
+	percent = false;
+	for (in = cmdline, out = buf; *in; in++) {
+		/* If we catch up with the input string, we're out of space */
+		if (out >= in)
+			return -ENOSPC;
+		if (percent) {
+			char var[12];
+			const char *val;
+
+			snprintf(var, sizeof(var), "bootargs_%c", *in);
+			val = env_get(var);
+			if (val) {
+				for (; *val; val++) {
+					if (out >= in)
+						return -ENOSPC;
+					*out++ = *val;
+				}
+				percent = false;
+				continue;
+			} else {
+				/* No value, put the % back */
+				*out++ = '%';
+			}
+		} else if (*in == '%') {
+			percent = true;
+			continue;
+		}
+		*out++ = *in;
+		percent = false;
+	}
+	*out++ = '\0';
+
+	return 0;
+}
+
 int bootm_process_cmdline(char *buf, int maxlen, int flags)
 {
 	int ret;
@@ -552,6 +614,11 @@ int bootm_process_cmdline(char *buf, int maxlen, int flags)
 		if (ret)
 			return log_msg_ret("silent", ret);
 	}
+	if (IS_ENABLED(CONFIG_BOOTARGS_SUBST) && (flags & BOOTM_CL_SUBST)) {
+		ret = process_subst(buf, maxlen);
+		if (ret)
+			return log_msg_ret("silent", ret);
+	}
 
 	return 0;
 }
@@ -567,7 +634,7 @@ int bootm_process_cmdline_env(int flags)
 	/* First check if any action is needed */
 	do_silent = IS_ENABLED(CONFIG_SILENT_CONSOLE) &&
 	    !IS_ENABLED(CONFIG_SILENT_U_BOOT_ONLY) && (flags & BOOTM_CL_SILENT);
-	if (!do_silent)
+	if (!do_silent && !IS_ENABLED(CONFIG_BOOTARGS_SUBST))
 		return 0;
 
 	env = env_get("bootargs");
@@ -700,8 +767,7 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (!ret && (states & BOOTM_STATE_OS_BD_T))
 		ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, images);
 	if (!ret && (states & BOOTM_STATE_OS_PREP)) {
-		ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX ?
-						BOOTM_CL_SILENT : 0);
+		ret = bootm_process_cmdline_env(images->os.os == IH_OS_LINUX);
 		if (ret) {
 			printf("Cmdline setup failed (err=%d)\n", ret);
 			ret = CMD_RET_FAILURE;
diff --git a/include/bootm.h b/include/bootm.h
index 8d95fb2a90a..7f88ec718b8 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -78,8 +78,9 @@ void switch_to_non_secure_mode(void);
 /* Flags to control bootm_process_cmdline() */
 enum bootm_cmdline_t {
 	BOOTM_CL_SILENT	= 1 << 0,	/* Do silent console processing */
+	BOOTM_CL_SUBST	= 1 << 1,	/* Do substitution */
 
-	BOOTM_CL_ALL	= 1,		/* All substitutions */
+	BOOTM_CL_ALL	= 3,		/* All substitutions */
 };
 
 /**
@@ -95,7 +96,10 @@ void board_preboot_os(void);
 /*
  * bootm_process_cmdline() - Process fix-ups for the command line
  *
- * This handles: making Linux boot silently if requested ('silent_linux' envvar)
+ * This handles:
+ *
+ *  - making Linux boot silently if requested ('silent_linux' envvar)
+ *  - performing substitutions in the command line ('bootargs_subst' envvar)
  *
  * @maxlen must provide enough space for the string being processed plus the
  * resulting string
@@ -111,8 +115,10 @@ int bootm_process_cmdline(char *buf, int maxlen, int flags);
 /**
  * bootm_process_cmdline_env() - Process fix-ups for the command line
  *
- * Updates the 'bootargs' envvar as required. This handles making Linux boot
- * silently if requested ('silent_linux' envvar)
+ * Updates the 'bootargs' envvar as required. This handles:
+ *
+ *  - making Linux boot silently if requested ('silent_linux' envvar)
+ *  - performing substitutions in the command line ('bootargs_subst' envvar)
  *
  * @flags: Flags to control what happens (see bootm_cmdline_t)
  * @return 0 if OK, -ENOMEM if out of memory
diff --git a/test/bootm.c b/test/bootm.c
index d0b29441d65..8a83be9719f 100644
--- a/test/bootm.c
+++ b/test/bootm.c
@@ -116,22 +116,122 @@ static int bootm_test_silent(struct unit_test_state *uts)
 }
 BOOTM_TEST(bootm_test_silent, 0);
 
+/* Test substitution processing */
+static int bootm_test_subst(struct unit_test_state *uts)
+{
+	char buf[BUF_SIZE];
+
+	strcpy(buf, "some%Athing");
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST));
+	ut_asserteq_str("some%Athing", buf);
+
+	/* Replace with shorter string */
+	ut_assertok(env_set("bootargs_A", "a"));
+	strcpy(buf, "some%Athing");
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST));
+	ut_asserteq_str("someathing", buf);
+
+	/* Replace with same-length string */
+	ut_assertok(env_set("bootargs_A", "ab"));
+	strcpy(buf, "some%Athing");
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST));
+	ut_asserteq_str("someabthing", buf);
+
+	/* Replace with longer string */
+	ut_assertok(env_set("bootargs_A", "abc"));
+	strcpy(buf, "some%Athing");
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST));
+	ut_asserteq_str("someabcthing", buf);
+
+	/* Check it is case sensitive */
+	strcpy(buf, "some%athing");
+	ut_assertok(bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST));
+	ut_asserteq_str("some%athing", buf);
+
+	/* Check too long - need 12 bytes for each string */
+	strcpy(buf, "some%Athing");
+	ut_asserteq(-ENOSPC,
+		    bootm_process_cmdline(buf, 12 * 2 - 1, BOOTM_CL_SUBST));
+
+	/* Check just enough space */
+	strcpy(buf, "some%Athing");
+	ut_assertok(bootm_process_cmdline(buf, 12 * 2, BOOTM_CL_SUBST));
+	ut_asserteq_str("someabcthing", buf);
+
+	/*
+	 * Check the substition string being too long. This results in a string
+	 * of 12 (13 bytes). Allow one more byte margin.
+	 */
+	ut_assertok(env_set("bootargs_A", "1234567890"));
+	strcpy(buf, "a%Ac");
+	ut_asserteq(-ENOSPC,
+		    bootm_process_cmdline(buf, 13, BOOTM_CL_SUBST));
+
+	strcpy(buf, "a%Ac");
+	ut_asserteq(0, bootm_process_cmdline(buf, 14, BOOTM_CL_SUBST));
+
+	/* Check multiple substitutions */
+	ut_assertok(env_set("bootargs_A", "abc"));
+	strcpy(buf, "some%Athing%Belse");
+	ut_asserteq(0, bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST));
+	ut_asserteq_str("someabcthing%Belse", buf);
+
+	/* Check multiple substitutions */
+	ut_assertok(env_set("bootargs_B", "123"));
+	strcpy(buf, "some%Athing%Belse");
+	ut_asserteq(0, bootm_process_cmdline(buf, BUF_SIZE, BOOTM_CL_SUBST));
+	ut_asserteq_str("someabcthing123else", buf);
+
+	return 0;
+}
+BOOTM_TEST(bootm_test_subst, 0);
+
 /* Test silent processing in the bootargs variable */
 static int bootm_test_silent_var(struct unit_test_state *uts)
 {
 	env_set("bootargs", NULL);
-	env_set("silent_linux", NULL);
-	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SUBST));
 	ut_assertnull(env_get("bootargs"));
 
-	env_set("bootargs", CONSOLE_STR);
-	env_set("silent_linux", "yes");
+	ut_assertok(env_set("bootargs", "some%Athing"));
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SUBST));
+	ut_asserteq_str("some%Athing", env_get("bootargs"));
+
+	return 0;
+}
+BOOTM_TEST(bootm_test_silent_var, 0);
+
+/* Test substitution processing in the bootargs variable */
+static int bootm_test_subst_var(struct unit_test_state *uts)
+{
+	env_set("bootargs", NULL);
 	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
 	ut_asserteq_str("console=", env_get("bootargs"));
 
+	ut_assertok(env_set("bootargs", "some%Athing"));
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_SILENT));
+	ut_asserteq_str("some%Athing console=", env_get("bootargs"));
+
 	return 0;
 }
-BOOTM_TEST(bootm_test_silent_var, 0);
+BOOTM_TEST(bootm_test_subst_var, 0);
+
+/* Test substitution and silent console processing in the bootargs variable */
+static int bootm_test_subst_both(struct unit_test_state *uts)
+{
+	ut_assertok(env_set("silent_linux", "yes"));
+	env_set("bootargs", NULL);
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_ALL));
+	ut_asserteq_str("console=", env_get("bootargs"));
+
+	ut_assertok(env_set("bootargs", "some%Athing " CONSOLE_STR));
+	ut_assertok(env_set("bootargs_A", "1234567890"));
+	ut_assertok(bootm_process_cmdline_env(BOOTM_CL_ALL));
+	ut_asserteq_str("some1234567890thing console=", env_get("bootargs"));
+
+	return 0;
+}
+BOOTM_TEST(bootm_test_subst_both, 0);
 
 int do_ut_bootm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-- 
2.29.0.rc1.297.gfa9743e501-goog

[PATCH 00/11] bootm: Support substitions in bootargs and add tests

[Heinrich Schuchardt]

On 19.10.20 15:55, Simon Glass wrote:
> This series adds tests to the fixup_silent-linux() function and extends
> the 'zimage' command to use it.
>
> It also adds a new string-substition feature to allow bootargs to be a
> template, rather than having to build it up piece by piece with
> information obtained in a build script.
>
> It also updates zimage to use the same command-line processing.
>
> With these additions it is possible to boot Chrome OS from a U-Boot script
> on most Chromebooks.

Hello Simon,

could you, please, add a patch documenting the usage of the new
string-substitution feature in the HTML documentation.

Best regards

Heinrich

>
>
> Simon Glass (11):
>   env: Allow returning errors from hdelete_r()
>   bootm: Add tests for fixup_silent_linux()
>   bootm: Update fixup_silent_linux() to return an error
>   bootm: Rename fixup_silent_linux()
>   bootm: Add a bool parameter to bootm_process_cmdline_env()
>   bootm: Use size rather than length for CONSOLE_ARG
>   bootm: Split out bootargs environment reading / writing
>   bootm: Update bootm_process_cmdline_env() to use flags
>   bootm: Allow updating the bootargs in a buffer
>   x86: zimage: Add silent-console processing
>   bootm: Support string substitution in bootargs
>
>  README                |  16 +++
>  arch/Kconfig          |   2 +
>  arch/x86/lib/zimage.c |  14 +++
>  cmd/nvedit.c          |   6 +-
>  common/Kconfig.boot   |  20 ++++
>  common/bootm.c        | 206 +++++++++++++++++++++++++++++------
>  include/bootm.h       |  40 +++++++
>  include/search.h      |  11 +-
>  include/test/suites.h |   1 +
>  lib/hashtable.c       |  12 +--
>  test/Makefile         |   1 +
>  test/bootm.c          | 243 ++++++++++++++++++++++++++++++++++++++++++
>  test/cmd_ut.c         |   1 +
>  test/env/hashtable.c  |   2 +-
>  14 files changed, 534 insertions(+), 41 deletions(-)
>  create mode 100644 test/bootm.c
>

[PATCH 11/11] bootm: Support string substitution in bootargs

[Rasmus Villemoes]

On 19/10/2020 15.56, Simon Glass wrote:
> In some cases it is necessary to pass parameters to Linux so that it will
> boot correctly. For example, the rootdev parameter is often used to
> specify the root device. However the root device may change depending on
> whence U-Boot loads the kernel. At present it is necessary to build up
> the command line by adding device strings to it one by one.
> 
> It is often more convenient to provide a template for bootargs, with
> U-Boot doing the substitution from other environment variables.
> 
> Add a way to substitute strings in the bootargs variable. This allows
> things like "rootdev=%U" to be used in bootargs, with the %U substitution
> providing the UUID of the root device.
> 
> For example, to substitute the GUID of the kernel partition:
> 
>   setenv bootargs "console=/dev/ttyS0 rootdev=%U/PARTNROFF=1 kern_guid=%U"
>   part uuid mmc 2:2 uuid
>   setenv bootargs_U $uuid
>   bootm
> 
> This is particularly useful when the command line from another place. For
> example, Chrome OS stores the command line next to the kernel itself. It
> depends on the kernel version being used as well as the hardware features,
> so it is extremely difficult to devise a U-Boot script that works on all
> boards and kernel versions. With this feature, the command line can be
> read from disk and used directly, with a few substitutions set up.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  README              |  16 +++++++
>  arch/Kconfig        |   1 +
>  common/Kconfig.boot |  20 ++++++++
>  common/bootm.c      |  72 +++++++++++++++++++++++++++--
>  include/bootm.h     |  14 ++++--
>  test/bootm.c        | 110 ++++++++++++++++++++++++++++++++++++++++++--
>  6 files changed, 221 insertions(+), 12 deletions(-)
> 
> diff --git a/README b/README
> index 91c5a1a8fa3..263e31ab7f6 100644
> --- a/README
> +++ b/README
> @@ -3229,6 +3229,22 @@ List of environment variables (most likely not complete):
>  
>    bootargs	- Boot arguments when booting an RTOS image
>  
> +  bootargs_subst - Substitution parameters for bootargs. These are applied after
> +		  the commandline has been built. The format is:
> +
> +		    <key>=<value>[!<key>=<value>...]
> +
> +		  where
> +		    <key> is a string to replace
> +		    <value> is the value to replace it with (either a simple
> +		       string or an environment variable starting with $
> +
> +		  One use for this is to insert the root-disk UUID into the
> +		  command line where bootargs contains "root=%U"
> +
> +		    part uuid mmc 2:2 uuid
> +		    setenv cmdline_repl %U=$uuid

cmdline_repl seems to be stale, it should be bootargs_subst. But the
whole paragraph seems stale, as you actually implement and test the
bootargs_X approach.

Anyway, this does seem useful, but I really question the choice of
leaving %A in there if bootargs_A does not exist. It's hard if not
impossible to create an environment variable whose value is empty, and I
can easily imagine it would be useful to allow a %A to expand to
nothing. So why not just use the usual semantics of requiring a double
%% to put a single % in the output? It's probably quite rare that one
would need that anyway.

Rasmus

[PATCH 07/11] bootm: Split out bootargs environment reading / writing

[Wolfgang Denk]

Dear Simon,

In message <20201019135602.3943835-8-sjg@chromium.org> you wrote:
...
>
> It is also useful for zimage to use a buffer, since it does not actually
> put the Linux command line in the bootargs variable.

...which I consider a bug that should be fixed.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A good marriage would be between a blind wife and deaf husband.
                                               -- Michel de Montaigne

[PATCH 09/11] bootm: Allow updating the bootargs in a buffer

[Wolfgang Denk]

Dear Simon Glass,

In message <20201019135602.3943835-10-sjg@chromium.org> you wrote:
> At present we only support updating the 'bootargs' environment
> variable. Add another function to update a buffer instead. This will
> allow zimage to use this feature.

I think this is the wrong way to address a problem.

Instead, zimage should use bootargs as well.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The faster I go, the behinder I get.                 -- Lewis Carroll

[PATCH 11/11] bootm: Support string substitution in bootargs

[Wolfgang Denk]

Dear Simon,

In message <20201019135602.3943835-12-sjg@chromium.org> you wrote:
> In some cases it is necessary to pass parameters to Linux so that it will
> boot correctly. For example, the rootdev parameter is often used to
> specify the root device. However the root device may change depending on
> whence U-Boot loads the kernel. At present it is necessary to build up
> the command line by adding device strings to it one by one.
>
> It is often more convenient to provide a template for bootargs, with
> U-Boot doing the substitution from other environment variables.
>
> Add a way to substitute strings in the bootargs variable. This allows
> things like "rootdev=%U" to be used in bootargs, with the %U substitution
> providing the UUID of the root device.

Argh, no, please don't.

You add something unconditionally to common code which very few
people need.  U-Boot size is growing all the time because of such
... features.  This may be acceptable on the systems you have in
mind, but I consider this selfish.

Why do we have to add yet another non-standard way of substituting
variables in a string?  Can we not use alreay existing methonds
instead?

Why do you have to use "%U" in your template instead of for example
"${uuid}" ?



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If you use modules, you pay the price. Sane embedded solutions
running in "tight" environments don't use modules :-)
    -- Benjamin Herrenschmidt in <1258234866.2140.451.camel@pasglop>

[PATCH 11/11] bootm: Support string substitution in bootargs

[Michael Walle]

Am 2020-10-19 16:54, schrieb Wolfgang Denk:
> Dear Simon,
> 
> In message <20201019135602.3943835-12-sjg@chromium.org> you wrote:
>> In some cases it is necessary to pass parameters to Linux so that it 
>> will
>> boot correctly. For example, the rootdev parameter is often used to
>> specify the root device. However the root device may change depending 
>> on
>> whence U-Boot loads the kernel. At present it is necessary to build up
>> the command line by adding device strings to it one by one.
>> 
>> It is often more convenient to provide a template for bootargs, with
>> U-Boot doing the substitution from other environment variables.
>> 
>> Add a way to substitute strings in the bootargs variable. This allows
>> things like "rootdev=%U" to be used in bootargs, with the %U 
>> substitution
>> providing the UUID of the root device.
> 
> Argh, no, please don't.
> 
> You add something unconditionally to common code which very few
> people need.  U-Boot size is growing all the time because of such
> ... features.  This may be acceptable on the systems you have in
> mind, but I consider this selfish.
> 
> Why do we have to add yet another non-standard way of substituting
> variables in a string?  Can we not use alreay existing methonds
> instead?
> 
> Why do you have to use "%U" in your template instead of for example
> "${uuid}" ?

I'd second that. Having variables evaluated inside the bootargs would
be very valuable IMHO.

At the moment we have some cumbersome constructs like
   set_bootargs="setenv bootargs bla ${var}"

-michael

[PATCH 11/11] bootm: Support string substitution in bootargs

[Simon Glass]

Hi Wolfgang,

On Mon, 19 Oct 2020 at 08:55, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <20201019135602.3943835-12-sjg@chromium.org> you wrote:
> > In some cases it is necessary to pass parameters to Linux so that it will
> > boot correctly. For example, the rootdev parameter is often used to
> > specify the root device. However the root device may change depending on
> > whence U-Boot loads the kernel. At present it is necessary to build up
> > the command line by adding device strings to it one by one.
> >
> > It is often more convenient to provide a template for bootargs, with
> > U-Boot doing the substitution from other environment variables.
> >
> > Add a way to substitute strings in the bootargs variable. This allows
> > things like "rootdev=%U" to be used in bootargs, with the %U substitution
> > providing the UUID of the root device.
>
> Argh, no, please don't.
>
> You add something unconditionally to common code which very few
> people need.  U-Boot size is growing all the time because of such
> ... features.  This may be acceptable on the systems you have in
> mind, but I consider this selfish.

Did you see the Kconfig option?

>
> Why do we have to add yet another non-standard way of substituting
> variables in a string?  Can we not use alreay existing methonds
> instead?

What sort of methods?

>
> Why do you have to use "%U" in your template instead of for example
> "${uuid}" ?

This is what Chrome OS uses, so it is easier this way, Otherwise I
have to replace %U with ${uuid} first.

Which code can I use to parse the ${var} thing?

Regards,
Simon

[PATCH 11/11] bootm: Support string substitution in bootargs

[Simon Glass]

Hi Rasmus,

On Mon, 19 Oct 2020 at 08:43, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 19/10/2020 15.56, Simon Glass wrote:
> > In some cases it is necessary to pass parameters to Linux so that it will
> > boot correctly. For example, the rootdev parameter is often used to
> > specify the root device. However the root device may change depending on
> > whence U-Boot loads the kernel. At present it is necessary to build up
> > the command line by adding device strings to it one by one.
> >
> > It is often more convenient to provide a template for bootargs, with
> > U-Boot doing the substitution from other environment variables.
> >
> > Add a way to substitute strings in the bootargs variable. This allows
> > things like "rootdev=%U" to be used in bootargs, with the %U substitution
> > providing the UUID of the root device.
> >
> > For example, to substitute the GUID of the kernel partition:
> >
> >   setenv bootargs "console=/dev/ttyS0 rootdev=%U/PARTNROFF=1 kern_guid=%U"
> >   part uuid mmc 2:2 uuid
> >   setenv bootargs_U $uuid
> >   bootm
> >
> > This is particularly useful when the command line from another place. For
> > example, Chrome OS stores the command line next to the kernel itself. It
> > depends on the kernel version being used as well as the hardware features,
> > so it is extremely difficult to devise a U-Boot script that works on all
> > boards and kernel versions. With this feature, the command line can be
> > read from disk and used directly, with a few substitutions set up.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  README              |  16 +++++++
> >  arch/Kconfig        |   1 +
> >  common/Kconfig.boot |  20 ++++++++
> >  common/bootm.c      |  72 +++++++++++++++++++++++++++--
> >  include/bootm.h     |  14 ++++--
> >  test/bootm.c        | 110 ++++++++++++++++++++++++++++++++++++++++++--
> >  6 files changed, 221 insertions(+), 12 deletions(-)
> >
> > diff --git a/README b/README
> > index 91c5a1a8fa3..263e31ab7f6 100644
> > --- a/README
> > +++ b/README
> > @@ -3229,6 +3229,22 @@ List of environment variables (most likely not complete):
> >
> >    bootargs   - Boot arguments when booting an RTOS image
> >
> > +  bootargs_subst - Substitution parameters for bootargs. These are applied after
> > +               the commandline has been built. The format is:
> > +
> > +                 <key>=<value>[!<key>=<value>...]
> > +
> > +               where
> > +                 <key> is a string to replace
> > +                 <value> is the value to replace it with (either a simple
> > +                    string or an environment variable starting with $
> > +
> > +               One use for this is to insert the root-disk UUID into the
> > +               command line where bootargs contains "root=%U"
> > +
> > +                 part uuid mmc 2:2 uuid
> > +                 setenv cmdline_repl %U=$uuid
>
> cmdline_repl seems to be stale, it should be bootargs_subst. But the
> whole paragraph seems stale, as you actually implement and test the
> bootargs_X approach.

Yes, thanks, will fix.

>
> Anyway, this does seem useful, but I really question the choice of
> leaving %A in there if bootargs_A does not exist. It's hard if not
> impossible to create an environment variable whose value is empty, and I
> can easily imagine it would be useful to allow a %A to expand to
> nothing. So why not just use the usual semantics of requiring a double
> %% to put a single % in the output? It's probably quite rare that one
> would need that anyway.

I did wonder about the empty env var thing. IMO it would be nice to
support empty variables, so we can distinguish between an empty one
and a missing one.

My concern with removing the var is if people have % in the string.
This way I don't have to worry about quoting, etc. See Wolgang's email
in this thread too.

Regards,
Simon

[PATCH 11/11] bootm: Support string substitution in bootargs

[Simon Glass]

Hi Michael,

On Mon, 19 Oct 2020 at 09:47, Michael Walle <michael@walle.cc> wrote:
>
> Am 2020-10-19 16:54, schrieb Wolfgang Denk:
> > Dear Simon,
> >
> > In message <20201019135602.3943835-12-sjg@chromium.org> you wrote:
> >> In some cases it is necessary to pass parameters to Linux so that it
> >> will
> >> boot correctly. For example, the rootdev parameter is often used to
> >> specify the root device. However the root device may change depending
> >> on
> >> whence U-Boot loads the kernel. At present it is necessary to build up
> >> the command line by adding device strings to it one by one.
> >>
> >> It is often more convenient to provide a template for bootargs, with
> >> U-Boot doing the substitution from other environment variables.
> >>
> >> Add a way to substitute strings in the bootargs variable. This allows
> >> things like "rootdev=%U" to be used in bootargs, with the %U
> >> substitution
> >> providing the UUID of the root device.
> >
> > Argh, no, please don't.
> >
> > You add something unconditionally to common code which very few
> > people need.  U-Boot size is growing all the time because of such
> > ... features.  This may be acceptable on the systems you have in
> > mind, but I consider this selfish.
> >
> > Why do we have to add yet another non-standard way of substituting
> > variables in a string?  Can we not use alreay existing methonds
> > instead?
> >
> > Why do you have to use "%U" in your template instead of for example
> > "${uuid}" ?
>
> I'd second that. Having variables evaluated inside the bootargs would
> be very valuable IMHO.
>
> At the moment we have some cumbersome constructs like
>    set_bootargs="setenv bootargs bla ${var}"

Yes it is a real pain. The substitution happens on first parse two, so
you have to put these commands in separate variables if you are
building things up.

Another thing to mention is that years ago I sent a series to help
with that last bit, i.e. make variables evaluate recursively. I'm
trying to remember what happened to it.

Regards,
Simon

[PATCH 11/11] bootm: Support string substitution in bootargs

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ3sg693VXsEpvjnvkuUzfF612cxWvpWuvx_XiVNgQH4Jg@mail.gmail.com> you wrote:
>
> > You add something unconditionally to common code which very few
> > people need.  U-Boot size is growing all the time because of such
> > ... features.  This may be acceptable on the systems you have in
> > mind, but I consider this selfish.
>
> Did you see the Kconfig option?

So you claim the size does not grow with the feature not selected?

> > Why do we have to add yet another non-standard way of substituting
> > variables in a string?  Can we not use alreay existing methonds
> > instead?
>
> What sort of methods?

Variable substitution?

> > Why do you have to use "%U" in your template instead of for example
> > "${uuid}" ?
>
> This is what Chrome OS uses, so it is easier this way, Otherwise I
> have to replace %U with ${uuid} first.

That's what I meant when I wrote "selfish" ;-)

> Which code can I use to parse the ${var} thing?

setenv and run ?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
My brother sent me a postcard the other day with this  big  sattelite
photo  of the entire earth on it. On the back it said: "Wish you were
here".                                                - Steven Wright

[PATCH 11/11] bootm: Support string substitution in bootargs

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ3gkCO6_DdQh3XJrsf9SdVa0v3pGuwejGBoDHPEijyRkg@mail.gmail.com> you wrote:
>
> I did wonder about the empty env var thing. IMO it would be nice to
> support empty variables, so we can distinguish between an empty one
> and a missing one.

What exactly is the use case for this that justifies the additional
code needed to implement this feature?

Please keep in mind that this is a boot loader, and we are not
attending a contest for feature-completeness.

[Non-random signature this time.]

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Perfection is reached, not when there is no longer anything  to  add,
but when there is no longer anything to take away.
                                           - Antoine de Saint-Exupery

[PATCH 11/11] bootm: Support string substitution in bootargs

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ2rAQZN_bdQ5C-9cB4u8F8PrhZkBWYx0zWxj_yEBrV2qw@mail.gmail.com> you wrote:
>
> > At the moment we have some cumbersome constructs like
> >    set_bootargs="setenv bootargs bla ${var}"
>
> Yes it is a real pain. The substitution happens on first parse two, so
> you have to put these commands in separate variables if you are
> building things up.

Come on, is it really that big a problem?

You define all your needed settings (foo, bar, baz, and maybe uuid),
and then you run a single command

	setenv bootargs "${foo} ${bar} %{baz} ${uuid}"

?

Yes, it takes one additional step, but it's simple and does not need
extra code.

[And if someone bothered to update hush to a recent version, and
while doing so revisited the adaptions needed for U-Boot, we could
also do much better.  IIRC, things like command substitution were
omitted then because of the code size it would have required; given
today's resources this might be an optional feature for many.]

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
...when fits of creativity run strong, more than  one  programmer  or
writer  has  been  known to abandon the desktop for the more spacious
floor.                                             - Fred Brooks, Jr.

[PATCH 09/11] bootm: Allow updating the bootargs in a buffer

[Simon Glass]

Hi Wolfgang,

On Mon, 19 Oct 2020 at 08:47, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon Glass,
>
> In message <20201019135602.3943835-10-sjg@chromium.org> you wrote:
> > At present we only support updating the 'bootargs' environment
> > variable. Add another function to update a buffer instead. This will
> > allow zimage to use this feature.
>
> I think this is the wrong way to address a problem.
>
> Instead, zimage should use bootargs as well.

OK I can add that. But for testing purposes it is still good to be
able to have the feature in a function that can be called without
going through environment variables.

Regards,
SImon

[PATCH 00/11] bootm: Support substitions in bootargs and add tests

[Simon Glass]

Hi Heinrich,

On Mon, 19 Oct 2020 at 08:45, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 19.10.20 15:55, Simon Glass wrote:
> > This series adds tests to the fixup_silent-linux() function and extends
> > the 'zimage' command to use it.
> >
> > It also adds a new string-substition feature to allow bootargs to be a
> > template, rather than having to build it up piece by piece with
> > information obtained in a build script.
> >
> > It also updates zimage to use the same command-line processing.
> >
> > With these additions it is possible to boot Chrome OS from a U-Boot script
> > on most Chromebooks.
>
> Hello Simon,
>
> could you, please, add a patch documenting the usage of the new
> string-substitution feature in the HTML documentation.

OK will do. I meant to send this series three weeks ago and forgot about it...

Regards,
Simon

[PATCH 07/11] bootm: Split out bootargs environment reading / writing

[Simon Glass]

Hi Wolfgang,

On Mon, 19 Oct 2020 at 08:45, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <20201019135602.3943835-8-sjg@chromium.org> you wrote:
> ...
> >
> > It is also useful for zimage to use a buffer, since it does not actually
> > put the Linux command line in the bootargs variable.
>
> ...which I consider a bug that should be fixed.

OK I was wondering about that.

The messy thing about zimage is that the command line comes from the
setup.bin in the kernel, and then needs to be modified. So you can't
just blindly use the 'bootargs' var. Perhaps that is why it wasn't
done?

I also feel eventually that bootm could subsume zboot given the
similarities. Or maybe zboot just dies if people stop using the old
boot approach?

Regards,
Simon

[PATCH 11/11] bootm: Support string substitution in bootargs

[Simon Glass]

Hi Wolfgang,

On Tue, 20 Oct 2020 at 07:17, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ3sg693VXsEpvjnvkuUzfF612cxWvpWuvx_XiVNgQH4Jg@mail.gmail.com> you wrote:
> >
> > > You add something unconditionally to common code which very few
> > > people need.  U-Boot size is growing all the time because of such
> > > ... features.  This may be acceptable on the systems you have in
> > > mind, but I consider this selfish.
> >
> > Did you see the Kconfig option?
>
> So you claim the size does not grow with the feature not selected?

That's what I see, except that there is some size growth due to the
refactoring / adding tests.

>
> > > Why do we have to add yet another non-standard way of substituting
> > > variables in a string?  Can we not use alreay existing methonds
> > > instead?
> >
> > What sort of methods?
>
> Variable substitution?
>
> > > Why do you have to use "%U" in your template instead of for example
> > > "${uuid}" ?
> >
> > This is what Chrome OS uses, so it is easier this way, Otherwise I
> > have to replace %U with ${uuid} first.
>
> That's what I meant when I wrote "selfish" ;-)
>
> > Which code can I use to parse the ${var} thing?
>
> setenv and run ?

Will respond on other emails.

Regards,
SImon

[PATCH 11/11] bootm: Support string substitution in bootargs

[Simon Glass]

Hi Wolfgang,

On Tue, 20 Oct 2020 at 07:26, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ2rAQZN_bdQ5C-9cB4u8F8PrhZkBWYx0zWxj_yEBrV2qw@mail.gmail.com> you wrote:
> >
> > > At the moment we have some cumbersome constructs like
> > >    set_bootargs="setenv bootargs bla ${var}"
> >
> > Yes it is a real pain. The substitution happens on first parse two, so
> > you have to put these commands in separate variables if you are
> > building things up.
>
> Come on, is it really that big a problem?
>
> You define all your needed settings (foo, bar, baz, and maybe uuid),
> and then you run a single command
>
>         setenv bootargs "${foo} ${bar} %{baz} ${uuid}"
>
> ?
>
> Yes, it takes one additional step, but it's simple and does not need
> extra code.

It is actually not simple, for three reasons:

1. With zboot the args come from the kernel setup.bin and must be modified
2. With Chrome OS the args come from the kernel partition and must be
augmented / substituted
3. The above command cannot be in the same env var as anything else,
since substitution breaks in that case

So you end up with a really complicated mess of environment variables
and scripts that is barely manageable. I want it to be simple.

See here for example (this only deals with 3 above, not 1 and 2, which
would still need custom code without my series)

https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/refs/heads/chromeos-v2013.06/include/configs/chromeos.h#204

>
> [And if someone bothered to update hush to a recent version, and
> while doing so revisited the adaptions needed for U-Boot, we could
> also do much better.  IIRC, things like command substitution were
> omitted then because of the code size it would have required; given
> today's resources this might be an optional feature for many.]

Yes I agree, so long as the code size increase is not horrific.

+Tom Rini what do you think?

Regards,
Simon

[PATCH 09/11] bootm: Allow updating the bootargs in a buffer

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ3hYf9F7eBqMQBKpe4jM=hW46dhCPxGU=cBVV8M_WNKDQ@mail.gmail.com> you wrote:
>
> > I think this is the wrong way to address a problem.
> >
> > Instead, zimage should use bootargs as well.
>
> OK I can add that. But for testing purposes it is still good to be
> able to have the feature in a function that can be called without
> going through environment variables.

OK with me - if it doesn't add to the code size.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The C-shell doesn't parse. It adhoculates.
    - Casper.Dik at Holland.Sun.COM in <3ol96k$b2j@engnews2.Eng.Sun.COM>

[PATCH 07/11] bootm: Split out bootargs environment reading / writing

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ0LCKLdpq85gfyOU-Z0BJgydo_KY+JEX7KTb_WFDpFM+A@mail.gmail.com> you wrote:
>
> > > It is also useful for zimage to use a buffer, since it does not actually
> > > put the Linux command line in the bootargs variable.
> >
> > ...which I consider a bug that should be fixed.
>
> OK I was wondering about that.
>
> The messy thing about zimage is that the command line comes from the
> setup.bin in the kernel, and then needs to be modified. So you can't
> just blindly use the 'bootargs' var. Perhaps that is why it wasn't
> done?

I can't say.  I never used zImages, and probably never will.

> I also feel eventually that bootm could subsume zboot given the
> similarities. Or maybe zboot just dies if people stop using the old
> boot approach?

I would not shed a tear.

I cannot understand why people still use this.  We should more
actively encourage the use of FIT images in favour of all these old
formats.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"If it ain't broke, don't fix it."                       - Bert Lantz

[PATCH 11/11] bootm: Support string substitution in bootargs

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ1SfwRa_LXh0zu3Oug=GLEUQx5MdCCvqW90-FV5hvX9uw@mail.gmail.com> you wrote:
>
> > Yes, it takes one additional step, but it's simple and does not need
> > extra code.
>
> It is actually not simple, for three reasons:
>
> 1. With zboot the args come from the kernel setup.bin and must be modified

Don't use zboot, then.  In my opinion, zboot is crap and should
never have been added, but there were so many requests for it.
Nevertheless, the use of crippleware is a sin that carries with it
its own punishment.

Don't do it, then.

Or, if you feel there is some value in zboot that should be
conserved, fix it to work like the rest of U-Boot.

> 2. With Chrome OS the args come from the kernel partition and must be
> augmented / substituted

OK.  But then why can we not still use the standard variable
substitution mechanism we already have?

> 3. The above command cannot be in the same env var as anything else,
> since substitution breaks in that case

Sorry, I don't understand what you mean heare.  What is "the same
env var" versus "anything else"?  Maybe you can give a specific example?


> So you end up with a really complicated mess of environment variables
> and scripts that is barely manageable. I want it to be simple.

Again, I can't follow you.  Why must there be a mess?

> See here for example (this only deals with 3 above, not 1 and 2, which
> would still need custom code without my series)
>
> https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/refs/heads/chromeos-v2013.06/include/configs/chromeos.h#204

Sorry - all I see there is some complex settings if these make sense
I can't tell) - but how would things be better if you could - for
example - use "%U" instead of "${uuid}" as you suggested?

Also, is your approach not necessarily limited? You can now think of
a handful of variables you may want to pass, say root device, root
partition, uuid, maybe MAC address etc.  But the next user will need
kernel args that you did not think of - so how do we proceed?  Add
all features anybody needs to that new code?  That certainly does
not scale.  Or mix "%FOO" and "${foo}" style arguments?  That's even
worse.  I really fail to see the benefits, I see only ugliness and
problems.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Good manners are the settled  medium  of  social,  as  specie  is  of
commercial, life; returns are equally expected for both.
           - Lord Chesterfield _Letters to his Son_, 25 December 1753

[PATCH 11/11] bootm: Support string substitution in bootargs

[Simon Glass]

Hi Wolfgang,

On Wed, 21 Oct 2020 at 01:16, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ1SfwRa_LXh0zu3Oug=GLEUQx5MdCCvqW90-FV5hvX9uw@mail.gmail.com> you wrote:
> >
> > > Yes, it takes one additional step, but it's simple and does not need
> > > extra code.
> >
> > It is actually not simple, for three reasons:
> >
> > 1. With zboot the args come from the kernel setup.bin and must be modified
>
> Don't use zboot, then.  In my opinion, zboot is crap and should
> never have been added, but there were so many requests for it.
> Nevertheless, the use of crippleware is a sin that carries with it
> its own punishment.
>
> Don't do it, then.
>
> Or, if you feel there is some value in zboot that should be
> conserved, fix it to work like the rest of U-Boot.

Well my series does that to a large extent. It is much more like bootm
now, in that it is properly split into stages. I think it would be
possible to combine parts of it into bootm as a future step, although
it is non-trivial, and I think we should build out more tests first.

But zboot does make use of an existing command line and does all the
weird x86 processing, so I don't know how we could get rid of it.

>
> > 2. With Chrome OS the args come from the kernel partition and must be
> > augmented / substituted
>
> OK.  But then why can we not still use the standard variable
> substitution mechanism we already have?

I don't think the actual mechanism is a big deal. We could do a string
replace (does U-Boot support that?) of %U with ${uuid} for example, to
make it work for my case.

Or we could go with an idea I previously rejected, to just provide a
simple string replace feature, with any special characters in the
search string. For example:

setenv bootargs_repl "%U=${uuid} %P=${partid}"

>
> > 3. The above command cannot be in the same env var as anything else,
> > since substitution breaks in that case
>
> Sorry, I don't understand what you mean heare.  What is "the same
> env var" versus "anything else"?  Maybe you can give a specific example?

Did you see the 'run regen_scripts' script?

At present I can do

setenv uuid blah; setenv partid blah; bootm

but with your scheme I need the 'run regen_script' to set the
variables correctly, which is a real pain as my example shows.

>
>
> > So you end up with a really complicated mess of environment variables
> > and scripts that is barely manageable. I want it to be simple.
>
> Again, I can't follow you.  Why must there be a mess?
>
> > See here for example (this only deals with 3 above, not 1 and 2, which
> > would still need custom code without my series)
> >
> > https://chromium.googlesource.com/chromiumos/third_party/u-boot/+/refs/heads/chromeos-v2013.06/include/configs/chromeos.h#204
>
> Sorry - all I see there is some complex settings if these make sense
> I can't tell) - but how would things be better if you could - for
> example - use "%U" instead of "${uuid}" as you suggested?

My point here is not about %U, it is about the pain of multiple stages
to get the variables right. WIth bootargs substitution we can just set
the variables and boot.

>
> Also, is your approach not necessarily limited? You can now think of
> a handful of variables you may want to pass, say root device, root
> partition, uuid, maybe MAC address etc.  But the next user will need
> kernel args that you did not think of - so how do we proceed?  Add
> all features anybody needs to that new code?  That certainly does
> not scale.  Or mix "%FOO" and "${foo}" style arguments?  That's even
> worse.  I really fail to see the benefits, I see only ugliness and
> problems.

These scripts are board-specific, so each board can do what it likes
here. But the nice thing is not having to manually build up the
bootargs.

Can we step past the %U business? I think we can use the ${var} stuff
with some substitution.

Regards,
Simon

[PATCH 07/11] bootm: Split out bootargs environment reading / writing

[Simon Glass]

Hi Wolfgang,

On Wed, 21 Oct 2020 at 01:02, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ0LCKLdpq85gfyOU-Z0BJgydo_KY+JEX7KTb_WFDpFM+A@mail.gmail.com> you wrote:
> >
> > > > It is also useful for zimage to use a buffer, since it does not actually
> > > > put the Linux command line in the bootargs variable.
> > >
> > > ...which I consider a bug that should be fixed.
> >
> > OK I was wondering about that.
> >
> > The messy thing about zimage is that the command line comes from the
> > setup.bin in the kernel, and then needs to be modified. So you can't
> > just blindly use the 'bootargs' var. Perhaps that is why it wasn't
> > done?
>
> I can't say.  I never used zImages, and probably never will.
>
> > I also feel eventually that bootm could subsume zboot given the
> > similarities. Or maybe zboot just dies if people stop using the old
> > boot approach?
>
> I would not shed a tear.
>
> I cannot understand why people still use this.  We should more
> actively encourage the use of FIT images in favour of all these old
> formats.

Well x86 does support FIT, but oddly it still has the setup.bin
business, because the kernel requires it! At least it uses bootm,
though.

Something for the future...

Regards,
Simon

[PATCH 09/11] bootm: Allow updating the bootargs in a buffer

[Simon Glass]

Hi Wolfgang,

On Wed, 21 Oct 2020 at 00:57, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ3hYf9F7eBqMQBKpe4jM=hW46dhCPxGU=cBVV8M_WNKDQ@mail.gmail.com> you wrote:
> >
> > > I think this is the wrong way to address a problem.
> > >
> > > Instead, zimage should use bootargs as well.
> >
> > OK I can add that. But for testing purposes it is still good to be
> > able to have the feature in a function that can be called without
> > going through environment variables.
>
> OK with me - if it doesn't add to the code size.

The bootm refactoring adds about 144 bytes of code for Thumb2 and 31
bytes of data. I think it is worth it for the ability to add tests.
The extra substitution feature doesn't add any size if not enabled.

Regards,
Simon

[PATCH 11/11] bootm: Support string substitution in bootargs

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ2Vn3oWAHy71smw-+fRo28HUA0K32DP6FPoGtjtK3XoFQ@mail.gmail.com> you wrote:
>
> Well my series does that to a large extent. It is much more like bootm
> now, in that it is properly split into stages. I think it would be
> possible to combine parts of it into bootm as a future step, although
> it is non-trivial, and I think we should build out more tests first.

Hm... I fear that this is rather cementing the zboot support so we
never get rid of it...
>
> But zboot does make use of an existing command line and does all the
> weird x86 processing, so I don't know how we could get rid of it.

How about a command that takes this existing command line and
"imports" it into a standard bootargs variable?

> I don't think the actual mechanism is a big deal. We could do a string
> replace (does U-Boot support that?) of %U with ${uuid} for example, to
> make it work for my case.

Yes, we can.

=> env print commandline bootargs
## Error: "commandline" not defined
## Error: "bootargs" not defined
=> env set commandline 'This is a %A test for %U replacement.'
=> setexpr bootargs gsub %A boring "${commandline}"
bootargs=This is a boring test for %U replacement.
=> setexpr bootargs gsub %U '${uuid}'
bootargs=This is a boring test for ${uuid} replacement.
=> env print bootargs
bootargs=This is a boring test for ${uuid} replacement.

> Or we could go with an idea I previously rejected, to just provide a
> simple string replace feature, with any special characters in the
> search string. For example:

But we have all this already, even with regular expressions and
(simple) backreferences.


> > Sorry, I don't understand what you mean heare.  What is "the same
> > env var" versus "anything else"?  Maybe you can give a specific example?
>
> Did you see the 'run regen_scripts' script?
>
> At present I can do
>
> setenv uuid blah; setenv partid blah; bootm
>
> but with your scheme I need the 'run regen_script' to set the
> variables correctly, which is a real pain as my example shows.

Just insert a "run" there, and you are done with it.

> My point here is not about %U, it is about the pain of multiple stages
> to get the variables right. WIth bootargs substitution we can just set
> the variables and boot.

I think this is not so much a bootargs question (or at least it
should not be it).  bootargs is just another environment variable
without any specific properties except that it's content is being
passed to the kerenl, so you want to have the needed variable
substituion doen before that.

It was an intentional decison not to do this automagically, based on
the thinking that accorsing to UNNIX philosophy it is not each
individual command which implements things like wildcard or variable
substitution, but rather this is something the shell does for you,
and the commands are dumb in this respect.  This works reasonably
well, except that we don't pass bootargs as argument to the bootm
command - rather it is done internally by default, this lacking the
variable substituion.

You may call this a design bug, and I will accept the blame for it.

To me the obvious fix would be to correct this behavious such that
we extend bootm to accept the bootargs argument as a command line
parameter, thus enabling variable substitution there, too.

As far as I'm concenred, I don't think this is necessary as all it
takes to work around this is a single call of a "run" command which
can do exactly that, too.

But I agree, this is just a workaround, and it would be more
consistent to pass bootargs as argument directly.


And this is also the direction I see how the zboot stuff should be
fixed:  import the Linux command line into the environment, modify
it as needed (if necessary using regular expression matching /
string substituion to fix it up to use common U-Boot variable
names), and then pass it like the rest of the world to the kernel.

> Can we step past the %U business? I think we can use the ${var} stuff
> with some substitution.

Don't we have that already?  What exactly is it you miss?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Steal five dollars and you were a petty  thief.  Steal  thousands  of
dollars and you are either a government or a hero.
                                   - Terry Pratchett, _Going_Postal_

[PATCH 11/11] bootm: Support string substitution in bootargs

[Simon Glass]

Hi Wolfgang,

On Thu, 22 Oct 2020 at 06:32, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Simon,
>
> In message <CAPnjgZ2Vn3oWAHy71smw-+fRo28HUA0K32DP6FPoGtjtK3XoFQ@mail.gmail.com> you wrote:
> >
> > Well my series does that to a large extent. It is much more like bootm
> > now, in that it is properly split into stages. I think it would be
> > possible to combine parts of it into bootm as a future step, although
> > it is non-trivial, and I think we should build out more tests first.
>
> Hm... I fear that this is rather cementing the zboot support so we
> never get rid of it...
> >
> > But zboot does make use of an existing command line and does all the
> > weird x86 processing, so I don't know how we could get rid of it.
>
> How about a command that takes this existing command line and
> "imports" it into a standard bootargs variable?
>
> > I don't think the actual mechanism is a big deal. We could do a string
> > replace (does U-Boot support that?) of %U with ${uuid} for example, to
> > make it work for my case.
>
> Yes, we can.
>
> => env print commandline bootargs
> ## Error: "commandline" not defined
> ## Error: "bootargs" not defined
> => env set commandline 'This is a %A test for %U replacement.'
> => setexpr bootargs gsub %A boring "${commandline}"
> bootargs=This is a boring test for %U replacement.
> => setexpr bootargs gsub %U '${uuid}'
> bootargs=This is a boring test for ${uuid} replacement.
> => env print bootargs
> bootargs=This is a boring test for ${uuid} replacement.
>
> > Or we could go with an idea I previously rejected, to just provide a
> > simple string replace feature, with any special characters in the
> > search string. For example:
>
> But we have all this already, even with regular expressions and
> (simple) backreferences.
>
>
> > > Sorry, I don't understand what you mean heare.  What is "the same
> > > env var" versus "anything else"?  Maybe you can give a specific example?
> >
> > Did you see the 'run regen_scripts' script?
> >
> > At present I can do
> >
> > setenv uuid blah; setenv partid blah; bootm
> >
> > but with your scheme I need the 'run regen_script' to set the
> > variables correctly, which is a real pain as my example shows.
>
> Just insert a "run" there, and you are done with it.
>
> > My point here is not about %U, it is about the pain of multiple stages
> > to get the variables right. WIth bootargs substitution we can just set
> > the variables and boot.
>
> I think this is not so much a bootargs question (or at least it
> should not be it).  bootargs is just another environment variable
> without any specific properties except that it's content is being
> passed to the kerenl, so you want to have the needed variable
> substituion doen before that.
>
> It was an intentional decison not to do this automagically, based on
> the thinking that accorsing to UNNIX philosophy it is not each
> individual command which implements things like wildcard or variable
> substitution, but rather this is something the shell does for you,
> and the commands are dumb in this respect.  This works reasonably
> well, except that we don't pass bootargs as argument to the bootm
> command - rather it is done internally by default, this lacking the
> variable substituion.
>
> You may call this a design bug, and I will accept the blame for it.
>
> To me the obvious fix would be to correct this behavious such that
> we extend bootm to accept the bootargs argument as a command line
> parameter, thus enabling variable substitution there, too.
>
> As far as I'm concenred, I don't think this is necessary as all it
> takes to work around this is a single call of a "run" command which
> can do exactly that, too.
>
> But I agree, this is just a workaround, and it would be more
> consistent to pass bootargs as argument directly.
>
>
> And this is also the direction I see how the zboot stuff should be
> fixed:  import the Linux command line into the environment, modify
> it as needed (if necessary using regular expression matching /
> string substituion to fix it up to use common U-Boot variable
> names), and then pass it like the rest of the world to the kernel.
>
> > Can we step past the %U business? I think we can use the ${var} stuff
> > with some substitution.
>
> Don't we have that already?  What exactly is it you miss?

I have sent a new version of this series that uses ${var}.

Regards,
Simon